Skip to content

Conversation

@Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Oct 2, 2024

The transaction behavior was initially introduced in 52862fa and the commit message mentions:

Because our tests run in a single transaction, we need to wrap that update to ensure that the error doesn't poison the transaction.

but that first part is no longer true, so we don't need the transaction anymore AFAICT.

This PR also improves our test suite for this functionality a little to catch potential regressions.

@Turbo87 Turbo87 added C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear A-backend ⚙️ labels Oct 2, 2024
@Turbo87 Turbo87 requested review from a team and eth3lbert October 2, 2024 15:09
@codecov
Copy link

codecov bot commented Oct 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.11%. Comparing base (7ecdc11) to head (2e60082).
Report is 6 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #9559   +/-   ##
=======================================
  Coverage   89.11%   89.11%           
=======================================
  Files         285      285           
  Lines       28922    28942   +20     
=======================================
+ Hits        25774    25793   +19     
- Misses       3148     3149    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Turbo87
Copy link
Member Author

Turbo87 commented Oct 3, 2024

nevermind... I now understand the purpose of this transaction: if find_by_api_token() is used within an outer transaction then a failing update() would cause the outer transaction to be poisoned on the database side, even though the error is handled in the or_else() invocation on the app side.

TIL: transactions with only a single statement are not always as pointless as I thought...

I'll try to come up with a couple of tests for this behavior in a follow-up PR

@Turbo87 Turbo87 closed this Oct 3, 2024
@Turbo87 Turbo87 deleted the token-transaction branch October 31, 2024 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-backend ⚙️ C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant