❤️ 💚 💙 💛 💜

@josevalim I completely disagree with this solution. It is not the documentation that needs to be fixed, it is the Repo.delete API that must be fixed to return {:error, ...} in case of an error.

If one wants to get an exception in case of a failure during deletion, e.g., because the item was not found, then one should call Repo.delete!(...) which is there exactly to throw exceptions.

Note that the Ecto.Multi.delete API is also broken, because it does not return {:error, ...}, it throws the stale error ...

The same issue happens with Repo.update.

Please see #2589. We are going to allow {:error, ...} to be returned with an option. We need an option because we need to know on which field to put the error on, as both update/delete operations return a changeset on errors and have it return something else would be breaking and surprising.

@josevalim I have a suggestion: why not set the action to :stale on the changeset, instead of throwing the exception? This would make sense because the Repo functions already set this action according to the function, and in this case :stale seems clean and appropriate. The resulting changeset in {:error ...} would be thrown away in any case as it is not useful in anyway.

There are two issues with this approach:

1. action is about the repository action that was trigger, such as Repo.insert. I think setting it to stale would cause lost of information, as you no longer know what you called, and it would be a misuse of the config

2. most projects are not used to check the value of action, because doing so is not required at all. So imagine someone is building an API. They may call the repository, they will call the repo and get {:error, changeset} and render changeset.errors back to the client. Except the errors are empty because the information of what went wrong is hidden inside action, which you don't care about 99% of the time. That's why I believe the best way to handle it is by explicitly adding it to a field of your choice as an error

@josevalim Ok, I tested this

iex(156)> x = %Avatar{nick_name: "fellowguy", user_id: 1} |> change |> foreign_key_constraint(:user_id) |>  Repo.insert

18:24:04.979 [debug] QUERY OK db=0.7ms
begin []

18:24:04.982 [debug] QUERY ERROR db=2.5ms
INSERT INTO "avatars" ("nick_name","user_id") VALUES ($1,$2) RETURNING "id" ["fellowguy", 1]

18:24:04.983 [debug] QUERY OK db=1.7ms
rollback []
{:error,
 #Ecto.Changeset<
   action: :insert,
   changes: %{nick_name: "fellowguy", user_id: 1},
   errors: [user_id: {"does not exist", []}],
   data: #EctoAssoc.Avatar<>,
   valid?: false
 >}

and adding an entry in errors seems more appropriate then.

But, why not put the error automatically on the primary key, like in this example? I am not seeing other Ecto.StaleEntryError situations when passing a struct or changset to a Repo.update or Repo.delete functions. And it would be consistent with the already existing behavior, of which the above code is an example.

Because:

1. Doing this change would be backwards compatible without an option
2. Often in the UI you don't remember to check or show errors that happen in the primary key field - which would trigger the same issue I mentioned before, where errors silently pass through

@josevalim

Point 1, I did not understand.

Point 2, if developers no longer handle the stale exception, I see them changing their code to look for errors of the primary key field. When the API changes, that is the expectable change as well.

Sorry about 1, I meant to say doing this change would be backwards incompatible. While we are working on Ecto 3 now, we are still very conservative with changes, there is no reason to push changes to the user unless we have a very strong reason to, and I don't think we have one here.

About point 2, that's exactly the point. The exception is really straight-forward to see and in the case you don't handle it, you likely get notified about it. If you forget to check the primary key, your users won't get any feedback and you won't ever know about it.

@josevalim I think we are probably talking about different things. I don't mind adding an option to generate an error, instead of throwing an exception, for backwards compatibility. What I was asking is, in case the option to generate the error is passed, why not generate the error on the primary key field?

Oh, I see, sorry! You can choose that if you want to. The argument to the option is exactly the field to add the error to. I can think of cases people would rather have it elsewhere (for example, a permalink, title or another unique field).

@josevalim Ok, so this option will come for the Repo.update and Repo.delete functions in what version of Ecto? the sooner the better, as always :-)

It is already in master and we are actively working towards ecto 3.0, so it should be out soon.

Thanks!