Canonical way to modifying a changeset before insertion...

[Mohamad El-Husseini]

I have a fairly typical account has_many users though account_users, where account_users is the joiner table. That table also has a role column. I want to modify this role column before I send the changeset to the Repo.

    defmodule App.AccountUser do

      schema "account_users" do

        belongs_to :account, App.Account

        belongs_to :user, App.User

      end

    

      def changeset(struct, params \\ %{}) do

        struct

        |> cast(params, [:account_id, :user_id, role])

        |> assoc_constraint(:account)

        |> assoc_constraint(:user)

        |> cast_assoc(:user, required: true)

      end

    end

    

    defmodule App.Account do

      schema "accounts" do

        has_many :memberships, App.AccountUser

        has_many :users, through: [:memberships, :user]

      end

    

      def changeset(struct, params \\ %{}) do

        struct

        |> cast(params, [:name])

        |> validate_required([:name])

        |> cast_assoc(:memberships, required: true)

      end

    end

    

    defmodule App.User do

      schema "users" do

        has_many :account_memberships, App.AccountUser

        has_many :accounts, through: [:account_memberships, :account]

      end

    

      def changeset(struct, params \\ %{}) do

        struct

        |> cast(params, [:first_name])

        |> validate_required([:first_name])

      end

    end

Here's what my controller looks like:

  def new(conn, _params) do

    changeset =

      Account.changeset(

        %Account{memberships: [

          %AccountUser{role: :staff, user: %User{}}]})

    render conn, "new.html", changeset: changeset

  end

  def create(conn, %{"account" => account_params}) do

    changeset = Account.changeset(%Account{}, account_params)

    case Repo.insert(changeset) do

      {:ok, account} ->

        redirect(conn, to: registration_account_path(conn, :new))

      {:error, changeset} ->

        IO.inspect changeset

        render conn, "new.html", changeset: changeset

    end

  end

So, how do I insert the role in the `account_params`? Should I just manually update the params map? That doesn't sound too good. I could add a `defaults: [role: :foo]` to the association, but that requires I define an association for every role.

Any ideas?

[José Valim]

The whole purpose of using cast_association, in my opinion, is if you want to expose your schema structure as your parameters structure. That generates coupling, but it also means things just work™. So ideally you would like parameters to be sent as:

   {"account" => {

     "account_users" => {

       "1" => {

         "id" => "1",

         "role" => "bar",

         "user" => {"name" => "hello}

       }

     }

    }

    

If you can't send the structure above, then you need to mangle parameters, have custom association, or build schemas that are specific to that operation, as outlined here.

José Valim

www.plataformatec.com.br

Skype: jv.ptec

Founder and Director of R&D

--
You received this message because you are subscribed to the Google Groups "elixir-ecto" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-ecto+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-ecto/96d7016a-65f2-476d-a0ba-5581033c47ac%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Mohamad El-Husseini]

Thank you for clarifying that. I can send the structure you gave, but it's not secure. When an account and a user are created, I want to ensure the joiner record has a specific role. Obviously, this is not safe to send as a parameter.

How would a custom association work? I assume you mean something like this:

    defmodule App.Account do

      schema "accounts" do

        has_many :admin_memberships, App.AccountUser, defaults: [role: :admin]

        has_many :admins, through: [:admin_memberships, :user]

But if the `role` param is sent from the client, wouldn't it overwrite the one set in the `defaults` option?--or do we filter it in the controller?

[José Valim]

Thank you for clarifying that. I can send the structure you gave, but it's not secure. When an account and a user are created, I want to ensure the joiner record has a specific role. Obviously, this is not safe to send as a parameter.

As any other parameter, you should validate the role and make sure it fits your business rules. There is nothing specific to role in here, any parameter needs to be validated.

[Mohamad El-Husseini]

Thanks. Another way to do it is to pipe the changeset to a function that ensures the role is set correctly in the AccountUser schema. But, overall, I think the schemaless changeset you suggested is the cleanest way to do this.

On Tuesday, 15 November 2016 11:19:26 UTC-2, Mohamad El-Husseini wrote:

[Mohamad El-Husseini]

Earlier I said a schemaless changeset would be the best way. Unfortunately, it's not, since emails usually have unique constraints, and it's not possible to have a unique_constraint on a schemaless changeset!

[José Valim]

You can still have the constraint and copy the error over from the final operation.

José Valim

www.plataformatec.com.br

Skype: jv.ptec

Founder and Director of R&D

--

You received this message because you are subscribed to the Google Groups "elixir-ecto" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-ecto+unsubscribe@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-ecto/455729a4-b756-4388-87eb-83ad1d6ac2b5%40googlegroups.com.

[Mohamad El-Husseini]

Sorry, I'm still getting to grips with Elixir, and Ecto has been the hardest transition for me. It's not immediately clear how I would copy over the error. I think a unique_constraint would need to travel to the database. Given your example blow (from your blog), I would have to check if the transaction completes successfully, then, if not, grab the error from the failing changeset, and copy it over to the Registration changeset?

if changeset.valid? do
  registration = Ecto.Changeset.apply_changes(changeset)

  MyApp.Repo.transaction fn ->
    MyApp.Repo.insert_all "accounts", Registration.to_account(registration)
    MyApp.Repo.insert_all "profiles", Registration.to_profile(registration)
  end

  {:ok, registration}
else
  changeset = %{changeset | action: :registration}
  {:error, changeset}
end

On Tuesday, 15 November 2016 11:19:26 UTC-2, Mohamad El-Husseini wrote:

[José Valim]

In the example you posted from the article, if you have constraints, I would not use insert_all but still have an Account and Profile schema that you would insert using MyApp.Repo.insert/2. So the plan is this:

1. Build a schema+changeset (or a schemaless changeset) that maps the data you want to receive from the external source (that's the Registration changeset)

2. Convert the changes from step 1 into Profile and Account schemas + changesets (those will map to your database sources)

3. Attempt to insert the data from step 2 inside a transaction. If the transaction fails, copy the errors back to the changeset from step 1

So tdl;dr, you are right. You need to grab the error from the failing changeset and copy it over to the Registration changeset.

Feel free to paste your actual code here if you want more suggestions.

José Valim

www.plataformatec.com.br

Skype: jv.ptec

Founder and Director of R&D

--

You received this message because you are subscribed to the Google Groups "elixir-ecto" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-ecto+unsubscribe@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-ecto/35b3e7bf-8c2a-45a9-a1dd-ed728ca05798%40googlegroups.com.

[Mohamad El-Husseini]

OK, so, I finally figured it out and came up with this monster. 

# models/registration.ex

defmodule App.Registration do
  use App.Web, :model

  embedded_schema do
    field :name
    field :email
    field :password
    field :organisation_name
  end

  @required_fields ~w(name email password organisation_name)a

  

def changeset(struct, params \\ %{}) do
    

struct
    |> cast(params, @required_fields)
    |> validate_required(@required_fields)
    |> validate_length(:name, max: 30)
    |> validate_length(:password, min: 8)
    |> validate_length(:organisation_name, max: 35)
    |> validate_format(:email, ~r/@/)
  end
end

# models/user.ex

defmodule App.User do
  use App.Web, :model

  schema "users" do
    field :name, :string
    field :email, :string
    field :password_hash, :string
    field :password, :string, virtual: true
  end

  @required_fields ~w(name email password)a

  

def changeset(struct, params \\ %{}) do
    

struct
    |> cast(params, @required_fields)
    |> validate_required(@required_fields)
    |> unique_constraint(:email)
  end
end

I didn't include the Account model since it's fairly straight forward. Here's the monstrous part:

defmodule App.RegistrationController do
  use App.Web, :controller

  alias App.{Account,Registration,User,AccountUser,Repo}

  def new(conn, _params) do
    changeset = Registration.changeset(%Registration{})
    render conn, :new, changeset: changeset
  end

  def create(conn, %{"registration" => registration_params}) do
    changeset = Registration.changeset(%Registration{}, registration_params)

    if changeset.valid? do
      Repo.transaction fn -> 
        case account_changeset(registration_params) |> Repo.insert do
          {:ok, account} ->
            case user_changeset(registration_params) |> Repo.insert do
              {:ok, user} ->
                %AccountUser{account_id: account.id, user_id: user.id, role: :admin} |> Repo.insert
                redirect conn, to: registration_path(conn, :new)
              {:error, user_changeset} ->
                changeset = copy_errors(user_changeset, changeset)
                render conn, :new, changeset: %{changeset | action: :insert}
            end
          {:error, account_changeset} ->
            changeset = copy_errors(account_changeset, changeset)
            render conn, :new, changeset: %{changeset | action: :insert}
        end
      end
    else
      render conn, :new, changeset: %{changeset | action: :insert}
    end
  end

  defp account_changeset(%{"organisation_name" => org_name}) do
    Account.changeset(%Account{}, %{name: org_name})
  end

  defp user_changeset(params) do
    user_params = Map.take(params, ["name", "email", "password"])
    User.changeset(%User{}, user_params)
  end

  defp copy_errors(from, to) do
    Enum.reduce from.errors, to, fn {field, {msg, _}}, acc ->
      Ecto.Changeset.add_error(acc, field, msg)
    end
  end
end

This looks like an awful lot of work. What do you think? Is this how you would do it, or have I gone off on a tangent?

On Tuesday, 15 November 2016 11:19:26 UTC-2, Mohamad El-Husseini wrote:

[José Valim]

I believe your code looks quite good except by the big transaction call.

Good news is: you can clean it up too.

Idea #1: use Elixir's with construct

    {_, conn} =

      Repo.transaction fn ->

        with {:ok, account} <- account_changeset(registration_params) |> Repo.insert,

             {:ok, user} <- user_changeset(registration_params) |> Repo.insert do

          %AccountUser{account_id: account.id, user_id: user.id, role: :admin} |> Repo.insert!

          redirect conn, to: registration_path(conn, :new)

        else

          {:error, repo_changeset} ->

            changeset = copy_errors(repo_changeset, changeset)

            render conn, :new, changeset: %{changeset | action: :insert}

        end

      end

    conn

"with" can drastically improve cases like the above where you have nested case statements.

Idea #2: use Ecto.Multi

    multi =

      Ecto.Multi.new

      |> Ecto.Multi.insert(:account, account_changeset(registration_params))

      |> Ecto.Multi.insert(:user, user_changeset(registration_params))

      |> Ecto.Multi.run(:account_user, fn changes ->

           Repo.insert %AccountUser{account_id: changes.account.id, user_id: changes.user.id, role: :admin}

         end)

    case Repo.transaction(multi) do

      {:ok, _} ->

        redirect conn, to: registration_path(conn, :new)

      {:error, _operation, repo_changeset, _changes} ->

        changeset = copy_errors(repo_changeset, changeset)

        render conn, :new, changeset: %{changeset | action: :insert}

    end

In a way, Ecto.Multi is very similar to "with" but it composes better because you can give it multiple instructions which are executed just later on when you pass it to a transaction. Another benefit of multi is that you can build the multi at some explicit step, unit test it, and just then call the transaction. It effectively decouples your repository operations from control flow.

My suggestion would be to add a Registration.to_multi(registration_changeset) function that builds Ecto.Multi and then:

        case Repo.transaction(Registration.to_multi(changeset)) do

      {:ok, _} ->

        redirect conn, to: registration_path(conn, :new)

      {:error, _operation, repo_changeset, _changes} ->

        changeset = copy_errors(repo_changeset, changeset)

        render conn, :new, changeset: %{changeset | action: :insert}

    end

This way you can still keep your controller fairly clean and the logic is broken apart into small functions.

What do you say?

José Valim

www.plataformatec.com.br

Skype: jv.ptec

Founder and Director of R&D

--

You received this message because you are subscribed to the Google Groups "elixir-ecto" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-ecto+unsubscribe@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-ecto/6a565798-a426-4c18-878b-4f98b9a8b465%40googlegroups.com.

[Mohamad El-Husseini]

Great stuff, José. I like both approaches, and I will implement them both to see how they feel. I wanted to use `with`, but I was missing the `else` clause. I didn't know it was added to Elixir. Here's the result of idea #1 (but there's an error that only appears in the console, which I believe it related to the `transaction`, as it appears only when the transaction block runs):

  def create(conn, %{"registration" => registration_params}) do
    changeset = Registration.changeset(%Registration{}, registration_params)

    if changeset.valid? do

      {_, conn} = 
        Repo.transaction fn -> 
          with {:ok, account} <- account_changeset(registration_params) |> Repo.insert,
               {:ok, user} <- user_changeset(registration_params) |> Repo.insert do
            %AccountUser{account_id: account.id, user_id: user.id, role: :admin} |> Repo.insert!
            redirect conn, to: registration_path(conn, :new)
          else
            {:error, repo_changeset} ->
              changeset = copy_errors(repo_changeset, changeset)
              render conn, :new, changeset: %{changeset | action: :insert}
          end
        end

else
      render conn, :new, changeset: %{changeset | action: :insert}
    end
  end

Cleaner for sure. But here's the stack trace from the conosle. This error doesn't appear in the browser; things just work.

[error] #PID<0.489.0> running App.Endpoint terminated
Server: localhost:4000 (http)
Request: POST /registrations
** (exit) an exception was raised:
    ** (RuntimeError) expected action/2 to return a Plug.Conn, all plugs must receive a connection (conn) and return a connection
        (app) web/controllers/registration_controller.ex:1: App.RegistrationController.phoenix_controller_pipeline/2
        (app) lib/app/endpoint.ex:1: App.Endpoint.instrument/4
        (app) lib/phoenix/router.ex:261: App.Router.dispatch/2
        (app) web/router.ex:1: App.Router.do_call/2
        (app) lib/app/endpoint.ex:1: App.Endpoint.phoenix_pipeline/1
        (app) lib/plug/debugger.ex:123: App.Endpoint."call (overridable 3)"/2
        (app) lib/app/endpoint.ex:1: App.Endpoint.call/2
        (plug) lib/plug/adapters/cowboy/handler.ex:15: Plug.Adapters.Cowboy.Handler.upgrade/4
        (cowboy) src/cowboy_protocol.erl:442: :cowboy_protocol.execute/4
I researched the error and found your answer: The error results from a conn struct not being returned from a controller action. But it's hard to see where this is happening in my create action. As mentioned, it only appears if the if changeset.valid? runs, which means the transaction is running.

I will look into Ecto Multi soon to see how the solution feels.

On Tuesday, 15 November 2016 11:19:26 UTC-2, Mohamad El-Husseini wrote:

[Mohamad El-Husseini]

Just an update on my previous code, I am returning the conn right after the transaction, but error still happens when there's a constraint error:

if changeset.valid? do
      {_, conn} = 
        Repo.transaction fn -> 
          with {:ok, account} <- account_changeset(registration_params) |> Repo.insert,
               {:ok, user} <- user_changeset(registration_params) |> Repo.insert do
            %AccountUser{account_id: account.id, user_id: user.id, role: :admin} |> Repo.insert!
            redirect conn, to: registration_path(conn, :new)
          else
            {:error, repo_changeset} ->
              changeset = copy_errors(repo_changeset, changeset)
              render conn, :new, changeset: %{changeset | action: :insert}
          end
        end

      conn
    else

On Tuesday, 15 November 2016 11:19:26 UTC-2, Mohamad El-Husseini wrote:

[José Valim]

> Just an update on my previous code, I am returning the conn right after the transaction, but error still happens when there's a constraint error:

Change this:

render conn, :new, changeset: %{changeset | action: :insert}

To:

Repo.rollback render conn, :new, changeset: %{changeset | action: :insert}

I.e. you need to tell the repo you are rolling back and what value the rollback has (in this case it is the connection). That's why I would prefer multi. :)

José Valim

www.plataformatec.com.br

Skype: jv.ptec

Founder and Director of R&D

--

You received this message because you are subscribed to the Google Groups "elixir-ecto" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-ecto+unsubscribe@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-ecto/29470df1-31f4-4ecf-88e3-09676aeb78c8%40googlegroups.com.

[Mohamad El-Husseini]

Thanks! I've implemented this feature using nested forms, table-less schema, and table-less schema + Ecto multi. I plan to write about the latter two, and I already blogged about using nested forms. Thanks for all of your help here, and for all the work you and other contributors have put into Elixir, Ecto, Phoenix, and the entire ecosystem.

An aside, I don't know which approach I like. I love the simplicity of nested forms, but for more complicated cases Ecto Multi feels really good.

On Tuesday, 15 November 2016 11:19:26 UTC-2, Mohamad El-Husseini wrote:

[José Valim]

That was great! I'm looking forward to the next articles!

José Valim

www.plataformatec.com.br

Skype: jv.ptec

Founder and Director of R&D

--

You received this message because you are subscribed to the Google Groups "elixir-ecto" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-ecto+unsubscribe@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-ecto/13bdae9d-00f5-40cf-93f2-d2eb40334510%40googlegroups.com.

[Mohamad El-Husseini]

As promised, here's the second part that outlines using Ecto.Multi. I'm hardly an authority on this, so feel free to point out any factual errors. I've given you credit at the end of the post. Thanks again!

On Tuesday, 15 November 2016 11:19:26 UTC-2, Mohamad El-Husseini wrote:

[Mohamad El-Husseini]

I've thought about the different approaches more, and I have some reservations.  I've simplified the code a little, but the examples stand. Looking at the changeset function below...

defmodule App.Registration do
  use App.Web, :model

  alias App.{Account, User, Membership, Repo}

  embedded_schema do
    field :email
    field :org_name
  end

  @required_fields ~w(email org_name)a

  

def changeset(struct, params \\ %{}) do
    

struct
    |> cast(params, @required_fields)
    |> validate_required(@required_fields)

    # ...
  end
  # ...
end

... we can see that we're duplicating validations in the Registration schema and in the User/Account schemas.  I decide to strip the Registration.changeset function to only cast and return the struct:

  def changeset(struct, params \\ %{}) do
    

struct
    |> cast(params, @required_fields)

  end

Because validations now fall through to their respective schemas, we can simplify the controller and remove one conditional. The create function becomes:

def create(conn, %{"registration" => registration_params}) do
  changeset = Registration.changeset(%Registration

{}, registration_params)
  case Repo.transaction(Registration.to_multi(registration_params)) do
    {:ok, _} ->
      redirect conn, to: registration_path(conn, :new)
    {:error, _operation, repo_changeset, _changes} ->
      changeset = copy_errors(repo_changeset, changeset)
      render conn, :new, changeset: %{changeset | action: :insert}
  end
end

Notice we removed this part below:

    # this part stays
    if changeset.valid? do

      # this part stays
    else

      render conn, :new, changeset: %{changeset | action: :insert}
    end

There is another issue, though, that's independent of these refactorings. To avoid clashes, field names in the Registration schema are likely to be prefixed. For example: user_name and account_name. This makes mapping params to their respective changesets harder, and copying the errors more complex.

defmodule Class.Registration do
  embedded_schema do
    field :user_name
    field :account_name
  end

  def to_multi(params \\ %{}) do
    Ecto.Multi.new
      |> Ecto.Multi.insert(:account, account_changeset(params))
      |> Ecto.Multi.insert(:user, user_changeset(params))
      # ...
  end

  defp account_changeset(params) do
    # map %{"account_name" => "bar"} to %{name: "foo"}
  end

  defp user_changeset(params) do
    # map %{"user_name" => "bar"} to %{name: "bar"}
  end
end

We have to repeat a similar process for errors. With all that considered, it seems a bit more complicated than simply using a nested form.

What do you think? Am I over complicating things?

On Tuesday, 15 November 2016 11:19:26 UTC-2, Mohamad El-Husseini wrote:

[José Valim]

I would actually try the opposite. You want to validate the data at the entry point, which means at the registration level. You don't really need to validate the User and Account after registration is valid. I mean, the only reason you need a changeset is for the uniqueness constraint but that can be handled when building the multi exclusively.

You may want run into issues of duplicate validations if you do use accounts and users for something else but that can be solved by sharing functions.

You can also remove the conditional by returning a multi that will fail right away:

def to_multi(%{valid?: false)) do

  Ecto.Multi.new |> Ecto.Multi.run(:registration, fn _ -> {:error, %{}} end)

end

José Valim

www.plataformatec.com.br

Skype: jv.ptec

Founder and Director of R&D

--

You received this message because you are subscribed to the Google Groups "elixir-ecto" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-ecto+unsubscribe@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-ecto/642bd82d-5b8b-4828-abf6-e25481f0acbf%40googlegroups.com.

[Mohamad El-Husseini]

Thanks for the tip. I'm going to sleep on this. It isn't unlikely that I will need validations in other scenarios than registration, so it makes sense to keep them there. Either way, it's good to know we have these tools at our disposal. 

On Tuesday, 15 November 2016 11:19:26 UTC-2, Mohamad El-Husseini wrote:

[José Valim]

I have added Ecto.Multi.error/3 so the above can be written on Ecto 2.1 as:

def to_multi(%{valid?: false)) do

  Ecto.Multi.new |> Ecto.Multi.error(:registration, %{errors: []})

end

José Valim

www.plataformatec.com.br

Skype: jv.ptec

Founder and Director of R&D