Interactions with Ecto Sandbox

[benwilson512]

Hey folks!

Thanks for making this very cool library. We are trying to retrofit Carbonite into an existing code base and are running into some potentially problematic interactions with the Ecto Sandbox. Consider this test:

test "inserts after an audit should raise" do
  MyApp.Repo.transaction(fn ->
    {:ok, _} = Carbonite.insert_transaction(MyApp.Repo, %{meta: %{type: "rabbit_inserted"}})

    %{}
    |> MyApp.Rabbit.create_changeset()
    |> MyApp.Repo.insert!
  end)
    
  # This isn't inside a carbonite transaction and should error
  %{}
  |> MyApp.Rabbit.create_changeset()
  |> MyApp.Repo.insert!

end

The second Repo.insert! should fail and it WOULD fail if you ran this in your dev console or in production. However in an ecto sandbox test, this will NOT fail. And the reason of course is that the entire test is run inside of a transaction that has already been created by Ecto, and so when the trigger runs on the second insert it is able to find the carbonite record we inserted earlier.

The consequence of this in particular when retrofitting Carbonite into an existing code base is that the tests are not able to provide ANY confidence that we have adequate coverage around transaction insertion. As long as there is any transaction inserted in the test setup, every subsequent insert / update will work just fine. When you run that same code in production, errors will abound.

Our "Solution"

The workaround we have is that in tests, we do:

  def attach_commit_handler() do
    :telemetry.attach(
      "audit_commit_sim",
      [:sensetra, :repo, :query],
      fn _, _, metadata, _ ->
        if String.upcase(metadata.query) == "COMMIT" do
          import Ecto.Query

          Carbonite.Transaction
          |> update([q], set: [xact_id: fragment("0::text::xid8")])
          |> Sensetra.Repo.update_all([])

          Carbonite.Change
          |> update([q], set: [transaction_xact_id: fragment("0::text::xid8")])
          |> Sensetra.Repo.update_all([])
        end
      end,
      %{}
    )
  end

In the ecto sandbox, there is still a COMMIT issued it just doesn't commit the outer sandbox transaction due to how the sandbox uses save points. Thus we can use the ecto telemetry to hook into this COMMIT and alter the saved xact_id values on any inserted carbonite transactions and changes. This way when a follow up insert happens AFTER a transaction is over, it can't / won't find any matching transaction record, and fails as intended.

While this works, it does feel like a bit of a strange pain point. What are other people doing for this situation?

[andreasknoepfle]

Hi @benwilson512,

to be honest, this is a shortcoming of the approach that carbonite takes and you are absolutely right that this is not ideal. In our test suits we usually just make sure that everything is covered with unit tests and we rarely would have two calls inserting a carbonite transaction in there, so we usually catch most of those cases that you describe in unit tests/reviews. But I can see that when adding carbonite to an existing codebase there is a stronger need for solving this.

But your workaround is actually pretty smart 💚 We could think about generalising this a bit and actually putting it into the library as a test helper kind of thing.

[benwilson512]

Gotcha. Because we have a lot of very integration heavy and IoT data analysis heavy code in our backend we tend to favor a lot of "integration" style tests to ensure everything works end to end. Hence the risk here where if some internal part of a data analysis layer happens to touch a carbonite tracked table it's likely this won't get caught without our addition.

[maltoe]

Adding to what @andreasknoepfle said, we also tended to have multiple / many partitions in our audits (e.g. even if component A and B's data lives in the same DB, related tables record audits to carbonite_a / carbonite_b schemas), which further prevented scenarios like yours.

But I also agree your solution is quite smart. Not sure if relevant enough to add as a (yet non-existent) Carbonite.Testing, but maybe as another subsection for this in the docs?