Add error reason to Ecto.Enum.cast/2

[satom99]

The current behaviour of Ecto.Enum.cast/2 is that only an :error atom is returned in case of error:

params = Ecto.Enum.init(values: [foo: 1, bar: 2, baz: 5])

assert :error = Ecto.Enum.cast("qux", params)

This PR changes the assertion above to be:

assert {:error, validation: :inclusion, enum: [:foo, "foo", 1, :bar, "bar", 2, :baz, "baz", 5]} =
               Ecto.Enum.cast("qux", params)

Where I chose to mimic the same :validation and :enum fields that Ecto emits for the Changeset.validate_inclusion/2 function.

Also, for this purpose I also defined a new Ecto.Enum.cast_values/1 function that returns all the possible values that the cast/2 function accepts — so [:foo, 1, :bar, 2, :baz, 5] for this example.

[josevalim]

I think including all values would be rather confusing. I would include only the on_cast keys, especially because the other ones are meant to be internal?

[satom99]

I am not sure. The cast/2 function really works with all those 3 values... so it makes sense that we include all 3. More so if validation: :inclusion where the error message can be understood as "must be one of %{enum}".

It is weird if we were to, say, emit this error from a JSON API, where atoms are transformed into strings. So [:foo, "foo", 1] would be transformed into ["foo", "foo", 1] by Jason and then we have duplicate values. But then I think it is on the final user to add a Enum.uniq/2 call, and not us here in Ecto.

But all in all, I think it is fine that we leave this as is 🤷

[josevalim]

My concern is that they can be inaccurate and we over-report. Or perhaps the mistake was in including dumped values by default. But I am certain we should not include both cast and load keys. load keys are atoms and it doesn't make sense to expose atoms (an Elixir construct) to an external system (which is allegedly when we call cast).

[satom99]

Agreed! However, us for example, we use embedded schemas as a replacement for vanilla structs and as a way to protect ourselves from ourselves.

We define functions like User.new/1 that rely on changesets under the hood. And in this case it would make sense that we do User.new(type: :admin). But maybe the :admin type was at some point removed, so this call will now fail. And because in this case schemas are used from within the system, and not necessarily from an external call, then it perhaps makes sense that we include atoms — so for example enum: [:regular, "regular", :moderator, "moderator"]. 🤔

[satom99]

@josevalim that was mostly just an example though. I am happy to change this to not include atoms, provided that the main intended usage of casting is from external data, as you mentioned. 🙂

And we still accept atoms but that remains as an implicit implementation detail.

Should we do that? Should we ask for a third opinion? Wdyt?

[greg-rychlewski]

I can kind of see it from two ways:

1. Cast is meant for normalizing/validating external data. In which case it only makes sense to include the on_cast keys.
2. Since on_dump is being utilized in the cast function, maybe some people are using this as a substitute for validate_inclusion as well. It seems a bit out of place but since it's there, I'm not sure if this was intentional. If it was intentional then maybe it makes sense for Ecto to be a bit "smart" in its error reporting. i.e. if the user provided an atom then only show the on_dump keys. Otherwise show the on_cast keys.

[greg-rychlewski]

After looking at the code more carefully, I believe the on_load and on_dump stuff in cast is meant for queries not for changesets. So my vote is for just publicizing on_cast keys in the error.

[satom99]

I am convinced 😄 I have made the necessary changes to only return on_cast keys in the error. Thank you both for your inputs! ❤️

[josevalim]

💚 💙 💜 💛 ❤️