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

More ways to specify embedded schema module #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pzingg
Copy link

@pzingg pzingg commented Nov 13, 2020

I was working on a library that uses polymorphic embeds. Since it's a library that is used by applications, it cannot know all the polymorphic types ahead of time. This PR adds two more ways of specifying the :types option for a PolymorphicEmbed Ecto type: a lookup function that translates between "type" keys and schema modules, and a :by_module method that means that the type key atom (or a string version) IS the module.

Feel free to ignore, edit or implement this another way.

I added two more embeds to the test Reminder module, added a number of tests, and added instructions in the README file.

lookup_type tests
@mathieuprog mathieuprog added the enhancement New feature or request label Dec 25, 2020
@mathieuprog
Copy link
Owner

Quite some work had to be done before I could look into this PR. I think we're definitely interested into adding this PR in the codebase. That's some quality work.

I guess you depend on your fork now? As there has been a lot of code refactoring in the meanwhile, would you be interested to bring these new functionalities into the current master code?

@Ivor
Copy link

Ivor commented Oct 3, 2022

This seems like a great feature that will make the library very dynamic. Any plans to merge this. What are the changes required to get this merged?

@mathieuprog
Copy link
Owner

The contributor created this PR while I was writing some major refactor when the library was in alpha.

It was created in November 2020 so the code changed quite a lot since then.

@fireproofsocks
Copy link

I haven't looked through this PR, but I would find it useful (and more common in my experience) if the field that qualified the polymorphism were outside the arguments. For example, I might have a database with columns for type and data, where type qualifies the ad-hoc data map, e.g. as x, y, or z. Often the code pivots on such a field to determine how to handle the data.

I worked around this by adding an append_type function that read a field from the incoming parameters and then inserted it (as an atom) into the map field used by the polymorphic join:

def changeset(x, %{} = params, opts \\ []) do
    params = append_type(params, :qualifier, :args)
    x
    |> cast(params, [...])
    # ...
end

  defp append_type(params, src_field, target_embed_field) do
    qualifier = Map.fetch!(params, src_field)

    Map.put(
      params,
      :args,
      params
      |> Map.get(target_embed_field, %{})
      |> Map.put(:__type__, String.to_atom(qualifier))
    )
  end

This works, but it feels a bit awkward because it cannot rely on the usual get_change/get_field etc. and the requirement that the qualifying value has to be an atom threw me for bit.

@a3kov a3kov mentioned this pull request Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feeback needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants