Code review needed: creating multiple records sharing some values on one API call

With the help and code samples from friends here (@idi527 and others) I have written this function to create multiple book related records. I pass to this function some shared data (current_user and borrower_id) along with a comma separated list of book_ids. I make some custom validations, insert if everything is fine or return error messages if something is wrong. What I receive back at the client side is the result list (as JSON):

  def create_record(%{"user_object" => user, "borrower_id" => borrower_id} = attrs, book_list) do

    book_list = String.split(book_list, ",")

    result = []

    for book_id <- book_list do
      book_id = book_id |> String.to_integer

      alias Ecto.Multi
      record_changeset = Mango.Records.Record.changeset(%Record{user_id: user.id, book_id: book_id}, attrs)

      Multi.new()
      |> Mango.Records.Validators.valid_book_exists_multi(book_id)
      |> Mango.Records.Validators.valid_book_belongs_to_current_user_and_borrower_multi(book_id, user, borrower_id)
      |> Mango.Records.Validators.valid_last_record_closed_multi(book_id)
      |> Multi.insert(:record, record_changeset)
      |> Repo.transaction()
      |> case do
        {:ok, :first_record} -> {:ok}
        {:ok, %{record: record}} -> result = result ++ record
        {:error, :book_belongs_to_current_user_and_borrower, reason, _changes} -> result = result ++ reason
        {:error, :book_exists, reason, _changes} -> result = result ++ reason
        {:error, :last_record_closed, reason, _changes} -> result = result ++ reason
      end
    end
  end

You’re welcome to review it and provide feedback :slight_smile:

Does it work?

Note that for ... <- ... is not a for loop, but a list comprehension. You don’t need to do result = [] and result = result ++ reason.

I’d probably try and “invert” the logic and first do the list comprehension, and then all these Multi and Repo.transaction

But without significantly changing the logic of your example, I’d probably write something like:

alias Ecto.Multi
alias Mango.Records.{Record, Validators}

# here I suppose book_list is an already parsed list of integers (book ids)
# so no `book_list = String.split(book_list, ",")` <-- cleaning up and preparing
# the inputs is the responsibility of a "boundary" (like a controller action in phoenix)

@typep multi_result :: {:ok, %Record{} | :first_record} | {:error, operation :: atom, reason :: any, reverted_changes :: map}

@spec create_records(%{(String.t | atom) => term}, %User{}, pos_integer, [pos_integer]) ::
      %{(book_id :: pos_integer) => multi_result}
def create_records(attrs, %User{id: user_id} = user, borrower_id, book_ids) do
  book_ids
  |> Stream.map(fn book_id -> 
    {book_id, Record.changeset(%Record{user_id: user_id, book_id: book_id}, attrs)}
  end)
  |> Stream.map(fn {book_id, record_changeset} ->
    multi_result = 
      Multi.new()
      |> Validators.valid_book_exists_multi(book_id)
      |> Validators.valid_book_belongs_to_current_user_and_borrower_multi(book_id, user, borrower_id)
      |> Validators.valid_last_record_closed_multi(book_id)
      |> Multi.insert(:record, record_changeset)
      |> Repo.transaction()

    {book_id, multi_result}
  end)
  |> Enum.into(%{})
end

It’s also not quite clear to me what this code is supposed to do and whether it should halt at invalid changesets / failed “validators” or continue …

2 Likes

@idi527, thank you very much for sharing. I will look into this tomorrow and surely provide feedback. It should continue, and if there are some validation errors it will report in the returned JSON response. The code as I wrote it above works. But I am looking forward to try your suggestions.

You might want to separate the Multi related code to a different function for code reuse. For example:

def validations(record_changeset, book_id, borrower_id) do
  Multi.new()
      |> Mango.Records.Validators.valid_book_exists_multi(book_id)
      |> Mango.Records.Validators.valid_book_belongs_to_current_user_and_borrower_multi(book_id, user, borrower_id)
      |> Mango.Records.Validators.valid_last_record_closed_multi(book_id)
      |> Multi.insert(:record, record_changeset)
end

def create_record(%{"user_object" => user, "borrower_id" => borrower_id} = attrs, book_list) do
  book_list = String.split(book_list, ",")
  result = []
  for book_id <- book_list do
    book_id = book_id |> String.to_integer
    record_changeset = Mango.Records.Record.changeset(%Record{user_id: user.id, book_id: book_id}, attrs)
    case Repo.transaction(validations(record_changeset, book_id, borrower_id)) do
      {:ok, :first_record} -> {:ok}
      {:ok, %{record: record}} -> result = result ++ record
      {:error, :book_belongs_to_current_user_and_borrower, reason, _changes} -> result = result ++ reason
      {:error, :book_exists, reason, _changes} -> result = result ++ reason
      {:error, :last_record_closed, reason, _changes} -> result = result ++ reason
    end
  end
end
1 Like