WIP: ex_machina support #26

maennchen · 2020-12-21T17:07:02Z

No description provided.

maennchen · 2020-12-21T17:08:19Z

@mathieuprog This produces the following error:

mix test  
Compiling 3 files (.ex)

18:06:30.660 [info]  == Running 20000101000000 PolymorphicEmbed.CreateTables.change/0 backward

18:06:30.662 [info]  drop table reminders

18:06:30.665 [info]  == Migrated 20000101000000 in 0.0s
warning: redefining module PolymorphicEmbed.CreateTables (current version defined in memory)
  test/support/migrations/20000101000000_create_tables.exs:1


18:06:30.702 [info]  == Running 20000101000000 PolymorphicEmbed.CreateTables.change/0 forward

18:06:30.702 [info]  create table reminders

18:06:30.711 [info]  == Migrated 20000101000000 in 0.0s
........

  1) test inputs_for/4 (PolymorphicEmbedTest)
     test/polymorphic_embed_test.exs:434
     Assertion with == failed
     code:  assert contents == ~s"<input id=\\"reminder_channel___type__\\" name=\\"reminder[channel][__type__]\\" type=\\"hidden\\" value=\\"email\\"><input id=\\"reminder_channel_address\\" name=\\"reminder[channel][address]\\" type=\\"text\\" value=\\"a\\">"
     left:  "<input id=\"reminder_channel___type__\" name=\"reminder[channel][__type__]\" type=\"hidden\" value=\"email\"><input id=\"reminder_channel_address\" name=\"reminder[channel][address]\" type=\"text\">"
     right: "<input id=\"reminder_channel___type__\" name=\"reminder[channel][__type__]\" type=\"hidden\" value=\"email\"><input id=\"reminder_channel_address\" name=\"reminder[channel][address]\" type=\"text\" value=\"a\">"
     stacktrace:
       test/polymorphic_embed_test.exs:456: (test)

.........

Finished in 0.3 seconds
18 tests, 1 failure

I'm not sure what the issue is.

maennchen · 2020-12-21T17:11:13Z

(Allow edits by maintainers is on, feel free to change things yourself...)

mathieuprog · 2020-12-21T17:30:26Z

I think the key was to change the cast function to accept a struct instead of map of attributes?
Could you explain why cast receives a struct?

maennchen · 2020-12-21T17:33:16Z

@mathieuprog We're not really casting anything at all anymore since we have a custom cast fn. We could also just raise an error to cast via our fn instead. That will break ex_machina however.

mathieuprog · 2020-12-21T17:38:24Z

Why did you decide to change the cast function if it's kinda unused? Just curious

maennchen · 2020-12-21T17:59:48Z

Because i simplified some things that caused problems while refactoring. My first choice was to raise an error, but that breaks a test.

Also I want an error if people try to cast via the normal cast.

If you're ok with breaking the machina test, I'd rather just raise an error no matter what is provided to the fn.

mathieuprog · 2020-12-21T18:06:01Z

There will be complaints. What if we check if we are in the test environment and that ex_machina is a dependency?

maennchen · 2020-12-21T18:14:47Z

@mathieuprog that would be a possibility. I'm not sure though why we would not allow this in general for other libraries.

The complete opposite would work too:

deprecate custom cast
People use normal cast instead

mathieuprog · 2020-12-21T18:22:05Z

You mean stop using cast_polymorphic_embed in favor of cast?
That won't work. The library at first worked through cast. Only recently cast_polymorphic_embed has been introduced. To receive the changeset.
#10

maennchen · 2020-12-21T18:32:09Z

@mathieuprog that makes sense

We could also forbid cast alltogether and provide a custom ex machina strategy. it seems one should be able to override it. That way we would not give it access to special / private apis

maennchen · 2020-12-21T18:37:35Z

@mathieuprog I'll push a "strict" version with an error being raised in cast and I'll see if we can add an ex_machina strategy.

mathieuprog · 2020-12-21T18:42:30Z

That's awesome 😐 of course this is the way to go... We need to update the readme as well in order to document the ex_machina strategy and how to use it. Just a code snippet is sufficient.

mathieuprog · 2020-12-21T19:04:24Z

The reason why we added cast_polymorphic_embed is to get the changeset to retrieve the existing data. That's why you could see in the code the merge of the data with the params. I don't think there is any way to use cast and keep the existing data.

The failing test before the introduction of cast_polymorphic_embed is named "keep existing data".

maennchen · 2020-12-21T19:28:49Z

@mathieuprog I've amended the PR. This shows that it is possible in general, but I've copied a lot of code.

I think if we like this route in general, we should open a ticket at ex_machina to make this simpler without copying everything.

mathieuprog · 2020-12-21T19:30:55Z

You decided to return the :error atom instead of raising eventually? What are your reasons?

maennchen · 2020-12-21T19:34:29Z

@mathieuprog No real reason, I'm not sure which is better.

mathieuprog · 2020-12-21T19:35:23Z

Raising an error allows to send a message to the developer, e.g. use cast_polymorphic_embed for field X instead

maennchen · 2020-12-21T19:41:28Z

@mathieuprog Before we talk about details:

How do you like the PRs direction in general?
Would you be willing to coordinate with ex_machina to resolve the issue of a lot of duplicated code?

maennchen · 2020-12-21T19:51:11Z

@mathieuprog The oposite could also be done: Add optional support to ex_machina instead of this way around. That would be easier to implement I think.

mathieuprog · 2020-12-21T19:53:18Z

The PRs are amazing:) I can also grant you more rights in this project if you'd like to contribute more in the future, as you have a good understanding of the codebase. Let me know in that case.

For ex_machina I have been thinking that you did enough. If now an ex_machina user who has the need of using ex_machina with polymorphic_embed asks for support (and the support and test that you have seen actually were written based on a request from an ex_machina user), we now offer a base (what you have done) to work from; he is free to contribute more for better support.

I think we both have a lack of interest to work with ex_machina so I consider this already a great effort for anyone who needs it.

mathieuprog · 2020-12-21T19:56:22Z

The opposite could also be done: Add optional support to ex_machina instead of this way around

I prefer the least possible dependency to ex_machina. So the strategy is the way to go?

maennchen · 2020-12-21T20:00:05Z

@mathieuprog I'm already maintainer of far too many libraries and I'm fine just creating PRs :)

What are you saying with this?

Should we add the files as they are in this PR?
Should we remove the ex_machina specific stuff and people can look up this PR for reference?

In any case I can change the :error => raise to an error. What error message would you like?

PolymorphicEmbed must not be casted using Ecto.Changeset.cast/3, use PloyMorphicEmbed.cast_polymorphic_embed/2 instead.

I prefer the least possible dependency to ex_machina. So the strategy is the way to go?

We'll have some kind of chicken / egg problem here. I suppose ex_machina might say the same thing. The only difference is that this library would have to duplicate basically the whole ecto strategy while ex_machina only has to add a few lines.

mathieuprog · 2020-12-21T20:10:26Z

Should we add the files as they are in this PR?

I didn't look thoroughly yet but I would add the files, and redirect a future requester to these files and this discussion. But of course feel free to open an issue on ex_machina's side, but that's up to you:D

I personally don't mind to have files in draft state in an ex_machina folder.

What error message would you like?
PolymorphicEmbed must not be casted using Ecto.Changeset.cast/3, use PloyMorphicEmbed.cast_polymorphic_embed/2 instead.

Good but it's cast/4, not cast/3
And "PloyMorphicEmbed" has typos.
👍

mathieuprog · 2020-12-22T08:59:03Z

How do you see finishing up that PR? I think all what's left is the error message? Or do you have anything else in mind?

maennchen · 2020-12-22T11:03:16Z

@mathieuprog Changed

About the files from ex_machina: Can we include MIT licensed code in an Apache licensed library?

mathieuprog · 2020-12-22T11:16:59Z

I will study your ex_machina code now:) by the way you mentioned there was an error (#26 (comment)); I checkout the code and the tests pass. So you fixed that error?

maennchen · 2020-12-22T11:18:48Z

@mathieuprog I fixed the test, there was an error in my code that the test correctly identified.

mathieuprog · 2020-12-22T11:47:45Z

Issue posted on ex_machina's repo:
beam-community/ex_machina#407

mathieuprog · 2020-12-22T11:55:14Z

It might take several days, maybe weeks to get an answer, so can we remove the support of ex_machina at all for now?
Also I linked a file in the ex_machina issue, so if you push further maybe that link will not work anymore? Is it possible to keep your work for a while while removing it for merging?

mathieuprog · 2020-12-22T12:01:23Z

Maybe we can create a new PR based on this one, and prefix this one by "[ON HOLD]" ?

maennchen · 2020-12-22T12:03:38Z

@mathieuprog I'll create a new one without the ex_machina files.

maennchen · 2020-12-22T12:07:51Z

@mathieuprog This PR now only contains ex_machina stuff and is based on the new #27

mathieuprog · 2020-12-22T12:20:00Z

Perfect!

richthedev · 2021-03-19T13:32:04Z

Hello 👋 is there a chance this could be merged soon ?

maennchen · 2021-03-19T14:03:57Z

@richthedev This is currently such an ugly hack that I don't think it should be merged.

It is really ugly since a lot of code had to be copied since we couldn't hook into the existing ex_machina code.

The options currently are:

Wait until we get some results in this PR & Cast polymorphic embeds beam-community/ex_machina#407
Copy the files from this PR and add them to you own project

richthedev · 2021-03-19T14:50:22Z

@maennchen Thanks for the swift response.

cjbell · 2021-03-28T16:41:50Z

lib/ex_machina/ecto_strategy.ex

+    defp schema_polymorphic_embeds(schema) do
+      Enum.filter(
+        schema.__schema__(:fields),
+        &match?({:parameterized, PolymorphicEmbed, _options}, schema.__schema__(:type, &1))
+      )
+    end


FYI this currently won't match arrays of polymorphic embeds. The fix is:

defp schema_polymorphic_embeds(schema) do Enum.filter( schema.__schema__(:fields), fn field -> case schema.__schema__(:type, field) do {:parameterized, PolymorphicEmbed, _options} -> true {:array, {:parameterized, PolymorphicEmbed, _options}} -> true _ -> false end end ) end

mathieuprog · 2021-05-18T08:41:53Z

An ex_machina maintainer beam-community/ex_machina#407 asked for ideas but we didn't get back with any yet.
I thought of a few:

1. Use of config in the main application

config :ex_machina, :cast_custom_types, MyApp.ExMachina.CastCustomTypes

MyApp.ExMachina.CastCustomTypes would have to implement a behaviour such as:

defmodule MyApp.ExMachina.CastCustomTypes do
  @behaviour ExMachina.CastCustomTypes
  
  @impl true
  def cast_custom_types(struct) do
    struct
    |> PolymorphicEmbed.ExMachina.CastCustomTypes.cast_custom_types()
    |> OtherLibrary.ExMachina.CastCustomTypes.cast_custom_types()
  end

  @impl true
  def schema_custom_types(struct) do
    PolymorphicEmbed.ExMachina.CastCustomTypes.schema_custom_types(struct)
      ++ OtherLibrary.ExMachina.CastCustomTypes.schema_custom_types(struct)
  end
end

defmodule PolymorphicEmbed.ExMachina.CastCustomTypes do
  @behaviour ExMachina.CastCustomTypes
  
  @impl true
  def cast_custom_types(struct) do
    cast_polymorphic_embeds(struct)
  end

  @impl true
  def schema_custom_types(struct) do
    schema_polymorphic_embeds(struct)
  end
end

This would be the easiest to code on ex_machina's side because the configuration is globally accessible.

2. Use of metaprogrammaing

The other way to inject code is through the use of use, but it seems already very complex ex_machina's side where you have use-inception: https://github.com/thoughtbot/ex_machina/blob/v2.7.0/lib/ex_machina/strategy.ex#L63

Then we'd have in the tests:

use ExMachina.Ecto, strategy: MyApp.ExMachina.EctoStrategy

defmodule MyApp.ExMachina.EctoStrategy do
  use ExMachina.EctoStrategy, cast_custom_types: [PolymorphicEmbed.ExMachina.CastCustomTypes]
end

defmodule PolymorphicEmbed.ExMachina.CastCustomTypes do
  @behaviour ExMachina.CastCustomTypes
  
  @impl true
  def cast_custom_types(struct) do
    cast_polymorphic_embeds(struct)
  end

  @impl true
  def schema_custom_types(struct) do
    schema_polymorphic_embeds(struct)
  end
end

3. Dependency

Or ask ex_machina maintainers to detect the use of PolymorphicEmbed library and hardcode a module to call:

if Code.ensure_loaded?(PolymorphicEmbed) do
  PolymorphicEmbed.ExMachina.#function
end

And have ex_machina have an optional dependency on PolymorphicEmbed.

Thoughts?

nallwhy · 2021-11-29T09:58:38Z

Any updates?
ExMachina is a very common library for Ecto users. I really hope this problem will be solved soon.

mathieuprog · 2021-12-02T00:10:38Z

@nallwhy it does not seem it will be solved any time soon except if someone that needs this comes up with a solution. The ex_machina library needs to call our custom cast function, there's an issue open on their side.

ElijahBrandyberry · 2024-02-15T00:01:23Z

lib/ex_machina/ecto_strategy.ex

+    defp cast_polymorphic_embeds(%{__struct__: schema} = struct) do
+      schema
+      |> schema_polymorphic_embeds()
+      |> Enum.reduce(struct, fn embed_key, struct ->
+        casted_value = struct |> Map.get(embed_key)
+
+        Map.put(struct, embed_key, casted_value)
+      end)
+    end


This doesn't handle nested embeds, or auto-generated binary ids on the embed schema. Making this change does the trick:

Suggested change

defp cast_polymorphic_embeds(%{__struct__: schema} = struct) do

schema

|> schema_polymorphic_embeds()

|> Enum.reduce(struct, fn embed_key, struct ->

casted_value = struct |> Map.get(embed_key)

Map.put(struct, embed_key, casted_value)

end)

end

defp cast_polymorphic_embeds(%{__struct__: schema} = struct) do

schema

|> schema_polymorphic_embeds()

|> Enum.reduce(struct, &cast_polymorphic_embed/2)

end

defp cast_polymorphic_embed(embed_key, struct) do

casted_value =

case Map.get(struct, embed_key) do

embeds when is_list(embeds) -> Enum.map(embeds, &build_polymorphic_embed/1)

embed when is_map(embed) -> build_polymorphic_embed(embed)

embed -> embed

end

Map.put(struct, embed_key, casted_value)

end

defp build_polymorphic_embed(%{__struct__: schema} = struct) do

case schema.__schema__(:autogenerate_id) do

{key, _source, :binary_id} -> Map.put(struct, key, Ecto.UUID.generate())

_ -> struct

end

|> cast()

end

mathieuprog · 2024-05-27T05:15:52Z

Closing. See: beam-community/ex_machina#407 (comment)

maennchen mentioned this pull request Dec 21, 2020

Use Ecto.embedded_load/3 #25

Merged

maennchen force-pushed the refactor_cast branch from 7eac451 to d1c9b35 Compare December 21, 2020 19:27

maennchen force-pushed the refactor_cast branch from d1c9b35 to 6c14b77 Compare December 22, 2020 11:00

maennchen mentioned this pull request Dec 22, 2020

Refactor Cast #27

Merged

ex_machina support

31547b4

maennchen force-pushed the refactor_cast branch from 6c14b77 to 31547b4 Compare December 22, 2020 12:06

maennchen changed the title ~~Refactor Cast~~ WIP: ex_machina support Dec 22, 2020

mathieuprog added the enhancement New feature or request label Dec 25, 2020

cjbell reviewed Mar 28, 2021

View reviewed changes

This was referenced May 18, 2021

feat: allow changing on_cast behavior #40

Closed

Cast polymorphic embeds beam-community/ex_machina#407

Closed

mathieuprog added the feeback needed label May 20, 2021

Miradorn mentioned this pull request Nov 13, 2023

Merge upstream PRs remoteoss/polymorphic_embed#3

Open

2 tasks

ElijahBrandyberry reviewed Feb 15, 2024

View reviewed changes

mathieuprog closed this May 27, 2024

-    defp cast_polymorphic_embeds(%{__struct__: schema} = struct) do
-      schema
-      |> schema_polymorphic_embeds()
-      |> Enum.reduce(struct, fn embed_key, struct ->
-        casted_value = struct |> Map.get(embed_key)
-        Map.put(struct, embed_key, casted_value)
-      end)
-    end
+    defp cast_polymorphic_embeds(%{__struct__: schema} = struct) do
+      schema
+      |> schema_polymorphic_embeds()
+      |> Enum.reduce(struct, &cast_polymorphic_embed/2)
+    end
+    defp cast_polymorphic_embed(embed_key, struct) do
+      casted_value =
+        case Map.get(struct, embed_key) do
+          embeds when is_list(embeds) -> Enum.map(embeds, &build_polymorphic_embed/1)
+          embed when is_map(embed) -> build_polymorphic_embed(embed)
+          embed -> embed
+        end
+      Map.put(struct, embed_key, casted_value)
+    end
+    defp build_polymorphic_embed(%{__struct__: schema} = struct) do
+      case schema.__schema__(:autogenerate_id) do
+        {key, _source, :binary_id} -> Map.put(struct, key, Ecto.UUID.generate())
+        _ -> struct
+      end
+      |> cast()
+    end

WIP: ex_machina support #26

WIP: ex_machina support #26

Conversation

maennchen commented Dec 21, 2020

maennchen commented Dec 21, 2020

maennchen commented Dec 21, 2020

mathieuprog commented Dec 21, 2020 • edited Loading

maennchen commented Dec 21, 2020

mathieuprog commented Dec 21, 2020

maennchen commented Dec 21, 2020

mathieuprog commented Dec 21, 2020

maennchen commented Dec 21, 2020

mathieuprog commented Dec 21, 2020

maennchen commented Dec 21, 2020

maennchen commented Dec 21, 2020

mathieuprog commented Dec 21, 2020

mathieuprog commented Dec 21, 2020

maennchen commented Dec 21, 2020

mathieuprog commented Dec 21, 2020

maennchen commented Dec 21, 2020

mathieuprog commented Dec 21, 2020

maennchen commented Dec 21, 2020

maennchen commented Dec 21, 2020

mathieuprog commented Dec 21, 2020 • edited Loading

mathieuprog commented Dec 21, 2020

maennchen commented Dec 21, 2020

mathieuprog commented Dec 21, 2020

mathieuprog commented Dec 22, 2020

maennchen commented Dec 22, 2020

mathieuprog commented Dec 22, 2020

maennchen commented Dec 22, 2020

mathieuprog commented Dec 22, 2020

mathieuprog commented Dec 22, 2020

mathieuprog commented Dec 22, 2020

maennchen commented Dec 22, 2020

maennchen commented Dec 22, 2020

mathieuprog commented Dec 22, 2020

richthedev commented Mar 19, 2021

maennchen commented Mar 19, 2021

richthedev commented Mar 19, 2021

cjbell Mar 28, 2021

Choose a reason for hiding this comment

mathieuprog commented May 18, 2021

nallwhy commented Nov 29, 2021

mathieuprog commented Dec 2, 2021

ElijahBrandyberry Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

mathieuprog commented May 27, 2024

mathieuprog commented Dec 21, 2020 •

edited

Loading

mathieuprog commented Dec 21, 2020 •

edited

Loading

ElijahBrandyberry Feb 15, 2024 •

edited

Loading