Need help to refactor the resource creation code

achempion

Hi,

I’m trying to create a form with file field to upload a file. In order to store file into the proper folder I need to know resourse’s id in advance. So, I’ve split the creation transaction to two separate change-sets: changeset for the form fields and audio_changeset to upload the file.

When I call audio_changeset to update changeset and upload the file with invalid data, it changes the form state to update which changes form rendering as put action. This all happen inside the creating resource transaction. To overcome this and reset form status back to creating resource state I need to reset changeset.data.__meta__.state to nil.

It works but I think it has a better and prettier solution

  def create_recording(attrs \\ %{}) do
    Repo.transaction(fn ->
      %Recording{}
      |> Recording.changeset(attrs)
      |> Repo.insert()
      |> case do
           {:ok, recording} ->
             Recording.audio_changeset(recording, attrs)
             |> Repo.update()
             |> case do
                  {:ok, recording_with_audio} -> recording_with_audio
                  {:error, changeset} ->
                    # make the form to a "new" state insted of "update"
                    meta = Map.put(changeset.data.__meta__, :state, nil)
                    data = Map.put(changeset.data, :__meta__, meta)

                    changeset
                    |> Map.put(:data, data)
                    |> Repo.rollback()
                end

           {:error, error} -> Repo.rollback(error)
      end
    end)
  end

5 comments

/phoenix #ecto #phoenix_ecto

0 832 5

2019-09-03 14:22:31 UTC

Showing Posts 1 to 5

Show Best Posts
Show All (oldest first)
Show All (newest first)

david_ex

One easy thing that will make this much more readable is

a
|> b()
|> c()
|> case do
  :foo -> nil
  :bar -> nil
end

instead of

case a |> b() |> c() do
  :foo -> nil
  :bar -> nil
end

Post #1

StefanL

You could use with statements if there are multiple failures possible.

However i think this does not fundamentally solves what you are trying to achieve. Maybe Ecto.Multi would be a better choice if you need to move files around as part of the transaction.

Its kind of hard to understand what is going on exactly in the snippet you posted. If it is one form, i guess you probably should have 1 changeset, also prepare_changes/2 allows you to run code in the transaction.

Using with instead of nested case statements could look like this: (i did not mess with the changeset)

    def create_recording(attrs \\ %{}) do
      Repo.transaction(fn ->
        with {:ok, recording} <- insert_recording(attrs),
             {:ok, with_audio} <- update_recording(recording) do
          with_audio
        else
          {:error, error} ->
            Repo.rollback(error)
        end
      end)
    end

    defp insert_recording(attrs \\ %{}) do
      %Recording{}
      |> Recording.changeset(attrs)
      |> Repo.insert()
    end

    defp update_recording(recording, attrs \\ %{}) do
      recording
      |> Recording.audio_changeset(attrs)
      |> Repo.update()
    end

If you are uploading a file and don’t want to depend on ids from the db, generate a random path for the uploaded file (eg uuid as filename). Maybe this helps you to get rid of the second changeset … You would probably still need a transaction to move/upload the file. Just be aware that storing a lot of files inside 1 directory is a performance killer.

Post #2

achempion

Thank you for the answer. Your code looks great, I think with is the way to go. Also, we still need to reset the state filed to make the form render properly.

I can’t use prepare_changes/2 because resource’s id is not available on this stage yet.

Post #3

fuelen

try this:

  def create_recording(attrs \\ %{}) do
    Ecto.Multi.new()
    |> Ecto.Multi.insert(:recording, Recording.changeset(%Recording{}, attrs))
    |> Ecto.Multi.update(:recording_with_audio, &Recording.audio_changeset(&1.recording, attrs))
    |> Repo.transaction()
    |> case do
      {:ok, %{recording_with_audio: recording}} -> {:ok, recording}
      {:error, _operation, changeset, _} -> Ecto.Changeset.apply_action(changeset, :insert)
    end
  end

Post #4

achempion

final version

  def create_recording(attrs \\ %{}) do
    Ecto.Multi.new()
    |> Ecto.Multi.insert(:recording, Recording.changeset(%Recording{}, attrs))
    |> Ecto.Multi.update(:recording_with_audio, &Recording.audio_changeset(&1.recording, attrs))
    |> Repo.transaction()
    |> case do
         {:ok, %{recording_with_audio: recording}} -> {:ok, recording}
         {:error, _operation, changeset, _} ->
           # make the form to a "new" state insted of "update"
           # https://github.com/phoenixframework/phoenix_ecto/blob/master/lib/phoenix_ecto/html.ex#L300
           meta = Map.put(changeset.data.__meta__, :state, nil)
           data = Map.put(changeset.data, :__meta__, meta)

           Map.put(changeset, :data, data)
           |> Ecto.Changeset.apply_action(:insert)
       end
  end

Post #5

— All posts loaded —