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

[acrolink]

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

[idi527]

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 …

[acrolink]

@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.

[joaquinalcerro]

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