maxmarcon / live_select

Dynamic (multi)selection field for LiveView
https://hex.pm/packages/live_select
Apache License 2.0
185 stars 35 forks source link

value_mapper is passed a map instead of a struct #73

Closed Munksgaard closed 4 months ago

Munksgaard commented 5 months ago

I've been following the guide on using embeds and assocs. Among other things, I've added a value_mapper to my <.live_select ...> and to my event handler:

        <.live_select
          field={@form[:foo_type]}
          value_mapper={&value_mapper/1}
        />
  def handle_event(
        "live_select_change",
        %{"text" => text, "id" => live_select_id} = params,
        socket
      ) do
    foos =
      text
      |> Foos.search()
      |> Enum.map(&value_mapper/1)

    send_update(LiveSelect.Component, id: live_select_id, options: foos)

    {:noreply, socket}
  end
  defp value_mapper(%Foo{name: name} = value) do
    %{label: name, value: value}
  end

Things work when I type in the search field, but when I select an item, I get the following error:

** (FunctionClauseError) no function clause matching in FooWeb.FooLive.value_mapper/1
    (foo 0.1.0) lib/foo_web/live/foo_live.ex:142: FooWeb.FooLive.value_mapper(%{"description" => "A foo", "id" => 3, "inserted_at" => "2024-06-20T08:39:13.814883Z", "name" => "Foo 1", "updated_at" => "2024-06-20T08:39:13.814883Z"})
    (live_select 1.4.0) lib/live_select/component.ex:540: LiveSelect.Component.normalize_selection_value/3
    (live_select 1.4.0) lib/live_select/component.ex:520: LiveSelect.Component.update_selection/5
    (phoenix_live_view 0.20.15) lib/phoenix_component.ex:1362: Phoenix.Component.update/3
    (live_select 1.4.0) lib/live_select/component.ex:150: LiveSelect.Component.update/2
    (phoenix_live_view 0.20.15) lib/phoenix_live_view/utils.ex:487: Phoenix.LiveView.Utils.maybe_call_update!/3
    (elixir 1.16.2) lib/enum.ex:1700: Enum."-map/2-lists^map/1-1-"/2
    (phoenix_live_view 0.20.15) lib/phoenix_live_view/diff.ex:671: anonymous fn/4 in Phoenix.LiveView.Diff.render_pending_components/6
    (telemetry 1.2.1) /home/munksgaard/src/foo/deps/telemetry/src/telemetry.erl:321: :telemetry.span/3
    (phoenix_live_view 0.20.15) lib/phoenix_live_view/diff.ex:666: anonymous fn/4 in Phoenix.LiveView.Diff.render_pending_components/6
    (stdlib 4.3.1.4) maps.erl:411: :maps.fold_1/3
    (phoenix_live_view 0.20.15) lib/phoenix_live_view/diff.ex:625: Phoenix.LiveView.Diff.render_pending_components/6
    (phoenix_live_view 0.20.15) lib/phoenix_live_view/diff.ex:217: Phoenix.LiveView.Diff.write_component/4
    (phoenix_live_view 0.20.15) lib/phoenix_live_view/channel.ex:662: Phoenix.LiveView.Channel.component_handle/4
    (stdlib 4.3.1.4) gen_server.erl:1123: :gen_server.try_dispatch/4
    (stdlib 4.3.1.4) gen_server.erl:1200: :gen_server.handle_msg/6
    (stdlib 4.3.1.4) proc_lib.erl:250: :proc_lib.wake_up/3

Of course, it would be easy enough for me to add a value_mapper variant that handles a map instead of a struct, but the documentation makes it sound like that is not necessary. Am I missing something or is the documentation incomplete?

maxmarcon commented 5 months ago

Hi! Can you please show me how you handle change events from the form?

Munksgaard commented 5 months ago

Of course:

  @impl true
  def handle_event("validate", %{"foo_form" => foo_params}, socket) do
    foo_params =
      Map.update!(foo_params, "foo", &LiveSelect.decode/1)

    changeset =
      socket.assigns.foos
      |> Foos.change_foo(foo_params)
      |> Map.put(:action, :validate)

    {:noreply, assign_form(socket, changeset)}
  end

  def handle_event("save", %{"foo" => foo_params}, socket) do
    save_foo(socket, socket.assigns.action, foo_params)
  end

  defp save_foo(socket, :new, foo_params) do
    case Foos.create_foo(foo_params) do
      {:ok, foo} ->
        # do stuff
      {:error, %Ecto.Changeset{} = changeset} ->
        # handle error
    end
  end
maxmarcon commented 5 months ago

I'm afraid I need to see more :)

Thanks

Munksgaard commented 5 months ago

Aha! It seems like I was missing a cast_assoc in my changeset.

I am a little weary about using cast_assoc in this case though, since that would allow users to change the value of a Foo (or insert a new Foo?). I just want to use one of the existing Foos.

Munksgaard commented 5 months ago

Changing my live_select to use :foo_type_id instead of :foo_type does what I expect when first creating the relation, but if I then try to edit it, live_select shows the id of foo_type instead of the name:

        <.live_select
          field={@form[:foo_type_id]}
          label={gettext("Type")}
        />
maxmarcon commented 5 months ago

again, I cant' really help if you don't show me more code. You said a cast_assoc was missing and adding it solves the problem right?

I am a little weary about using cast_assoc in this case though, since that would allow users to change the value of a Foo (or insert a new Foo?). I just want to use one of the existing Foos.

Sounds like you need to add some validation

Munksgaard commented 5 months ago

Here is a complete example that shows what I mean:

defmodule Bar do
  use Ecto.Schema
  import Ecto.Changeset

  @derive {Jason.Encoder, except: [:__meta__]}
  schema "bar" do
    field :name, :string
  end

  def changeset(bar, attrs \\ %{}) do
    bar
    |> cast(attrs, [:name])
    |> validate_required([:name])
  end

  def search(_text) do
    [%Bar{name: "baz", id: 2}]
  end
end

defmodule Foo do
  use Ecto.Schema
  import Ecto.Changeset

  schema "foo" do
    belongs_to :bar, Bar
  end

  def changeset(foo, attrs \\ %{}) do
    foo
    |> cast(attrs, [])
    |> cast_assoc(:bar, required: true)
  end
end

defmodule FooWeb.FooLive do
  use FooWeb, :live_view

  @impl true
  def mount(_assigns, _session, socket) do
    foo = %Foo{} |> Foo.Repo.preload(:bar)

    changeset =
      Foo.changeset(foo)

    {:ok,
     socket
     |> assign(:foo, foo)
     |> assign_form(changeset)}
  end

  @impl true
  def render(assigns) do
    ~H"""
    <.simple_form for={@form} id="foo-form" phx-change="validate" phx-submit="save">
      <.live_select field={@form[:bar]} label="Bar" value_mapper={&value_mapper/1} />
      <.button phx-disable-with="Saving...">Save</.button>
    </.simple_form>
    """
  end

  @impl true
  def handle_event("validate", %{"foo" => foo_params}, socket) do
    foo_params =
      Map.update!(foo_params, "bar", &LiveSelect.decode/1)

    changeset =
      socket.assigns.foo
      |> Foo.changeset(foo_params)
      |> Map.put(:action, :validate)

    {:noreply, assign_form(socket, changeset)}
  end

  def handle_event("save", %{"foo" => foo_params} = params, socket) do
    foo_params =
      Map.update!(foo_params, "bar", &LiveSelect.decode/1)

    {:ok, foo} = socket.assigns.foo |> Foo.changeset(foo_params) |> Foo.Repo.insert() |> dbg

    {:noreply, socket}
  end

  def handle_event(
        "live_select_change",
        %{"text" => text, "id" => live_select_id} = params,
        socket
      ) do
    bars = [%Bar{name: "asd"}] |> Enum.map(&value_mapper/1)

    send_update(LiveSelect.Component, id: live_select_id, options: bars)

    {:noreply, socket}
  end

  defp value_mapper(%Bar{name: name} = value) do
    %{label: name, value: value}
  end

  defp assign_form(socket, %Ecto.Changeset{} = changeset) do
    assign(socket, :form, to_form(changeset))
  end
end

And here are the SQL queries I've run:

create table bar(id serial primary key, name text not null unique);
insert into bar (name) values ('asd');
create table foo(id serial primary key, bar_id integer references bar not null);

Typing in the select box and selecting the "asd" that shows up works fine. However, when I submit, I get an error because cast_assoc tries to insert a new row in the bar table instead of reusing the entry already there:

09:02:37.789 [debug] HANDLE EVENT "save" in FooWeb.FooLive
  Parameters: %{"foo" => %{"bar" => "{\"id\":null,\"name\":\"asd\"}", "bar_text_input" => "asd"}}
09:02:37.790 [debug] QUERY OK db=0.4ms queue=0.1ms idle=1375.7ms
begin []
↳ FooWeb.FooLive.handle_event/3, at: lib/foo_web/live/foo_live.ex:80
09:02:37.793 [debug] QUERY ERROR source="bar" db=1.1ms
INSERT INTO "bar" ("name") VALUES ($1) RETURNING "id" ["asd"]
↳ anonymous fn/3 in Ecto.Adapters.SQL.checkout_or_transaction/4, at: lib/ecto/adapters/sql.ex:1358
09:02:37.794 [debug] QUERY OK db=0.3ms
rollback []
↳ FooWeb.FooLive.handle_event/3, at: lib/foo_web/live/foo_live.ex:80
09:02:37.794 [error] GenServer #PID<0.16142.0> terminating
** (Ecto.ConstraintError) constraint error when attempting to insert struct:

    * "bar_name_key" (unique_constraint)

If you would like to stop this constraint violation from raising an
exception and instead add it as an error to your changeset, please
call `unique_constraint/3` on your changeset with the constraint
`:name` as an option.

The changeset has not defined any constraint.

    (ecto 3.11.2) lib/ecto/repo/schema.ex:815: anonymous fn/4 in Ecto.Repo.Schema.constraints_to_errors/3

I realize that this is not the fault of LiveSelect, but it does mean that the example shown in the documentation is hard to actually use. I guess my question is, how would you implement the save-handler in such a way that we reuse the Bar selected instead of trying to insert a new Bar?

A second issue is that using cast_assoc also means that a malicious user can craft a save-query with the following parameters and send it to the server:

 %{"foo" => %{"bar" => "{\"id\":null,\"name\":\"baz\"}", "bar_text_input" => "baz"}}

That would create a new Bar in the database, which in this case is undesired (the user should only be able to select from the predetermined list).

maxmarcon commented 5 months ago

I realize that this is not the fault of LiveSelect, but it does mean that the example shown in the documentation is hard to actually use

Thanks for the feedback. I wrote the documentation assuming a certain level of knowledge of Ecto associations. I could amend the docs to be more useful to people who are still stumbling on many of Ecto's footguns :)

I guess my question is, how would you implement the save-handler in such a way that we reuse the Bar selected instead of trying to insert a new Bar?

I don't think the problem is your save handler. The problem is that your associated structure (Bar) is passed to LiveSelect without IDs:

bars = [%Bar{name: "asd"}] |> Enum.map(&value_mapper/1)

Consequently, when you try to insert Foo, Ecto will see that there is no ID for Bar and will think "hey! A new record, let's insert it". Ecto uses the ID to find out if the associated record has changed. By default it doesn't allow you to change the value of the association and will raise: (https://hexdocs.pm/ecto/Ecto.Schema.html#belongs_to/3, see on_replace option).

My suggestions:

Hope this helps

Munksgaard commented 5 months ago

Thanks for the feedback. I wrote the documentation assuming a certain level of knowledge of Ecto associations. I could amend the docs to be more useful to people who are still stumbling on many of Ecto's footguns :)

You're welcome! I forgot to mention that I'm really enjoying using LiveSelect, so thanks for building it! I am in no way criticizing your work, I just want to help others like me :-)

Definitely count me as one of those who are still confounded by Ecto now and then, though...

retrieve associations from the DB in your live_select_change handler, that will ensure that an ID is passed to LiveSelect and then back to the "save" handler, so Ecto won't try to insert a new record

I do not think this is actually true when it comes to belongs_to relations. To test it out, I tried to replace the offending line with the following and still got the same error:

    bars = Bar |> Leaf.Repo.all() |> Enum.map(&value_mapper/1)

Reading the logs seems to confirm my suspicion:

11:47:59.874 [debug] HANDLE EVENT "save" in FooWeb.FooLive
  Parameters: %{"foo" => %{"bar" => "{\"id\":1,\"name\":\"asd\"}", "bar_text_input" => "asd"}}
11:47:59.874 [debug] QUERY OK db=0.2ms idle=876.6ms
begin []
↳ FooWeb.FooLive.handle_event/3, at: lib/foo_web/live/foo_live.ex:76
11:47:59.875 [debug] QUERY ERROR source="bar" db=0.3ms
INSERT INTO "bar" ("id","name") VALUES ($1,$2) [1, "asd"]
↳ anonymous fn/3 in Ecto.Adapters.SQL.checkout_or_transaction/4, at: lib/ecto/adapters/sql.ex:1358
11:47:59.875 [debug] QUERY OK db=0.1ms
rollback []
↳ FooWeb.FooLive.handle_event/3, at: lib/foo_web/live/foo_live.ex:76
11:47:59.875 [error] GenServer #PID<0.2979.0> terminating
** (Ecto.ConstraintError) constraint error when attempting to insert struct:

    * "bar_pkey" (unique_constraint)

If you would like to stop this constraint violation from raising an
exception and instead add it as an error to your changeset, please
call `unique_constraint/3` on your changeset with the constraint
`:name` as an option.

The changeset has not defined any constraint.

    (ecto 3.11.2) lib/ecto/repo/schema.ex:815: anonymous fn/4 in Ecto.Repo.Schema.constraints_to_errors/3
    ...
I had to make some changes (namely including `:id` in my call to `cast`. Here is the updated code: ```elixir defmodule Bar do use Ecto.Schema import Ecto.Changeset @derive {Jason.Encoder, except: [:__meta__]} schema "bar" do field :name, :string end def changeset(bar, attrs \\ %{}) do bar |> cast(attrs, [:id, :name]) |> validate_required([:id, :name]) end end defmodule Foo do use Ecto.Schema import Ecto.Changeset schema "foo" do belongs_to :bar, Bar end def changeset(foo, attrs \\ %{}) do foo |> cast(attrs, []) |> cast_assoc(:bar, required: true) end end defmodule FooWeb.FooLive do use FooWeb, :live_view @impl true def mount(_assigns, _session, socket) do foo = %Foo{} |> Foo.Repo.preload(:bar) changeset = Foo.changeset(foo) {:ok, socket |> assign(:foo, foo) |> assign_form(changeset)} end @impl true def render(assigns) do ~H""" <.simple_form for={@form} id="foo-form" phx-change="validate" phx-submit="save"> <.live_select field={@form[:bar]} label="Bar" value_mapper={&value_mapper/1} /> <.button phx-disable-with="Saving...">Save """ end @impl true def handle_event("validate", %{"foo" => foo_params}, socket) do foo_params = Map.update!(foo_params, "bar", &LiveSelect.decode/1) changeset = socket.assigns.foo |> Foo.changeset(foo_params) |> Map.put(:action, :validate) {:noreply, assign_form(socket, changeset)} end def handle_event("save", %{"foo" => foo_params} = params, socket) do foo_params = foo_params |> Map.update!("bar", &LiveSelect.decode/1) {:ok, foo} = socket.assigns.foo |> Foo.changeset(foo_params) |> Foo.Repo.insert() {:noreply, socket} end def handle_event( "live_select_change", %{"text" => text, "id" => live_select_id} = params, socket ) do bars = Bar |> Foo.Repo.all() |> Enum.map(&value_mapper/1) send_update(LiveSelect.Component, id: live_select_id, options: bars) {:noreply, socket} end defp value_mapper(%Bar{name: name} = value) do %{label: name, value: value} end defp assign_form(socket, %Ecto.Changeset{} = changeset) do assign(socket, :form, to_form(changeset)) end end ```

If you want to prevent the user from sending arbitrary data so it can't create new Bar records, I think you shouldn't use cast_assoc at all cause it does exactly that. Instead, set the Bar's id you get from LiveSelect in Foo as foo_id, then use https://hexdocs.pm/ecto/Ecto.Changeset.html#foreign_key_constraint/3 or https://hexdocs.pm/ecto/Ecto.Changeset.html#assoc_constraint/3 in Foo's changeset to add an error if the given Bar doesnt' exist. The docs show some examples.

This is kind of what I ended up doing. I find it a bit frustrating that this particular use-case is so weird to handle in Ecto, given how nice everything else is. But again, that's got nothing to do with LiveSelect :-)

maxmarcon commented 5 months ago

Reading the logs seems to confirm my suspicion:

This is probably because cast_assoc doesn't care whether the element already exists in the DB, all it cares about is the elements in the association. So if you associate a Foo with a new %Bar{id :1}, even though %Bar{id: 1} already exists in the DB, Ecto will try to insert it because the element is new relative to the association. But If you then reload Foo with the same Bar and try to save it, Ecto shouldn't try to insert %Bar{id: 1} anymore, because it recognizes %Bar{id: 1} as an existing element within the association. Does it make sense?

I find it a bit frustrating that this particular use-case is so weird to handle in Ecto, given how nice everything else is

I don't think it's weird, it's just that you're using cast_assoc/3 for something it's not supposed to be used for.

Quoting the Ecto docs:

This function should be used when working with the entire association at once (and not a single element of a many-style association) and receiving data external to the application.

If you don't want to receive data external from the application (i.e. you don't want user to be able to create/delete associated records) then cast_assoc/3 is the wrong tool.

Munksgaard commented 4 months ago

Yes, thanks for helping me clear it all up :-)