Adzz / ecto_morph

morph your Ecto capabilities into the s t r a t o s p h e r e !
Apache License 2.0
109 stars 8 forks source link

postgres JSONb dynamic field type #2

Closed mikepc closed 4 years ago

mikepc commented 5 years ago

Hello there!

I'm having a devil of a time getting a dynamic embedded schema to save in postgres.

So I started here, basically https://github.com/elixir-ecto/ecto/pull/2113#issuecomment-412829999

But my core problem is that despite several iterations, when it goes to save it to the database, it throws

(UndefinedFunctionError) function :map.schema/1 is undefined (module :map is not available)

So in my hunt, I came across this package and it slimmed down the code, but I still have the same problem :(. It's like ecto isn't seeing the type update before it goes into schema. With your package the changeset automatically sets the type, which is really super nice, but for some reason I can't get this to save to postgres:

## This is the latest iteration, of many attempts.

defmodule Geeks.Activities.Activity do
  @moduledoc false

  use Ecto.Schema
  import Ecto.Changeset
  import GatheringRole
  alias Geeks.Activities.Types

  embedded_schema do
    field :type, ActivityType
    field :title, :string
    field :max_participants, :integer
    field :next_start, :utc_datetime
    embeds_one :properties, :map
    embeds_many :rsvp, Geeks.Activities.ActivityRsvp
    embeds_many :history, Geeks.Activities.ActivityHistory do
      field :history_event, :string
      timestamps([types: :utc_datetime])
    end
    timestamps([types: :utc_datetime])
  end

  @all_fields ~w(type title max_participants next_start)a
  @required_fields ~w(type title)a
  defp properties_type(props_schema) do
    {:embed,
      %Ecto.Embedded{
        cardinality: :one,
        field: :properties,
        on_cast: &props_schema.changeset(&1, &2),
        on_replace: :update,
        owner: nil,
        related: props_schema,
        unique: true
      }}
  end

  def create_changeset(%{ properties: props } = params) do
    {:ok, props_schema} = Types.get_schema(:ttrpg)
    props_cs = EctoMorph.generate_changeset(props, props_schema)
    case props_cs.valid? do
      true ->
        changeset =
          %__MODULE__{}
          |> cast(params, @required_fields)
          |> Map.update!(:types, &Map.put(&1, :properties, properties_type(props_schema)))
          |> cast_embed(:properties)
          |> validate_required(@required_fields)
      false ->
        props_cs
    end

  end

  def create_changeset(%{} = params ) do
    create_changeset(%{params | properties: %{}})
  end

end

And elsewhere, in the context:

  def add_activity(%Gathering{ activities: activities} = gathering, activity_params \\ %{properties: %{}}) do
   changeset = Ecto.Changeset.change(gathering)
   activity = Geeks.Activities.Activity.create_changeset(activity_params)
   case activity.valid? do
     true -> changeset = Ecto.Changeset.put_embed(changeset, :activities, activities ++ [activity])
             |> Geeks.Repo.update!
     false -> {:error, activity}
   end

I'm going to try to cross-post this to the elixir forums, but man it's a brainweaver!

Adzz commented 4 years ago

Hello! So sorry I have only just seen this issue, for some reason I wasn't following the Repo. Let me see if I can help.

@mikepc I wrote an article showing how you can use Ecto embeded schemas as jsonb fields, is that what you were going for? https://medium.com/@ItizAdz/creating-a-has-one-of-association-in-ecto-with-ectomorph-3932adb996d9

What are you trying to do?

mikepc commented 4 years ago

It seems that the article might be somewhat outdated? There were a couple of required functions (embed_as and equal?). It appears that dump and load do get called:

** (RuntimeError) This will never be called code: activity = activity_fixture() stacktrace: (geeks) lib/geeks/types/JsonField.ex:23: Geeks.JsonField.dump/1 (ecto) lib/ecto/type.ex:859: Ecto.Type.process_dumpers/3 (ecto) lib/ecto/repo/schema.ex:927: Ecto.Repo.Schema.dump_field!/6 (ecto) lib/ecto/repo/schema.ex:940: anonymous fn/6 in Ecto.Repo.Schema.dump_fields!/5 (stdlib) maps.erl:232: :maps.fold_1/3 (ecto) lib/ecto/repo/schema.ex:938: Ecto.Repo.Schema.dump_fields!/5 (ecto) lib/ecto/repo/schema.ex:871: Ecto.Repo.Schema.dump_changes!/6 (ecto) lib/ecto/repo/schema.ex:255: anonymous fn/15 in Ecto.Repo.Schema.do_insert/4 (ecto) lib/ecto/repo/schema.ex:916: anonymous fn/3 in Ecto.Repo.Schema.wrap_in_transaction/6 (ecto_sql) lib/ecto/adapters/sql.ex:898: anonymous fn/3 in Ecto.Adapters.SQL.checkout_or_transaction/4 (db_connection) lib/db_connection.ex:1415: DBConnection.run_transaction/4 (ecto) lib/ecto/repo/schema.ex:164: Ecto.Repo.Schema.insert!/4 (geeks) lib/geeks/services/activities.ex:55: Geeks.Services.Activities.create/1 test/geeks/activity_attendance_test.exs:12: (test)

mikepc commented 4 years ago

`

defp deps do [ {:ecto_sql, "~> 3.1"}, {:postgrex, ">= 0.0.0"}, {:jason, "~> 1.0"}, {:comeonin, "~> 4.0"}, {:argon2_elixir, "~> 1.2"}, {:geo_postgis, "~> 3.1"}, {:timex, "~> 3.5"}, {:csv, "~> 2.3"}, {:combine, "~> 0.10.0"}, {:nanoid, "~> 2.0.2"}, {:ecto_enum, "~> 1.3"}, {:slugify, "~> 1.2"}, {:phoenix, "~> 1.4.10"}, {:phoenix_ecto, "~> 4.0"}, {:earmark, "~> 1.4.2"}, {:html_sanitize_ex, "~> 1.3.0-rc3"}, {:geolix, "~> 1.0"}, {:geolix_adapter_mmdb2, "~> 0.1.0"}, {:tesla, "~> 1.3.0"}, {:ecto_morph, "~> 0.1.13"}, {:dotenv, "~> 3.0.0"} ] end

`

mikepc commented 4 years ago

Basically I'm trying to do exactly what your article is about! I've made progress, but right now stuck on loader trying to load

defmodule Geeks.JsonField do
      alias Geeks.Services.ActivityTypes
      import Ecto.Schema
      @behaviour Ecto.Type
      @impl true
      @doc "Returns the underlying type of a scenario"
      def type, do: :map

      @doc "Casts data retrieved from the DB into the correct struct"
      @impl true
      def cast(json_ob = %{ type: t}) do
        EctoMorph.cast_to_struct(json_ob,  ActivityTypes.props_class(t) )
      end
      def cast(json_ob) do
        {:error, "Json field type #{inspect(json_ob)} is not supported"}
      end
      @impl true
      @doc """
      Because the rewards are embedded types, they do not use dump/1
      when saving rules into the db.
      """
     def dump(_), do: raise("This will never be called")

      @impl true
      @doc """
      Because the rewards are embedded types, they do not use load/1
      when saving rewards into the db.
      """
      def load(_), do: raise("This will never be called")

      @impl true
      def equal?(_term, nil), do: false
      @impl true
      def equal?(nil, nil), do: false
      @impl true
      def equal?(nil,  %_{}), do: false
      @impl true
      def equal?(term, %_{} = ob), do: Map.equal?(term, EctoMorph.map_from_struct(ob))

      @impl true
      def embed_as(_), do: :map
end

@derive Jason.Encoder
  defmodule RpgProperties do
    use Ecto.Schema
     embedded_schema do
      field :type, Geeks.Types.AtomType, default: :ttrpg
      field :max_players, :integer
      field :backstory, :string
    end

    def changeset(%RpgProperties{} = props, %{} = params) do
      props
     |> cast(params, [:max_players, :backstory])
     |> validate_number(:max_players, greater_than: 0, less_than_or_equal_to: 20)
     |> validate_length(:backstory, less_than_or_equal_to: 1000)
    end
   end
schema "activities" do
    field :type, Geeks.Types.AtomType
    field :public_id, :string
    field :title, :string
    field :max_participants, :integer
    field :start_x, :utc_datetime
    field :start, :string
    field :where, :string
    field :where_x, Geo.PostGIS.Geometry
    embeds_one :properties, Geeks.JsonField # Using embeds_one gets around the dumps() requirement
    has_many :rsvp, Geeks.Activities.ActivityRsvp
    belongs_to :gathering, Geeks.Gatherings.Gathering
    timestamps([types: :utc_datetime])
  end

The load, on the other hand..

   (geeks) Geeks.JsonField.__schema__(:load)
   (ecto) lib/ecto/schema/loader.ex:31: Ecto.Schema.Loader.unsafe_load/3
   (ecto) lib/ecto/type.ex:627: Ecto.Type.load_embed/3
   (ecto) lib/ecto/type.ex:842: Ecto.Type.process_loaders/3
Adzz commented 4 years ago

You are correct! Apologies I will update the article. There are now two extra callbacks in the custom type behaviour, which I can see you have implemented.

I think I also missed out some configuration from the article which might be the cause of the issue, as previously you did not need to implement load and dump, whereas now it seems you do.

If we step back for a minute, to make sure I am on the same page: I think what we are trying to do is have a column in our table that is JSONB. This column can be one of several types of things, so when we fetch that column from the DB, we want to decide which struct it will get serialised to (based on what it looks like). We also need to ensure that when it goes from a struct -> the database, it gets turned into JSON correctly. So let's go through each step. There are two options here though, the jsonb column can be an array (a kind of has_many_of relation) or it can be just a map. I'll assume the later.

The migration:

defmodule PostgresTest.Repo.Migrations.AddTable do
  use Ecto.Migration

  def change do
    create(table(:athletes)) do
      add(:reward, :map)
      timestamps()
    end
  end
end

The Schema

defmodule PostgresTest.Athlete do
  use Ecto.Schema

  schema "athletes" do
    field(:reward, Reward)
    timestamps()
  end
end

In my example there will be two kinds of rewards also:

defmodule Medal do
  use Ecto.Schema

  # This is needed so that we can serialize the struct into JSON when putting it in the DB
  # See the Jason docs for more info on the Encoder.
  @derive {Jason.Encoder, only: [:colour]}
  embedded_schema do
    field(:colour, :string)
  end
end

defmodule PrizeMoney do
  use Ecto.Schema
  @derive {Jason.Encoder, only: [:amount]}
  embedded_schema do
    field(:amount, :integer)
  end
end

The Custom Ecto Type

Here it might help to understand when the callbacks get used (I don't think the docs do a good job of explaining that, so I've added docs in the code snippet below)

defmodule Rewards do
  @behaviour Ecto.Type

  @impl true
  def type, do: :map

  @impl true
  @doc """
  The docs explain this well: https://hexdocs.pm/ecto/Ecto.Type.html#c:cast/1
  """
  def cast(reward = %{"colour" => _}), do: EctoMorph.cast_to_struct(reward, Medal)
  def cast(reward = %{colour: _}), do: EctoMorph.cast_to_struct(reward, Medal)
  def cast(reward = %{"amount" => _}), do: EctoMorph.cast_to_struct(reward, PrizeMoney)
  def cast(reward = %{amount: _}), do: EctoMorph.cast_to_struct(reward, PrizeMoney)

  @doc """
  This is called when we try and update / insert our Athlete struct, it gets passed the changes we
  want to make. For example if we do this:
  `Ecto.Changeset.change(%Athlete{}, %{reward: %{amount: 100}})`
  dump will be given: `%{amount: 100}`

  That means if there is a chance of this happening:
  `Ecto.Changeset.change(%Athlete{}, %{reward: %{"amount" => 100}})`
  We need to handle that below in the cases:
  """
  @impl true
  def dump(reward = %{"colour" => _}), do: EctoMorph.cast_to_struct(reward, Medal)
  def dump(reward = %{colour: _}), do: EctoMorph.cast_to_struct(reward, Medal)
  def dump(reward = %{"amount" => _}), do: EctoMorph.cast_to_struct(reward, PrizeMoney)
  def dump(reward = %{amount: _}), do: EctoMorph.cast_to_struct(reward, PrizeMoney)

  @impl true
  @doc """
  This is called when we go from the DB -> elixir. It gets passed whatever postgres has in the column,
  which if we have stipulated a :map type in our migration, will be JSON.
  """
  def load(reward = %{"colour" => _}), do: EctoMorph.cast_to_struct(reward, Medal)
  def load(reward = %{"amount" => _}), do: EctoMorph.cast_to_struct(reward, PrizeMoney)

  @impl true
  @doc """
  This is only used if we define an `embeds_one` or `embeds_many` relationship. In that case,
  there are two options, use `:self` which will do nothing to the relation and just pass it on to
  the db to be put into it. Or you can use `:dump` which will call the `dump` function before 
  inserting the embedded relation.
  """
  def embed_as(_), do: :dump

  @impl true
  @doc """
  This function is used to determine if the changes we want to make exist
  in the struct we are creating / updating already. If we do an Ecto.Changeset.cast
  or Ecto.Changeset.change, this will be called to determine if there are actually any changes
  to apply.
  """
  def equal?(left, right), do: left == right
end

Does that help at all?

Adzz commented 4 years ago

I think where you have this line:

 embeds_one :properties, Geeks.JsonField 

you are a little off. embeds_one and embeds_many both will only allow an Ecto schema as their last argument, but Geeks.JsonField is a custom type. That's why you have to do this:

field(:property, Geeks.JsonField)

Then this I think.


defmodule Geeks.JsonField do
  alias Geeks.Services.ActivityTypes
  import Ecto.Schema
  @behaviour Ecto.Type
  @impl true
  @doc "Returns the underlying type of a scenario"
  def type, do: :map

  @doc "Casts data retrieved from the DB into the correct struct"
  @impl true
  def cast(json_ob = %{type: t}) do
    EctoMorph.cast_to_struct(json_ob, ActivityTypes.props_class(t))
  end

  def cast(json_ob) do
    {:error, "Json field type #{inspect(json_ob)} is not supported"}
  end

  @impl true
  def dump(json_ob = %{type: t}) do
    EctoMorph.cast_to_struct(json_ob, ActivityTypes.props_class(t))
  end

  @impl true
  def load(json_ob = %{type: t}) do
    EctoMorph.cast_to_struct(json_ob, ActivityTypes.props_class(t))
  end

  @impl true
  def equal?(_term, nil), do: false
  @impl true
  def equal?(nil, nil), do: false
  @impl true
  def equal?(nil, %_{}), do: false
  @impl true
  def equal?(term, %_{} = ob), do: Map.equal?(term, EctoMorph.map_from_struct(ob))

  @impl true
  def embed_as(_), do: :map
end
mikepc commented 4 years ago

Thank you so much! I'm going to to try this tonight!

mikepc commented 4 years ago

If I get this to work, would you like me to see if I can figure out how to package it up as a re-usable type? Optimally I would love folks to be able to

schema do
  field :type, :string
  field :properties, EctoMorph.DynamicField, cast_by: :type
end

I'm not sure I can do it, but I will try!

Adzz commented 4 years ago

I had the same thought and I've started something, but I think it needs some thought, as we'll have to pass in a callback to determine which struct gets created. I'd say by all means feel free to have a crack, might be fun to try regardless!

mikepc commented 4 years ago

How about instead of cast_by, we make it cast_as: &cast_to_properties/1 <- This function would be user-defined, passing in the owner struct, and returning whatever schema the user selects on that criteria? Not sure of the availability of the parameters at that stage, but that would be a pretty clean api.

Adzz commented 4 years ago

That's the way we'd have to do it. But my concern is if you want to provide a different function for load dump and cast or any of the other functions in the custom type behaviour, then we haven't really saved anyone anything.

Did you manage to get it working at all?

mikepc commented 4 years ago

Still struggling, but I'm just getting back to it, was away for the holidays. Will keep you updated.

mikepc commented 4 years ago

Ok, I've got happy path working, but validation isn't running:


defmodule Geeks.Activities.ActivityProperties do
  alias Geeks.Services.ActivityTypes
  import Ecto.Schema
  @behaviour Ecto.Type
  @impl true
  @doc "Returns the underlying type of a scenario"
  def type, do: :map

  @doc "Casts data retrieved from the DB into the correct struct"
  @impl true
  def cast(json_ob = %{type: t}) do
    klass =  ActivityTypes.props_class(t)
    case EctoMorph.cast_to_struct(json_ob, klass) do
      {:ok, struct} -> {:ok, struct}
      _ -> {:error, "cannot cast struct of type #{t}"}
    end

  end
  def cast(json_ob = %{"type" => t}) do
    cast(Map.put(json_ob, :type, String.to_existing_atom(t)))
  end
  def cast(_json_ob) do
    {:error, "activity properties type was not provided"}
  end

  @impl true
  def dump(json_ob = %{type: t}) do
    EctoMorph.cast_to_struct(json_ob, ActivityTypes.props_class(t))
  end
  def dump(_), do: raise "type not provided to dump"

  @impl true
  def load(json_ob = %{"type" =>  t}) when is_atom(t) do
    case EctoMorph.cast_to_struct(json_ob, ActivityTypes.props_class(t)) do
      {:ok, struct} -> {:ok, struct}
      _ -> {:error, "cannot cast struct of type #{t}"}
    end
  end
  def load(json_ob = %{"type" => t}) when is_binary(t),
    do: load(Map.put(json_ob, "type", String.to_existing_atom(t)))

  @impl true
  def equal?(_term, nil), do: false
  @impl true
  def equal?(nil, nil), do: false
  @impl true
  def equal?(nil, %_{}), do: false
  @impl true
  def equal?(term, %_{} = ob), do: Map.equal?(term, EctoMorph.map_from_struct(ob))

  @impl true
  def embed_as(_), do: :map
end

Schema:

defmodule Geeks.Activities.Activity do
  @moduledoc false
  use Ecto.Schema
  import Ecto.Changeset
  import Geeks.Types.DynamicEmbeds, only: [cast_dynamic_embed: 2]

  @doc """
  A gathering can host multiple activities.
  where: A human, loosey goosey location, "Mike's house"
  where_x: A geospatial coordinate of where it physically will happen.
  when: A human, loosey goosey start time, eg. "after school"
  """
  schema "activities" do
    field :type, Geeks.Types.AtomType
    field :public_id, :string
    field :title, :string
    field :max_participants, :integer
    field :start_x, :utc_datetime
    field :start, :string
    field :where, :string
    field :where_x, Geo.PostGIS.Geometry
    field :properties, Geeks.Activities.ActivityProperties
    has_many :rsvp, Geeks.Activities.ActivityRsvp
    belongs_to :gathering, Geeks.Gatherings.Gathering
    timestamps([types: :utc_datetime])
  end

  @all_fields ~w(type title public_id gathering_id properties  )a
  @required_fields ~w(type title public_id gathering_id properties)a

  def changeset(%__MODULE{} = struct, %{ type: t, properties: _} = params) do
    params = Map.put_new(params, :type, t)
    struct
    |> cast(params, @all_fields)
    |> cast_assoc(:rsvp)
    |> cast_assoc(:gathering)
    |> validate_required(@required_fields)
    |> validate_length(:title, max: 35, min: 3)
 end
 def changeset(%__MODULE{type: t} = struct, %{ properties: _} = params) do
  params = Map.put_new(params, :type, t)
  struct
  |> cast(params, @all_fields)
  |> cast_assoc(:rsvp)
  |> cast_assoc(:gathering)
  |> validate_required(@required_fields)
  |> validate_length(:title, max: 35, min: 3)
end
  def changeset(%__MODULE{ type: t} = struct,  %{} = params) do
     params = Map.put_new(params, :type, t)
     struct
     |> cast(params, @all_fields)
     |> cast_assoc(:rsvp)
     |> cast_assoc(:gathering)
     |> validate_required(@required_fields)
     |> validate_length(:title, max: 35, min: 3)
  end

 def changeset(%__MODULE{} = struct, %{}),
   do: change(struct) |> add_error(:type, "type is required")

end

The specific property type:

defmodule Geeks.Activities.ActivityProperties.RpgProperties do
  def props_type, do: :ttrpg
  @derive Jason.Encoder
  use Ecto.Schema
  import Ecto.Changeset

  @primary_key false
   embedded_schema do
    field :id, :string, primary_key: true
    field :type, Geeks.Types.AtomType, default: :ttrpg
    field :max_players, :integer
    field :backstory, :string
  end

  def changeset(props, %{} = params) do
    IO.puts("rpg properties changeset running")
    props
   |> cast(params, [:id, :max_players, :backstory])
   |> validate_number(:max_players, greater_than: 0, less_than_or_equal_to: 20)
   |> validate_length(:backstory, less_than_or_equal_to: 1000)
  end
end

cast_to_struct is giving the right struct, but in the case where a validation error happens, it should fail to save, which is where the tests are failing.

mikepc commented 4 years ago

Ok I got it working, but it's kind of wonky. I had to use custom_errors, have any better ideas?

defmodule Geeks.Activities.ActivityProperties do
  alias Geeks.Services.ActivityTypes
  import Ecto.Schema
  @behaviour Ecto.Type
  @impl true
  @doc "Returns the underlying type of a scenario"
  def type, do: :map

  @doc "Casts data retrieved from the DB into the correct struct"
  @impl true
  def cast(json_ob = %{type: t}) do
    klass =  ActivityTypes.props_class(t)
    cs = klass.changeset(json_ob)
    case cs.valid? do
      true -> case EctoMorph.cast_to_struct(json_ob, klass) do
        {:ok, struct} -> {:ok, struct}
        _ -> {:error, "cannot cast struct of type #{t}"}
      end
      false -> {:error,  cs.errors}

    end

  end
  def cast(json_ob = %{"type" => t}) do
    cast(Map.put(json_ob, :type, String.to_existing_atom(t)))
  end
  def cast(_json_ob) do
    {:error, "activity properties type was not provided"}
  end

  @impl true
  def dump(json_ob = %{type: t}) do
    EctoMorph.cast_to_struct(json_ob, ActivityTypes.props_class(t))
  end
  def dump(_), do: raise "type not provided to dump"

  @impl true
  def load(json_ob = %{"type" =>  t}) when is_atom(t) do
    case EctoMorph.cast_to_struct(json_ob, ActivityTypes.props_class(t)) do
      {:ok, struct} -> {:ok, struct}
      _ -> {:error, "cannot cast struct of type #{t}"}
    end
  end
  def load(json_ob = %{"type" => t}) when is_binary(t),
    do: load(Map.put(json_ob, "type", String.to_existing_atom(t)))

  @impl true
  def equal?(_term, nil), do: false
  @impl true
  def equal?(nil, nil), do: false
  @impl true
  def equal?(nil, %_{}), do: false
  @impl true
  def equal?(term, %_{} = ob), do: Map.equal?(term, EctoMorph.map_from_struct(ob))

  @impl true
  def embed_as(_), do: :map
end
Adzz commented 4 years ago

That's great you got it working!

Personally I would separate the validation of the changeset from the casting in the custom type. I would have somewhere where I created the changeset, did all of the validations on it, then only if it was valid save it to the db. That might look something like this (I'm imagining that params is some user input of some kind)

# Get the row we want to update:
activity = MyApp.Repo.get(Geeks.Activities.Activity, 9)

# Create and validate changesets:
activity_params = %{"title" => "thing", "max_participants" => 2}
property_params = %{"type" => "thing", "backstory" => "Tragic", "max_players" => 1}

activity_changeset = 
  EctoMorph.generate_changeset(activity_params, activity)
  |> validate_required(@required_fields)
  |> validate_length(:title, max: 35, min: 3)

property_changeset = 
  EctoMorph.generate_changeset(property_params, ActivityTypes.props_class(property_params.type))
  |> validate_number(:max_players, greater_than: 0, less_than_or_equal_to: 20)
  |> validate_length(:backstory, less_than_or_equal_to: 1000)

# Insert if the changes are valid
Ecto.Changeset.put_change(activity_changeset, :properties, property_changeset)
|> Repo.update!

then I would simplify the cast function in the custom type to be:

def cast(json_ob = %{type: t}) do
  case ActivityTypes.props_class(t) do
    nil -> {:error, "cannot cast struct of type #{t}"}
    klass -> EctoMorph.cast_to_struct(json_ob, klass)
  end
end
def cast(_json_ob) do
  {:error, "unrecognised type"}
end
mikepc commented 4 years ago

So what's the thinking behind splitting it out into two separate changesets? I'm noticing that traverse_errors won't work with changes as part of the main changeset. Is that why? So I'll need to treat validation of the properties totally separate from the parent object. That's not ideal, but I can certainly live with it!

Optimally, I'd like traverse errors to bring back %{ properties: %{ max_players: "must be less than or equal to 20" } }, otherwise I will need to do some dancing around the map keys at the view layer since I'm sending in an activity update and getting back a properties error.

Adzz commented 4 years ago

The reason I did that was because it looked like the validations for each were different. Also if you do this:

activity_changeset = 
EctoMorph.generate_changeset(activity_params, activity)
|> validate_required(@required_fields)
|> validate_length(:title, max: 35, min: 3)
# this is a property validation
|> validate_length(:backstory, less_than_or_equal_to: 1000)

then Ecto will rightfully complain that the Activities doesn't have a field backstory.

It did get me thinking about the API of Ecto though, I'm going to propose to the mailing group that we change the ecto validations to take a path so we could do this:

|> validate_length([:properties, :backstory], less_than_or_equal_to: 1000)

and have it traverse changes to find backstory in the nested relation. If they reject it I will add it to this lib.

mikepc commented 4 years ago

You were totally right about the intent. There are going to be 8-15 different schemas for the "activity properties" object. This is the main impetus behind doing it this way and why I've pushed so hard to get it to work. Making 8 polymorphic tables just doesn't make any sense whatsoever, and with pgsql's jsonb support it's exactly the right solution from my point of view.

The use case is activities are scheduled, and each of them have different types of attributes that I want to collect on them (for instance if the activity is war game, we'll want to ask about point value for army, if it's a collectible card game tournament we will ask what format it will take), but the activity as an entity is shared amongst the various types. I could achieve this somewhat simply using a purely relational model, it just doesn't make sense to do it that way.

mikepc commented 4 years ago

I think this is the implementation I'm going to ship with starting out.

I did put in the changes you suggested above to the activity_properties object. In the "client" layer, I will separate out the changesets, but this I think will ultimately be as clean as I can make it.

      defp upsert_properties(activity_changeset, %{type: t} = props_params) do
        klass = ActivityTypes.props_class(t)
        properties_changeset = klass.changeset(props_params) # This method uses EctoMorph.generate_changeset() 
        case properties_changeset.valid? do
          true -> activity_changeset = Ecto.Changeset.put_change(activity_changeset, :properties, properties_changeset)
                  %{valid?: activity_changeset.valid?, activity_changeset: activity_changeset, properties_changeset: properties_changeset}
          false -> %{valid?: false, properties_changeset: properties_changeset }
        end

      end

      @doc """
      When upserting an activity with properties, in the event of an invalid changeset,
      two changesets are returned: activity_changeset and properties_changeset.
      """
      def upsert(%Activity{} = struct, %{  properties: props  } = params) do
        params = Map.put_new(params, :public_id, Nanoid.generate(25))
        changeset = Activity.changeset(struct, params) # This method uses EctoMorph.generate_changeset
        change_properties = upsert_properties(changeset, props)
        case change_properties.valid? do
            true -> Repo.insert_or_update(change_properties.activity_changeset)
            false -> {:error, [activity_changeset: changeset,
                               properties_changeset: change_properties.properties_changeset]}
        end
      end
      @doc """
      When the properties key is not present in params, only the changeset for the activity is returned.
      """
      def upsert(%Activity{} = struct, %{ } = params) do
        changeset = Activity.changeset(struct, params)
        case changeset.valid? do
            true -> Repo.update(changeset)
            false -> {:error, changeset}
        end
      end
Adzz commented 4 years ago

I have just added a feature that may be relevant here:

https://github.com/Adzz/ecto_morph#valiating-nested-changesets

Now you can validate nested changesets easily like so:

# or
json = %{
  "has_many" => [
    %{"steamed_hams" => [%{"pickles" => 1}, %{"pickles" => 2}]},
    %{"steamed_hams" => [%{"pickles" => 1}]},
    %{"steamed_hams" => [%{"pickles" => 4}, %{"pickles" => 5}]}
  ]
}

# Here each of the steamed_hams above will have their pickle count validated:

EctoMorph.generate_changeset(json, MySchema)
|> EctoMorph.validate_nested_changeset([:has_many, :steamed_hams], fn changeset ->
  changeset
  |> Ecto.Changeset.validate_number(:pickles, greater_than: 3)
end)

With that I'm gonna close this issue.