Problem with nested .inputs_for

Hello,
I have a problem (and potential bug) with nested form.
I have a form for a “product” witch has many “languages” witch has many “attributes”.


I want to be able to add some “attributes”.

If I have a create a new product with this changeset :

      Product.changeset(
        %Product{
          languages: [
            %Language{name: "FR", attributes: []},
            %Language{name: "EN", attributes: []}
          ]
        },
        %{}
      )

Everything seems to work fine.

But if I want to edit a product from the following seed

Repo.insert!(%Product{
  name: "a product",
  languages: [
      %Language{name: "French",
      attributes: [
        %Attribute{label: "Couleur",
        value: "Noir"}
      ]
    },
    %Language{name: "English",
    attributes: [
      %Attribute{label: "Color",
      value: "Black"}
    ]
  }]
})

the button add attribute doesn’t work any more.
The line Ecto.Changeset.put_assoc(changeset, :languages, udpated_languages)
returns

#Ecto.Changeset<action: nil, changes: %{}, errors: [],
 data: #NestedInputsFor.Product.Product<>, valid?: true>

Here is my live view component :

defmodule NestedInputsForWeb.ProductForm do
  use Phoenix.LiveView
  alias NestedInputsFor.Product.{Attribute, Language, Product}
  alias NestedInputsFor.Repo
  import NestedInputsForWeb.CoreComponents

  def render(assigns) do
    ~H"""
    <h1>Product form</h1>
    <.form for={@form} as={:product} phx-submit={:save} phx-change={:validate}>
      <label>Product name</label>
      <.input field={@form[:name]} type="text" />
      <div class="flex">
        <.inputs_for :let={language_form} field={@form[:languages]}>
          <.input field={language_form[:name]} type="hidden" />
          <div class="flex-initial">
            <%= Ecto.Changeset.get_field(language_form.source, :name) %>
            <br/>
            <.inputs_for :let={attribute_form} field={language_form[:attributes]}>
              <label>Label</label>
              <.input field={attribute_form[:label]} type="text" />
              <label>Value</label>
              <.input field={attribute_form[:value]} type="text" />
            </.inputs_for>
          </div>
        </.inputs_for>
      </div>
      <div phx-click="add_attribute">Add attribute</div>
      <button>Save</button>
    </.form>
    """
  end

  def mount(_params, _, socket) do
    changeset = Repo.all(Product)
    |> Repo.preload(languages: :attributes)
    |> hd()
    |> Product.changeset(%{})

    {:ok, assign(socket, %{form: to_form(changeset), changeset: changeset})}
  end

  def handle_event("add_attribute", _params, socket) do
    changeset = socket.assigns.changeset
    udpated_languages =
      Ecto.Changeset.get_field(changeset, :languages)
      |> Enum.map(fn language ->
        Map.put(language, :attributes,  language.attributes ++ [%Attribute{}])
      end)

    changeset = Ecto.Changeset.put_assoc(changeset, :languages, udpated_languages)
    {:noreply, assign(socket, %{form: to_form(changeset), changeset: changeset})}
  end

  def handle_event("validate", %{"product" => params}, socket) do
    changeset = Product.changeset(socket.assigns.changeset, params)
    {:noreply, assign(socket, %{form: to_form(changeset), changeset: changeset})}
  end

  def handle_event("save", params, socket) do
    IO.inspect(params)
    {:noreply, socket}
  end
end

This demo project is available here https://github.com/ChristopheBelpaire/nested_inputs_for

Did I do something wrong or is it a bug :slight_smile: ?

Thanks in advance!

I think where you’re putting the attribute into language is the problem.

EctoNestedChangeset module simplifies that.

I had several forms like this in an app i am working on and I ended up creating a small module to provide the nested form:

1 Like

I suspect this is part of the problem: manually updating associations on Language structs won’t be detected by the changeset machinery.

Yes probably, I suppose put_assoc don’t support properly nested structure.
It would be nice to have it, maybe in future version of liveview/ecto.
It is weird it is working with new record.

Thanks a lot!
I will have a look :slight_smile:

It does. The issue you’re running into is that you’re supplying structs to put_assoc though instead of changesets.

Different to passing changesets, structs are not change tracked in any fashion. In other words, if you change a comment struct and give it to put_assoc/4, the updates in the struct won’t be persisted.

You need the following:

  def handle_event("add_attribute", _params, socket) do
    changeset = socket.assigns.changeset

    udpated_languages =
      Ecto.Changeset.get_field(changeset, :languages)
      |> Enum.map(fn language ->
        Ecto.Changeset.change(language)
        |> Ecto.Changeset.put_assoc(:attributes, language.attributes ++ [%Attribute{}])
      end)

    changeset = Ecto.Changeset.put_assoc(changeset, :languages, udpated_languages)
    {:noreply, assign(socket, %{form: to_form(changeset), changeset: changeset})}
  end

In the upcoming ecto release (3.10) there will be Ecto.Changeset.get_assoc/3, which will return you changesets by default (including existing changes), which will work better then get_field/2.

4 Likes

It seems to work!
Thanks a lot :grin:
Indeed it make sense.
Waw this is quite powerful, I could edit the whole database with one n-nested form :sunglasses:

Hello,
I went further with my little exemple and had another problem
I want to create and edit a structure like this one :

Product.changeset(
        %Product{
          languages: [
            %Language{name: "FR", attributes: [
              %Attribute{label: "Label FR 1", value: "Value FR 1"},
              %Attribute{label: "Label FR 2", value: "Value FR 2"}
            ]},
            %Language{name: "EN", attributes: [
              %Attribute{label: "Label EN 1", value: "Value EN 1"},
              %Attribute{label: "Label EN 2", value: "Value EN 2"}
            ]}
          ]
        },
        %{}
      )

I added buttons add and delete attributes for each languages :

It fail to delete the attributes with the following warning :

[warning] found duplicate primary keys for association/embed `:attributes` in `NestedInputsFor.Product.Language`. In case of duplicate IDs, only the last entry with the same ID will be kept. Make sure that all entries in `:attributes` have an ID and the IDs are unique between them

I understand my attributes needs an id, it works if I replace my changeset with :

      Product.changeset(
        %Product{
          languages: [
            %Language{name: "FR", attributes: [
              %Attribute{id: 1, label: "Label FR 1", value: "Value FR 1"},
              %Attribute{id: 2, label: "Label FR 2", value: "Value FR 2"}
            ]},
            %Language{name: "EN", attributes: [
              %Attribute{id: 3, label: "Label EN 1", value: "Value EN 1"},
              %Attribute{id: 4, label: "Label EN 2", value: "Value EN 2"}
            ]}
          ]
        },
        %{}
      )

But of course, I don’t have the id before the records are inserted.
Is there a way to fix this in a clean way ?

Here is my updated component :

defmodule NestedInputsForWeb.ProductForm do
  use Phoenix.LiveView
  alias NestedInputsFor.Product.{Attribute, Language, Product}
  alias NestedInputsFor.Repo
  import NestedInputsForWeb.CoreComponents

  @spec render(any) :: Phoenix.LiveView.Rendered.t()
  def render(assigns) do
    ~H"""
    <h1>Product form</h1>
    <.form for={@form} as={:product} phx-submit={:save} phx-change={:validate}>
      <label>Product name</label>
      <.input field={@form[:name]} type="text" />
      <div class="flex">
        <.inputs_for :let={language_form} field={@form[:languages]}>
          <.input field={language_form[:name]} type="hidden" />
          <div class="flex-initial">
            <%= Ecto.Changeset.get_field(language_form.source, :name) %>
            <br/>
            <.inputs_for :let={attribute_form} field={language_form[:attributes]}>
              <div>
                <label  class="inline-block">Label</label>
                <.input field={attribute_form[:label]} type="text" class="inline-block"/>
                <label  class="inline-block">Value</label>
                <.input field={attribute_form[:value]} type="text"  class="inline-block"/>
                <button type="button" class="btn-blue" phx-click="delete_attribute" phx-value-index={attribute_form.index} phx-value-language={Ecto.Changeset.get_field(language_form.source, :name)}>X</button>
              </div>
            </.inputs_for>
            <br/>
            <button type="button" class="btn-blue" phx-click="add_attribute" phx-value-language={Ecto.Changeset.get_field(language_form.source, :name)}>Add attribute</button>

          </div>
        </.inputs_for>
      </div>
      <br/>
      <button class="btn-blue">Save</button>
    </.form>
    """
  end

  def mount(_params, _, socket) do
    changeset =
    # Repo.all(Product)
    # |> Repo.preload(languages: :attributes)
    # |> hd()
    # |> Product.changeset(%{})

      Product.changeset(
        %Product{
          languages: [
            %Language{name: "FR", attributes: [
              %Attribute{id: 1, label: "Label FR 1", value: "Value FR 1"},
              %Attribute{id: 2, label: "Label FR 2", value: "Value FR 2"}
            ]},
            %Language{name: "EN", attributes: [
              %Attribute{id: 3, label: "Label EN 1", value: "Value EN 1"},
              %Attribute{id: 4, label: "Label EN 2", value: "Value EN 2"}
            ]}
          ]
        },
        %{}
      )

    {:ok, assign(socket, %{form: to_form(changeset), changeset: changeset})}
  end

  def handle_event("add_attribute", %{"language" => clicked_language}, socket) do
    changeset = socket.assigns.changeset
    udpated_languages =
      Ecto.Changeset.get_field(changeset, :languages)
      |> Enum.map(fn language ->
        if(language.name == clicked_language) do
          Ecto.Changeset.change(language)
          |> Ecto.Changeset.put_assoc(:attributes, language.attributes ++ [%Attribute{}])
        else
          Ecto.Changeset.change(language)
        end
      end)
    changeset = Ecto.Changeset.put_assoc(changeset, :languages, udpated_languages)
    {:noreply, assign(socket, %{form: to_form(changeset), changeset: changeset})}
  end


  def handle_event("delete_attribute", %{"index" => index, "language" => clicked_language}, socket) do
    changeset = socket.assigns.changeset
    index = String.to_integer(index)
    udpated_languages =
      Ecto.Changeset.get_field(changeset, :languages)
      |> Enum.map(fn language ->
        if(language.name == clicked_language) do
          Ecto.Changeset.change(language)
          |> Ecto.Changeset.put_assoc(:attributes, List.delete_at(language.attributes, index))
        else
          Ecto.Changeset.change(language)
        end
      end)
    changeset = Ecto.Changeset.put_assoc(changeset, :languages, udpated_languages)
    {:noreply, assign(socket, %{form: to_form(changeset), changeset: changeset})}
  end


  def handle_event("validate", %{"product" => params}, socket) do
    changeset = Product.changeset(socket.assigns.changeset, params)
    {:noreply, assign(socket, %{form: to_form(changeset), changeset: changeset})}
  end

  def handle_event("save", params, socket) do
    IO.inspect(params)
    {:noreply, socket}
  end
end

The code is also here : https://github.com/ChristopheBelpaire/nested_inputs_for

Thanks in advance!

I’d suggest using indexes instead of ids. I’ve written this down in this one:

1 Like

I’ve run into this before - the issue is that the unsaved, no-id Languages that were passed into Product.changeset are considered “pre-existing data” by put_assoc. That triggers the code path that checks for duplicate IDs.

You should get better results by starting with languages empty and using put_assoc to build the initial state:

%Product{languages: []}
|> Product.changeset(%{})
|> Ecto.Changeset.put_assoc(:languages, [
  %Language{name: "FR", attributes: [
    %Attribute{label: "Label FR 1", value: "Value FR 1"},
    %Attribute{label: "Label FR 2", value: "Value FR 2"}
  ]},
  %Language{name: "EN", attributes: [
    %Attribute{label: "Label EN 1", value: "Value EN 1"},
    %Attribute{label: "Label EN 2", value: "Value EN 2"}
  ]}
 ])

Thanks for the link!
I think I have this problem only with nested inputs_for.

Ok, still digging :slight_smile:

Now I have a single script showing the problem :

Mix.install([:ecto])

defmodule Product do
  use Ecto.Schema
  import Ecto.Changeset

  schema "products" do
    field :name, :string
    has_many :languages, Language
    timestamps()
  end

  def changeset(product, attrs) do
    product
    |> cast(attrs, [:name])
    |> cast_assoc(:languages)
  end
end

defmodule Language do
  use Ecto.Schema
  import Ecto.Changeset

  schema "languages" do
    field :name, :string
    belongs_to :product, Product
    has_many :attributes, Attribute, on_replace: :delete
    timestamps()
  end

  def changeset(language, attrs) do
    language
    |> cast(attrs, [:name])
    |> cast_assoc(:attributes)
  end
end


defmodule Attribute do
  use Ecto.Schema
  import Ecto.Changeset

  schema "attributes" do
    field :label, :string
    field :value, :string
    belongs_to :language, Language
    timestamps()
  end

  def changeset(attribute, attrs) do
    attribute
    |> cast(attrs, [:label, :value])
  end
end


defmodule Main do
  def error() do
    changeset = Product.changeset(%Product{},%{
      "languages" => %{
        "0" => %{
          "attributes" => %{
            "0" => %{"label" => "", "value" => ""},
            "1" => %{"label" => "", "value" => "k"}
          },
          "name" => "FR"
        },
        "1" => %{"name" => "EN"}
      },
      "name" => ""
    })

    index = 1
    selected_language = "FR"
    udpated_languages = Ecto.Changeset.get_field(changeset, :languages) |> Enum.map(fn language ->
      if(language.name == selected_language) do
        Ecto.Changeset.change(language)
        |> Ecto.Changeset.put_assoc(:attributes,
           List.delete_at(language.attributes, index)
        )
      else
        Ecto.Changeset.change(language)
      end
    end)

    updated_changeset = Ecto.Changeset.put_assoc(changeset, :languages, udpated_languages)
    Ecto.Changeset.get_field(updated_changeset, :languages)
    |> IO.inspect()
  end

  def ok() do
    changeset = Product.changeset(%Product{},%{
      "languages" => %{
        "0" => %{"name" => "FR"},
        "1" => %{"name" => "EN"}
      },
      "name" => ""
    })
    index = 1
    languages = Ecto.Changeset.get_field(changeset, :languages)
    updated_changeset = Ecto.Changeset.put_assoc(changeset, :languages, List.delete_at(languages, index))
    Ecto.Changeset.get_field(updated_changeset, :languages)
    |> IO.inspect()
  end
end

IO.puts("Delete in normal input for, working : ")
Main.ok()
IO.puts("Delete in nested input for, not working and warning: ")
Main.error()

It is still about the warning :

12:45:35.661 [warning] found duplicate primary keys for association/embed `:attributes` in `Language`. In case of duplicate IDs, only the last entry with the same ID will be kept. Make sure that all entries in `:attributes` have an ID and the IDs are unique between them

It only happen with nested forms on two (or more) levels.
Maybe it is linked to : Warning about duplicated primary keys for new records · Issue #3514 · elixir-ecto/ecto · GitHub

If someone has an idea how to make this work, I’m super interresed :slight_smile:

Thanks!

If you have previous changes don’t use Ecto.Changeset.get_field, but Ecto.Changeset.get_change. Or use ecto master till 3.10 is released and Ecto.Changeset.get_assoc, so you don’t need to care for the difference.

Thanks for the suggestion!
I tried with get_assoc and it seems to work, for instance :

    changeset =
      Product.changeset(%Product{}, %{
        "languages" => %{
          "0" => %{
            "attributes" => %{
              "0" => %{"label" => "", "value" => ""},
              "1" => %{"label" => "", "value" => "k"}
            },
            "name" => "FR"
          },
          "1" => %{"name" => "EN"}
        },
        "name" => ""
      })

    index = 1
    selected_language = "FR"

    udpated_languages =
      Ecto.Changeset.get_assoc(changeset, :languages)
      |> Enum.map(fn language ->
        if(Ecto.Changeset.get_field(language, :name) == selected_language) do
          attributes = Ecto.Changeset.get_field(language, :attributes)

          language
          |> Ecto.Changeset.put_assoc(
            :attributes,
            List.delete_at(attributes, index)
          )
        else
          Ecto.Changeset.change(language)
        end
      end)

    updated_changeset = Ecto.Changeset.put_assoc(changeset, :languages, udpated_languages)

The full example is here : nested_inputs_for/error.exs at main · ChristopheBelpaire/nested_inputs_for · GitHub

Thanks a lot for the suggestion!

I think you want to use it consistently for all assoc/embes – so :languages as well as their :attributes.

Indeed, the changeset seems to loose track of some changes if I use get_field.
If you have time, you should try to make a multiple nested inputs_for (and maybe update your article ;)) it is quite challenging :slight_smile:

Thanks again !

I don’t think that’s really necessary. The things you’re running into are exactly the same issues, just on more than one level.

Probably, but I ran on much less issues on one level.
Thanks again for the article, it really helped!