Allow defining foreign key constraints for on_replace: delete / inputs_for use cases

[Tobias Pfeiffer]

Hello ecto team & friends,

first of all thanks for all your work 💚 I love elixir, but ecto is by far the one I enjoy using the most and the one that I constantly bring up talking about why I'm happy working in elixir.

Problem Setup

(running ecto 3.12.4 against Postgres)

Roughly, I have:

* a container schema ("brainstorming" in the code)

* has_many :labels, on_replace: :delete

* custom changesets in the cast_assoc (and sort_params and drop_params)

* those labels have foreign key relationship with "ideas" --> can't delete a label that is still associated with a label

* this foreign key constraint is established in the changeset given to cast_assoc

Now when the drop_params are set in a way that a label which still has an association to an an idea is deleted, the whole thing blows up:

1) test freaking relationships and on_replace stuff oof (Mindwendel.Brainstormings.BrainstormingTest)
test/mindwendel/brainstormings/brainstorming_test.exs:11
** (Ecto.ConstraintError) constraint error when attempting to delete struct:

* "idea_idea_labels_idea_label_id_fkey" (foreign_key_constraint)

If you would like to stop this constraint violation from raising an
exception and instead add it as an error to your changeset, please
call `foreign_key_constraint/3` on your changeset with the constraint
`:name` as an option.

The changeset has not defined any constraint.

code: assert {:ok, new_brain} = Repo.update(changeset)
stacktrace:
(ecto 3.12.4) lib/ecto/repo/schema.ex:885: anonymous fn/4 in Ecto.Repo.Schema.constraints_to_errors/3
(elixir 1.15.8) lib/enum.ex:1693: Enum."-map/2-lists^map/1-1-"/2
(ecto 3.12.4) lib/ecto/repo/schema.ex:869: Ecto.Repo.Schema.constraints_to_errors/3
(ecto 3.12.4) lib/ecto/repo/schema.ex:844: Ecto.Repo.Schema.apply/4
(ecto 3.12.4) lib/ecto/repo/schema.ex:614: anonymous fn/13 in Ecto.Repo.Schema.do_delete/4
(ecto 3.12.4) lib/ecto/association.ex:973: Ecto.Association.Has.on_repo_change/5
(ecto 3.12.4) lib/ecto/association.ex:958: Ecto.Association.Has.on_repo_change/5
(ecto 3.12.4) lib/ecto/association.ex:708: Ecto.Association.on_repo_change_unless_halted/7
(ecto 3.12.4) lib/ecto/association.ex:660: anonymous fn/8 in Ecto.Association.on_repo_change/7
(elixir 1.15.8) lib/enum.ex:2510: Enum."-reduce/3-lists^foldl/2-0-"/3
(ecto 3.12.4) lib/ecto/association.ex:649: Ecto.Association.on_repo_change/7
(elixir 1.15.8) lib/enum.ex:2510: Enum."-reduce/3-lists^foldl/2-0-"/3
(ecto 3.12.4) lib/ecto/association.ex:590: Ecto.Association.on_repo_change/4
(ecto 3.12.4) lib/ecto/repo/schema.ex:1030: Ecto.Repo.Schema.process_children/5
(ecto 3.12.4) lib/ecto/repo/schema.ex:1108: anonymous fn/3 in Ecto.Repo.Schema.wrap_in_transaction/6
(ecto_sql 3.12.1) lib/ecto/adapters/sql.ex:1400: anonymous fn/3 in Ecto.Adapters.SQL.checkout_or_transaction/4
(db_connection 2.7.0) lib/db_connection.ex:1756: DBConnection.run_transaction/4
test/mindwendel/brainstormings/brainstorming_test.exs:53: (test)

Confusingly it says that the changeset doesn't specify any constraints, but it does (they just get dropped, as I'll get into soon).

I have not yet created a minimized example of this, but the repo I encountered this in is open source and I wrote a fairly isolated test there which can be found here: https://github.com/b310-digital/mindwendel/pull/466/files#diff-75abca38573cb1eedeae70134ed6dd76dfec31b81f7d649bc12c961c76e5e216

(there's another more full stack test involving LiveView that also faills but yeah)

Analysis

To the best of my understanding as the record is supposed to be deleted (via drop_params) the cast_assoc is basically creating a new changeset by which we lose the changesets' defined constraints leading to the blow up.

In the following code our changeset is referred to as on_cast as we get it from the with option (or default, for us with) and then passed on to Relation.cast

https://github.com/elixir-ecto/ecto/blob/892dc85ed4bdae4b83218693ef6dc3726da80252/lib/ecto/changeset.ex#L1272-L1283

on_cast = Keyword.get_lazy(opts, :with, fn -> on_cast_default(type, related) end)

sort = opts_key_from_params(:sort_param, opts, params)

drop = opts_key_from_params(:drop_param, opts, params)

changeset =

if is_map_key(params, param_key) or is_list(sort) or is_list(drop) do

value = Map.get(params, param_key)

original = Map.get(data, key)

current = Relation.load!(data, original)

value = cast_params(relation, value, sort, drop) |> dbg()

case Relation.cast(relation, data, value, current, on_cast) do

However, Relation.cast then uses it to create a function based on do_cast which uses on_cast (which includes the constraints, as this is the changeset we had), which is used further down the line:

https://github.com/elixir-ecto/ecto/blob/892dc85ed4bdae4b83218693ef6dc3726da80252/lib/ecto/changeset/relation.ex#L108

def cast(%{related: mod} = relation, owner, params, current, on_cast) do

pks = mod.__schema__(:primary_key)

fun = &do_cast(relation, owner, &1, &2, &3, &4, on_cast)

 

But in the cast of removing the element (via drop_params) do_cast calls on_replace - but this one drops the context of on_cast wholly by ignoring it and hence losing the constraints:

https://github.com/elixir-ecto/ecto/blob/892dc85ed4bdae4b83218693ef6dc3726da80252/lib/ecto/changeset/relation.ex#L136-L138

defp do_cast(relation, _owner, nil = _params, current, _allowed_actions, _idx, _on_cast) do

on_replace(relation, current)

end

This means we end up with a changeset that has no foreign keys defined and hence the Repo.update fails loudly.

Wish/Desired Behavior

I understand why the original changeset is discarded for replace/delete - after all we don't need to validate it. But dropping the constraints doesn't seem ideal (and a bit confusing given the error messages).

My ideas for a fix would be the following:

* carry on changeset defined constraints even in the replace case (as in, create a new changeset, but copy constraints over) - this is the behavior I'd expect esp. given the error message

* add a an option to associations for delete behavior for which we can specify constraints that should be applied to them before deletion

That all said, I'm no ecto maintainer and these are my first ideas. Maybe there's also already another solution to this problem that I failed to find?

Thank you all for your help & work!

Tobi

PS: Sorry for the formatting,  I miss my markdown in email!

[José Valim]

Thank you for the well written email.

We may be able to carry the changeset altogether, as long as action is set to :delete. That's what I would try first and a PR is very welcome! :) If not, we can consider copying constraints.

José Valim

https://dashbit.co/

--
You received this message because you are subscribed to the Google Groups "elixir-ecto" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-ecto...@googlegroups.com.
To view this discussion visit https://groups.google.com/d/msgid/elixir-ecto/6b09c13d-c306-4cad-bac4-ab6d20b03c6en%40googlegroups.com.

[Tobias Pfeiffer]

Thanks for the quick answer José! I'll probably give it a shot :) I assumed there was a reason for not carrying the whole changeset but I guess the test suite shall reveal whether it's possible or not :)

Cheers,

Tobi

To view this discussion visit https://groups.google.com/d/msgid/elixir-ecto/CAGnRm4%2BbTvk0g%2BZArgWjmFiSoQUfmUKhZd7Fp6eQ1kbtHUXTOg%40mail.gmail.com.