elixir-ecto / ecto

A toolkit for data mapping and language integrated query.
https://hexdocs.pm/ecto
Apache License 2.0
6.14k stars 1.43k forks source link

Adding custom embed validation errors to Ecto.Changeset #2055

Closed AndrewDryga closed 7 years ago

AndrewDryga commented 7 years ago

Hello guys,

We often face situation in which we have schema that contain :map or{:array, :map} in it's fields. For this fields we have custom validations (sometimes they are based on Ecto.Changeset, sometimes on JSON Schema's).

When we apply this rules it's hard to add validations errors to existing changeset, since it doesn't allow us to add custom embeds. cast_embed is hardcoded to work only with pre-defined embeds, and add_error does not allow to add errors that looks like ones that occurred in embed schema.

Right now we use hacks like this: https://github.com/Nebo15/annon.api/blob/master/lib/annon_api/plugin/json_schema_validator.ex#L28 in pair with our own very complex traverse_errors functions.

There are many ways to implement this, for a start of discussion I propose something like:

add_error_in(:settings, [:signature], "Signature is not base64 encoded string", validation: :cast, params: :base64)
josevalim commented 7 years ago

add_error_in sounds reasonable. The problem is how are we going to store those errors internally?

AndrewDryga commented 7 years ago

Maybe we can create Changeset on-a-fly (if it doesn't exist) and inject it into existing changeset struct? Thus we won't break any existing code.

josevalim commented 7 years ago

@AndrewDryga if that's the case then we should rather focus on the PR that adds support for dynamic embeds.

AndrewDryga commented 7 years ago

I did research on dynamic embeds a while ago, then I found issue where you were agains them :). I've even built a proof of concept that fools Ecto.Changeset validations: https://github.com/Nebo15/tokenizer.api/blob/master/lib/repo/changeset/dynamic_embed.ex

It also required hack with type that resolved which embeds can be in a :map: https://github.com/Nebo15/tokenizer.api/blob/master/lib/repo/types/dynamic_embed.ex

josevalim commented 7 years ago

Right, I don't think we should have dynamic embeds based on the struct key or something of sorts. If we are supporting dynamic ones, we should do it using schemaless changesets. So we can just pass a schemaless changeset around.

I think it even started here? https://github.com/elixir-ecto/ecto/issues/1763 :)

AndrewDryga commented 7 years ago

Ah, yes. Totally forgot about this one.

josevalim commented 7 years ago

Closing in favor of #1763. Once work there is done, we can discuss this issue once more.

jlevycpa commented 7 years ago

The test case in #2113 looks like it takes a schemaless changeset and embeds a named schema inside of that. I'm wondering if it's possible to embed a schemaless changeset inside of another schemaless changeset, and if so, what the syntax for that would be. Essentially I just want a way to hand off validation and casting of a :map or {:array, :map} field to another function.

It looks like there are still some __schema__ calls that would prevent this. Am I misunderstanding?

michalmuskala commented 7 years ago

Nested schemaless changesets are not supported as of right now. We'd like to do that, but it is not straightforward.

lessless commented 2 years ago

@michalmuskala any updates on nested schemaless changesets?