Allow array of polymorphic to be empty list

[Matsa59]

Just a quick fix to allow array of polymorphic embed to have [] as default value

Every tests works locally

[mathieuprog]

This is not an option for embeds_many though? https://hexdocs.pm/ecto/Ecto.Schema.html#embeds_many/3

[Matsa59]

Hello,

TL;DR

Embeds_many is by default equal to [] so there is no default opts for embeds_many :/

---

let's suppose we have.

defmodule MyApp.SchemaTest do
  use Ecto.Schema

  import Ecto.Changeset

  @primary_key false
  schema "test" do
    embeds_many(:nested, __MODULE__.Nested)
  end

  def changeset(struct, attrs \\ %{}) do
    struct
    |> cast(attrs, [])
    |> cast_embed(:nested)
  end
end

defmodule MyApp.SchemaTest.Nested do
  use Ecto.Schema

  import Ecto.Changeset

  embedded_schema do
    field(:name, :string)
  end

  def changeset(struct, attrs \\ %{}) do
    cast(struct, attrs, [:name])
  end
end

iex(1)> MyApp.SchemaTest.changeset(%MyApp.SchemaTest{}) |> Ecto.Changeset.apply_changes()
%MyApp.SchemaTest{__meta__: #Ecto.Schema.Metadata<:built, "test">, nested: []}

From polymorphic embeds the default is actually nil, so I just used the existing code to add the default value as []

[mathieuprog]

I don't know if I miss something cause I'm on my mobile but I don't see any use of the :default in the changes, nor do I see the test checking on [].

[Matsa59]

Ecto's embeds_many put the default to []. Field put default to nil. Because polymorphic embeds use field, at the end we have to add the default to []

[mathieuprog]

Oh whoops, I see. Sorry am on mobile and didn't touch for a while.

Can you check for the default value in the test? Cause I see no check for default [] value?

[Matsa59]

Oh whoops, I see. Sorry am on mobile and didn't touch for a while. no problem ;)

The added test failed before I add default: [] so I guess it fixes the issue ^^

[mathieuprog]

The added test failed before I add default: [] so I guess it fixes the issue ^^

Are you sure? I commented default: [] out and the tests seem to run without fails.

[Matsa59]

oh yeah you're right ... forgeted to test the value of the context I have updated the test, now it checks the default

added changes

assert inserted_result.contexts == []

[mathieuprog]

What do you think about replacing this line 94 https://github.com/mathieuprog/polymorphic_embed/blob/v2.0.0/lib/polymorphic_embed.ex#L94 by:

put_in(changeset, [Access.key!(:data), Access.key!(field)], [])

And then we would no longer need that option?

[Matsa59]

Not a big fan :/ Using the default will allow us (if needs in the future) to override the default value (using opts). If we put_in changeset it gonna override the default value set using the opts.

[mathieuprog]

embeds_many has no :default option, because I guess it doesn't make sense especially for something that needs to be an array.

The philosophy of polymorphic embed is that it should work as closely as possible to its embeds_one/embeds_many counterparts. Hence options should ideally be the same, and you have noticed the use of generators in the test to make sure that both polymorphic embed and embeds_one/embeds_many behave in the exact same way.

You actually found a difference in behavior when an embed list is not provided in cast, and I believe we should make it behave the same way as embeds_many, thus without a default option it should result into an empty list.

[mathieuprog]

By the way, my suggestion should only be applied for lists of embeds, so there's a condition missing I guess

[mathieuprog]

Actually I'm not sure that my solution is really robust. What happens if we simply create a new struct %Reminder{}, with embeds_many the :contexts field is [] right? And with polymorphic embed array we always have nil... Maybe we need the default option. Not sure. Anyway I'm sleeping, talk tmw.

[Matsa59]

I used the default because it is the least "invasive"

[mathieuprog]

So as expected this will fail for the polymorphic embed, when simply creating a new struct (no cast):

 assert struct(reminder_module).contexts == []

I'm merging the PR, however I want to make the option mandatory and accept [] as the only possible value. As I'll introduce a breaking change, I'll bump version to 3.x.

I'm also thinking about introducing polymorphic_embeds_one and polymorphic_embeds_many as this would avoid the user to specify such required option.

So instead of:

    field(:contexts, {:array, PolymorphicEmbed},
      types: [
        location: PolymorphicEmbed.Reminder.Context.Location,
        age: PolymorphicEmbed.Reminder.Context.Age,
        device: PolymorphicEmbed.Reminder.Context.Device
      ],
      on_replace: :delete,
      default: []
    )

have:

    polymorphic_embeds_many(:contexts,
      types: [
        location: PolymorphicEmbed.Reminder.Context.Location,
        age: PolymorphicEmbed.Reminder.Context.Age,
        device: PolymorphicEmbed.Reminder.Context.Device
      ],
      on_replace: :delete
    )

I used the default because it is the least "invasive"

😂 yah I'm gonna screw this up

[mathieuprog]

Okay so actually I was wondering why we pass an array in the autogenerate_id function clause. And that is actually because of a bug I introduced when adding support for IDs 😬

So I'll merge but I'll eventually remove this function clause and fix ma shit.

[mathieuprog]

I fixed the bug, could you re-create a PR based on the new code? You can push the code you added in /test, the clause for autogenerate_id has been added.

[mathieuprog]

I released version 3 already with the macros mentioned above. Sorry that I couldn't merge your pr, I could only work on that today. Awesome contribution though!