PM-33527 Database SQL scripts#7616
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7616 +/- ##
==========================================
+ Coverage 60.20% 64.61% +4.40%
==========================================
Files 2137 2139 +2
Lines 94412 94462 +50
Branches 8409 8409
==========================================
+ Hits 56840 61034 +4194
+ Misses 35580 31345 -4235
- Partials 1992 2083 +91 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@claude Please add a review |
|
Claude finished @Banrion's task in 2m 56s —— View job Code Review — PM-33527 Database SQL scripts
Thanks for the GDPR cleanup scaffolding. The DDL/sproc/Dapper repo look coherent on their own, but there are gaps before this is wireable into the worker. Findings below grouped by severity. 🔴 Blocking1. Missing EF Core implementation (dual‑ORM strategy)
See the precedent set by 2. DI registration missing
Without this, 3. SELECT TOP 1 * FROM [dbo].[OrganizationEventCleanup]
WHERE [CompletedAt] IS NULL
ORDER BY [QueuedAt] ASCIf more than one worker instance polls (which is the normal case in App Services / k8s), every worker will pick the same row. Recommended fix — atomically claim and return in one statement: UPDATE TOP (1) c
SET [StartedAt] = SYSUTCDATETIME(),
[LastProgressAt] = SYSUTCDATETIME()
OUTPUT inserted.*
FROM [dbo].[OrganizationEventCleanup] c WITH (UPDLOCK, READPAST, ROWLOCK)
WHERE [CompletedAt] IS NULL
AND [StartedAt] IS NULL
ORDER BY [QueuedAt] ASCIf you do want a separate "peek" + "start", at minimum use 🟠 Important4. No max-attempts / terminal failure handling 5. No retry backoff 6. Missing tests 7. CREATE NONCLUSTERED INDEX [IX_Event_OrganizationId]
ON [dbo].[Event]([OrganizationId] ASC)
WHERE [OrganizationId] IS NOT NULL
WITH (ONLINE = ON, MAXDOP = 4);Also: the existing 🟡 Minor / Style8. 9. Filtered index would be cheaper CREATE NONCLUSTERED INDEX [IX_OrganizationEventCleanup_Pending]
ON [dbo].[OrganizationEventCleanup]([QueuedAt] ASC)
WHERE [CompletedAt] IS NULL;10. 11. 12. 13. GDPR retention question 14. SonarCloud flagged 2 new issues on the Sonar dashboard — please review. SummaryThe shape of the queue table is reasonable, but as written this PR can't ship: there is no EF Core implementation for non-MSSQL providers, the repository isn't registered with DI, and |
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES This PR adds the database schema, stored procedures, entity, interface, and Dapper repository for tracking organization event-log cleanup jobs (GDPR-driven Azure Table Storage purge of orphaned events). Recent commits address several earlier review comments — stored procedures were renamed to the Code Review Details
Other open threads from prior review passes are tracked in the PR conversation (sproc naming, |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
mkincaid-bw
left a comment
mkincaid-bw
left a comment
There was a problem hiding this comment.
This PR creates a new table and modifies an existing table but there are no EF migrations to handle these changes for self-host/lite customers. Is that intentional?
| ON [dbo].[Event]([Date] DESC, [OrganizationId] ASC, [ActingUserId] ASC, [CipherId] ASC) INCLUDE ([ServiceAccountId], [GrantedServiceAccountId]); | ||
|
|
||
| GO | ||
| CREATE NONCLUSTERED INDEX [IX_Event_OrganizationId] |
There was a problem hiding this comment.
ℹ️ Just noting that Events for our cloud database are kept in Azure Table storage, not in the SQL database, so this index would only be applicable to self-host/lite customers.
| @@ -0,0 +1,15 @@ | |||
| CREATE PROCEDURE [dbo].[OrganizationEventCleanup_IncrementProgress] | |||
There was a problem hiding this comment.
This stored procedure name does not follow our naming conventions. This should be named something like OrganizationEventCleanup_UpdateProgress.
| [dbo].[OrganizationEventCleanup] | ||
| SET | ||
| [EventsDeletedCount] = [EventsDeletedCount] + @Delta, | ||
| [LastProgressAt] = SYSUTCDATETIME() |
There was a problem hiding this comment.
When updating a value in the database, the value should be generated in code and passed in as parameters to the stored procedure.. We should not be using built-in SQL functions like SYSUTCDATETIME or GETUTCDATE
| @@ -0,0 +1,14 @@ | |||
| CREATE PROCEDURE [dbo].[OrganizationEventCleanup_MarkCompleted] | |||
There was a problem hiding this comment.
This stored procedure name does not follow our naming conventions. This should be named something like OrganizationEventCleanup_UpdateCompleted.
| [CompletedAt] = SYSUTCDATETIME(), | ||
| [LastProgressAt] = SYSUTCDATETIME() |
There was a problem hiding this comment.
See previous comment regarding SYSUTCDATETIME
| @@ -0,0 +1,14 @@ | |||
| CREATE PROCEDURE [dbo].[OrganizationEventCleanup_MarkStarted] | |||
There was a problem hiding this comment.
This stored procedure name does not follow our naming conventions. This should be named something like OrganizationEventCleanup_UpdateStarted.
| [StartedAt] = COALESCE([StartedAt], SYSUTCDATETIME()), | ||
| [LastProgressAt] = SYSUTCDATETIME() |
There was a problem hiding this comment.
See previous comments regarding SYSUTCDATETIME
| @@ -0,0 +1,16 @@ | |||
| CREATE PROCEDURE [dbo].[OrganizationEventCleanup_RecordError] | |||
There was a problem hiding this comment.
This stored procedure name does not follow our naming conventions. This should be named something like OrganizationEventCleanup_UpdateError.
| SET | ||
| [Attempts] = [Attempts] + 1, | ||
| [LastError] = @Message, | ||
| [LastProgressAt] = SYSUTCDATETIME() |
There was a problem hiding this comment.
See previous comments regarding SYSUTCDATETIME
Yes because my thinking is if they are self-hosted and they want to delete their organization they should have the ability to just delete the table or database on their server since Events are stored in a table for self-hosted/lite customers. @mkincaid-bw |
…warden/server into dirt/pm-33527/db-orphaned-event-logs
|
|
@claude Please re-review |
|
Claude finished @prograhamming's task in 2m 49s —— View job Code Review — PM-33527 Database SQL scripts (re-review)
SummaryMost prior feedback has been addressed. The atomic claim implementation is solid, naming conventions are now consistent with the contributing guide, and the runaway ✅ Resolved since last review
🟠 Important — still open1. SonarCloud reliability gate still failing 2. No tests added
🟡 Minor3. Hardcoded 10-minute lease in 4. Filtered index would be cheaper for queue read pattern CREATE NONCLUSTERED INDEX [IX_OrganizationEventCleanup_Pending]
ON [dbo].[OrganizationEventCleanup]([CreationDate] ASC)
INCLUDE ([StartDate], [RevisionDate])
WHERE [CompletedDate] IS NULL;5. 6. No terminal-failure / max-attempts cutoff 7. Whitespace-only change to 8. The shape is much closer to mergeable. The SonarCloud reliability finding and the missing tests are the two items I would not ship without; the rest are polish. • Branch: |
3 participants
🎟️ Tracking
PM-33527
📔 Objective
In order to stay within GDPR compliance when a organization wants to be deleted we need to remove the event logs from Azure Table Storage. This PR is the database changes needed in order to delete the event logs using jobs and long running processes.
📸 Screenshots