Repository navigation
[PM-40199] feat: Add shared-folder alias routes to CiphersController - #8753
aaronjordan wants to merge 5 commits into
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES This PR adds Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8753 +/- ##
=======================================
Coverage 68.48% 68.49%
=======================================
Files 2750 2751 +1
Lines 117618 117677 +59
Branches 10781 10788 +7
=======================================
+ Hits 80554 80601 +47
- Misses 34524 34537 +13
+ Partials 2540 2539 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| // FIXME: Update this file to be null safe and then delete the line below | ||
| #nullable disable |
There was a problem hiding this comment.
The FIXME comment above CipherBulkUpdateSharedFoldersRequestModel marks null-safety work as unfinished and leaves that deferral in production code.
| // FIXME: Update this file to be null safe and then delete the line below | |
| #nullable disable | |
| #nullable disable |
Details
✨ AI Reasoning
The comment marks null-safety work as unfinished. It has no ticket reference or complete handoff plan, so it is a lingering deferral marker in production code.
Reply on this comment to give feedback or ignore the issue.
More info
There was a problem hiding this comment.
This effectively clones the existing 'collections' file, so I'm not concerned about the new FIXME.
7f25518 to
b9cbab8
Compare
Add /shared-folders, /shared-folders_v2, /shared-folders-admin (PUT+POST) and /bulk-shared-folders aliases that delegate to the existing collections handlers. Mark the legacy routes obsolete (retained for backwards compatibility).
Make the shared-folders methods the primary implementations and turn the legacy collections methods into obsolete stubs that forward to them. Drop the shared-folders POST aliases, since the POST collections endpoints are already deprecated in favor of PUT.
b9cbab8 to
1ddda3c
Compare
|
TIL: the merge commit to sync up to main was the wrong decision. Also: added changes to address PM-46444, an extension of the initial ticket. |
| SutProvider<CiphersController> sutProvider) | ||
| { | ||
| var response = await InvokeVNextAsync(id, userId, collectionId, sutProvider, | ||
| (sut, model) => sut.PutCollections_vNext(id, new CipherCollectionsRequestModel { CollectionIds = model })); |
There was a problem hiding this comment.
❌ CRITICAL: Direct calls to the now-[Obsolete] PutCollections_vNext and PostBulkCollections raise CS0618, and TreatWarningsAsErrors turns that into an Api.Test build failure.
Details and fix
Directory.Build.props sets TreatWarningsAsErrors=true, and Api.Test.csproj only exempts CA1304. The obsolete legacy methods are called directly, with no suppression, at:
- line 218:
sut.PutCollections_vNext(...)(new) - line 234:
sutProvider.Sut.PostBulkCollections(model)(new) - lines 3904 and 3943:
sutProvider.Sut.PostBulkCollections(...)(existing tests, which break now that this PR marksPostBulkCollectionsobsolete)
These tests are meant to exercise the legacy routes, so wrap the calls the way this file already does elsewhere (see lines 1930 and 3423):
#pragma warning disable CS0618 // Deliberately exercising the legacy collections route.
(sut, model) => sut.PutCollections_vNext(id, new CipherCollectionsRequestModel { CollectionIds = model })
#pragma warning restore CS0618Or point the lease-gate tests at lines 3904 and 3943 to PostBulkSharedFolders, since the legacy method now only forwards to it.
code-review
There was a problem hiding this comment.
Claude claims this is a false positive.
The Claude review's one finding is a false positive, so the REQUEST CHANGES verdict doesn't hold.
Claim: The tests call the now-obsolete PutCollections_vNext and PostBulkCollections without a CS0618 suppression. The review says this breaks the Api.Test build under TreatWarningsAsErrors.
Findings:
- The calls are unsuppressed, as it says, but CS0618 is set to suggestion in .editorconfig:80. It never surfaces as a warning, so it can't be promoted to an error.
- I forced a full recompile of Api.Test on the current head (1ddda3c). It built with 0 errors and 10 warnings, and all 10 are BWA0001, which the repo already exempts from TreatWarningsAsErrors.
- The comment was posted on 2026-10-26 against an earlier commit. The PR has since been rebased and extended with PM-46444.
- In CI, Lint and the Docker image builds have passed, apart from Admin and SeederApi, which were still pending when I checked. Run tests was also still pending, so that remains the last confirmation for this claim.
No change needed in response to this finding.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-40199
https://bitwarden.atlassian.net/browse/PM-46444
📔 Objective
Adds
/shared-folders,/shared-folders_v2,/shared-folders-adminand/bulk-shared-foldersaliases that delegate to the existing collections handlers. I did not rename the already-obsolete POST methods as callers should be moving onto PUT for those already.📸 Screenshots