From efc8cb929e7eb812826cb11cb5e15b987bbfce1b Mon Sep 17 00:00:00 2001
From: Nick <nick.duitz@9elements.com>
Date: Wed, 21 Oct 2026 16:21:49 +0200
Subject: [PATCH] Normalize child parameters before matching existing embeds

---
 README.md                              |   8 +-
 lib/polymorphic_embed.ex               |  32 ++-
 test/polymorphic_embed/params_test.exs | 294 +++++++++++++++++++++++++
 3 files changed, 331 insertions(+), 3 deletions(-)
 create mode 100644 test/polymorphic_embed/params_test.exs

diff --git a/README.md b/README.md
index 28c7bfc..76ab296 100644
--- a/README.md
+++ b/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
diff --git a/lib/polymorphic_embed.ex b/lib/polymorphic_embed.ex
index 47e8ad9..e85b9f2 100644
--- a/lib/polymorphic_embed.ex
+++ b/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
diff --git a/test/polymorphic_embed/params_test.exs b/test/polymorphic_embed/params_test.exs
new file mode 100644
index 0000000..17f95df
--- /dev/null
+++ b/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