[PM-13013] add delete many async method to i user repository and i user service for bulk user deletion #5035
Conversation
|
WIP: I am yet to add this functionality to |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5035 +/- ##
==========================================
- Coverage 42.66% 42.63% -0.04%
==========================================
Files 1411 1411
Lines 65087 65137 +50
Branches 5959 5960 +1
==========================================
Hits 27772 27772
- Misses 36075 36125 +50
Partials 1240 1240 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
New Issues
Fixed Issues
|
There was a problem hiding this comment.
It may be better to avoid changing this stored procedure and instead create a new one called User_DeleteManyByIds. This approach could simplify maintenance by allowing the database to be updated first without needing to worry about request failures if the API hasn't been updated yet.
Then, in a later release, we can switch to just using the new sproc and delete the old one.
| CREATE PROCEDURE [dbo].[User_DeleteById] | ||
| @Id UNIQUEIDENTIFIER | ||
| CREATE PROCEDURE [dbo].[User_DeleteByIds] | ||
| @Ids [dbo].[GuidIdArray] |
There was a problem hiding this comment.
We're no longer using GuidIdArray, instead we parse a json string. Example: https://github.com/bitwarden/server/blob/main/src/Sql/dbo/Stored%20Procedures/User_ReadByIdsWithCalculatedPremium.sql
There was a problem hiding this comment.
Infrastructure.EFIntegration.Test is deprecated. Database testing should be done in Infrastructure.Integration.Test.
…serRepository-and-IUserService-for-Bulk-User-Deletion
| @@ -167,7 +167,19 @@ public override async Task DeleteAsync(User user) | |||
| { | |||
| await connection.ExecuteAsync( | |||
| $"[{Schema}].[{Table}_DeleteById]", | |||
| new { Id = user.Id }, | |||
| new { Ids = new List<Guid> { user.Id }.ToGuidIdArrayTVP() }, | |||
There was a problem hiding this comment.
Revert this change to pass the single Id. The failing integration test will pass then.
| -- Delete ciphers | ||
| WHILE @BatchSize > 0 | ||
| BEGIN | ||
| BEGIN TRANSACTION User_DeleteById_Ciphers |
There was a problem hiding this comment.
I think we may want a big transaction that scopes deleting everything related to each single user. @rkac-bw will have much better insights
There was a problem hiding this comment.
As a single migration on release night I like a large transaction, it is a one time script and must succeed. As a reoccurring script that runs at any time creating a large transaction will lead to lots of locking and potential deadlocks on a busy db like ours, I would prefer a retry on error with smaller transactions and possibly
give it a deadlock priority of high so it is not the victim
|
@BTreston, I forgot to mention this in the description of the ticket but can you wire up this new method in ? |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-13013
📔 Objective
WIP: Creates the method
DeleteManyAsyncand addsUser_DeleteByIdsstored procedure to enable bulk deletion of users📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes