[PM-25106] Refactor Misleading Stored Procedure/Repository Language

[sven-bitwarden]

🎟️ Tracking

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

📔 Objective

When CollectionType + DefaultCollection was added as a concept to Collection, we made a couple of sproc names mask some of their underlying behavior because they filter out these implicit default collections.

Renames

Stored Procedures

• OrganizationUserUserDetails_ReadWithCollectionsById → OrganizationUserUserDetails_ReadWithSharedCollectionsById
• CollectionUser_ReadByOrganizationUserIds → CollectionUser_ReadSharedCollectionsByOrganizationUserIds
• Collection_ReadByOrganizationIdWithPermissions → Collection_ReadSharedCollectionsByOrganizationIdWithPermissions

Repository Methods

• GetDetailsByIdWithCollectionsAsync → GetDetailsByIdWithSharedCollectionsAsync
• GetManyByOrganizationIdWithPermissionsAsync → GetManySharedByOrganizationIdWithPermissionsAsync

Parameters

• includeCollections → includeSharedCollections

Where possible, comparisons of CollectionType were also changed away from Type != DefaultCollection to Type == Shared, so future collection types must be explicitly rather than implicitly added.

Rollout Strategy

Following EDD, this effort will be fully completed over two deploys. This first deploy is additive, creating new sprocs with the desired name. The second deploy will either remove the old sprocs, or change their behavior to not filter (so GetAllCollections.sql appropriately gets all collections without filtering).

⏰ 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

[codecov]

Codecov Report

❌ Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 59.99%. Comparing base (bab4750) to head (976fa81).
⚠️ Report is 17 commits behind head on main.

Files with missing lines	Patch %	Lines
...Console/Repositories/OrganizationUserRepository.cs	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6890      +/-   ##
==========================================
+ Coverage   56.05%   59.99%   +3.93%     
==========================================
  Files        1966     1968       +2     
  Lines       86892    86974      +82     
  Branches     7737     7748      +11     
==========================================
+ Hits        48709    52176    +3467     
+ Misses      36382    32911    -3471     
- Partials     1801     1887      +86     

☔ 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.

[github-actions]

Checkmarx One – Scan Summary & Details – 749c3f65-480e-4289-89aa-d2bb76e87bd1

New Issues (1)

Checkmarx found the following issues in this Pull Request

#	Severity	Issue	Source File / Package	Checkmarx Insight
1		CVE-2025-13465	Npm-lodash-4.17.21	details Description: Lodash versions 4.0.0 through 4.17.22 are vulnerable to prototype pollution in the _.unsetand _.omitfunctions. An attacker can pass crafted paths w... Attack Vector: NETWORK Attack Complexity: LOW Vulnerable Package

[BTreston]

This looks good to me, just one non-blocking thought. I'll let a backend dev have the final approval

[BTreston]

💭 Since we technically are modifying the logic in the sprocs my knee jerk reaction is to throw in a feature flag. That is just my personal paranoia though and is creating mostly just tedious work. Not a blocker, but I think worth a brain cycle.

[sven-bitwarden]

You mean modifying the renamed sprocs to also have Type = 0 instead of Type != 1? I see what you mean - trivial, yet a change nonetheless. We do have examples of it being done elsewhere e.g. OrganizationUserUserDetails_ReadByOrganizationId_V2.sql, but I understand the caution. I feel safe with the integration tests exercising the sprocs appropriately - if anyone feels strongly on the flags, I have no problem adding 'em.

[claude]

Claude finished @sven-bitwarden's task in 2m 4s —— View job

---

Code Review Summary

Reviewed commit: 976fa81 (Fix dedented if)

Overview

This PR refactors stored procedure and repository method names to clarify that they filter to shared collections only, excluding default user collections. It follows the Evolutionary Database Design (EDD) pattern with additive stored procedures in this first deploy.

Changes Reviewed

Category	Files	Status
API Controllers	3	Renamed method calls
Repository Interfaces	2	Method and parameter renames
Dapper Repositories	2	Updated sproc calls and parameter names
EF Repositories	2	Updated method names and query logic
SQL Stored Procedures	3 new	Added with explicit Type = 0 filter
Migration Script	1	Creates new sprocs
Tests	4	Updated method names + new integration tests

Assessment

Risk Level: Low

The changes are well-structured:

1. Additive deployment pattern - New stored procedures are created alongside existing ones. Old sprocs will be handled in a subsequent deploy, following EDD practices.

2. Semantic improvement - Changed filtering logic from Type != 1 (exclude DefaultUserCollection) to Type = 0 (only SharedCollection). This is functionally equivalent for the current enum values but makes the intent explicit and safer for future collection types.

3. Comprehensive test coverage - Integration tests verify that both GetDetailsByIdWithSharedCollectionsAsync and GetManyDetailsByOrganizationAsync correctly exclude default collections. Tests create scenarios with both collection types and assert only shared collections are returned.

4. Consistent EF implementation - Entity Framework queries mirror the stored procedure behavior with c.Type == CollectionType.SharedCollection.

Findings

No critical, important, or suggested issues identified. The implementation is clean, well-tested, and follows established patterns.

---

Result: Approved

This is a well-executed refactoring that improves code clarity without changing runtime behavior.

---

Automated review by Claude Opus 4.5

[JimmyVo16]

Plus one on this. I don’t think we agreed on a standard here, but I’m in favor of being explicit like this. That way, in the future, when we add more types, they won’t be automatically included. There are trade-offs, since this requires manual work to opt in future types, but I think it’s safer and more predictable.

[mkincaid-bw]

LGTM