[PM-25520] - Adding a transaction for mutliple repo calls#7555
[PM-25520] - Adding a transaction for mutliple repo calls#7555jrmccannon wants to merge 12 commits intomainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7555 +/- ##
===========================================
- Coverage 59.50% 14.73% -44.77%
===========================================
Files 2089 1296 -793
Lines 92421 56254 -36167
Branches 8214 4376 -3838
===========================================
- Hits 54994 8289 -46705
- Misses 35484 47819 +12335
+ Partials 1943 146 -1797 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
New Issues (2)Checkmarx found the following issues in this Pull Request
Fixed Issues (27)Great job! The following issues were fixed in this Pull Request
|
# Conflicts: # bitwarden_license/src/Scim/Users/PostUserCommand.cs
|
Test is failing due to the fact that its creating a SqlServerTestDatabase and it won't be able to pull the connection string to do so. Unsure of how to proceed with this. Do we want to add a SQL server db for this test or just skip it in CI? |
We definitely don't want to skip it in CI, or it'll never be run in practice. The CI job does set up and connect to SQL server, e.g. https://github.com/bitwarden/server/blob/master/.github/workflows/test-database.yml#L131-L132. I assume your issue is caused by the |
|
mzieniukbw
left a comment
mzieniukbw
left a comment
There was a problem hiding this comment.
Nice, i love the direction. There is a bit duplication between EF and Dapper, but generally good direction in my opinion.
| Guid organizationId, | ||
| ScimProviderType scimProvider) | ||
| { | ||
| await using var transactionScope = await transactionManager.BeginTransactionAsync(IsolationLevel.Serializable); |
There was a problem hiding this comment.
❓ Serializable transaction isolation is the strictest you can get, but it have side effects compared to default, like it can throw error if there was a concurrent write (in different transaciton). Is this by choice here ?
On a side note, this changes the business logic, which i wonder, whether this PR should do.
In most cases this transaction isolation is unnecessary, in which case, it is better to begin the transaction just before the first write, not during the get.
One important note, the copy paste code trend is unavoidable and we will see other places using this isolation level for no reason, just because it was used somewhere else. If we would use serializable isolation level by choice here, i think a reasonable comment should be added explaining why it might be needed here.
@bitwarden/dept-dbops Opinions ?
There was a problem hiding this comment.
The isolation is needed so that we can get the current seat count and ensure that it won't change before we complete our request.
For this specific command, multiple requests come into SCIM grab the organization at the same time and attempt to add their user. Currently, the orgservice#inviteuser method grabs resources multiple times including calling out to stripe. An optimized command for inviting users was created, but that introduced an issue where multiple requests would race to update the seat count and all set it to the same value instead of incrementing it.
3 requests come in => get Org: { Seats: 2 } and each add a seat to the org.Seat value. Each request would set org.Seat to 3 when in reality they should set it to 5 (assuming both Org.Seats are occupied). There's also the ability for an Org to set a limit (Org.MaxSeats: 4). So two should succeed and one should fail.
The SCIM protocol requires the server tell it whether the user was safely provisioned so we have to succeed or fail and can't just switch it to an asynchronous add.
With serialized, this would ensure that the seat count is updated correctly as the requests come in.
So that was the reason for this approach. I'd be interested in another way as this is very heavy handed.
| var existing = TransactionState.Current; | ||
| if (existing is not null) | ||
| { | ||
| existing.ReferenceCount++; |
There was a problem hiding this comment.
❓ Since NestedTransactionScope takes care of decreasing the reference count on disposal, shouldn't it also own increasing reference count ?
(Same for dapper)
3 participants
🎟️ Tracking
PM-25520
📔 Objective
Adding a transaction around subsequent repo calls.
📸 Screenshots