Skip to content

[PM-35184] Use EF inserts instead of linq2db bulk copy on SQLite - #8264

Draft
AlexRubik wants to merge 2 commits into
mainfrom
dirt/pm-35184/sqlite-event-bulk-copy
Draft

[PM-35184] Use EF inserts instead of linq2db bulk copy on SQLite#8264
AlexRubik wants to merge 2 commits into
mainfrom
dirt/pm-35184/sqlite-event-bulk-copy

Conversation

@AlexRubik

@AlexRubik AlexRubik commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

🎟️ Tracking

PM-35184

📔 Objective

EventRepository.CreateManyAsync wrote event batches through linq2db's BulkCopyAsync, which resolves a SQLite provider adapter before emitting any SQL. A self-hosted SQLite reporter saw that resolution select the Classic System.Data.SQLite adapter, an assembly this repository does not reference at any version, and the resulting InvalidOperationException propagated out of BaseRequestValidator.BuildSuccessResultAsync and failed their login.

This branches on Database.IsSqlite() and uses AddRangeAsync with a single SaveChangesAsync instead, matching the provider-branching precedent in CipherRepository and CollectionRepository. Batches on this path are one event for the user plus one per events-enabled organization or provider, so bulk copy was buying nothing here. MySQL and Postgres keep the bulk copy path, and SQL Server self-host uses the Dapper implementation and is unaffected.

Also adds EventRepositoryCreateManyTests, since CreateManyAsync previously had no test asserting it persists what it was given. Its only coverage was incidental, through EventRepositoryDeleteManyTests, where the writes are setup for a delete assertion.

Worth flagging for the reviewer

The reporter's exception does not reproduce on main. The existing SQLite integration tests exercise this path and pass, so the adapter resolves correctly from source, and System.Data.SQLite appears in no .csproj or packages.lock.json in this repo. The failure appears specific to how the lite:2026.3.2 image is published rather than to this code.

So this is hardening with a test, not a confirmed-reproduction fix: it removes the dependency the reporter's stack trace blames from the authentication path. The image packaging question is being followed up on the ticket with the reporter.

Two smaller notes:

  • CipherRepository has six unguarded BulkCopyAsync calls and was suspected of breaking self-hosted import the same way. It does not: a throwaway probe exercising CreateAsync(userId, ciphers, folders) passed on SQLite, despite it passing KeepIdentity = true, which asks more of the provider adapter than the call changed here. Deliberately left alone.
  • CreateManyAsync_NoEvents_DoesNotThrow is named for what it actually pins. Its Assert.Empty is near-vacuous, since the organization id is freshly generated; the real assertion is that an empty batch does not throw on entities.First().

Verification

  • Full dotnet build bitwarden-server.slnx: 0 errors.
  • Infrastructure.IntegrationTest filtered to EventRepository against a migrated SQLite database: 6 passed, 0 failed.
  • Confirmed the new branch is the one actually taken: with the BulkCopyAsync line temporarily replaced by a throw, the SQLite tests still passed.
  • MySQL, Postgres and SQL Server were not run locally. The Database testing workflow covers all four providers and is triggered by the paths in this PR.

📸 Screenshots

Not applicable, no UI changes.

EventRepository.CreateManyAsync wrote event batches through linq2db's
BulkCopyAsync, which resolves a SQLite provider adapter before emitting any
SQL. On self-hosted SQLite deployments that resolution can select the Classic
System.Data.SQLite adapter, an assembly this repository does not reference at
any version, and the resulting InvalidOperationException propagates out of
BaseRequestValidator.BuildSuccessResultAsync and fails the login.

Batches on this path are one event for the user plus one per events-enabled
organization or provider, so bulk copy buys nothing here. Branch on
Database.IsSqlite() and use AddRangeAsync with a single SaveChangesAsync,
matching the provider-branching precedent in CipherRepository and
CollectionRepository. MySQL and Postgres keep the bulk copy path.

[PM-35184]
CreateManyAsync had no test asserting it persists what it was given. The only
coverage was incidental, through EventRepositoryDeleteManyTests, where the
writes are setup for a delete assertion.

Add multi-provider coverage for the batch path, the single-event short
circuit, and the empty guard, so the SQLite branch and the bulk copy branch
are both exercised on every configured provider.

[PM-35184]
@AlexRubik AlexRubik added ai-review Request a Claude code review t:bugfix Change Type - Bugfix labels Aug 27, 2026
@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the SQLite provider branch added to EventRepository.CreateManyAsync in the EF Core implementation, plus the new EventRepositoryCreateManyTests integration test class. The IsSqlite() branch matches the existing provider-branching precedent in CipherRepository and CollectionRepository, Microsoft.EntityFrameworkCore.Sqlite is already referenced by Infrastructure.EntityFramework, and the early return inside the using scope still disposes correctly. No schema change is involved, so no Dapper/MSSQL migration parity is required, and SQL Server self-host continues to use the Dapper implementation unchanged. The new tests follow the [DatabaseData(SelfHosted = true)] pattern established by EventRepositoryDeleteManyTests and scope every read to a freshly generated organization id, so they are isolated across providers.

Code Review Details

No findings met the bar for posting. Notes on what was checked and cleared:

  • Batch size on the new EF path: the largest callers chunk at 100 (CipherService), and SaveChangesAsync batches within a single transaction, so the switch away from bulk copy does not introduce a measurable regression on this path.
  • SetNewId() assigns a non-default Guid before AddRangeAsync, so EF's client-side key generator does not overwrite it — ids stay consistent with the previous bulk copy behavior.
  • DatabaseContext has no SaveChanges override, so routing through EF introduces no additional side effects.
  • The unguarded BulkCopyAsync calls in CipherRepository are explicitly scoped out in the PR description with a stated probe result; not re-litigated here.

@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 17.65%. Comparing base (e607c3f) to head (b219186).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8264      +/-   ##
==========================================
+ Coverage   17.61%   17.65%   +0.03%     
==========================================
  Files        1476     1476              
  Lines       63928    63933       +5     
  Branches     5116     5118       +2     
==========================================
+ Hits        11262    11285      +23     
+ Misses      52468    52450      -18     
  Partials      198      198              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review t:bugfix Change Type - Bugfix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant