[PM-33041] Organization Ability: Refactor CipherResponseModel#7202
[PM-33041] Organization Ability: Refactor CipherResponseModel#7202
Conversation
|
New Issues (2)Checkmarx found the following issues in this Pull Request
Fixed Issues (2)Great job! The following issues were fixed in this Pull Request
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7202 +/- ##
==========================================
+ Coverage 57.45% 57.51% +0.06%
==========================================
Files 2032 2032
Lines 89415 89547 +132
Branches 7951 7965 +14
==========================================
+ Hits 51369 51499 +130
+ Misses 36203 36202 -1
- Partials 1843 1846 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…pherresponsemodel
|
Claude finished @JimmyVo16's task in 2m 5s —— View job Overall Assessment: APPROVE Reviewed the latest commit (ceb3dd3) addressing prior code review feedback. The two issues raised have been resolved: Code Review DetailsNo new findings. Prior findings have been addressed:
|
Pull Request ReviewObjective: Refactor CipherResponseModel to accept a single OrganizationAbility instead of a dictionary, moving off the deprecated GetOrganizationAbilitiesAsync() to enable migration to the new caching pattern. Overall the refactor direction is sound and aligns well with the stated goal. The SyncController batch approach is well thought out. I found one correctness issue and one performance concern detailed in inline comments below. Findings
|
|
test |
Important -- Correctness: PutPartial returns stale dataFile: src/Api/Vault/Controllers/CiphersController.cs, line 709 The previous code re-fetched the cipher after UpdatePartialAsync to return the updated state. The new code reuses the pre-update cipher object. Since CipherResponseModel reads FolderId and Favorite directly from the cipher object (CipherResponseModel.cs lines 126-127), the response will return stale values for both fields. The database is updated correctly, but the immediate API response will not reflect the change. To fix, either re-fetch the cipher after the update (as before), or mutate the in-memory object before building the response: cipher.FolderId = folderId; Suggested -- Performance: N+1 async cache lookups in list endpointsFile: src/Api/Vault/Controllers/CiphersController.cs, lines 151-156 (also 918, 1122) In list endpoints (Get_ManyByListCiphers, bulk archive, bulk unarchive), each cipher triggers an individual GetOrganizationAbilityAsync call inside Task.WhenAll. While the underlying cache is in-memory, this creates N async state machines and N Task allocations instead of a single batch fetch. The SyncController took the right approach: extract distinct org IDs, batch-fetch into a dictionary via GetOrganizationAbilitiesAsync(orgIds), then do a simple dictionary lookup per cipher. Consider applying the same pattern in CiphersController for consistency and to avoid unnecessary overhead in large vaults. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| user, | ||
| await _applicationCacheService.GetOrganizationAbilitiesAsync(), | ||
| _globalSettings); | ||
| var response = new CipherResponseModel(cipher, user, await GetOrganizationAbilityAsync(cipher), _globalSettings); |
There was a problem hiding this comment.
This issue was flagged in the previous review and is still present. The cipher object is fetched before UpdatePartialAsync runs. Since CipherResponseModel reads FolderId and Favorite from the cipher object (lines 126-127 of CipherResponseModel.cs), the response will return the pre-update values.
The previous code re-fetched the cipher after the update. Either restore that re-fetch, or mutate the in-memory object before building the response:
| var response = new CipherResponseModel(cipher, user, await GetOrganizationAbilityAsync(cipher), _globalSettings); | |
| cipher.FolderId = folderId; | |
| cipher.Favorite = model.Favorite; | |
| var response = new CipherResponseModel(cipher, user, await GetOrganizationAbilityAsync(cipher), _globalSettings); |
There was a problem hiding this comment.
Good suggestion. Originally, I didn’t see that I accidentally deleted the updatedCipher and was confused. Re-adding the changes now.
| private async Task<IDictionary<Guid, OrganizationAbility>> GetOrganizationAbilitiesAsync(ICollection<CipherDetails> ciphers) | ||
| { | ||
| var orgIds = ciphers | ||
| .Where(c => c.OrganizationId.HasValue) | ||
| .Select(c => c.OrganizationId!.Value) | ||
| .Distinct() | ||
| .ToList(); | ||
|
|
||
| if (orgIds.Count == 0) | ||
| { | ||
| return new Dictionary<Guid, OrganizationAbility>(); | ||
| } | ||
|
|
||
| var organizationAbilities = await _applicationCacheService.GetOrganizationAbilitiesAsync(orgIds); | ||
|
|
||
| return organizationAbilities; | ||
| } |
There was a problem hiding this comment.
🎨 Suggested — DRY: Duplicated helper method
This method is nearly identical to CiphersController.GetOrganizationAbilitiesAsync (lines 1682-1697). Similarly, SyncResponseModel.GetOrganizationAbility duplicates CiphersController.GetOrganizationAbility. Consider extracting these into a shared static helper (e.g., on a base class or a small utility) to avoid maintaining three copies of the same logic.
| #nullable enable | ||
| public class CipherResponseModel : CipherMiniResponseModel |
There was a problem hiding this comment.
💡 Nitpick — Nullable scope: The #nullable enable here only covers CipherResponseModel and below, while CipherMiniResponseModel remains under #nullable disable. This is fine for this PR, but the split placement (disable at top of namespace, enable mid-file) is a bit unusual. The existing FIXME comment acknowledges this — just noting it for awareness.
| cipher, | ||
| user, | ||
| organizationAbilities: null, // Emergency access only retrieves personal ciphers so organizationAbilities is not needed | ||
| null, // Emergency access only retrieves personal ciphers so organizationAbilities is not needed |
There was a problem hiding this comment.
The comment is no longer accurate: organizationAbilities was changed to organizationAbility?
| var folderId = string.IsNullOrWhiteSpace(model.FolderId) ? null : (Guid?)new Guid(model.FolderId); | ||
| await _cipherRepository.UpdatePartialAsync(id, user.Id, folderId, model.Favorite); | ||
|
|
||
| var updatedCipher = await GetByIdAsync(id, user.Id); |
There was a problem hiding this comment.
❓ Was there an scenario that caused you to remove this call to get the updated cipher? Claude is also pointing out that this would lead to stale data.
|
3 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-33041
📔 Objective
CipherResponseModelto accept a singleorganizationAbilityinstead of a dictionary oforganizationAbility.CipherResponseModelto move off the deprecatedGetOrganizationAbilitiesAsync(), which will enable us to migrate to the new caching pattern.This is the main point of the PR.
📸 Screenshots
Quick sanity tests on GET /sync.