elixir-ecto / ecto

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

Allow struct params in Ecto.Changeset.cast/4 #4168

Closed yordis closed 1 year ago

yordis commented 1 year ago

Elixir version

Elixir 1.14.4

Database and Version

PostgresSQL 15

Ecto Versions

3.9.5

Database Adapter and Versions (postgrex, myxql, etc)

postgrex 0.16.5

Current behavior

I used phx.gen.auth command as a baseline and modified it a bit.

We are leveraging Ecto Schemas and Changesets to validate Phoenix Parameters to ensure the "correctness" of the information at the Web/App level before passing it to other parts of the systems.

Here is my controller:

defmodule Web.Api.UpdatePasswordIamIdentitySetting do
  use Web, :controller

  defmodule Command do
    use OnePiece.Commanded.ValueObject # This is a wrapper around Ecto.Schema
    @enforce_keys [:current_password, :password, :password_confirmation]
    embedded_schema do
      field :current_password, :string
      field :password, :string
      field :password_confirmation, :string
    end
  end

  def handle(conn, params) do
    command = Command.new!(params) # this is the same as doing Ecto.Changeset.cast inside
    current_identity = IdentityAuth.current_identity(conn)

    # notice that I am passing `command` into it, which is not a map, but a struct of
    # `Web.Api.UpdatePasswordIamIdentitySetting.Command`
    # because the `new!/1` does the casting inside.
    with {:ok, identity} <- Iam.update_identity_password(current_identity, command.current_password, command) do
      conn
      |> IdentityAuth.log_in_identity(identity)
      |> send_resp(:ok, "")
    end
  end
end
defmodule Iam.Identity do
  # ... 

  def password_changeset(identity, attrs, opts \\ []) do
    identity
    |> cast(attrs, [:password]) # here is the issue
    # ...
  end
end
** (Ecto.CastError) expected params to be a :map, got: `%Web.Api.UpdatePasswordIamIdentitySetting.Command{current_password: "hello world!", password: "new valid password", password_confirmation: "new valid password"}`
     code: new_password_conn = post(conn, ~p"/api/iam/identities/settings/update-password", %{
     stacktrace:
       (ecto 3.9.5) lib/ecto/changeset.ex:498: Ecto.Changeset.cast/4
       (my_app 0.1.0) lib/my_app/iam/identity.ex:137: Iam.Identity.password_changeset/3

That is not the only place where we need to do this, we face these situations in other places as well, and we have to map all the keys to a map to avoid this issue.

Expected behavior

Be able to pass a struct to Ecto.Changeset.cast/4 function.

v0idpwn commented 1 year ago

This behaviour is pretty old and was added here: https://github.com/elixir-ecto/ecto/commit/1b224d19126fc26950f0185ddf204ce2245bd490

I believe it behaves this way because standard structs would make it impossible to differentiate when the field wasn't sent from when it was sent as nil, while with maps this is trivial.

josevalim commented 1 year ago

Correct. No plans to change this at the moment but you can always call Map.from_struct before. :)

yordis commented 1 year ago

I believe it behaves this way because standard structs would make it impossible to differentiate when the field wasn't sent from when it was sent as nil, while with maps this is trivial.

@v0idpwn that went over my head, do you mind sharing a bit more about it? I would love to understand a bit more.

v0idpwn commented 1 year ago

Assume we are updating the following schema: %Person{name: "John Doe", age: 30}, and you want to update the age to 31.

We can receive the following attrs: %{age: 31}. :name is not a key and isn't considered changed in the changeset, thus is not updated.

Now, assume we receive %{age: 31, name: nil}. This time, :name is being changed and will be set to nil in the database.

The thing with structs is that they have all the keys, so if you had the following struct:

defmodule UserAttrs do
  defstruct [:name, :age]
end

You wouldn't be able to represent the situation where the name isn't sent. If you do %UserAttrs{age: 31} its equivalent to %UserAttrs{age: 31, name: nil}.

yordis commented 1 year ago

Thank you so much for the extra context. I can see the potential problem now.

Although it makes sense if somebody is passing Map/Struct and you cast the value, I wouldn't expect much magic. If the value is nil, the new value is nil. At some point, people must understand their data flow.

Being said, a problem for another day. For now, the workaround is acceptable đŸ˜„