Ecto: Repo.transaction returns {:error, :rollback} when unique constraint fails

[manukall]

Precheck

• Do not use the issues tracker for help or support requests (try Elixir Forum, Stack Overflow, IRC or mailing lists, etc).
• For proposing a new feature, please start a discussion on elixir-ecto.
• For bugs, do a quick search and make sure the bug has not yet been reported.
• Finally, be nice and have fun!

Environment

• Elixir version (elixir -v): 1.6.6
• Database and version (PostgreSQL 9.4, MongoDB 3.2, etc.): PostgreSQL 10.4
• Ecto version (mix deps): 2.2.10
• Database adapter and version (mix deps): postgrex 0.13.5
• Operating system: Linux

Current behavior

Repo.transaction returns {:error, :rollback} when a unique constraint failed but was caught with unique_constraint in the changeset.

Example:

Repo.transaction(fn ->
  Repo.insert(User.changeset(%User{}, email: "[email protected]"))
end)

User.changeset in this example would have the line

|> unique_constraint(:email)

and the database would include the unique constraint on the email field.

Expected behavior

https://hexdocs.pm/ecto/Ecto.Repo.html#c:transaction/2-use-with-function says

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.

The failing constraint is not an unhandled error, so I would expect the transaction to be commited and {:ok, result} to be returned.

[wojtekmach]

I couldn't reproduce this on ecto 2.2.10, it behaves as expected:

iex(1)> Repo.transaction(fn -> change(%Post{}, title: "foo") |> unique_constraint(:title) |> Repo.insert() end)
{:ok,
 {:error,
  #Ecto.Changeset<
    action: :insert,
    changes: %{title: "foo"},
    errors: [title: {"has already been taken", []}],
    data: #Bloggy.Blog.Post<>,
    valid?: false
  >}}

could you create a sample app that reproduces the error?

[manukall]

I can't reproduce this in a clean project, either. some things that are special about the original case:

• the unique index is over multiple fields and is a partial index: CREATE UNIQUE INDEX subscriptions_publication_id_member_id_index ON public.subscriptions USING btree (publication_id, member_id) WHERE (state = ANY (ARRAY['active'::text, 'in_trial'::text]))
• one of those fields (publication_id) is set by a BEFORE trigger on insert

I've tried both in my example app, but still the {:error, :rollback} function isn't triggered. The function inside the transaction doesn't do anything besides building and inserting the new record. Any ideas where else to look?

[manukall]

After putting some IO.inspects into DBConnection and testing some more, I don't think it's related to the index kind or trigger. Even running
Repo.transaction(fn -> %User{} |> change(%{email: "[email protected]"}) |> unique_constraint(:email) |> Repo.insert() end) in IEx has the same result, while in my plain test project it returns {:ok, {:error, ...}} as expected.

Now I think the difference is in the "bad" project it seems I'm always in a nested transaction. I think that, because I've put IO.inspects into DBConnection.transaction_nested and they only print things in the bad project, not in the plain testing one.

I've no idea if that's really the case or how that could happen, though.

Also, please let me know if I'm spamming here. Not sure if that's an Ecto issue anymore, but I also don't really know how to find out. The Ecto codebase is pretty complicated and I'm having some trouble making sense of it.

[josevalim]

@manukall as far as I understand your description of the problem, this is expected behaviour. Once an operation fails, for example because of a constraint violation, the database voids the whole transaction and puts it into rolled back state. So if you attempt to do any other operation, then it will cause those further operations to fail, with {:error, :rollback} reason.

[manukall]

@josevalim now i'm really confused. i would expect the behavior @wojtekmach described, which is also what happened when i generated a new app to reproduce my problem. are you saying what @wojtekmach is not expected?

[wojtekmach]

@manukall how many DB operations are you doing within a transaction? If more than 1, I believe the last operation may return :rollback indeed.

When PG hits a constraint violation it refuses to make any other operations in that transaction.

[manukall]

@wojtekmach I'm running this (in IEx):

Repo.transaction(fn -> %User{} |> change(%{email: "[email protected]"}) |> unique_constraint(:email) |> Repo.insert() end)

Maybe it's because of read_after_writes fields in the schema? Otherwise I would assume it's only 1 DB operation.

[josevalim]

@manukall you need to do further insert/update/delete operations in the transaction. If you are using nested transactions, the number of operations in the outer one is the one that counts.

[manukall]

@josevalim i'm pasting from iex:

iex(4)> Repo.transaction(fn -> %User{} |> change(%{email: "[email protected]"}) |> unique_constraint(:email) |> Repo.insert() end)
[debug] QUERY OK db=0.4ms queue=0.1ms
begin []
[debug] QUERY ERROR db=27.9ms
INSERT INTO "users" ("email","inserted_at","updated_at","id") VALUES ($1,$2,$3,$4) RETURNING "shipping_country_code", "shipping_last_name", "shipping_first_name" ["[email protected]", {{2018, 7, 13}, {12, 5, 17, 763527}}, {{2018, 7, 13}, {12, 5, 17, 763553}}, <<157, 138, 32, 216, 249, 106, 66, 170, 148, 199, 45, 108, 139, 166, 180, 238>>]
[debug] QUERY OK db=0.3ms
rollback []
{:error, :rollback}

I'm not aware of using nested transactions. Is there any way of something starting a transaction when my app is started, so that basically every other transaction is nested within that?

[josevalim]

That's weird. Which pool are you using? Maybe something related to the PostgreSQL version?

[manukall]

I haven't configured a pool, so I guess the default one. If it's the PostgreSQL version it shouldn't work in the other app, so I've no idea. Should DBConnection.transaction_nested be called if it's not a nested transaction? I've got some GenServers running that access the database in regular intervals. Could those somehow interfere?

[josevalim]

I've got some GenServers running that access the database in regular intervals. Could those somehow interfere?

I don't think they would. I think at this point, without a way to reproduce, there isn't much we can do. :(

[manukall]

I've managed to create a minimal app with a test that reproduces the issue: https://github.com/manukall/ecto_transaction_constraint_test

It seems like adding a has_many association to the schema makes Repo.insert run inside a transaction, which means Repo.transaction(fn -> Repo.insert...) results in nested transactions?

[josevalim]

@manukall yes.

[manukall]

@josevalim is this intended behavior? it felt quite weird that that test failed after adding the seemingly unrelated has_many to the schema.

[josevalim]

@manukall if you have no entries in has_many, then it should not fail. But if you are inserting something in the has_many too, then it should fail.

[manukall]

@josevalim In this test I'm inserting a user with the same email twice, without any other records. The second time, Repo.transaction returns {:error, :rollback}. I would assume that this test should pass and Repo.transaction returns {:ok, {:error, changeset}}.

[josevalim]

Thanks, I will investigate. You are also using the SQL sandbox and that may affect things.

[manukall]

Thank you. Please let me know if I can help.

[josevalim]

Thanks, fixed in 1020cfe2450a56ad97417e6bff68ebef94830522!

[lpil]

I was unable to find 1020cfe2450a56ad97417e6bff68ebef94830522 so I couldn't be sure, is this fix released yet?

[josevalim]

Good catch @lpil! I probably had to rebase or something and the SHA change, the correct SHA is: fa9490af749eade1cea38d343a39dfa19c229b6b

[lpil]

Thank you @josevalim!

[coladarci]

This is some serious bad behavior on my part by commenting on this closed issue, but I've found myself writing my apps over the last few years around this behavior; not using transactions when I really should, etc and I have to ask if I'm missing something and every time I run into it, I feel dirtier and dirtier.

Consider this code. I'd expect this to always return either :ok or {:error, _error_code}. But this can sometimes return {:error, :rollback} depending on attrs and/or the implimentation details of the schema.

Repo.transaction(fn ->
  with {:ok, _first} <- create_record(attrs),
       {:ok, _second} <- create_record(attrs) do
    :ok
  else
    {:error, changeset} ->
      # Look at the problems and decide on an error code. 
      # `determine_error_code` could raise if an unexpected cs is passed 
      # I'd want to make sure I don't have `first` kicking around if `second` failed; this would be a big problem.
      changeset   
      |> determine_error_code()
      |> Repo.rollback()
  end
end)

The contract holds until something about create_record changes; in our case it was a cast_embed. Or in this issue it was a has_many. This feels wrong, no? Furthermore, the fact that attrs may or may not contain the field that triggers the inner transaction means that this contract will sometimes hold and sometimes not. So instead we've to resorted to writing our own "rollbacks" by looking for records and deleting them in specific situations; this doesn't feel great, to say the least.

So my question is, if this is truly a feature and not a bug, am I missing an obvious way to avoid these problems?

Thanks a million for any advice!

[josevalim]

Hi @coladarci! Can you please provide an example that reproduces the error? It will be much easier for us to give advice if we can look and run the code. Thanks!

[coladarci]

Thanks @josevalim I reproduced this in a new project and I suspect this is just me not understanding transactions. My failing tests WEREN'T when I was calling Repo.rollback(:error_code); those work. My problems came from when I was returning {:error, :error_code} w/out calling Rollback. As it happens, there was a use-case where there was nothing to rollback so I was just returning the error code.

tl;dr Returning {:error, :foo} from inside a transaction is not a good idea. You lose foo and just get get :rollback. If you want to retain the error code, send it in a Rollback.

I assume this is "working as expected" - I just got tripped up. Thanks (as always) for quickly jumping in to help me.

[josevalim]

Thanks for the follow up!