Search code examples
ectoelixir

Refactor Ecto transaction for efficiency using Ecto.Multi

I have a function that assigns multiple skills to a user. The skills already exist and have a many_to_many relationship with the user.

# skills will look like [%{id: "1"}, %{id: "4"}, %{id: "5"}] etc.
def reset(user, skills) do
  user
  |> Repo.preload(:skills)
  |> Ecto.Changeset.change()
  |> Ecto.Changeset.put_assoc(:skills, [])
  |> Repo.update()
  ids = Enum.map(skills, &Map.get(&1, :id))
  query = Repo.all(from(p in Skill, where: p.id in ^ids))
  skills = Enum.each(query, &Skill.create(user, &1))
end

Currently this works but feels inefficient:

  • We may be removing skills that will just be added back again
  • Might not need to run Repo.all to fetch skills, can just join them by ID if they exist
  • Could be wrapped in Ecto.Multi for database efficiency

In addition to this, it would be nice to return the created skills rather than just the :ok atom that Enum.each returns.

What would be the best way to refactor the code?

Solution

  • One improvement would be not assigning each skill one by one

    def reset(user, skills) do
  ids = Enum.map(skills, &Map.get(&1, :id))
  skills = Repo.all(from(p in Question.Skill, where: p.id in ^ids))

  user
  |> Repo.preload(:skills)
  |> Ecto.Changeset.change()
  |> Ecto.Changeset.put_assoc(:skills, skills)
  |> Repo.update()
end

    If no change is needed (that is any deletion or addition to the user's skills), this makes two query to the database (assuming :skills hasn't been loaded, otherwise there'll be just one query, which is fetching the skills).

    Also because we're making a changeset, it doesn't reset the whole thing. It only deletes or adds what is necessary.