{:replace_all_except, [:other_field]} with `conflict_target` may raise ArgumentError when :other_field is ONLY other field

[mashton]

Elixir version

Elixir 1.18.3 (compiled with Erlang/OTP 27)

Database and Version

PostgreSQL 16.3

Ecto Versions

3.13.2

Database Adapter and Versions (postgrex, myxql, etc)

3.12.1

Current behavior

1. Create a schema with only :id and :other_field.

defmodule EctoTest.TestSchema do
  use Ecto.Schema
  import Ecto.Query

  @primary_key {:id, :binary_id, autogenerate: true}
  @foreign_key_type :binary_id
  schema "test_schema" do
    field :other_field, :string
  end

  def break() do
    query =
      __MODULE__
      |> select([ts], %{
        id: fragment("gen_random_uuid()"),
        other_field: ts.other_field
      })

    EctoTest.Repo.insert_all(__MODULE__, query,
      on_conflict: {:replace_all_except, [:id]},
      conflict_target: :other_field
    )
  end
end

defmodule EctoTest.Repo.Migrations.AddTestSchema do
  use Ecto.Migration

  def change do
    create table(:test_schema) do
      add(:other_field, :string)
    end
  end
end

2. Try to run insert_all like in EctoTest.TestSchema.break/0 above
3. Note the error:

iex(1)> EctoTest.TestSchema.break()
** (ArgumentError) empty list of fields to update, use the `:replace` option instead
    (ecto 3.13.2) lib/ecto/repo/schema.ex:927: Ecto.Repo.Schema.on_conflict/6
    (ecto 3.13.2) lib/ecto/repo/schema.ex:74: Ecto.Repo.Schema.do_insert_all/7
    iex:1: (file)

Expected behavior

Should this fail? I'll confess I haven't spent a lot of time in this particular space, so the logic of it doesn't come to me quickly.

Why should a conflict_target itself not be replaced? Is this a requirement for HOT updates?

If this is indeed a bug, I wonder if the bug is in https://github.com/elixir-ecto/ecto/blob/cd0f70b4cdd949767ea7cbe7d635e70917384b38/lib/ecto/repo/schema.ex#L924:

-- to_remove = List.wrap(conflict_target) ++ fields
++ to_remove = fields -- List.wrap(conflict_target) 

Such that we ensure that the conflict_target is always in the list of fields that get replaced. I wonder if to_remove is misleading here, since it seems to suggest that it's the list of fields to "replace", but in fact it's the list of fields to remove from the list of fields to replace.

Is this all accurate, or have I missed something?

[josevalim]

Yes, this is intentional, as it allows us to apply some optimizations, but we should improve the error message. Using the :replace option is the way to go.

[mashton]

Thanks!

What if I want to grow my TestSchema with new fields, but always want to replace_all_except: [:id]? One would need to remember to add all the new fields.

[greg-rychlewski]

I think the only way to do that right now is to utilize the reflection API. You would need a helper function that uses these: https://hexdocs.pm/ecto/Ecto.Schema.html#module-reflection

[greg-rychlewski]

@josevalim Maybe we need to make __schema__(:updatable_fields) public for this

defp replace_all_fields!(_kind, schema, to_remove) do
    {updatable_fields, _} = schema.__schema__(:updatable_fields)
    Enum.map(updatable_fields -- to_remove, &field_source!(schema, &1))
  end

[mashton]

That would mean there are two recommended ways to support this requirement (if it is intended to be supported):
replace: [:other_field], conflict_target: :other_field
Once your schema grows beyond :id and :other_field, then it ought to be safe to switch it to:
replace_all_except: [:id], conflict_target: :other_field

or instead, as @greg-rychlewski suggests:

replace_all_except: some_introspection(), conflict_target: :other_field

Regardless, I wonder if this is accidentally a breaking change, albeit for an obscure case. Alternatively, if Ecto doesn't want to support this case I suppose it could benefit from documentation?

[josevalim]

Perhaps we should add a new option, like discard_changed then?

[mashton]

Not sure I grok it. Like this?

    EctoTest.Repo.insert_all(__MODULE__, query,
      conflict_target: :other_field,
      discard_changed: [:id]
    )

where:

{:discard_changed, fields} ->
  to_remove = fields # don't remove anything else

[josevalim]

 on_conflict: {:replace_all_except, [:id]},
 replace_changed: true