Repository navigation
Conversation
ecto_parity_test.exs mirrors test/ecto/changeset/embedded_test.exs from Ecto v3.12.1 for polymorphic embeds. form_parity_test.exs compares the forms of embeds_one and embeds_many with the forms of the polymorphic helpers for the same params. The goal is output parity: for the same input, a polymorphic embed gives the same output as an Ecto embed. Tests for Ecto features that polymorphic embeds do not have are left out. A polymorphic embed keeps a valid child as a struct in the changes, and an Ecto embed keeps a changeset. So the changeset tests leave out the tests that read the changes, the action or the validity of a valid child, and the tests that expect :replace changesets. The form tests compare the form contract: ids, names, errors, row params, hidden inputs, input values, and the data that Ecto.Changeset.get_field/2 gives for each field. Some tests fail on purpose. Each failure shows a difference from Ecto that a later commit fixes.
The changes of PR mathieuprog#132, in one commit: - After the cast, changeset.params holds the params of each child for the field. For a list of embeds, it is a list in the order of the cast, without dropped and ignored rows. The README describes this. - Each child form of a list gets the params of its own row. Before, the first form got the params of all rows, so _persistent_id and other row values were lost. - form.data of a child form is the data of the child, not a changeset. - A child form takes the action of the parent form. A parent without an action hides the errors of the children, and a parent with an action shows them, also for a child changeset without an action. test/polymorphic_embed/html/helpers_test.exs holds regression tests for these cases.
Before, a string or a list for a single embed, or a string, nil or a
list with values that are not maps for a list of embeds, crashed with a
CaseClauseError or a FunctionClauseError. Now the cast adds the same
error as Ecto: {"is invalid", [validation: :embed, type: :map]} or
type {:array, :map}.
nil for a list of embeds is now invalid, as in Ecto. Before, the cast
ignored it. The test "supports lists of polymorphic embeds" sent
contexts2: nil without a reason, so the line is removed. The new test
"nil for a list of embeds is invalid" checks the error for the regular
and the polymorphic schema.
With required: true, the cast now: - adds the field to changeset.required - adds "can't be blank" when the value is nil or an empty list after the cast, also for an existing empty list - puts a change to nil when the params set an existing embed to nil - adds no "can't be blank" when the field already has an error
The cast looked for the row id with params[:id]. Params from a form have string keys, so no row matched. Each existing row became a new :insert, and the row lost all data that the params did not set. Now a row matches when it has the same type and each primary key, cast to its type, is equal to the param value. This is the rule that Ecto uses, so "1" matches the integer id 1. With nilify_unlisted_types_on_load or retain_unlisted_types_on_load, a loaded list can hold nil or plain maps. These entries match no params. A new test checks such a list.
Ecto raises an ArgumentError when on_replace has a value that the embed does not accept. The polymorphic embed raised a RuntimeError. The messages do not change.
The cast crashed with a CaseClauseError for an embedded schema with an autogenerated integer id, because Ecto 3.12 returns a 3-tuple from __schema__(:autogenerate_id). Now the cast keeps the id nil, as Ecto does. dump/3 raises the same ArgumentError as Ecto when the id is still nil on insert.
phoenix_ecto puts the primary key of the data into form.hidden, so that the form sends the id back. The polymorphic form helper put only the type field there. Without the id, the cast could not match the row of the data and built a new row. Now a child form gets the primary key when the parent data holds the row. A new row gets no id in an Ecto form, so its form gets none, also when the cast generated one. All form parity tests pass.
With use_parent_field_for_type, the params of the nested embed have no type key. get_polymorphic_type/2 reads form[field].value, which is the raw param map, so it returns nil. The test checks a single embed, a list of rows and an index map of rows, as an HTML form sends them. The test fails. The next commit fixes it.
get_polymorphic_type/2 read form[field].value: the change, else the param, else the data. Each child form now holds the params of its own row. So for a nested embed the value is the raw param map. With use_parent_field_for_type, that map has no type key, and the function returned nil. Now the function reads Ecto.Changeset.get_field/2 first, and uses the input value only when the field has no data. After a cast, the cast data gives the type also when the params have no type key.
The changelog lists the breaking changes and the fixes of the cast and of the form helpers. It also says how to read the data of a nested embed in a child form: with Ecto.Changeset.get_field/2, because form[:field].value gives the value for an input.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It's me again :P
I've been tinkering around a lot with this, so sorry for the opened/closed PR mess I created.
While I thought it might be wise to achieve full Ecto parity I noticed there are some downsides to that.
E.g. Ecto itself does not create id's when applying changes, instead
Repo.insertdoes it.But
Repo.inserthas ways to handle embeds, of course it has no way to handle polymorphic_embeds.So this is the minimal approach I went for. It tries to reproduce the behaviour of Ecto where it made sense to me.
Most notably:
:idwe match rows by the primary_key of the embedThis PR contains breaking changes so it is a candidate for a major release.
I will test this in our prod app soon.
This also makes my other to PRs obsolete:
#132
#145
Thank you