Revise apply_changes/1 for nested schemaless changesets

[azizk]

Elixir version

1.16.3

Database and Version

13.16

Ecto Versions

3.12.3

Database Adapter and Versions (postgrex, myxql, etc)

0.19.1

Current behavior

This is related to the issue #4514 and the merged PR #4516.

I wanted to further discuss three things:

1. Missing case: when the type is {:map, _} the schemaless changeset is not converted.
2. The nested schemaless changeset is converted to an atoms key map (instead of a strings key map).
3. We're not checking if the changeset is actually a schemaless changeset.

Expected behavior

1. {:map, _} types should be handled as well.
2. A map with string keys should be returned, as that's what is most likely expected by code working with these schemas. The Ecto field type says it's a map with string keys. The data should be consistent with the type. It shouldn't be a strings map in some cases and an atoms map in others.
3. Should we check?

I'd like to propose to alter apply_changes/1 like this:

def apply_changes(%Changeset{changes: changes, data: data, types: types}) do
  Enum.reduce(changes, data, fn {key, value}, acc ->
    case Map.fetch(types, key) do
      {:ok, {tag, relation}} when tag in @relations ->
        apply_relation_changes(acc, key, relation, value)

      {:ok, type} ->
        schemaless? = is_struct(value, Changeset) and not is_struct(value.data)

        value =
          if schemaless? and (type == :map or match?({:map, _}, type)),
            do: apply_changes(value) |> Map.new(fn {k, v} -> {"#{k}", v} end),
            else: value

        Map.put(acc, key, value)

      :error ->
        acc
    end
  end)
end

[josevalim]

I would love @greg-rychlewski but the amount of custom behavior here makes me think we should revert the earlier PR and move towards a custom type.

[greg-rychlewski]

I agree. I was going to let it sit overnight in my head but when I saw this it made me think the last change was too hasty.

[josevalim]

I believe it is alright, it helped us move towards the next step. :) I will revert it now.