Skip to content

[PM-28555] Add idempotent sproc to create My Items collections#6801

Merged
eliykat merged 6 commits intomainfrom
ac/pm-28555/add-default-collections-sproc
Jan 15, 2026
Merged

[PM-28555] Add idempotent sproc to create My Items collections#6801
eliykat merged 6 commits intomainfrom
ac/pm-28555/add-default-collections-sproc

Conversation

@eliykat
Copy link
Copy Markdown
Member

@eliykat eliykat commented Jan 6, 2026

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-28555

📔 Objective

Add a sproc to:

  • allow for the creation of smaller numbers of My Items collections, without using SqlBulkCopy
  • ensure My Items collections are properly arranged, rather than relying on the caller
  • ensure that duplicate My Items collections are not created for a combination of user and organization.

After some iteration in #6791 , we are accepting some time-of-check time-of-use risk here. Architecture did not want to enforce uniqueness at the database level so we will keep it simple, filter out existing My Items collections in the sproc, and we can increase the isolation level later if it proves to be a problem.

📸 Screenshots

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

- DRY up collection arrangement
- DRY up tests because bulk and non-bulk share same behavior
- use EF native AddRange instead of bulk insert, because
  we expect smaller data sizes on self-host
@eliykat eliykat requested a review from withinfocus January 6, 2026 05:52
@eliykat eliykat requested review from a team as code owners January 6, 2026 05:52
@eliykat eliykat requested a review from BTreston January 6, 2026 05:52
@eliykat eliykat added the ai-review Request a Claude code review label Jan 6, 2026
@eliykat eliykat requested review from r-tome and removed request for BTreston January 6, 2026 05:53
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Jan 6, 2026

Claude finished @eliykat's task in 1m 41s —— View job


Code Review Summary

PR: PM-28555 Add idempotent sproc to create My Items collections

Status: Approved

Review Assessment

This PR adds a stored procedure to create My Items (default user) collections with idempotent behavior, ensuring no duplicate collections are created.

Key Changes Reviewed:

  • New CollectionUtils.cs utility class for building default collection entities
  • New Collection_CreateDefaultCollections stored procedure with internal filtering
  • Repository method split into CreateDefaultCollectionsAsync (sproc-based) and CreateDefaultCollectionsBulkAsync (bulk insert)
  • EF implementation uses native AddRangeAsync instead of bulk copy for self-hosted scenarios
  • Integration tests with shared test logic for both implementations

Architecture:

  • The stored procedure correctly filters out users who already have default collections using a NOT EXISTS subquery
  • Temp table #FilteredIds approach is clean and efficient
  • Transaction handling is properly implemented in both Dapper and EF repositories

Security: No concerns - parameterized queries via stored procedure, no sensitive data exposure

Correctness: The idempotent design prevents duplicate collections. The acknowledged TOCTOU race condition is an acceptable tradeoff per architecture decision.

Test Coverage: Good integration test coverage with three scenarios: success case, partial creates (new users only), and full idempotency verification.

No issues identified that would block merge.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 6, 2026

Logo
Checkmarx One – Scan Summary & Details011440ea-30a4-4e98-af4b-7af3f9bf7bdb

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 6, 2026

Codecov Report

❌ Patch coverage is 91.11111% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.90%. Comparing base (51d90cc) to head (ec33413).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ucture.Dapper/Repositories/CollectionRepository.cs 75.75% 7 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6801      +/-   ##
==========================================
+ Coverage   56.00%   59.90%   +3.89%     
==========================================
  Files        1965     1966       +1     
  Lines       86886    86883       -3     
  Branches     7740     7741       +1     
==========================================
+ Hits        48663    52045    +3382     
+ Misses      36422    32955    -3467     
- Partials     1801     1883      +82     

☔ View full report in Codecov by Sentry.
📢 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.

mkincaid-bw
mkincaid-bw previously approved these changes Jan 12, 2026
Copy link
Copy Markdown
Contributor

@mkincaid-bw mkincaid-bw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eliykat eliykat removed the needs-qa label Jan 15, 2026
@eliykat eliykat enabled auto-merge (squash) January 15, 2026 22:46
@eliykat eliykat merged commit ebb0712 into main Jan 15, 2026
47 of 49 checks passed
@eliykat eliykat deleted the ac/pm-28555/add-default-collections-sproc branch January 15, 2026 22:49
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants