Ecto `get!` then `delete!` Anti-pattern?

Chat / Discussions
, , ,
1

I’ve seen this pattern used often in the Ecto documentation, Phoenix controller templates, and numerous other places. It’s not clear to me why it is often used:

The pattern is:

user = MyRepo.get!(User, id)
{:ok, _user} = MyRepo.delete(user)

There are a few variations on this pattern, some would use MyRepo.delete!/1 with a bang instead, but all of them retrieved a record and then deleted it. In most situations the user value is discarded and never used again after the delete!/1 call.

Why is it common practice to retrieve a record prior to deleting it? In most of the places I’ve seen this code used, the code that invokes the Ecto queries is not wrapped in a transaction, so it doesn’t guard against the scenario where another process/client deletes the same record between the get and the delete. It will be possible for the code to try to delete record that is already gone and raise a Ecto.StaleEntryError (which Phoenix turns into 409 if you are using the phoenix_ecto package).

One answer this question would be that Ecto doesn’t provide a function that makes it easy to delete a record by ID. Why does Ecto have a function for retrieving a record by ID, but not one for deleting a record by ID? For example, Ecto provides:

MyRepo.get!(User, 42)

But in order to delete a record by ID I have to do this:

MyRepo.delete_all(from(u in User, where: u.id == ^id))

Or this (hacky):

MyRepo.delete(%User{id: id})

Is there a reason we can’t have a nice Repo.delete!(queryable :: Ecto.Queryable.t(), id :: term(), Keyword.t()) :: Ecto.Schema.t() callback?

Places where I see this pattern used:

Related: Ecto delete a record WITHOUT selecting first - #8 by fireproofsocks

2 Likes
2

We cannot do Repo.delete!(queryable, …) is because there is no way for Ecto to guarantee that there will be only 1 deleted entry.

I prefer Repo.delete_all because in many cases it is what user really wants - they do not care whether the row exist at all, if it do not exists, then you already deleted it :wink:

3 Likes
3

If such a function existed, it would have to be very selective about what it supported in queryable - DELETE (at least in Postgres) doesn’t support everything SELECT does.

If there are two simultaneous requests to delete a record (we know users never double-click buttons, but they happen somehow :stuck_out_tongue: ), one of them is either going to get a 409 (if the delete fails) or a 404 (if the get fails). In either case, the record’s state is consistent, so not sure if it’s problem.

IMO this is a good stepping-stone to the pattern:

user = get_visible_user(id, current_user)
{:ok, _user} = MyRepo.delete(user)

where get_visible_user handles verifying that current_user has access to the specified user.

3 Likes
4

What if I have code similar to this:

def delete_post(%Post{} = post) do
  if has_comments?(post) do
    {:error, :has_comments}
  else
    Repo.delete(post)
  end
end

defp has_comments?(%Post{} = post) do
  post = Repo.preload(post, :comments, force: true)

  Enum.count(post.comments) > 0
end

Should the code inside the delete_post/1 function be wrapped in Repo.transaction to prevent the situation that there was a comment added between calling the has_comments? and the Repo.delete functions?

(that example is intentionally simplified, but you get the idea… :slightly_smiling_face: )

Thanks a lot for any input.

5

The standard way to avoid check-then-act issues like that is to use a foreign key on comments.post_id with the ON DELETE RESTRICT option.

1 Like
6

Thanks, that makes sense. But there are situations when you don’t want that rule to be enforced always (by the DB). Would the Repo.transaction help in that case?