Fix embeds cast and forms

CHANGELOG.md

@@ -1,5 +1,44 @@
 # Changelog
 
+## Unreleased
+
+  **Breaking changes**:
+
+  * `nil` for a list of embeds gives the error "is invalid", as in Ecto. Before, the cast
+    ignored it.
+  * With `required: true`, `changeset.required` contains the field, and an empty list in
+    the data gives "can't be blank", as in Ecto.
+  * An invalid `:on_replace` option raises `ArgumentError`, not `RuntimeError`.
+  * 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 form of an invalid child takes its params from the child changeset, as in
+    phoenix_ecto. Before, it took the params of the parent by index. When a `with:` function
+    builds the changeset without `cast/4`, the changeset has no params, and the form gets
+    `%{}`. Use `cast/4` in a `with:` function, so that the form keeps the params of the row.
+  * The hidden inputs of a child form hold the primary key of a row that the data holds,
+    as in phoenix_ecto, so the form sends the id back.
+
+  **Fixes**:
+
+  * Params that are not maps give the error "is invalid", as in Ecto. Before, the cast
+    crashed.
+  * A list of embeds matches the rows of the data by primary key, as in Ecto. Before, params
+    with string keys matched no row, so each row got a new id and lost the data that the
+    params did not set.
+  * An embedded schema with an autogenerated integer id no longer crashes the cast.
+    `dump/3` raises the same `ArgumentError` as Ecto when the id is still `nil` on insert.
+  * Each child form of a list gets the params of its own row. Before, the first form got
+    the params of all rows.
+  * A child form takes the action of the parent form, so it shows the errors of an invalid
+    child, and `form.data` is the data of the child, not a changeset.
+  * `get_polymorphic_type/2` of the HTML helpers reads the type from the cast data first,
+    then from the input value. Before, it returned `nil` for a nested embed when the params
+    had no type key, for example with `:use_parent_field_for_type`.
+
+  `form[:field].value` gives the value for an input: the change, else the param, else the
+  data. For a nested embed of a child form, read the data with
+  `Ecto.Changeset.get_field(form.source, field)`.
+
 ## 5.0.2
 
   * Remove warning when aliases cannot be expanded (#123).

README.md

@@ -100,6 +100,8 @@ add(:channel, :map)
 
 `cast_polymorphic_embed/3` must be called to cast the polymorphic embed's parameters.
 
+After casting a polymorphic embed, its entry in `changeset.params` contains the child changeset's parameters. For `polymorphic_embeds_many`, this is a list in the final cast order, with dropped and ignored rows removed. Form helpers use these parameters to preserve submitted values and row identifiers.
+
 #### Options
 
 * `:required` – if the embed is a required field.

lib/polymorphic_embed.ex

@@ -79,9 +79,8 @@ defmodule PolymorphicEmbed do
     OptionsValidator.validate!(opts)
 
     if Keyword.get(opts, :on_replace) not in [:update, :delete] do
-      raise(
-        "`:on_replace` option for polymorphic embed must be set to `:update` (single embed) or `:delete` (list of embeds)"
-      )
+      raise ArgumentError,
+            "`:on_replace` option for polymorphic embed must be set to `:update` (single embed) or `:delete` (list of embeds)"
     end
 
     types_metadata =
@@ -125,7 +124,7 @@ defmodule PolymorphicEmbed do
 
     raise_if_invalid_options(field, field_opts)
 
-    %{array?: array?, types_metadata: types_metadata} = field_opts
+    %{types_metadata: types_metadata} = field_opts
 
     required = Keyword.get(cast_opts, :required, false)
     with = Keyword.get(cast_opts, :with, nil)
@@ -136,6 +135,29 @@ defmodule PolymorphicEmbed do
     sort = param_value_for_cast_opt(:sort_param, cast_opts, changeset.params)
     drop = param_value_for_cast_opt(:drop_param, cast_opts, changeset.params)
 
+    changeset =
+      if required, do: update_in(changeset.required, &[field | &1]), else: changeset
+
+    changeset
+    |> do_cast_polymorphic_embed(field, cast_opts, field_opts, changeset_fun, sort, drop)
+    |> maybe_add_required_error(field, required)
+  end
+
+  def cast_polymorphic_embed(_, _, _) do
+    raise "cast_polymorphic_embed/3 only accepts a changeset as first argument"
+  end
+
+  defp do_cast_polymorphic_embed(
+         changeset,
+         field,
+         cast_opts,
+         field_opts,
+         changeset_fun,
+         sort,
+         drop
+       ) do
+    %{array?: array?} = field_opts
+
     case Map.fetch(changeset.params || %{}, to_string(field)) do
       # consider sort and drop params even if the assoc param was not given, as in Ecto
       :error when (array? and is_list(sort)) or is_list(drop) ->
@@ -150,26 +172,23 @@ defmodule PolymorphicEmbed do
           field_opts
         )
 
-      :error when required ->
-        if data_for_field = Map.fetch!(changeset.data, field) do
-          data_for_field = autogenerate_id(data_for_field, changeset.action)
-          Ecto.Changeset.put_change(changeset, field, data_for_field)
-        else
-          Ecto.Changeset.add_error(changeset, field, "can't be blank", validation: :required)
-        end
-
-      :error when not required ->
+      :error ->
         if data_for_field = Map.fetch!(changeset.data, field) do
           data_for_field = autogenerate_id(data_for_field, changeset.action)
           Ecto.Changeset.put_change(changeset, field, data_for_field)
         else
           changeset
         end
 
-      {:ok, nil} when required ->
-        Ecto.Changeset.add_error(changeset, field, "can't be blank", validation: :required)
+      {:ok, params_for_field}
+      when array? and not is_list(params_for_field) and not is_map(params_for_field) ->
+        add_invalid_error(changeset, field, array?)
 
-      {:ok, nil} when not required ->
+      {:ok, params_for_field}
+      when not array? and not is_map(params_for_field) and not is_nil(params_for_field) ->
+        add_invalid_error(changeset, field, array?)
+
+      {:ok, nil} ->
         Ecto.Changeset.put_change(changeset, field, nil)
 
       {:ok, map} when map == %{} and not array? ->
@@ -198,8 +217,16 @@ defmodule PolymorphicEmbed do
     end
   end
 
-  def cast_polymorphic_embed(_, _, _) do
-    raise "cast_polymorphic_embed/3 only accepts a changeset as first argument"
+  # As in Ecto, a required embed must not be nil or an empty list after the cast.
+  defp maybe_add_required_error(changeset, _field, false), do: changeset
+
+  defp maybe_add_required_error(changeset, field, true) do
+    if Keyword.has_key?(changeset.errors, field) or
+         Ecto.Changeset.get_field(changeset, field) not in [nil, []] do
+      changeset
+    else
+      Ecto.Changeset.add_error(changeset, field, "can't be blank", validation: :required)
+    end
   end
 
   defp sort_create(%{sort_param: _} = cast_opts, field_opts) do
@@ -319,6 +346,7 @@ defmodule PolymorphicEmbed do
       {action, struct} ->
         embed_changeset = changeset_fun.(struct, params)
         embed_changeset = %{embed_changeset | action: action}
+        changeset = put_embed_params(changeset, field, embed_changeset.params || params)
 
         case embed_changeset do
           %{valid?: true} = embed_changeset ->
@@ -394,6 +422,36 @@ defmodule PolymorphicEmbed do
   end
 
   defp cast_polymorphic_embeds_many(changeset, field, changeset_fun, list_params, field_opts) do
+    if Enum.all?(list_params, &is_map/1) do
+      do_cast_polymorphic_embeds_many(changeset, field, changeset_fun, list_params, field_opts)
+    else
+      add_invalid_error(changeset, field, true)
+    end
+  end
+
+  # As in Ecto, the params match a row when each primary key, cast to its
+  # type, is equal to the primary key of the row.
+  defp same_pk?(%module{} = datum, params) do
+    pks = module.__schema__(:primary_key)
+
+    pks != [] and
+      Enum.all?(pks, fn pk ->
+        value = Map.get(datum, pk)
+
+        case Ecto.Type.cast(module.__schema__(:type, pk), Attrs.get(params, pk)) do
+          {:ok, param_value} -> not is_nil(value) and param_value == value
+          _ -> false
+        end
+      end)
+  end
+
+  # The same error as Ecto gives for params that are not a map (or a list of maps).
+  defp add_invalid_error(changeset, field, array?) do
+    type = if array?, do: {:array, :map}, else: :map
+    Ecto.Changeset.add_error(changeset, field, "is invalid", validation: :embed, type: type)
+  end
+
+  defp do_cast_polymorphic_embeds_many(changeset, field, changeset_fun, list_params, field_opts) do
     %{
       types_metadata: types_metadata,
       on_type_not_found: on_type_not_found,
@@ -415,14 +473,13 @@ defmodule PolymorphicEmbed do
             :ignore
 
           module ->
+            # With nilify_unlisted_types_on_load or retain_unlisted_types_on_load,
+            # the loaded list can hold nil or plain maps. They match no params.
             data_for_field =
-              Enum.find(list_data_for_field, fn
-                %{id: id} = datum when not is_nil(id) ->
-                  id == params[:id] and datum.__struct__ == module
-
-                _ ->
-                  nil
-              end)
+              Enum.find(
+                list_data_for_field,
+                &(is_struct(&1, module) and same_pk?(&1, params))
+              )
 
             embed_changeset =
               if data_for_field do
@@ -431,21 +488,27 @@ defmodule PolymorphicEmbed do
                 %{changeset_fun.(struct(module), params) | action: :insert}
               end
 
-            maybe_apply_changes(embed_changeset)
+            {maybe_apply_changes(embed_changeset), embed_changeset.params || params}
         end
       end)
 
     if Enum.any?(embeds, &(&1 == :error)) do
       Ecto.Changeset.add_error(changeset, field, "is invalid")
     else
-      embeds = Enum.filter(embeds, &(&1 != :ignore))
+      {embeds, params} =
+        embeds
+        |> Enum.reject(&(&1 == :ignore))
+        |> Enum.unzip()
 
       any_invalid? =
         Enum.any?(embeds, fn
           %{valid?: false} -> true
           _ -> false
         end)
 
+      # Keep each row's parameters in cast order, including children converted to structs.
+      changeset = put_embed_params(changeset, field, params)
+
       changeset = Ecto.Changeset.put_change(changeset, field, embeds)
 
       if any_invalid? do
@@ -464,6 +527,10 @@ defmodule PolymorphicEmbed do
 
   defp maybe_apply_changes(%Changeset{valid?: false} = changeset), do: changeset
 
+  defp put_embed_params(changeset, field, params) do
+    %{changeset | params: Map.put(changeset.params || %{}, to_string(field), params)}
+  end
+
   @impl true
   def cast(_data, _params),
     do:
@@ -533,6 +600,14 @@ defmodule PolymorphicEmbed do
           raise "polymorphic_embed is not able to add an autogenerated key without casting through cast_polymorphic_embed/3"
         end
 
+      {key, _source, :id} ->
+        unless Map.get(struct, key) do
+          raise ArgumentError,
+                "embedded schema `#{inspect(module)}` cannot autogenerate `:id` primary keys, " <>
+                  "those are typically used for auto-incrementing constraints. " <>
+                  "Maybe you meant to use `:binary_id` instead?"
+        end
+
       _ ->
         nil
     end
@@ -649,11 +724,13 @@ defmodule PolymorphicEmbed do
     end
 
     if array? and on_replace != :delete do
-      raise "`:on_replace` option for field #{inspect(field)} must be set to `:delete`"
+      raise ArgumentError,
+            "`:on_replace` option for field #{inspect(field)} must be set to `:delete`"
     end
 
     if not array? and on_replace != :update do
-      raise "`:on_replace` option for field #{inspect(field)} must be set to `:update`"
+      raise ArgumentError,
+            "`:on_replace` option for field #{inspect(field)} must be set to `:update`"
     end
   end
 
@@ -783,8 +860,9 @@ defmodule PolymorphicEmbed do
           schema
         end
 
-      {_key, :id} ->
-        raise("embedded schemas cannot autogenerate `:id` primary keys")
+      # As in Ecto, the cast keeps an integer id. Ecto raises on insert, see dump/3.
+      {_key, _source, :id} ->
+        schema
 
       nil ->
         schema