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

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

1 Like

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

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