[PM-23134] Update PolicyDetails sprocs for performance #6421
Conversation
…zationUserRepository - Implemented multiple test cases to verify the behavior of GetByUserIdWithPolicyDetailsAsync for different user statuses (Confirmed, Accepted, Invited, Revoked). - Ensured that the method returns correct policy details based on user status and organization. - Added tests for scenarios with multiple organizations and non-existing policy types. - Included checks for provider users and custom user permissions. These tests enhance coverage and ensure the correctness of policy retrieval logic.
… access as a provider
…access logic - Introduced a Common Table Expression (CTE) for organization users to streamline the selection process based on user status and email. - Added a CTE for providers to enhance clarity and maintainability. - Updated the main query to utilize the new CTEs, improving readability and performance. - Ensured that the procedure correctly identifies provider access based on user permissions.
…ure to enhance user access logic - Introduced a Common Table Expression (CTE) for organization users to improve selection based on user status and email. - Updated the main query to utilize the new CTEs, enhancing readability and performance. - Adjusted the logic for identifying provider access to ensure accurate policy retrieval based on user permissions.
- Created a new view, UserProviderAccessView, to streamline user access to provider organizations. - Introduced two stored procedures: PolicyDetails_ReadByUserId and OrganizationUser_ReadByUserIdWithPolicyDetails, enhancing the logic for retrieving policy details based on user ID and policy type. - Utilized Common Table Expressions (CTEs) to improve query readability and performance, ensuring accurate policy retrieval based on user permissions and organization status.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6421 +/- ##
==========================================
+ Coverage 51.76% 55.88% +4.12%
==========================================
Files 1870 1870
Lines 82408 82364 -44
Branches 7271 7271
==========================================
+ Hits 42662 46033 +3371
+ Misses 38118 34626 -3492
- Partials 1628 1705 +77 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Great job! No new security vulnerabilities introduced in this pull request |
…licyType and update unit tests
… implementations in PolicyRepository classes
| [dbo].[OrganizationUserView] OU | ||
| WHERE | ||
| (OU.[Status] <> 0 AND OU.[UserId] = @UserId) | ||
| OR (OU.[Status] = 0 AND OU.[Email] = @UserEmail AND @UserEmail IS NOT NULL) |
There was a problem hiding this comment.
We should use UNION instead of OR. Here’s an example: maybe we can turn this UNION into a view and use it for both stored procedures.
There was a problem hiding this comment.
@bitwarden/dept-dbops what do you think?
There was a problem hiding this comment.
I did a quick test and the UNION does perform slightly better than the OR clause, which is what I expected.
|
@JimmyVo16 has already been looking at this so I'll let him handle the review. |
…ure to use UNION instead of OR
| ON P.[OrganizationId] = OU.[OrganizationId] | ||
| WHERE P.[Type] = @PolicyType AND | ||
|
|
||
| DECLARE @UserEmail NVARCHAR(320) |
There was a problem hiding this comment.
⛏️ Purely cosmetic here but the email column in the DB is NVARCHAR(256), so NVARCHAR(320) is larger than needed. However, SQL Server only allocates memory to hold the actual value, so no need to change unless you want to be consistent with the table column size.
| BEGIN | ||
| SET NOCOUNT ON | ||
|
|
||
| DECLARE @UserEmail NVARCHAR(320) |
There was a problem hiding this comment.
⛏️ Purely cosmetic here but the email column in the DB is NVARCHAR(256), so NVARCHAR(320) is larger than needed. However, SQL Server only allocates memory to hold the actual value, so no need to change unless you want to be consistent with the table column size.
…r consistency in stored procedures
r-tome
deleted the
ac/pm-23134/update-policydetails-sprocs-for-performance
branch
5 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-23134
📔 Objective
Modified
OrganizationUser_ReadByUserIdWithPolicyDetailsto use a CTE and the new stored procedureUserProviderAccessView, as outlined in the dbops update.Updated
PolicyRequirementQueryto usePolicyDetails_ReadByUserIdsPolicyType.⏰ 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