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

Polymorphic embeds are not kept as changesets but are turned into structs - as opposed to what happens with ordinary embeds #74

Open
maxmarcon opened this issue Jan 27, 2023 · 7 comments

Comments

@maxmarcon
Copy link

Observed behavior: When I build a changeset with ordinary embeds, these embeds stay changesets. When I build the changesets with polymorphic embeds, the embeds are turned into structs if they're valid.

Expected behavior: I would expect polymorphic embeds to be kept as changeset until I call apply_changes or apply_action on the parent changeset, just as normal embeds do.

Ecto version: 3.9.4
polymorphic_embed version: 3.0.5

Here's a working example:

Mix.install([{:ecto, "~> 3.9.4"}, {:polymorphic_embed, "~> 3.0.5"}])

defmodule ChildSchema do
  use Ecto.Schema
  import Ecto.Changeset

  embedded_schema do
    field(:name, :string)
    field(:age, :integer)
  end

  def changeset(source \\ %__MODULE__{}, changes) do
    cast(source, changes, [:name, :age])
  end
end

defmodule ParentSchema do
  use Ecto.Schema
  import Ecto.Changeset

  embedded_schema do
    embeds_many(:children, ChildSchema)
  end

  def changeset(source \\ %__MODULE__{}, changes) do
    cast(source, changes, [])
    |> cast_embed(:children)
  end
end

defmodule ParentSchemaWithPolymorphicChildren do
  use Ecto.Schema
  import PolymorphicEmbed
  import Ecto.Changeset

  embedded_schema do
    polymorphic_embeds_many(:children,
      types: [
        child_schema: ChildSchema
      ],
      on_replace: :delete,
      on_type_not_found: :raise
    )
  end

  def changeset(source \\ %__MODULE__{}, changes) do
    cast(source, changes, [])
    |> cast_polymorphic_embed(:children)
  end
end

ParentSchema.changeset(%{children: [%{name: "Tic", age: 10}, %{name: "Tac", age: 12}]})
|> IO.inspect(label: "changeset for schema with traditional embeds: embeds are still changesets")

ParentSchemaWithPolymorphicChildren.changeset(%{
  children: [
    %{name: "Tic", age: 10, __type__: "child_schema"},
    %{name: "Tac", age: 12, __type__: "child_schema"}
  ]
})
|> IO.inspect(
  label: "changeset for schema with polymorphic embeds: embeds are have already been applied"
)

Result of executing the above code:

changeset for schema with traditional embeds: embeds are still changesets

#Ecto.Changeset<
  action: nil,
  changes: %{
    children: [
      #Ecto.Changeset<
        action: :insert,
        changes: %{age: 10, name: "Tic"},
        errors: [],
        data: #ChildSchema<>,
        valid?: true
      >,
      #Ecto.Changeset<
        action: :insert,
        changes: %{age: 12, name: "Tac"},
        errors: [],
        data: #ChildSchema<>,
        valid?: true
      >
    ]
  },
  errors: [],
  data: #ParentSchema<>,
  valid?: true
>

changeset for schema with polymorphic embeds: changes in embeds have already been applied

#Ecto.Changeset<
  action: nil,
  changes: %{
    children: [
      %ChildSchema{
        id: "cd555105-4033-425f-b984-29f8c2062de9",
        name: "Tic",
        age: 10
      },
      %ChildSchema{
        id: "5c55473d-e1d5-4f76-96a9-b63685c55ca3",
        name: "Tac",
        age: 12
      }
    ]
  },
  errors: [],
  data: #ParentSchemaWithPolymorphicChildren<>,
  valid?: true
>
@mathieuprog
Copy link
Owner

This was due to a limitation with what I can do with Ecto Type, if I recall correctly. Does this behaviour cause you any issue?

@maxmarcon
Copy link
Author

Yes. When I call Ecto.Changeset APIs like get_change, or update_change my code expects changesets not structs.
When dealing with polymorphic embeds now I have to also deal with structs.

I added a workaround in my code, but I thought you might want to know about this. It should be fixed IMO.

I can give it a try and come up with a PR unless you tell me you prefer to keep it this way.

@mathieuprog
Copy link
Owner

As far as I can recall there was no possible fix, but if you you think you can find a way to make that work it'd be awesome.

The idea of the project is to have the same behavior as embeds_one and embeds_many, so this is ideally fixed.

@maxmarcon
Copy link
Author

Ok I've taken a look and I think I understand why you're doing this.

You're applying the changes as soon as the polymorphic embedded changeset is valid here (in the case of single embeds) because otherwise, if you keep it as a changeset, Ecto would never apply the changes.

That's because in Ecto.Changeset.apply_changes/1, Ecto treats embeds and associations as special cases where changes are recursively applied. In all other cases, the change is simply copied over to the data.

Is this accurate?

@tmjoen
Copy link

tmjoen commented Feb 13, 2023

This could be important towards change tracking with Live View IIRC? If we replace with structs on every pass through the cast function, everything will be marked as a new change and sent over the wire for each run through phx_change.

If it is impossible to fix, maybe we could float this to Ecto and see if there is anything to do on that end?

@Miradorn
Copy link

I think this behaviour also leads to another bug I'm encountering: If you have a "normal" embed inside a polymorphic_embed, the "normal" embedded one no longer gets automatically generated ids from ecto.
I think this is due to Ecto expecting the data to be changesets (see here), but my ecto knowledge is not good enough to actually verify this.

If you think this is unrelated i can open up another issue!

@mathieuprog
Copy link
Owner

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