Fix decimal load from JSON serialized map

[lukaszsamson]

Given schemas

defmodule MySettings do
  use Ecto.Schema
  import Ecto.Changeset

  embedded_schema do
    field : my_decimal_field, :decimal
  end
end

defmodule MyEntity do
  schema "my_entities" do
    field :settings, :map, default: %{}
  end
end

storing & loading MySettings in Postgres JSON column

# store
settings_changeset = %MySettings{}
|> MySettings.changeset(%{"my_decimal_field" => "1.0"}

{:ok, data} = Ecto.Changeset.apply_action(settings_changeset, :update)

my_entity = Repo.get(MyEntity, some_id)
|> Ecto.Changeset.change(%{settings: data |> Map.from_struct()})
|> Repo.update!()

# load
my_entity = Repo.get(MyEntity, some_id)
Repo.load(MySettings, my_entity.settings)

results in

** (ArgumentError) cannot load `"1.0"` as type :decimal for field `my_decimal_field` in schema MySettings

[wojtekmach]

So this fix may solve different use cases, but isn't the proper solution here to:

- field :settings, :map, default: %{}
+ embeds_one :settings, MySettings

?

then it all works correctly.

below is a script that I used to test this out.

Details

defmodule Repo do
  use Ecto.Repo,
    adapter: Ecto.Adapters.Postgres,
    otp_app: :playground
end

defmodule MySettings do
  use Ecto.Schema

  @primary_key false
  embedded_schema do
    field(:my_decimal_field, :decimal)
  end
end

defmodule MyEntity do
  use Ecto.Schema

  schema "my_entities" do
    # field(:settings, :map, default: %{})
    embeds_one :settings, MySettings
  end
end

defmodule Main do
  def main() do
    {:ok, _} = Repo.start_link(database: "wojtek")
    Repo.query!("DROP TABLE IF EXISTS my_entities")
    Repo.query!("CREATE TABLE my_entities (id serial, settings jsonb)")

    import Ecto.Changeset

    settings =
      %MySettings{}
      |> cast(%{"my_decimal_field" => "1.0"}, ~w(my_decimal_field)a)
      |> apply_action!(:insert)

    true = settings.my_decimal_field == Decimal.new("1.0")

    some_id = Repo.insert!(%MyEntity{}).id

    my_entity =
      Repo.get(MyEntity, some_id)
      |> Ecto.Changeset.change(%{settings: Map.from_struct(settings)})
      |> Repo.update!()

    my_entity = Repo.get(MyEntity, some_id)
    IO.inspect my_entity
    Repo.load(MySettings, my_entity.settings)
  end
end

Main.main()

[lukaszsamson]

@wojtekmach In my use case there are a few different settings schemas and I need to dynamically load the correct one from the backing JSON column

[wojtekmach]

we solved this for decimal but the same problem exists for e.g. :time, :naive_datetime, :utc_datetime and it can be easily shown in my test script by swapping the field type and value. Should we use the same solution for these or there's a more general approach? Maybe something to do with the embed_as/1 callback?

[lukaszsamson]

I guess it should work for all ecto primitive types This includes basic types, structs (Decimal, Time, Date etc), arrays, maps and nested schemas

[josevalim]

@lukaszsamson i am working on something that will make your life a bit easier. Give me 10.

[josevalim]

@lukaszsamson

So the issue in your approach is that you are assuming that the data encoded/decoded for embedding is the same as the data encoded/decoded for JSON. Unfortunately that's not true and Repo.load won't work for said cases.

All database adapters therefore have to write a encoding/decoding layer, which I have ported to Ecto as Ecto.Type.embedded_load and Ecto.Type.embedded_dump. Before encoding the value to JSON, you should call Ecto.Type.embedded_dump for each field and value. After decoding the value from JSON, you should call Ecto.Type.embedded_load for each field and value. That said, it is best if your field :settings, :map, default: %{} is converted to a custom type to encapsulate this. Once you do , please share the custom type here, in case we find oppoprtunity for improvement.

[lukaszsamson]

I can give it a try.

[lukaszsamson]

@josevalim I managed to refactor my app to use a custom ecto type as you suggested. Here's the code I ended up with. I have to say that is's a lot more complex than the previous version. The obvious advantage though is that loading/dumping now works automatically and the data is more strongly typed. The downside is that i had to add a marker field to the serialized JSON as there is no context available in Ecto.Type callbacks. Otherwise load would not know into which struct to deserialize.

defmodule EctoJsonEmbeddedSchema do
  use Ecto.Type

  @type_field "__type__"

  @impl true
  def cast(%module{} = data) do
    if is_embedded_schema(module) do
      {:ok, data}
    else
      :error
    end
  end

  def cast(_), do: :error

  @impl true
  def dump(%module{} = data) do
    if is_embedded_schema(module) do
      map = Map.from_struct(data)

      get_fields_types(module)
      |> Enum.reduce_while(
        {:ok,
         %{
           @type_field => inspect(module)
         }},
        fn {field, type}, {:ok, acc} ->
          case Ecto.Type.embedded_dump(type, map[field], :json) do
            {:ok, dumped} ->
              updated_acc = Map.put(acc, Atom.to_string(field), dumped)
              {:cont, {:ok, updated_acc}}

            _ ->
              {:halt, :error}
          end
        end
      )
    else
      :error
    end
  end

  def dump(_), do: :error

  @impl true
  def load(%{@type_field => module_string} = data) when is_binary(module_string) do
    try do
      module = String.to_existing_atom("Elixir." <> module_string)

      if is_embedded_schema(module) do
        load_result =
          get_fields_types(module)
          |> Enum.reduce_while({:ok, []}, fn {field, type}, {:ok, acc} ->
            value = Map.get(data, Atom.to_string(field))

            case Ecto.Type.embedded_load(type, value, :json) do
              {:ok, loaded} ->
                {:cont, {:ok, [{field, loaded} | acc]}}

              _ ->
                {:halt, :error}
            end
          end)

        case load_result do
          {:ok, fields} ->
            {:ok, struct(module, fields)}

          :error ->
            :error
        end
      end
    rescue
      ArgumentError -> :error
    end
  end

  def load(_), do: :error

  @impl true
  def type, do: :map

  defp is_embedded_schema(module) do
    Code.ensure_loaded?(module) and
      function_exported?(module, :__schema__, 1) and
      function_exported?(module, :__schema__, 2) and
      module.__schema__(:source) == nil
  end

  defp get_fields_types(module) do
    for field <- module.__schema__(:fields) do
      {field, module.__schema__(:type, field)}
    end
  end
end

[josevalim]

Yes, the marker field is essential, otherwise you will have strings on decimal fields, encrypted data won't be handled, etc.

The only change I would do in your code is to make the type field a mapping. Something like:

@types %{
  "video" => MyApp.Video,
  "audio" => MyApp.Audio
}

This way you don't couple the data in the database with your application names and you can skip the whole Code.ensure_loaded? dance, as you know these schemas exist.

[lukaszsamson]

The coupling would still be there, just less explicit :)

[josevalim]

Not really! The coupling I was pointing to was the need to change the data in your database in case you rename a module in Elixir, which is definitely not a good idea. By introducing a mapping, The database doesn’t keep any knowledge of your application.