Hi,
Looking at the documentation of the transaction/2 function it should return a {:ok, result} or {:error, reason} tuple. However, if I use insert_all/3 with transaction/2 I don’t get the expected result, but an exception is thrown anyway.
For example: if I try to insert a duplicate record an exception is thrown instead of returning a tuple with {:error, reason}.
With function:
case Repo.transaction(fn -> Repo.insert_all(SomeSchema, inserts) end) do
{:ok, value} -> :ok
{:error, reason} -> :error
end
With Ecto.Multi:
Multi.new()
|> Multi.insert_all("insert", SomeSchema, inserts)
|> Repo.transaction()
|> case do
{:ok, _} -> :ok
{:error, _, _, _} -> :error
end
I know that insert_all/3 has an option (on_conflict: :nothing) to prevent an exception from being raised. Even knowing this, it is a surprise that the code works in an “unsafe” way, why does it work like this?
That code re-raises everything but the specific :throw on lines 866-868.
Repo.transaction rolls back when an exception occurs but does not stop them. From the docs:
If an unhandled error occurs the transaction will be rolled back and the error will bubble up from the transaction function. If no error occurred the transaction will be committed when the function returns. A transaction can be explicitly rolled back by calling rollback/1, this will immediately leave the function and return the value given to rollback as {:error, value}.
That’s actually fine and allowed. Maybe you misremember something…
The way it works may come as a surprise to you, but it’s not really unsafe, or is it? Any exception that occurs inside a transaction, whether due to the database or Elixir, will cause a rollback of all changes and in that sense it is rather safe. So your idea of safety here seems to be that you expect transaction/2 to always return an error tuple when something went wrong including when an exception was raised.
Why does it work like that? I think it has to do with the concept and the nature of exceptions. The philosophy is namely to let them “bubble” up and not to suppress/convert them at an arbitrary point. Imagine transaction/2 caught all of them and always returned an error tuple. In that case you wouldn’t be able to have an outer/wrapping try/1 statement that listens on certain exceptions. What’s more, such exceptions wouldn’t be reported to the bug database by libraries like Sentry. Unless you pattern-match and re-raise, of course, but you’d have to remember to do that every time you call the transaction function. So in general it’s better to allow exceptions to bubble up.
However, considering how some other functions work you do probably have a point. For example, Jason.decode/2 always returns an error tuple containing the exception struct when there was an error. But there’s the counterpart Jason.decode!/2 which raises in case of errors. Ecto has insert/2 and insert!/2. Should it also have two transaction functions in the same way? Seems reasonable, but it would be a huge breaking change I believe. Maybe you’d like to comment, @josevalim?
If you need to have a “safe” transaction function I’d suggest to add a safe_transaction/2 function to your Repo module which just catches all exceptions and converts them to an error tuple.
I think its a valid question. There is a convention in the Elixir standard library that functions which raise exceptions end with a bang, (such as Map.fetch!) and often have a corollary that returns an error tuple.
The difference here, is that the exception is not coming from Repo.transaction. It’s coming from the code you are running inside the thunk; that is your code and your exception. Catching that and turning it into a tuple would be much more surprising to me than raising it.
Maybe the question is really about Repo.insert_all. This function will raise exceptions for all kinds of underlying database conditions including constraint violations, read-only databases, missing tables etc. The conflict behaviour is well-documented in the function docs and is consistent with other Ecto operations. The other exceptions are … well…exceptional. Those are almost always programmer errors and there is no sensible way to handle them other than to crash in my opinion.
Now I wish we’d had a transaction!/2 alternative because letting it raise now means that one would have to spread try/catch’es all over the code for those edge cases, isn’t it? I heard that this could be considered kind of an antipattern exactly because of the premises you exposed.
BTW, the docs say this about the bang convention:
A trailing bang (exclamation mark) signifies a function or macro where failure cases raise an exception.
It doesn’t seem to make a distinction from where or how far down the call stack the exception is raised but actually if the failure case raises an exception or not.
Since transaction/2 abstracts the underlying code, why should I care about what happens inside of it? BTW, you also could make the same argument for the File.read/1 function, because the error doesn’t actually come from inside the function but from erlang / disk - the code must be abstracted away from implementation details somewhere, don’t you think? It seems that it depends on how abstract your code should be.
And now I’m also curious how does Ecto.Multi come to play if this is the expected behavior? I always thought that Ecto.Multi was meant to allow you to do “safe” operations (steps) and treat any error that prevents your operation from succeeding - which is still kinda true. But since there are cases that simply raise, this means that one cannot just match on :error and expect that it would just work, right? I’ve successfully used Ecto.Multi in the past and now I’m thinking about unexpected edge cases left behind.
It isn’t abstracting the underlying code at all though, the underlying code is what you passed to it in the function closure. It is YOUR code that is raising the exception, not the function or it’s dependencies. Imagine if you wrote this:
Repo.transaction(fn ->
raise "bad thing!"
end)
What do you expect the output of that should be?
You can’t, because it isn’t invoking a user-supplied function!
Agree, and it does make sense taking your example into consideration. But if we are talking about abstraction, at a higher level, isn’t this true for all arguments you pass to a function (I thought this was what having first-class functions meant at least)?
BTW, would you make the same claim for Ecto.Multi if you are using only internal APIs like Ecto.Multi.insert_all/5 for instance?
I though “thunk” was a typo - the example was off because of that, sry.
Taking all that into consideration, does anybody know what is considered a failed operation for the purposes of using Ecto.Multi to treat failures instead of raising an exception?
case result do
{:ok, %{account: account, log: log, sessions: sessions}} ->
# Operation was successful, we can access results (exactly the same
# we would get from running corresponding Repo functions) under keys
# we used for naming the operations.
{:error, failed_operation, failed_value, changes_so_far} ->
# One of the operations failed. We can access the operation's failure
# value (like changeset for operations on changesets) to prepare a
# proper response. We also get access to the results of any operations
# that succeeded before the indicated operation failed. However, any
# successful operations would have been rolled back.
end
People generally use Multi to chain together a bunch of Multi.run closures, and so it fails if any of those return {:error, something} which is most typically {:error, changeset} as a result of passing an invalid changeset to Repo.insert or Repo.update. If you have constraint functions in your changeset then pretty much every failure will be an {:error, changeset} that you can handle normally, exceptions would only happen for things like timeouts or migrations that haven’t run typically.
Non-bang functions never means it will never raise. For example, pass an integer to File.read/1. They will return tagged tuples for some class of errors that are up to the library to decide.
Plus transaction/2 is running your code via a function, so we shouldn’t compare it with File.read/1. A more apt comparison here would be File.open/2, which also doesn’t handle your own exceptions inside the function either.
Or from op’s example, an insert_all that raises a constraint error, which means it won’t treat “exceptional” (pun intended) cases at all, so you have to be explicit about the failing cases - got it.
Hey @josevalim, yes of course! I think that the “tricky part” here (hence OP’s question), is that the transaction/2 spec shows that it returns an error tuple. So one could assume it should be safe at a first glance. But again, like was shown in the thread, the other part is also documented.
From how the naming convention is phrased, it gives the impression that it’s always one or the other (but again, we don’t have a paired function here as well):
Many functions come in pairs, such as File.read/1 and File.read!/1. File.read/1 will return a success or failure tuple, whereas File.read!/1 will return a plain value or else raise an exception:
So, this use case signals to me is that is perfectly possible to have a “”“unsafe”“” function that still returns an error tuple, is that right?
Yes, that was a poor example. I didn’t understand what @jeremyjh meant by “thunk” in the previous comment. I agree it is not the same use case. That helped clearing things out a bit.
