fix(content-drive): expose folder permissions and edit-dialog fields on folder search #36595 - #36889
Open
ihoffmann-dot wants to merge 1 commit into
Open
fix(content-drive): expose folder permissions and edit-dialog fields on folder search #36595#36889ihoffmann-dot wants to merge 1 commit into
ihoffmann-dot wants to merge 1 commit into
Conversation
Open
5 tasks
Contributor
|
Claude finished @ihoffmann-dot's task in 4m 33s —— View job Code Review: expose folder permissions & edit-dialog fields on folder search
I traced the full path and could not find a blocking issue. The implementation is precise and matches the claims in the description. Notes below. Verified correct
Minor (non-blocking)
No security, transaction, replay-safety, or convention issues found.
|
dario-daza
approved these changes
Aug 5, 2026
|
Tick the box to add this pull request to the merge queue (same as
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Parent Issue
#36595
Summary
Backend half of #36595. Right-clicking a folder in the Content Drive sidebar tree does nothing, while right-clicking a folder row in the table opens a context menu (Edit folder / Edit permissions). The shared
dot-folder-list-context-menucomponent gates those items oncontentlet.permissionsand pre-populates the "Edit folder" dialog from the folder object it is handed — but the sidebar is backed byGET /api/v1/folder/search→FolderSearchView, which carried neither.This PR enriches that view so the sidebar can gate the menu and pre-populate the dialog exactly the way the table does. No FE changes here — the FE leg is the other half of the same issue, and the contract obligations it inherits are written up in a comment on #36595.
What changed
Always present (no flag) — the fields the "Edit folder" dialog reads, all read straight off the
Folderalready loaded by the existingSELECT folder.*, so zero additional DB cost:title,sortOrder,filesMasks,defaultFileType,showOnMenuOpt-in via
?includePermissions=true:permissions: List<String>— the permission types the requesting user holds, drawn fromREAD, EDIT, PUBLISH, EDIT_PERMISSIONS, CAN_ADD_CHILDREN. Same field name, same 5-type meaning and spelling as the table'sDotContentDriveFolder.permissions, so one gating implementation serves both views.nullwhen not requested — deliberately not[], so the FE can distinguish "not fetched" from "fetched, no grants".Returning the full 5-type set rather than only the two the menu reads today costs 3 extra batch calls instead of 2 (
READis implicit — the folder passed the pre-pagination READ filter to be here;CAN_ADD_CHILDRENreuses the set already computed foraddChildrenAllowed). That one extra call buys away a whole ambiguity class: ifpermissionsmeant 5 types on a table-sourced folder and 2 on a sidebar-sourced one, a futurePUBLISHcheck would silently evaluatefalsedepending on provenance, and the symptom is a missing menu action, not an error.New 400 guardrail.
PaginationUtilputs no upper bound onperPage, andPermissionBitFactoryImpl.getPermittedIdschunks ids by 500 — so?perPage=10000&includePermissions=truewas reachable by any authenticated backend user at 60 extra queries. With the flag on,perPageis now capped bycontent.drive.folder.search.permissions.max.per.page(default 200); above it the request is rejected with a 400 naming the cap. Rejecting rather than silently dropping the flag is the point: dropping it yieldspermissions: null→ "no grants" → an empty menu, indistinguishable from a user who genuinely has none. With the flag off, anyperPagebehaves exactly as before — there is a test asserting precisely that.All new batch calls are scoped to
page, the post-pagination slice, matching the existingCAN_ADD_CHILDRENcall. With the flag off they issue no query at all.OpenAPI
FolderSearchViewwas undocumented — 0 occurrences inopenapi.yaml, because the endpoint declared the genericResponseEntityPaginatedDataViewwhoseentityis a baretype: object. Regenerating alone would have produced no field diff at all.This PR adds a concrete
ResponseEntityFolderSearchView, references it from the@ApiResponse, and adds@Schemadescriptions to every field. The regenerated yaml now publishes the full 13-field schema plus theincludePermissionsparameter — a real contract for the FE leg to build against.Rollback
M-3 (REST API contract change), 🟢 LOW. Purely additive: new response fields plus a new query param whose default reproduces current behavior exactly. No DB schema change, no migration, no index change. Nothing calls
includePermissions=trueuntil the FE leg ships, so there is no mixed-version window for this change on its own.Payload cost — measured
Six new fields ride every row, and
"permissions": nullserializes even with the flag off (the default mapper has noNON_NULLinclusion). Measured on a representative deep-link hydration payload — 10,000 folders, flag off — serialized withDotObjectMapperProvider.createDefaultMapper():@JsonInclude(NON_NULL)on the recordThe raw +67.5% collapses to +9.9% (+52 KB) over the wire — the added fields are highly repetitive and compress well. Not material, so "detail fields always present" stands. Alternatives priced above in case a reviewer reads the raw number differently; note
NON_NULLwould also stop serializingdefaultBaseType: null, a contract change on an existing field.Testing
54 tests green locally.
FolderSearchPaginatorTest(unit)FolderResourceSearchTestFolderAPIImplFilterTestFolderFactoryImplFilterTestPlus 4 new Postman requests on
/v1/folder/search(flag on, flag omitted, over-cap 400, over-cap without the flag).Every permission-gating test runs as a limited, non-admin user.
filterCollectionshort-circuits for CMS Admin and the system user (PermissionBitAPIImpl.java:1433-1434), so the same assertions run as an admin would exercise none of this logic and would pass against an implementation that computes nothing. Coverage: all 5 types granted; READ + EDIT only withPUBLISH/EDIT_PERMISSIONS/CAN_ADD_CHILDRENasserted absent; READ only; permissions correct for page 2 of a paginated result rather than page 1's; read-your-writes after a grant with no reindex; exact string assertions on every type name.Two pre-existing test failures fixed here
Both were red on
mainbefore this PR and are unrelated to it, but one lives in a file this PR edits and would have read as a regression from this change:FolderAPIImplFilterTest.test_searchFolders_withPathScope_returnsOnlyDescendants— created a folder namedassetsdirectly under a site;assetsis in the defaultRESERVEDFOLDERNAMES(FolderAPIImpl:110), so the save threwInvalidFolderNameException. Renamed tosite-assets. Introduced by 27191bc.FolderFactoryImplFilterTest— all 7 tests threwNullPointerException: user is required. The class never passed.user(...), andFolderSearchParams.Builder.build()began requiring one in 6c92187. Added the system user, with a comment noting the factory resolves in SQL and never reads it — permission filtering happens a layer up inFolderAPIImpl.Notable landmines documented in code
PermissionAPI.Type.CANONICAL_TYPEScannot be used to derive these names. It is{READ, WRITE, PUBLISH, EDIT_PERMISSIONS, CAN_ADD_CHILDREN}— theWRITEalias, notEDIT. Deriving from it emits"WRITE", the FE'sEDITcheck silently fails, and the symptom is a missing menu item. The implementation uses explicitType.EDIT.name()etc., and a test assertsWRITEis never emitted.HttpStatusCodeExceptionrunsString.formatover the message you hand it, so pre-formatting would format twice and throw on a literal%. The cap error uses the varargs form.Explicitly out of scope
DotFolderTransformerImpl.contentDriveView's per-folder N+1 permission loop — real inefficiency, different code path.POST /api/v1/drive/search/BrowserAPIImpl— untouched.POST /api/v1/folder/byPath— not extended.READfilter running over the full pre-pagination set (FolderAPIImpl:819), which makes endpoint load scale with site size rather thanperPage. It predates this ticket and is the endpoint's dominant cost; worth its own ticket if folder-search latency shows up in profiling.🤖 Generated with Claude Code