feat (Repo): Raise an error on query-like keyword opts to Repo functions (#4717)

Consider the following, written by a new-to-Elixir developer:

```elixir
Repo.delete_all(User, where: [account_id: user.account_id])
```

The developer mistakenly believed that the `opts` to the Repo function worked like the Ecto.Query DSL, when in fact these keyword options were entirely ignored (making this an unscoped deletion of all users).

With this change, Ecto will raise a runtime error when given `opts` keys that seem to indicate the user was trying to modify the queryable passed in to:

- Repo.all/2
- Repo.all_by/3
- Repo.delete_all/2
- Repo.exists?/2
- Repo.get/3
- Repo.get_by/3
- Repo.one/2
- Repo.stream/2

Author: s3cur3

lib/ecto/repo.ex

@@ -326,6 +326,37 @@ defmodule Ecto.Repo do
         |> Keyword.merge(opts)
       end
 
+      # Keyword list keys a user may try to pass to something like Repo.all/2, under
+      # the mistaken belief that the keyword arguments operate like the Ecto.Query DSL.
+      # We want to proactively raise an error on something like:
+      #
+      #     Repo.delete_all(User, where: [id: user.id])
+      #
+      # ...since the query will be unscoped in a way that the caller almost certainly
+      # did not intend.
+      @unsupported_query_opts [
+        :where,
+        :preload,
+        :order_by,
+        :limit,
+        :group_by,
+        :distinct,
+        :select,
+        :update
+      ]
+
+      defp validate_query_opts!(opts, function_name) do
+        Enum.each(opts, fn
+          {key, _} when key in @unsupported_query_opts ->
+            raise ArgumentError,
+                  "unsupported option #{inspect(key)} for Repo.#{function_name}. " <>
+                    "Instead, use Ecto.Query to build a query, and pass that query into this function."
+
+          _ ->
+            :ok
+        end)
+      end
+
       ## Transactions
 
       if Ecto.Adapter.Transaction in behaviours do
@@ -491,6 +522,8 @@ defmodule Ecto.Repo do
           end
 
           def delete_all(queryable, opts \\ []) do
+            validate_query_opts!(opts, :delete_all)
+
             repo = get_dynamic_repo()
 
             Ecto.Repo.Queryable.delete_all(
@@ -502,6 +535,8 @@ defmodule Ecto.Repo do
         end
 
         def all(queryable, opts \\ []) do
+          validate_query_opts!(opts, :all)
+
           repo = get_dynamic_repo()
 
           Ecto.Repo.Queryable.all(
@@ -512,6 +547,8 @@ defmodule Ecto.Repo do
         end
 
         def all_by(queryable, clauses, opts \\ []) do
+          validate_query_opts!(opts, :all_by)
+
           repo = get_dynamic_repo()
 
           Ecto.Repo.Queryable.all_by(
@@ -523,6 +560,8 @@ defmodule Ecto.Repo do
         end
 
         def stream(queryable, opts \\ []) do
+          validate_query_opts!(opts, :stream)
+
           repo = get_dynamic_repo()
 
           Ecto.Repo.Queryable.stream(
@@ -533,6 +572,8 @@ defmodule Ecto.Repo do
         end
 
         def get(queryable, id, opts \\ []) do
+          validate_query_opts!(opts, :get)
+
           repo = get_dynamic_repo()
 
           Ecto.Repo.Queryable.get(
@@ -555,6 +596,8 @@ defmodule Ecto.Repo do
         end
 
         def get_by(queryable, clauses, opts \\ []) do
+          validate_query_opts!(opts, :get_by)
+
           repo = get_dynamic_repo()
 
           Ecto.Repo.Queryable.get_by(
@@ -597,6 +640,8 @@ defmodule Ecto.Repo do
         end
 
         def one(queryable, opts \\ []) do
+          validate_query_opts!(opts, :one)
+
           repo = get_dynamic_repo()
 
           Ecto.Repo.Queryable.one(
@@ -657,6 +702,8 @@ defmodule Ecto.Repo do
         end
 
         def exists?(queryable, opts \\ []) do
+          validate_query_opts!(opts, :exists?)
+
           repo = get_dynamic_repo()
 
           Ecto.Repo.Queryable.exists?(

test/ecto/repo_test.exs

@@ -1600,6 +1600,63 @@ defmodule Ecto.RepoTest do
     assert schema.__meta__.prefix == %{key: :private}
   end
 
+  test "get, get_by, one, all, all_by, exists?, stream, and delete_all raise an error when given Ecto.Query-like opts" do
+    for {unsupported_option, bad_opts} <- [
+          {:where, [where: [user_id: Ecto.UUID.generate()]]},
+          {:preload, [preload: :users]},
+          {:order_by, [order_by: :name]},
+          {:limit, [limit: 10]}
+        ] do
+      assert_raise ArgumentError,
+                   ~r"unsupported option #{inspect(unsupported_option)} for Repo.get",
+                   fn ->
+                     TestRepo.get(MySchema, 123, bad_opts)
+                   end
+
+      assert_raise ArgumentError,
+                   ~r"unsupported option #{inspect(unsupported_option)} for Repo.get_by",
+                   fn ->
+                     TestRepo.get_by(MySchema, [id: 123], bad_opts)
+                   end
+
+      assert_raise ArgumentError,
+                   ~r"unsupported option #{inspect(unsupported_option)} for Repo.one",
+                   fn ->
+                     TestRepo.one(MySchema, bad_opts)
+                   end
+
+      assert_raise ArgumentError,
+                   ~r"unsupported option #{inspect(unsupported_option)} for Repo.all",
+                   fn ->
+                     TestRepo.all(MySchema, bad_opts)
+                   end
+
+      assert_raise ArgumentError,
+                   ~r"unsupported option #{inspect(unsupported_option)} for Repo.all_by",
+                   fn ->
+                     TestRepo.all_by(MySchema, [id: 123], bad_opts)
+                   end
+
+      assert_raise ArgumentError,
+                   ~r"unsupported option #{inspect(unsupported_option)} for Repo.exists?",
+                   fn ->
+                     TestRepo.exists?(MySchema, bad_opts)
+                   end
+
+      assert_raise ArgumentError,
+                   ~r"unsupported option #{inspect(unsupported_option)} for Repo.stream",
+                   fn ->
+                     TestRepo.stream(MySchema, bad_opts)
+                   end
+
+      assert_raise ArgumentError,
+                   ~r"unsupported option #{inspect(unsupported_option)} for Repo.delete_all",
+                   fn ->
+                     TestRepo.delete_all(MySchema, bad_opts)
+                   end
+    end
+  end
+
   describe "changeset prepare" do
     defp prepare_changeset() do
       %MySchema{id: 1}
@@ -1970,7 +2027,9 @@ defmodule Ecto.RepoTest do
 
     test "includes conflict target in :replace_all when replace_changed is false" do
       fields = [:map, :array, :z, :yyy, :x, :id]
+
       TestRepo.insert(%MySchema{id: 1}, on_conflict: :replace_all, conflict_target: [:id], replace_changed: false)
+
       assert_received {:insert, %{source: "my_schema", on_conflict: {^fields, [], [:id]}}}
     end