elixir-ecto / db_connection

Database connection behaviour
http://hexdocs.pm/db_connection/DBConnection.html
306 stars 113 forks source link

Transaction Conclude discards result #244

Closed maennchen closed 3 years ago

maennchen commented 3 years ago

I have a transaction in my application, where the conclude part of a transaction throws a rollback.

The rollback currently discards the result: https://github.com/elixir-ecto/db_connection/blob/dd623062ba6f2fd33f3ab1eb4ad69bb29ef4de01/lib/db_connection.ex#L1556

This might be hard to change without a breaking change, but is there a way to keep the result?

Reasoning

I'm using postgres RLS that checks against local variables to authorize the query and I would like a generic function that starts a transaction, authenticates it, runs a callback and then returns the callbacks result.

Example

This code is simplified a lot but should be enough to understand the intent...

Caller

with_authentication(fn ->
  # Will result in `{:error, %Ecto.Changeset{...}}`
  Acme.Repo.insert(Example.changeset(%Example{}, %{invalid: :data}))
end)

with_authentication

def with_authentication(callback) do
  Acme.Repo.transaction(fn ->
    repo.query!("SELECT SET_CONFIG('acme.username', $1, true)", ["john-doe"])

    callback.()
  end)
end

Expected Result

Something like

{:error, {:rollback, {:error, %Ecto.Changeset{...}}}}

Current Result

{:error, :rollback}
josevalim commented 3 years ago

You are correct, this would certainly be a breaking change. You can call Repo.rollback though with an explicit value. This could be handled in with_authentication itself, so it is transparent to users.

maennchen commented 3 years ago

@josevalim How would the with_authentication function know wether to call rollback?

The idea is that this function is called with a generic (-> term) callback. The code that is using the wrapper is already handling the error cases of the things it is calling. Since the wrapper function is not doing any checks on the result, it would be very counter-intuitive for the user of the function to decide if the wrapper function should call rollback or not.

There is no function like Repo.is_transaction_aborted?() or something comparable in ecto either to make this possible.

What would you think about a Repo.is_transaction_aborted?() in ecto that checks if the current transaction is still ok?

josevalim commented 3 years ago

You can establish your own contract. Have the function return ok or error tuples.