Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Identify type by a field of the parent schema #100

Open
rubynho opened this issue May 29, 2024 · 13 comments
Open

Identify type by a field of the parent schema #100

rubynho opened this issue May 29, 2024 · 13 comments

Comments

@rubynho
Copy link

rubynho commented May 29, 2024

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
Copy link
Owner

mathieuprog commented May 30, 2024

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
Copy link
Author

rubynho commented May 30, 2024

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

@mathieuprog
Copy link
Owner

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
Copy link
Author

rubynho commented May 30, 2024

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
Copy link
Owner

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
Copy link
Owner

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
Copy link

fuelen commented May 31, 2024

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
Copy link

fuelen commented May 31, 2024

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

@rubynho
Copy link
Author

rubynho commented May 31, 2024

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
Copy link
Author

rubynho commented May 31, 2024

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
Copy link
Author

rubynho commented Jun 2, 2024

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
Copy link
Owner

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.

@maxsalven
Copy link

I think this might be completed via use_parent_field_for_type in 5.0.0?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants