Ecto: Using on_conflict option of Repo.insert with a conditional update raises Ecto.StaleEntryError when the condition is not met

[skateinmars]

Environment

• Elixir version (elixir -v): Elixir 1.6.5 (compiled with OTP 19)
• Database and version (PostgreSQL 9.4, MongoDB 3.2, etc.): PostgreSQL 9.6.9 on x86_64-pc-linux-gnu (Debian 9.6.9-2.pgdg90+1), compiled by gcc (Debian 6.3.0-18+deb9u1) 6.3.0 20170516, 64-bit
• Ecto version (mix deps): 2.2.10
• Database adapter and version (mix deps): postgrex 0.13.5
• Operating system: Ubuntu 17.10

Current behavior

I am trying to use Repo.insert with its on_conflict option to conditionally update my record, based on its exiting attributes. Here is a small example to reproduce the issue:

defmodule User do
  use Ecto.Schema

  schema "users" do
    field(:email, :string)
    field(:comment, :string)

    timestamps()
  end
end

defmodule UserTest do
  use ExUnit.Case
  import Ecto.Query, only: [from: 2, update: 2]

  setup do
    :ok = Ecto.Adapters.SQL.Sandbox.checkout(Repo)
  end

  test "Raises an error" do
    email = "[email protected]"
    first_updated_at = DateTime.from_naive!(~N[2017-06-12 11:11:11.004], "Etc/UTC")
    _first_record = Repo.insert!(%User{email: email, updated_at: first_updated_at})

    second_updated_at = DateTime.from_naive!(~N[2010-01-01 11:11:11.004], "Etc/UTC")

    on_conflict_changes = [comment: "Duplicate found!"]

    conditional_update_query =
      from(p in User, where: p.updated_at < ^second_updated_at)
      |> update(set: ^on_conflict_changes)

    try do
      Repo.insert(
        %User{email: email, updated_at: second_updated_at},
        returning: true,
        on_conflict: conditional_update_query,
        conflict_target: [:email]
      )
    rescue
      Ecto.StaleEntryError ->
        IO.inspect "Should not have raised?"
    end
  end
end

Expected behavior

I am not exactly certain what should be returned in this case. Returning the existing record may be confusing, so maybe a changeset error?

The current behaviour may actually be the one expected, but if so it could be documented.

[josevalim]

The code is very helpful but can you plaese let us know the actual behaviour without us having to execute the code? Thank you!

[josevalim]

Ok, I think I get it. You are getting a stale entry exception when you were not expecting one.

The reason why this is happening is because there is nothing actually being inserted, since you get a conflict but your query is then filtered to not update anything.

I would say this for sure shouldn't happen, as the stale entry error is semantically invalid. Maybe this is a good opportunity to remove the Ecto.StaleEntryError in favor of adding something to the changeset errors. But where? Maybe to the primary key? My concern with adding it to the primary key is that it may be hard to notice on actual applications. We will think about it more.

Thanks!

[skateinmars]

You are getting a stale entry exception when you were not expecting one.

Exactly, sorry this was not clearer before.
The main issue is that the exception is not really explicit in this case.

Maybe this is a good opportunity to remove the Ecto.StaleEntryError in favor of adding something to the changeset errors. But where? Maybe to the primary key?

Being still new to Ecto, I wasn't sure what to suggest as a replacement. An error on the primary does look acceptable.

Thanks for the quick feedback on this!

[josevalim]

We have decided to keep this behaviour but allow it to be disabled via an option. Note it also affects deletes. See #2603.

[josevalim]

Done in #2687.