mathieuprog / polymorphic_embed

Polymorphic embeds in Ecto
Apache License 2.0
323 stars 63 forks source link

Identify type by a field of the parent schema #100

Open rubynho opened 3 months ago

rubynho commented 3 months ago

There is the option identify_by_fields used to infer the embedded schema type by the attributes it receives to cast. I'm thinking in a similar feature but looking at the value of a field of the parent schema. Something like:

defmodule Payments do
  use Ecto.Schema

  import Ecto.Changeset
  import PolymorphicEmbed

  schema "payments" do
    field :method, :string

    polymorphic_embeds_one :details,
      types: [
        credit_card: [module: CreditCard, identify_by_parent_field: :method],
        debit_card: [module: DebitCard, identify_by_parent_field: :method]
      ],
      on_replace: :update
  end

  def changeset(payment, attrs) do
    payment
    |> cast(attrs, [:method])
    |> cast_polymorphic_embed(:details)
  end
end

Or

  schema "payments" do
    field :method, :string

    polymorphic_embeds_one :details,
      types: [
        credit_card: CreditCard,
        debit_card: DebitCard
      ],
      identify_by_parent_field: :method,
      on_replace: :update
  end

Sounds a good feature or too specific?

mathieuprog commented 3 months ago

When loading from the db, we get just the map (in the Ecto Type) (we don't have the other fields of the schema), so we can't infer the type from any other fields outside the map.

rubynho commented 3 months ago

But when loading we have access to the __type__ field and don't need to infer, no?

mathieuprog commented 3 months ago

You're right. I got confused. So let's talk about dumping into db. We don't have the other field values from the schema.

rubynho commented 3 months ago

Is it necessary? After casting we know the structure to be embedded so when dumping we could get the type for that structure.

As far as I can see the proposed feature would impact only the casting process, the rest would remain as is.

mathieuprog commented 3 months ago

I guess you're right again..? šŸ˜‚

But then we have 2 sources of truths for the type (in another adjacent field + in the actual embed). A developer could change the :method field to another value but not changing the :details field. Is it a good practice?

mathieuprog commented 3 months ago

Never mind what I said, we might want to search on a text field rather than a jsonb field, so I see how that could be advantageous.

As for the naming, :identify_by_fields is misunderstood: it detects the presence of fields to infer the type (e.g. could be useful for data coming from an external api that doesn't have a type field, or existing data in db that doesn't have the type field).

How about :type_from_adjacent_field for the feature you suggest?

But possible problem, what if in the changeset the developer changes the :method after casting the embed?

fuelen commented 3 months ago

I started using the library yesterday and planned the schema of the table to have a distinct enum field in the table. And it turned out that I have to copy the field to the embedded data. For the time being, I decided not to add a type field, but it would be great to have an option to have such a design.

fuelen commented 3 months ago

I came to issues to check whether this was discussed to understand the rationale of being type field a part of the embedded data :slightly_smiling_face:

rubynho commented 3 months ago

I guess you're right again..? šŸ˜‚

But then we have 2 sources of truths for the type (in another adjacent field + in the actual embed). A developer could change the :method field to another value but not changing the :details field. Is it a good practice?

Maybe imposing a priority in these methods can solve it, if the adjacent field is set, it is the source of truth otherwise we follow the __type__ field (only when casting).

The way I see the adjacent field, when set, will become tightly coupled to the embedded type, their values must not contradict each other, so when changing the value of :method the :details should validate against the right type, even if it already exists. Makes sense?

I made it work with this function, right before the cast_polymorphic_embed/2 in the changeset:

  defp set_details_type(%{params: %{"details" => %{} = details}} = changeset, payment) do
    method = get_change(changeset, :method) || payment.method

    details
    |> Map.keys()
    |> List.first()
    |> is_binary()
    |> if(do: "__type__", else: :__type__)
    |> then(&Map.put_new(details, &1, method))
    |> then(&Map.put(changeset, :params, Map.put(changeset.params, "details", &1)))
  end

  defp set_details_type(changeset, _payment), do: changeset
rubynho commented 3 months ago

Never mind what I said, we might want to search on a text field rather than a jsonb field, so I see how that could be advantageous.

As for the naming, :identify_by_fields is misunderstood: it detects the presence of fields to infer the type (e.g. could be useful for data coming from an external api that doesn't have a type field, or existing data in db that doesn't have the type field).

How about :type_from_adjacent_field for the feature you suggest?

But possible problem, what if in the changeset the developer changes the :method after casting the embed?

Yeah exactly! Sorry, rushed on replying the other comment. Kind answers the last question, as their values should not contradict each other, the change must occur using the changeset to be able to validate the right polymorphic type based on the adjacent field.

rubynho commented 3 months ago

About the name, maybe it could have a more common word than adjacent, how about :type_from_parent_field? I have a felling that the word parent is already in the people's heads when it comes to thinking about an outside layer.

mathieuprog commented 3 months ago

Right, depends how you look at it. The field :method is adjacent (sibling) to the field :details holding the embed, but also, the field :method is a field in the parent schema of the actual embed. The thing is that the option is defined on the schema holding the embed; parent would have been more clear if we wrote it from inside the embed. Let's see.