WIP: ex_machina support

[maennchen]

[maennchen]

@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]

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

[mathieuprog]

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]

@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]

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

[maennchen]

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]

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

[maennchen]

@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]

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. https://github.com/mathieuprog/polymorphic_embed/issues/10

[maennchen]

@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]

@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]

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]

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]

@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]

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

[maennchen]

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

[mathieuprog]

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

[maennchen]

@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]

@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]

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]

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]

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

What are you saying with this?

1. Should we add the files as they are in this PR?
2. 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]

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]

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]

@mathieuprog Changed

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

[mathieuprog]

I will study your ex_machina code now:) by the way you mentioned there was an error (https://github.com/mathieuprog/polymorphic_embed/pull/26#issuecomment-749089178); I checkout the code and the tests pass. So you fixed that error?

[maennchen]

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

[mathieuprog]

Issue posted on ex_machina's repo: https://github.com/thoughtbot/ex_machina/issues/407

[mathieuprog]

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]

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

[maennchen]

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

[maennchen]

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

[mathieuprog]

Perfect!

[richthedev]

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

[maennchen]

@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 & https://github.com/thoughtbot/ex_machina/issues/407
• Copy the files from this PR and add them to you own project

[richthedev]

@maennchen Thanks for the swift response.

[mathieuprog]

An ex_machina maintainer https://github.com/thoughtbot/ex_machina/issues/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]

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

[mathieuprog]

@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.