Normalize child parameters before matching existing embeds

README.md

@@ -104,7 +104,7 @@ add(:channel, :map)
 
 * `:required` – if the embed is a required field.
 
-* `:with` – allows you to specify a custom changeset.
+* `:with` – allows you to specify a custom changeset. Callbacks receive string-keyed parameters, including when callers supply atom-keyed parameters.
 ```elixir
 changeset
 |> cast_polymorphic_embed(:channel,
@@ -121,6 +121,12 @@ changeset
 
 * `:default_type_on_sort_create` – in some cases, [`sort` creates a new entry](https://github.com/elixir-ecto/ecto/blob/v3.11/test/ecto/changeset/embedded_test.exs#L464); this option specifies which type to use by default for the entry.
 
+#### Parameter keys and existing children
+
+Each child parameter map may use atom keys or string keys. The caster converts its keys to strings before type selection, ID matching, and changeset callbacks. Maps with mixed key types raise `Ecto.CastError`. Nested values remain unchanged.
+
+For `polymorphic_embeds_many`, a matching non-nil `id` and type select the existing child as the changeset's data. This lets child changeset functions retain fields omitted from the submitted parameters. Nested embeds follow their configured `on_replace` rules.
+
 ### PolymorphicEmbed Ecto type
 
 The `:types` option for the `PolymorphicEmbed` custom type contains a keyword list mapping an atom representing the type

lib/polymorphic_embed.ex

@@ -279,6 +279,31 @@ defmodule PolymorphicEmbed do
 
   defp key_as_int(key_val), do: key_val
 
+  defp normalize_params(params) when is_map(params) and not is_struct(params) do
+    keys = Map.keys(params)
+
+    cond do
+      Enum.all?(keys, &is_binary/1) ->
+        params
+
+      Enum.all?(keys, &is_atom/1) ->
+        Map.new(params, fn {key, value} -> {Atom.to_string(key), value} end)
+
+      true ->
+        raise_invalid_params(params)
+    end
+  end
+
+  defp normalize_params(params), do: raise_invalid_params(params)
+
+  defp raise_invalid_params(params) do
+    raise Ecto.CastError,
+      type: :map,
+      value: params,
+      message:
+        "expected polymorphic embed params to be a map with only atom keys or only string keys, got: #{inspect(params)}"
+  end
+
   defp changeset_fun(struct, params, with, types_metadata) when is_list(with) do
     type = do_get_polymorphic_type(struct, types_metadata)
 
@@ -300,6 +325,7 @@ defmodule PolymorphicEmbed do
 
   defp cast_polymorphic_embeds_one(changeset, field, changeset_fun, params, field_opts) do
     %{on_type_not_found: on_type_not_found} = field_opts
+    params = normalize_params(params)
 
     data_for_field = Map.fetch!(changeset.data, field)
 
@@ -342,7 +368,7 @@ defmodule PolymorphicEmbed do
     } = field_opts
 
     if parent_field_for_type != nil do
-      type_from_map = Attrs.get(params, type_field_name)
+      type_from_map = Map.get(params, to_string(type_field_name))
       type_from_parent_field = Ecto.Changeset.fetch_field!(changeset, parent_field_for_type)
 
       cond do
@@ -404,6 +430,8 @@ defmodule PolymorphicEmbed do
 
     embeds =
       Enum.map(list_params, fn params ->
+        params = normalize_params(params)
+
         case get_polymorphic_module_from_map(params, type_field_name, types_metadata) do
           nil when on_type_not_found == :raise ->
             raise_cannot_infer_type_from_data(params)
@@ -418,7 +446,7 @@ defmodule PolymorphicEmbed do
             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
+                  id == params["id"] and datum.__struct__ == module
 
                 _ ->
                   nil

test/polymorphic_embed/params_test.exs

@@ -0,0 +1,294 @@
+defmodule PolymorphicEmbed.ParamsTest do
+  use ExUnit.Case, async: true
+
+  alias Ecto.Changeset
+
+  defmodule Part do
+    use Ecto.Schema
+
+    embedded_schema do
+      field(:body, :string)
+    end
+
+    def changeset(data, params), do: Changeset.cast(data, params, [:body])
+  end
+
+  defmodule Story do
+    use Ecto.Schema
+
+    embedded_schema do
+      field(:title, :string)
+      field(:media_file, :map, virtual: true)
+      field(:payload, :map)
+      embeds_many(:parts, Part, on_replace: :delete)
+    end
+
+    def changeset(data, params) do
+      data
+      |> Changeset.cast(params, [:title, :payload])
+      |> Changeset.validate_required(:title)
+      |> Changeset.cast_embed(:parts)
+    end
+  end
+
+  defmodule Other do
+    use Ecto.Schema
+
+    embedded_schema do
+      field(:title, :string)
+      field(:media_file, :map, virtual: true)
+    end
+
+    def changeset(data, params), do: Changeset.cast(data, params, [:title])
+  end
+
+  defmodule RestrictedStory do
+    use Ecto.Schema
+
+    embedded_schema do
+      embeds_many(:parts, Part)
+    end
+
+    def changeset(data, params) do
+      data
+      |> Changeset.cast(params, [])
+      |> Changeset.cast_embed(:parts)
+    end
+  end
+
+  defmodule Content do
+    use Ecto.Schema
+    import PolymorphicEmbed
+
+    embedded_schema do
+      polymorphic_embeds_many(:children,
+        types: [story: Story, other: Other, restricted: RestrictedStory],
+        on_replace: :delete
+      )
+
+      polymorphic_embeds_one(:featured,
+        types: [story: Story],
+        type_field_name: :kind,
+        on_replace: :update
+      )
+    end
+  end
+
+  for key_type <- [:atoms, :strings], shape <- [:list, :indexed] do
+    test "preserves existing data with #{key_type} as parameter keys in a #{shape}" do
+      existing = story()
+      row = %{__type__: "story", id: existing.id, title: "updated"}
+      row = if unquote(key_type) == :strings, do: stringify(row), else: row
+      rows = if unquote(shape) == :list, do: [row], else: %{"0" => row}
+
+      changeset = cast(%Content{children: [existing]}, %{children: rows}, :children)
+
+      assert [%Story{} = child] = changeset.changes.children
+      assert child.id == existing.id
+      assert child.media_file == existing.media_file
+      assert child.title == "updated"
+    end
+  end
+
+  for key_type <- [:atoms, :strings] do
+    test "passes normalized parameters and existing data to with callbacks for #{key_type} keys" do
+      existing = story()
+      payload = %{keep_atom_key: %{nested: "unchanged"}}
+      row = %{__type__: "story", id: existing.id, title: "updated", payload: payload}
+      expected = stringify(row)
+      row = if unquote(key_type) == :strings, do: expected, else: row
+
+      callback = fn data, params ->
+        assert data == existing
+        assert params == expected
+        Story.changeset(data, params)
+      end
+
+      changeset =
+        cast(%Content{children: [existing]}, %{children: [row]}, :children,
+          with: [story: callback]
+        )
+
+      assert [child] = changeset.changes.children
+      assert child.payload == payload
+      assert child.media_file == existing.media_file
+    end
+  end
+
+  test "retains the existing child and update action when validation fails" do
+    existing = story()
+
+    changeset =
+      cast(
+        %Content{children: [existing]},
+        %{"children" => [%{"__type__" => "story", "id" => existing.id, "title" => ""}]},
+        :children
+      )
+
+    assert [%Changeset{} = child] = changeset.changes.children
+    assert child.data == existing
+    assert child.action == :update
+    refute child.valid?
+    assert Keyword.has_key?(child.errors, :title)
+  end
+
+  test "normalizes single embed parameters before a custom changeset function" do
+    existing = story()
+    row = %{kind: "story", id: existing.id, title: "updated"}
+
+    callback = fn data, params ->
+      assert data == existing
+      assert params == stringify(row)
+      Story.changeset(data, params)
+    end
+
+    changeset =
+      cast(%Content{featured: existing}, %{featured: row}, :featured, with: [story: callback])
+
+    assert changeset.changes.featured.media_file == existing.media_file
+    assert changeset.changes.featured.title == "updated"
+  end
+
+  test "normalizes rows created by sort before the changeset callback" do
+    callback = fn data, params ->
+      assert params == %{"__type__" => :story}
+      Story.changeset(data, params)
+    end
+
+    changeset =
+      cast(%Content{}, %{order: ["new"]}, :children,
+        sort_param: :order,
+        default_type_on_sort_create: :story,
+        with: [story: callback]
+      )
+
+    assert [%Changeset{action: :insert}] = changeset.changes.children
+  end
+
+  for field <- [:children, :featured],
+      row <- [
+        %{"__type__" => "story", "kind" => "story", title: "mixed"},
+        %{"__type__" => "story", "kind" => "story", "id" => "one", id: "two"},
+        %{"__type__" => "story", "kind" => "story", 1 => "invalid key"}
+      ] do
+    test "rejects invalid parameter keys for #{field}: #{inspect(row)}" do
+      row = unquote(Macro.escape(row))
+      value = if unquote(field) == :children, do: [row], else: row
+
+      callback = fn _data, _params ->
+        flunk("Invalid keys must not reach the changeset callback")
+      end
+
+      assert_raise Ecto.CastError, fn ->
+        cast(%Content{}, %{unquote(field) => value}, unquote(field), with: [story: callback])
+      end
+    end
+  end
+
+  test "requires both ID and module to match an existing child" do
+    existing = story()
+
+    changeset =
+      cast(
+        %Content{children: [existing]},
+        %{"children" => [%{"__type__" => "other", "id" => existing.id, "title" => "new"}]},
+        :children
+      )
+
+    assert [%Other{media_file: nil, title: "new"}] = changeset.changes.children
+  end
+
+  test "does not match existing children without an ID" do
+    existing = %{story() | id: nil}
+
+    changeset =
+      cast(
+        %Content{children: [existing]},
+        %{children: [%{__type__: "story", title: "new"}]},
+        :children
+      )
+
+    assert [%Story{media_file: nil} = child] = changeset.changes.children
+    assert child.id
+  end
+
+  test "preserves child data through sorting and dropping" do
+    first = story()
+    second = %{story() | media_file: %{path: "second-file"}}
+    dropped = story()
+
+    params = %{
+      "children" => %{
+        "0" => %{"__type__" => "story", "id" => first.id, "title" => "first"},
+        "1" => %{"__type__" => "story", "id" => second.id, "title" => "second"},
+        "2" => %{"__type__" => "story", "id" => dropped.id, "title" => "dropped"}
+      },
+      "order" => ["1", "2", "0"],
+      "remove" => ["2"]
+    }
+
+    changeset =
+      cast(%Content{children: [first, second, dropped]}, params, :children,
+        sort_param: :order,
+        drop_param: :remove
+      )
+
+    assert Enum.map(changeset.changes.children, &{&1.id, &1.media_file}) == [
+             {second.id, second.media_file},
+             {first.id, first.media_file}
+           ]
+  end
+
+  test "updates, reorders and removes nested embeds on existing data" do
+    first = %Part{id: Ecto.UUID.generate(), body: "first"}
+    second = %Part{id: Ecto.UUID.generate(), body: "second"}
+    dropped = %Part{id: Ecto.UUID.generate(), body: "dropped"}
+    existing = %{story() | parts: [first, second, dropped]}
+
+    params = %{
+      "children" => [
+        %{
+          "__type__" => "story",
+          "id" => existing.id,
+          "parts" => [%{"id" => second.id, "body" => "updated"}, %{"id" => first.id}]
+        }
+      ]
+    }
+
+    changeset = cast(%Content{children: [existing]}, params, :children)
+    assert [%Story{} = child] = changeset.changes.children
+    assert child.media_file == existing.media_file
+
+    assert Enum.map(child.parts, &{&1.id, &1.body}) == [
+             {second.id, "updated"},
+             {first.id, "first"}
+           ]
+  end
+
+  test "honors nested on_replace restrictions on existing data" do
+    existing = %RestrictedStory{
+      id: Ecto.UUID.generate(),
+      parts: [%Part{id: Ecto.UUID.generate(), body: "kept"}]
+    }
+
+    params = %{
+      "children" => [%{"__type__" => "restricted", "id" => existing.id, "parts" => []}]
+    }
+
+    assert_raise RuntimeError, ~r/on_replace/, fn ->
+      cast(%Content{children: [existing]}, params, :children)
+    end
+  end
+
+  defp story do
+    %Story{id: Ecto.UUID.generate(), title: "original", media_file: %{path: "existing-file"}}
+  end
+
+  defp stringify(params), do: Map.new(params, fn {key, value} -> {Atom.to_string(key), value} end)
+
+  defp cast(data, params, field, options \\ []) do
+    data
+    |> Changeset.cast(params, [])
+    |> PolymorphicEmbed.cast_polymorphic_embed(field, options)
+  end
+end