snofang

snofang

Ecto.CastError Mixed Keys Issue

In a typical business development task, having a function in a context module which accepts attributes of map type and passes them to Ecto for a validated Changeset is a common case.

  def create_foo(%{} = attrs) do
    attrs
    |> Foo.create_changeset()
    |> Repo.insert()
  end

Those functions may be called from everywhere; Normally, it is quite common to express the keys by atom while calls from Phoenix Controllers and LiveViews have keys in string. So these business functions should support both types of keys and thanks to the cast method, they do by default.

But, what if by some requirement it is needed to change the passed in attributes before passing them to Ecto for changeset or validations? By which data type the attribute keys should be addressed? string or atom? Suppose the following function:

  def create_bar_foo(%{} = attrs) do
    attrs
    |> Map.put(bar_case: :default_or_computed_value)
    |> Foo.create_changeset()
    |> Repo.insert()
  end

If it called from a test with normal atom keys, every thing would be fine while if it called from a LiveView with string keys, there will be a raise:

(Ecto.CastError) expected params to be a map with atoms or string keys, got a map with mixed keys ...

So the keys should be normalized before processing and this article suggest some solution. Also this post addresses it somehow.

There can be lots of other solutions to this problem, for example one can simply cast passed attributes before manipulation them and so on. But I think this can be included in Ecto.Changeset.cast method because:

  • Ecto.Changeset.cast is already supporting both atom and binary keys and also touching the case by doing conversion from string keys to atom ones. Also it sounds feasible to support mix of both types.
  • There is no harm to existing codes as actually a new capability is being added.
  • It encourages having some business implemented in context before using Ecto directly or having lots of ###_changeset functions per specific business case.

Most Liked

tfwright

tfwright

I think a more specific example use case might me useful here because I do think it is quite rare, but otherwise I strongly agree with other commenters that modifying params is an antipattern. In fact I would go as far as to say even if it was supported I wouldn’t make use of it. We as developers are naturally lazy and that is generally speaking a good thing as it motivates us to avoid producing spaghetti code. But here the extra code actually simplifies the design because it represents the innate complexity of the data insofar as it implicitly must deal with multiple input sources (as José says if the param in question is system data there is no reason to cast it). When troubleshooting/debugging or even just grokking part of a program it is very useful to be able to clearly and easily trace the path of a piece of data, and if necessary, modify it in isolation. Like many things it might seem a simple matter to recognize the “mixing” these things tend to multiply overtime and in aggregate produce more fragile, less maintainable code.

A computed/calculated value wouldn’t necessarily call for an extra changeset, but as shown in examples above it has its own logic path which starts from the changeset, not the changeset params.

sodapopcan

sodapopcan

I respectfully disagree with this whole proposal.

You semi-handwaved this solution but for me this is the right answer and one of those things I’m pretty religious about in my own code. We should be converting—ie casting—our data into a known shape before doing anything with it, and this is exactly what Changeset.cast provides us. This is especially important in a dynamic language.

For me, string keys mean “untrusted”. Using this definition, general application code should rarely ever have a need to set a string key (of course there are always exceptions).

Multiple changesets are generally encouraged by the framework. “Citation needed,” yes sorry I don’t have doc or discussion links atm, the best I can give right now is to look at the User schema generated by phx_gen_auth. I actually do prefer to keep a single changeset myself when possible (it’s not a hard rule) but it results in me writing functions like maybe_assign_slug/1 which are probably more complex than they need to be if I’d just use multiple changesets.

If you really want to do these things in the context, there is no harm in manipulating changesets in the context—changesets are kind of wild as they are having a strong presence in the web layer as well as the business layer.

Of course, you could also do stuff like this in the context:

def create_article(attrs) do
  %Article{}
  |> Article.changeset(attrs)
  |> MyApp.ChangesetHelpers.assign_slug_from(:title) # module name for illustrative purposes :D
  |> Repo.insert()
end

Finally, I think any promotion of the idea that mixing map key types is ok is a bad idea, again, especially in a dynamic language.

josevalim

josevalim

Creator of Elixir

It all depends on what is the source of the data.

If the source of the data is your own application, validations are pointless, because it makes no sense to tell the user that “bar_case is invalid” when they have no power over setting :bar_case. My reply was written from that perspective (I will clarify this in my previous message to avoid future confusion).

However, if you are manipulating other external params to compute values (and now reading back on your original thread, you said that this is indeed the case), then I agree with your concerns. In such cases, I would try to cast the initial params, then compute additional changes, and then validate the additional changes:

data
|> cast(params, ~w(foo baz))
|> validate_foo_and_baz()
|> compute_bar_case_as_change()
|> validate_bar()

But even this requires care. If you add a validation error to bar, and it is a computed value from foo and baz, you need to make sure the error points to the correct place.

tfwright

tfwright

This is why I suggested more specific examples, there is no universal law here, but generally it is a matter of avoiding being overly defensive. Yes it is advisable to place safeguards at boundaries (e.g. a db constraint) but adding a changeset validation to a data set in the same function is a poor safeguard because it is as easy for a dev to remove a validation as it is to change the data path, and in such a case an application error would actually be significantly more useful than a changeset error.

Just to reiterate, the problem with overly defensive code (which otherwise sounds like an oxymoron because our user’s data is Very Important etc) is that there are an infinite number of potential bugs that could arise from developer error. Code can only reasonably guard against user error. In the case of a database constraint, we can think of our application as a “user” of our database, which has its own internal logic with different goals (persistence and enforcement of relationships vs data transformation).

josevalim

josevalim

Creator of Elixir

As @joaquinalcerro mentioned, my suggestion is to pass any additional default value explicitly instead of trying to manipulate params.

EDIT: the code below assumes that the bar_case is computed based on already valid and existing data. If you need to compute the data based on external potentially invalid parameters, then keep on reading the thread. :slight_smile:

You could do this:

  def create_foo(%{} = attrs) do
    attrs
    |> Foo.create_changeset(bar_case: :default_or_computed_value)
    |> Repo.insert()
  end

And then create_changeset does:

  def create_changeset(params, defaults \\ []) do
    %Foo{}
    |> cast(params)
    |> change(defaults)
  end

Alternatively you can do this:

  def create_foo(%{} = params) do
    %Foo{bar_case: :default_or_computed_value}
    |> Foo.create_changeset(params)
    |> Repo.insert()
  end

Etc. Feel free to change create_changeset as necessary to conform to your code.

Where Next?

Popular in Proposals: Ideas Top

jakeprem
Goal: To make JS.patch and JS.navigate more interoperable with JS.push. Scenario: Imagine making a reusable Phoenix component and you wa...
New
dimitarvp
To @jonatanklosko and @the-mikedavis: I see that there is a Rust crate at crates.io: Rust Package Registry but it is pointing at https:/...
New
mudasobwa
I am not sure it deserves to be discussed in the mailing list, so I’d start here. assert/1 and/or refute/1 macros print the following er...
New
MeerKatDev
many times we do stuff like (e.g. test setups, json views in phoenix) aaa = ... bbb = ... ccc = ... %{aaa: aaa, bbb: bbb, ccc: ccc} and...
New
rhcarvalho
Hi all, I would like to gather some feedback before a more intentional proposal to add a new :depth option when specifying a Git depende...
New
markevans
Hi! I’m excited about everything that’s going on re. gradual typing and am really pleased to see that Jose and the team seem to be think...
New
pdgonzalez872
Hi! There has been some discussion about hiring/jobs on here and I thought about running this by everyone. I wanted to try to help recr...
New
bartblast
This could resolve to {[a: 1, b: 2]}. Was it ever considered to allow such syntax? Notice this: {:abc, a: 1, b: 2} and this: my_fun(:abc,...
New
cevado
IEx is a very powerfull shell and it would be awesome to have all this power integrated inside a code editor. Clojure enables something l...
New
sezaru
When writing my code, I always find __MODULE__ very useful to use as alias of that module “inner dependencies”, ex: alias __MODULE__.{Im...
New

Other popular topics Top

yurko
Here are few pieces of (common) Linux knowledge that we use for reasonably small one server apps. We use Ubuntu but this should work for ...
New
bsollish-terakeet
Credo is smart enough to check for (something like) this: assert length(the_list) == 0 with this response: Checking if an enum is empt...
New
albydarned
Hello all! I am typing this post from my new MacBook Pro with the M1 chip. I’m loving it so far, and will probably use it as my daily dr...
New
malloryerik
Hi, this is for people who, like me, have had some friction using .html.heex templates in VSCode. The solution seems to be, in a hyphena...
New
stefanchrobot
What’s the safe way to decode a JSON string into a struct? I want to avoid calling String.to_atom. Jason.decode can give me a map with st...
New
baxterw3b
Hi guys, i’m new in the Elixir world, and i have to say, that i love it! i’m having some problem to understand anonymous functions with ...
New
belgoros
I’m not a pro in using Regex and can’t figure out why the following behaviour happens, especially if we take into account the difference ...
New
Fl4m3Ph03n1x
About me? ( if you have nothing better to do than reading about some random guy in the internet :stuck_out_tongue: ) Hello all, this is ...
New
skosch
To my knowledge, put_in, Map.update etc. all have the one limitation of not automatically creating intermediate keys when needed (for exa...
New
jay1
Why is it that the mnesia database isn’t the most preferred database for use in Elixir/Phoenix?
New

We're in Beta

About us Mission Statement