[PM-25520] - Adding a transaction for mutliple repo calls#7555
[PM-25520] - Adding a transaction for mutliple repo calls#7555jrmccannon wants to merge 17 commits intomainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7555 +/- ##
==========================================
+ Coverage 59.73% 64.16% +4.43%
==========================================
Files 2109 2115 +6
Lines 92752 92981 +229
Branches 8246 8261 +15
==========================================
+ Hits 55402 59658 +4256
+ Misses 35385 31261 -4124
- Partials 1965 2062 +97 ☔ 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)
There was a problem hiding this comment.
Good point this isn't even read anywhere so I'll remove it and add a base class the transaction manager.
…transaction manager and updated dapper/ef to use it.
|
3 participants
🎟️ Tracking
PM-25520
📔 Objective
Adding a transaction around subsequent repo calls.
📸 Screenshots