Repository: elixir-ecto/ecto
Pull request #2162 (2017-08-09 21:31:38)

Add validate_unique_tentatively/3
Provide a way to check for uniqueness in the *validation* phase.

This PR comes after much discussion - see https://github.com/elixir-ecto/ecto/pull/2006 - but that doesn't mean that the implementation (or the tests) are perfect.

ebert[bot]: Ebert has finished reviewing this Pull Request and has found:

* 1 possible new issue (including those that may have been commented here).

But beware that this branch is **[1 commit behind the `elixir-ecto:master` branch](https://github.com/elixir-ecto/ecto/compare/nathanl:nathanl/validate_unique_tentatively...elixir-ecto:master)**, and a review of an up to date branch would produce more accurate results.

You can see more details about this review at https://ebertapp.io/github/elixir-ecto/ecto/pulls/2162.

<!-- Ebert auto generated comment -->

nathanl: Working on fixing test failures
michalmuskala: I wonder about the name. Often something that bypasses some rules is called "dirty". Would it make sense to call the function `validate_dirty_uniqueness` or `dirty_validate_uniqueness`?
johnnyshields: @michalmuskala "dirty" implies dirty tracking (i.e. check if an object that was touched)

I prefer the name "validate_unique_optimistically"
nathanl: @michalmuskala I tried the process dictionary approach - see latest commit.

Personally I like this approach less than the previous one. I had to guess to discover the values `{1, [true]}` and `{0, []}` that the adapter returns, and relying on side effects elsewhere doesn't seem as clean as passing something like `FakeRepoWithDup` to the validation. But I'm happy to use this approach if it's more in line with Ecto test conventions.

Regarding the name: all the existing validation names are of the pattern `validate_{some property of the changeset}`, so I like the idea of having  this one start with `validate_unique`, then adding an adverb. `tentatively` seems to fit well; the definition is "subject to further confirmation", which is exactly what we want to convey: this needs to be followed up with a constraint check.

`optimistically` also seems OK given how the word is used in programming contexts, but to me it's less obvious what it means here; the point is not that we have a sunny outlook on life 😆 , but that we already know we'll need to do some further checking.
nathanl: I should say - I'm not adamant about the name. Happy to call it whatever y'all think best.
michalmuskala: My primary concern with "tentatively" is that it's not a very common word and might be confusing for non-native speakers.
chrismcg: Perhaps `validate_unique_naively` to fit in with `NaiveDateTime`
nathanl: > Perhaps validate_unique_naively to fit in with NaiveDateTime

That could work. But it might convey "this approach isn't very sophisticated" as opposed to "this is intended to be the first check, to be followed up somehow with a final check." "Tentative" has the built-in idea of "needs confirmation".

> My primary concern with "tentatively" is that it's not a very common word and might be confusing for non-native speakers.

Ah. Maybe so. It's also not super easy to type. 😆 
johnnyshields: I still believe "optimistic" is the correct word in the computer engineering context. https://en.wikipedia.org/wiki/Optimistic_concurrency_control

Patch:
diff --git a/lib/ecto/adapters/postgres/connection.ex b/lib/ecto/adapters/postgres/connection.ex
index 5288925e..7db48ebe 100644
--- a/lib/ecto/adapters/postgres/connection.ex
+++ b/lib/ecto/adapters/postgres/connection.ex
@@ -796,7 +796,7 @@ if Code.ensure_loaded?(Postgrex) do
       do: []
 
     defp index_expr(literal) when is_binary(literal),
-      do: [?(, literal, ?)]
+      do: literal
     defp index_expr(literal),
       do: quote_name(literal)
 
diff --git a/lib/ecto/changeset.ex b/lib/ecto/changeset.ex
index eb2427bc..a352a95d 100644
--- a/lib/ecto/changeset.ex
+++ b/lib/ecto/changeset.ex
@@ -31,10 +31,10 @@ defmodule Ecto.Changeset do
   Ecto changesets provide both validations and constraints which
   are ultimately turned into errors in case something goes wrong.
 
-  The difference between them is that validations can be executed
-  without a need to interact with the database and, therefore, are
-  always executed before attempting to insert or update the entry
-  in the database.
+  The difference between them is that validations (with the exception of
+  `validate_unique_tentatively/3`) can be executed without a need to interact
+  with the database and, therefore, are always executed before attempting to
+  insert or update the entry in the database.
 
   However, constraints can only be checked in a safe way when
   performing the operation in the database. As a consequence,
@@ -219,6 +219,7 @@ defmodule Ecto.Changeset do
 
   """
 
+  require Ecto.Query
   alias __MODULE__
   alias Ecto.Changeset.Relation
 
@@ -1392,6 +1393,72 @@ defmodule Ecto.Changeset do
     end
   end
 
+  @doc """
+  Validates (tentatively) that no existing record with a different primary key
+  has the same values for these fields.
+
+  This is tentative because a race condition may occur. A unique constraint
+  should also be used to ensure uniqueness.
+
+  However, in most cases where conflicting data exists, it will have been
+  inserted prior to the current validation phase. Noticing those conflicts
+  during validation gives the user a chance to correct them at the same time
+  as other validation errors.
+
+  ## Examples
+      validate_unique_tentatively(changeset, [:email], repo)
+      validate_unique_tentatively(changeset, [:city_name, :state_name], repo)
+      validate_unique_tentatively(changeset, [:city_name, :state_name], repo, "city must be unique within state")
+
+  """
+  def validate_unique_tentatively(changeset, field_names, repo, error_message \\ "has already been taken") do
+    field_names = List.wrap(field_names)
+    where_clause = Enum.map(field_names, fn (field_name) ->
+      {field_name, get_field(changeset, field_name)}
+    end)
+
+    # If we don't have values for all fields, we can't query for uniqueness
+    if Enum.any?(where_clause, fn (tuple) -> is_nil(elem(tuple, 1)) end) do
+      changeset
+    else
+      dups_query = Ecto.Query.from q in changeset.data.__struct__, where: ^where_clause
+
+      # For updates, don't flag a record as a dup of itself
+      pk_fields_and_vals = pk_fields_and_vals(changeset)
+      incomplete_pk? = Enum.any?(
+        pk_fields_and_vals,
+        fn {_field, val} -> is_nil(val) end
+      )
+      dups_query = if incomplete_pk? do
+        dups_query
+      else
+        Enum.reduce(
+          pk_fields_and_vals,
+          dups_query, fn ({field_name, val}, query) ->
+            Ecto.Query.from q in query, where: field(q, ^field_name) != ^val
+          end)
+      end
+
+      dups_exist_query = Ecto.Query.from q in dups_query, select: true, limit: 1
+      case repo.one(dups_exist_query) do
+        true -> add_error(
+          changeset,
+          hd(field_names),
+          error_message,
+          [validation: [:validate_unique_tentatively, field_names]]
+        )
+        nil  -> changeset
+      end
+    end
+  end
+
+  defp pk_fields_and_vals(%Changeset{} = changeset) do
+    primary_key_field_names = changeset.data.__struct__.__schema__(:primary_key)
+    Enum.map(primary_key_field_names, fn(field_name) ->
+      {field_name, get_field(changeset, field_name)}
+    end)
+  end
+
   defp ensure_field_exists!(%Changeset{types: types, data: data}, field) do
     unless Map.has_key?(types, field) do
       raise ArgumentError, "unknown field #{inspect field} for changeset on #{inspect data}"
diff --git a/lib/ecto/migration.ex b/lib/ecto/migration.ex
index 9f965ef3..126dc6cf 100644
--- a/lib/ecto/migration.ex
+++ b/lib/ecto/migration.ex
@@ -531,14 +531,17 @@ defmodule Ecto.Migration do
       # The index type can be specified
       create index("products", [:name], using: :hash)
 
-      # Create an index on custom expressions
-      create index("products", ["lower(name)"], name: :products_lower_name_index)
-
       # Create a partial index
       create index("products", [:user_id], where: "price = 0", name: :free_products_index)
 
+  Indexes also support custom expressions. Some databases may require the
+  index expression to be written between parens:
+
+      # Create an index on custom expressions
+      create index("products", ["(lower(name))"], name: :products_lower_name_index)
+
       # To create a tsvector index with GIN on Postgres
-      create index("products", ["to_tsvector('english', name)"],
+      create index("products", ["(to_tsvector('english', name))"],
                    name: :products_name_vector, using: "GIN")
   """
   def index(table, columns, opts \\ [])


Test patch:
diff --git a/integration_test/cases/repo.exs b/integration_test/cases/repo.exs
index beca62ed..2ea6ed64 100644
--- a/integration_test/cases/repo.exs
+++ b/integration_test/cases/repo.exs
@@ -550,6 +550,37 @@ defmodule Ecto.Integration.RepoTest do
     assert %Ecto.Changeset{} = changeset.changes.item
   end
 
+  test "validate_unique_tentatively/3" do
+    {:ok, inserted_post} = TestRepo.insert(%Post{title: "Greetings", text: "hi"})
+
+    new_post_changeset =
+      %Post{}
+      |> Post.changeset(%{title: "Greetings", text: "ho"})
+
+    assert Ecto.Changeset.validate_unique_tentatively(
+      new_post_changeset,
+      [:title],
+      TestRepo
+    ).errors[:title] ==
+      {"has already been taken", [validation: [:validate_unique_tentatively, [:title]]]}
+
+    assert Ecto.Changeset.validate_unique_tentatively(
+      new_post_changeset,
+      [:title, :text],
+      TestRepo
+    ).errors[:title] == nil
+
+    update_changeset =
+      inserted_post
+      |> Post.changeset(%{text: "ho"})
+
+    assert Ecto.Changeset.validate_unique_tentatively(
+      update_changeset,
+      [:title],
+      TestRepo
+    ).errors[:title] == nil # cannot conflict with itself
+  end
+
   test "get(!)" do
     post1 = TestRepo.insert!(%Post{title: "1", text: "hai"})
     post2 = TestRepo.insert!(%Post{title: "2", text: "hai"})
diff --git a/test/ecto/adapters/postgres_test.exs b/test/ecto/adapters/postgres_test.exs
index 4b2f6d30..b438a990 100644
--- a/test/ecto/adapters/postgres_test.exs
+++ b/test/ecto/adapters/postgres_test.exs
@@ -960,7 +960,7 @@ defmodule Ecto.Adapters.PostgresTest do
 
     create = {:create, index(:posts, ["lower(permalink)"], name: "posts$main")}
     assert execute_ddl(create) ==
-      [~s|CREATE INDEX "posts$main" ON "posts" ((lower(permalink)))|]
+      [~s|CREATE INDEX "posts$main" ON "posts" (lower(permalink))|]
   end
 
   test "create index with prefix" do
@@ -970,7 +970,7 @@ defmodule Ecto.Adapters.PostgresTest do
 
     create = {:create, index(:posts, ["lower(permalink)"], name: "posts$main", prefix: :foo)}
     assert execute_ddl(create) ==
-      [~s|CREATE INDEX "posts$main" ON "foo"."posts" ((lower(permalink)))|]
+      [~s|CREATE INDEX "posts$main" ON "foo"."posts" (lower(permalink))|]
   end
 
   test "create index with comment" do
diff --git a/test/ecto/changeset_test.exs b/test/ecto/changeset_test.exs
index 88854cb6..8251b80d 100644
--- a/test/ecto/changeset_test.exs
+++ b/test/ecto/changeset_test.exs
@@ -37,6 +37,7 @@ defmodule Ecto.ChangesetTest do
     use Ecto.Schema
 
     schema "posts" do
+      field :id_part_2, :integer, primary_key: true
       field :title, :string, default: ""
       field :body
       field :uuid, :binary_id
@@ -54,7 +55,7 @@ defmodule Ecto.ChangesetTest do
   end
 
   defp changeset(schema \\ %Post{}, params) do
-    cast(schema, params, ~w(title body upvotes decimal topics virtual))
+    cast(schema, params, ~w(id id_part_2 title body upvotes decimal topics virtual))
   end
 
   ## cast/4
@@ -1044,6 +1045,61 @@ defmodule Ecto.ChangesetTest do
     assert changeset.errors == [terms_of_service: {"must be abided", [validation: :acceptance]}]
   end
 
+  alias Ecto.TestRepo
+
+  test "validate_unique_tentatively/3" do
+    dup_result = {1, [true]}
+    no_dup_result = {0, []}
+    base_changeset = changeset(%Post{}, %{"title" => "Hello World", "body" => "hi"})
+
+    Process.put(:test_repo_all_results, dup_result)
+    # validate uniqueness of one field
+    changeset = validate_unique_tentatively(base_changeset, :title, TestRepo)
+    assert changeset.errors == [
+      title: {
+        "has already been taken",
+        [validation: [:validate_unique_tentatively, [:title]]],
+      }
+    ]
+    Process.put(:test_repo_all_results, no_dup_result)
+    changeset = validate_unique_tentatively(base_changeset, :title, TestRepo)
+    assert changeset.valid?
+
+    Process.put(:test_repo_all_results, dup_result)
+    # validate uniqueness of multiple fields
+    changeset = validate_unique_tentatively(
+      base_changeset, [:title, :body], TestRepo
+    )
+    assert changeset.errors == [
+      title: {
+        "has already been taken",
+        [validation: [:validate_unique_tentatively, [:title, :body]]],
+      }
+    ]
+    Process.put(:test_repo_all_results, no_dup_result)
+    changeset = validate_unique_tentatively(
+      base_changeset, [:title, :body], TestRepo
+    )
+    assert changeset.valid?
+
+    Process.put(:test_repo_all_results, dup_result)
+    # custom error message
+    changeset = validate_unique_tentatively(
+      base_changeset, [:title], TestRepo, "is taken"
+    )
+    assert changeset.errors == [
+      title: {
+        "is taken",
+        [validation: [:validate_unique_tentatively, [:title]]],
+      }
+    ]
+    Process.put(:test_repo_all_results, no_dup_result)
+    changeset = validate_unique_tentatively(
+      base_changeset, [:title], TestRepo, "is taken"
+    )
+    assert changeset.valid?
+  end
+
   ## Locks
 
   test "optimistic_lock/3 with changeset" do
diff --git a/test/support/test_repo.exs b/test/support/test_repo.exs
index 8b4bb1d5..5339e81d 100644
--- a/test/support/test_repo.exs
+++ b/test/support/test_repo.exs
@@ -41,7 +41,7 @@ defmodule Ecto.TestAdapter do
   end
 
   def execute(_repo, _, {:nocache, {:all, _}}, _, _, _) do
-    {1, [[1]]}
+    Process.get(:test_repo_all_results) || {1, [[1]]}
   end
 
   def execute(_repo, _meta, {:nocache, {:delete_all, %{from: {_, SchemaMigration}}}}, [version], _, _) do