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

:map type in Schemaless Changeset.cast/3 only accepts single type #4271

Open
ozthegnp opened this issue Aug 25, 2023 · 14 comments
Open

:map type in Schemaless Changeset.cast/3 only accepts single type #4271

ozthegnp opened this issue Aug 25, 2023 · 14 comments

Comments

@ozthegnp
Copy link

Elixir version

1.14.4

Database and Version

n/a

Ecto Versions

3.9.5

Database Adapter and Versions (postgrex, myxql, etc)

n/a

Current behavior

This works, and it means that all keys in the map must be strings.

types = %{foo: {:map, :string}}
cs = cast({%{}, types}, %{foo: %{bar: "hello", baz: "world"}}, Map.keys(types))
IO.inspect(cs.valid?)IO.inspect(cs.valid?)
# true

This also works to detect non-string fields within the nested map.

types = %{foo: {:map, :string}}
cs = cast({%{}, types}, %{foo: %{bar: 3}}, Map.keys(types))
IO.inspect(cs.valid?)
# false
IO.inspect(cs.errors)
# errors: [foo: {"is invalid", [type: {:map, :string}, validation: :cast]}]

However, when we try to define a specific type of the nested key, an error is raised.

types = %{foo: {:map, %{bar: :string}}}
Ecto.Changeset.cast({%{}, types}, %{foo: %{bar: "hello}}, Map.keys(types))
# ** (FunctionClauseError) no function clause matching in Ecto.Type.cast_fun/1

# The following arguments were given to Ecto.Type.cast_fun/1:

#     # 1
#     %{bar: :string}

It seems we are limited to uniform types exclusively.

Expected behavior

We should be able to define custom types for each key in the map. e.g:

types = %{foo: {:map, %{bar: :string, baz: :integer}}}
Ecto.Changeset.cast({%{}, types}, %{foo: %{bar: "hello", baz: 1}}, Map.keys(types))
IO.inspect(cs.valid?)
# true

Thank you

@greg-rychlewski
Copy link
Member

I think if you are going to define the keys like that in the types then you could go with an embedded schema instead?

@ozthegnp
Copy link
Author

I think if you are going to define the keys like that in the types then you could go with an embedded schema instead?

It would be nice to do validations on schemaless embedded maps. In my case, I'm trying to validate the payload of a RESTful PATCH call. Creating schemas brings complexity to the code and also generates a primary key for the objects, which is not needed in certain cases.

The current behavior of schemaless seems counterintuitive for type :map as it enforces the same type for all keys which I think could be a desired behavior in very few cases.

I also tried to implement casting with Ecto.ParameterizedType, but nested error messages are not supported which kinda defeats the purpose of tracking down lower level conflicts.

For {:array, CustomType} or {:map, CustomType} the returned keyword list will be erased and the default error will be shown.

@greg-rychlewski
Copy link
Member

Thanks for the context. I'm not too sure what to do atm but just a few things to note:

  1. {:map, type} is not just for schemaless fields it's for schemas as well. So we would need to be able to handle the proposal when going back/forth from the database . Or maybe say they are not allowed for schemas. But that might be a hard position to maintain when people ask for it in schemas.
  2. You can disable primary key generation for embedded schemas.

@josevalim
Copy link
Member

It would be nice to do validations on schemaless embedded maps. In my case, I'm trying to validate the payload of a RESTful PATCH call. Creating schemas brings complexity to the code and also generates a primary key for the objects, which is not needed in certain cases.

This is the important context! It is always best to describe the problem so it gives more space for solutions. :)

I am happy to allow more nesting on schemaless changesets, but I don’t think it should be via the map type, especially since maps are often serialized as JSON which is a bit different from how other values are serialized to the database.

one option would be to alllow %{child: %{attr: :string}}, specifically as a schemaless changeset feature, but investigating how to connect this feature with embeds would be important too.

@ozthegnp
Copy link
Author

@greg-rychlewski , Thanks for the notes!

{:map, type} is not just for schemaless fields it's for schemas as well. So we would need to be able to handle the proposal when going back/forth from the database . Or maybe say they are not allowed for schemas. But that might be a hard position to maintain when people ask for it in schemas.

That makes sense, I forgot that it could be used in schemas as well.

You can disable primary key generation for embedded schemas.

Great, despite the complexity, this could help me to go in the embedded schema direction.

@ozthegnp
Copy link
Author

@josevalim Thanks, I'll make sure to add context from the begging.

I am happy to allow more nesting on schemaless changesets, but I don’t think it should be via the map type, especially since maps are often serialized as JSON which is a bit different from how other values are serialized to the database.

one option would be to alllow %{child: %{attr: :string}}, specifically as a schemaless changeset feature, but investigating how to connect this feature with embeds would be important too.

Also, thanks for considering adding a child option for schemaless changesets. After thinking about it a bit more, I realized that besides connecting the feature with embeds, we would also have to consider how to target nested values with the validation functions. I guess we could pass a list of atoms, but this would bring a lot more complexity than I initially thought.

Based on @greg-rychlewski's suggestion wouldn't mind going in the direction of embedded schemas. What do you think about adding a traverse_changes/2 function for convenience?

@josevalim
Copy link
Member

Sorry, I was on my phone, what I meant for :child was something like:

post = %{title: :string, author: %{name: ...}}

Notice that, for embedded schemas, we do this:

struct = Ecto.Embedded.init(opts)
Module.put_attribute(mod, :ecto_changeset_fields, {name, {:embed, struct}})

One possible option would be for us to change that to do this:

struct = Ecto.Embedded.init(opts)
Module.put_attribute(mod, :ecto_changeset_fields, {name, {:parameterized_type, Ecto.Embedded, struct}})

If we could make this work, then you could use "schemaless embeds" (we could improve the API ergonomics too). It was a while since we tried and we ran into corner cases, but it may be worth trying again.

@greg-rychlewski
Copy link
Member

+1 from me

@ozthegnp
Copy link
Author

@josevalim thanks for explaining the lower level implementation of embeds. Yeah, this sounds like something we could use for our problem. I'm a regular user of the library but haven't contributed yet. If there is anything I can help with, please let me know and I would be more than happy to make this happen. Thanks!

@Anonyfox
Copy link

Running into the very same issue from a different angle. I have some dynamic key/value pairs of questions that admins can create freely, and I "embed" customers values for these questions as a map into a db column and still need validations. Basically like a dynamic CMS. At this point I am totally lost because it seems I cannot embed_one with a map structure only known at runtime and the code to get form fields/validations working is becoming tedious!

@josevalim
Copy link
Member

josevalim commented Sep 30, 2023

Note you can use :map, which means we won't perform any checking whatsoever. So you can have an embed, perform all validation logic, and then load/dump it into the :map column. :)

@Anonyfox
Copy link

this actually is a nice idea, thanks @josevalim ! my problem is "just" that final part, where applying the changeset to the data struct it gets blanked out again and I end up with NULL values in my db :-( but kinda virtual embeds_one schema that has no fields (gets stuff at runtime) and I sync this to the actual map field within the changeset pipeline ... weird but might help :-)

@alvivi
Copy link

alvivi commented Oct 24, 2023

I found myself in a similar situation, but in our case the map is a schema, as it's being used in different places. I thought I could use something like:

{%{}, %{foo:  Ecto.ParameterizedType.init(Ecto.Embeded, cadinality: :one, field: :foo, related: Foo)}}
|> cast([:foo])
|> ...

but I think that this pattern match (/lib/ecto/embedded.ex:117):

def cast(%{__struct__: schema} = struct, %{cardinality: :one, related: schema}) do

is blocking us, as there is no __struct__ there.

@josevalim josevalim changed the title :map type in Schemaless Changeset.cast/3 only accepts single type. :map type in Schemaless Changeset.cast/3 only accepts single type Nov 14, 2023
@senconscious
Copy link

Hello, I also encountered problem with using schemaless embeds. I wanted to cast and validate params from requests. But I also need to invoke Enum functions on casted params. So in the end I want my casted params to be a map (recursively).

This can be achieved via implementing Enumarable protocol for each embedded schema, or simply calling recursively Map.from_struct/2.

But It'll be much simpler if we've got schemaless embeds out of the box. I forked and implemented dummy prototype but I'm not sure whether it is a valid approach according to the conversation. As it's not providing new flow for bare map type but rather builds on top on embeds.

Also I haven't tested in with updating existing data, as for validation I always build new map.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants