Skip to content

fix(content-drive): expose folder permissions and edit-dialog fields on folder search #36595 - #36889

Open
ihoffmann-dot wants to merge 1 commit into
mainfrom
issue-36595-sidebar-folder-context-menu-be
Open

fix(content-drive): expose folder permissions and edit-dialog fields on folder search #36595#36889
ihoffmann-dot wants to merge 1 commit into
mainfrom
issue-36595-sidebar-folder-context-menu-be

Conversation

@ihoffmann-dot

Copy link
Copy Markdown
Member

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-menu component gates those items on contentlet.permissions and pre-populates the "Edit folder" dialog from the folder object it is handed — but the sidebar is backed by GET /api/v1/folder/searchFolderSearchView, 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.

The issue's "Proposed resolution options" prose names POST /api/v1/folder/byPath / FolderSearchResultView. That endpoint is deprecated (forRemoval = true) with zero references in core-web; the sidebar actually calls GET /api/v1/folder/search, which is what this PR extends. The comparison table in the issue body already says this correctly.

What changed

Always present (no flag) — the fields the "Edit folder" dialog reads, all read straight off the Folder already loaded by the existing SELECT folder.*, so zero additional DB cost:

title, sortOrder, filesMasks, defaultFileType, showOnMenu

Opt-in via ?includePermissions=true:

permissions: List<String> — the permission types the requesting user holds, drawn from READ, EDIT, PUBLISH, EDIT_PERMISSIONS, CAN_ADD_CHILDREN. Same field name, same 5-type meaning and spelling as the table's DotContentDriveFolder.permissions, so one gating implementation serves both views. null when 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 (READ is implicit — the folder passed the pre-pagination READ filter to be here; CAN_ADD_CHILDREN reuses the set already computed for addChildrenAllowed). That one extra call buys away a whole ambiguity class: if permissions meant 5 types on a table-sourced folder and 2 on a sidebar-sourced one, a future PUBLISH check would silently evaluate false depending on provenance, and the symptom is a missing menu action, not an error.

New 400 guardrail. PaginationUtil puts no upper bound on perPage, and PermissionBitFactoryImpl.getPermittedIds chunks ids by 500 — so ?perPage=10000&includePermissions=true was reachable by any authenticated backend user at 60 extra queries. With the flag on, perPage is now capped by content.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 yields permissions: null → "no grants" → an empty menu, indistinguishable from a user who genuinely has none. With the flag off, any perPage behaves exactly as before — there is a test asserting precisely that.

All new batch calls are scoped to page, the post-pagination slice, matching the existing CAN_ADD_CHILDREN call. With the flag off they issue no query at all.

OpenAPI

FolderSearchView was undocumented — 0 occurrences in openapi.yaml, because the endpoint declared the generic ResponseEntityPaginatedDataView whose entity is a bare type: object. Regenerating alone would have produced no field diff at all.

This PR adds a concrete ResponseEntityFolderSearchView, references it from the @ApiResponse, and adds @Schema descriptions to every field. The regenerated yaml now publishes the full 13-field schema plus the includePermissions parameter — 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=true until 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": null serializes even with the flag off (the default mapper has no NON_NULL inclusion). Measured on a representative deep-link hydration payload — 10,000 folders, flag off — serialized with DotObjectMapperProvider.createDefaultMapper():

shape raw gzip
before (7 fields) 2.19 MB · 229 B/row 0.49 MB · 51.7 B/row
after (13 fields) 3.66 MB · 384 B/row · +67.5% 0.54 MB · 56.9 B/row · +9.9%
alt: detail fields gated behind the flag 2.37 MB · +8.3% 0.50 MB · +0.5%
alt: @JsonInclude(NON_NULL) on the record 3.09 MB · +41.4% 0.53 MB · +7.6%

The 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_NULL would also stop serializing defaultBaseType: null, a contract change on an existing field.

Testing

54 tests green locally.

Class Result
FolderSearchPaginatorTest (unit) 10/10
FolderResourceSearchTest 21/21
FolderAPIImplFilterTest 16/16
FolderFactoryImplFilterTest 7/7

Plus 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. filterCollection short-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 with PUBLISH/EDIT_PERMISSIONS/CAN_ADD_CHILDREN asserted 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 main before 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 named assets directly under a site; assets is in the default RESERVEDFOLDERNAMES (FolderAPIImpl:110), so the save threw InvalidFolderNameException. Renamed to site-assets. Introduced by 27191bc.
  • FolderFactoryImplFilterTest — all 7 tests threw NullPointerException: user is required. The class never passed .user(...), and FolderSearchParams.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 in FolderAPIImpl.

Notable landmines documented in code

  • PermissionAPI.Type.CANONICAL_TYPES cannot be used to derive these names. It is {READ, WRITE, PUBLISH, EDIT_PERMISSIONS, CAN_ADD_CHILDREN} — the WRITE alias, not EDIT. Deriving from it emits "WRITE", the FE's EDIT check silently fails, and the symptom is a missing menu item. The implementation uses explicit Type.EDIT.name() etc., and a test asserts WRITE is never emitted.
  • HttpStatusCodeException runs String.format over 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

  • Any FE change (the other half of [BUG] Content Drive: Sidebar folders don't open a right-click context menu like the table #36595).
  • DotFolderTransformerImpl.contentDriveView's per-folder N+1 permission loop — real inefficiency, different code path.
  • POST /api/v1/drive/search / BrowserAPIImpl — untouched.
  • The deprecated POST /api/v1/folder/byPath — not extended.
  • The pre-existing READ filter running over the full pre-pagination set (FolderAPIImpl:819), which makes endpoint load scale with site size rather than perPage. 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

@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Claude finished @ihoffmann-dot's task in 4m 33s —— View job


Code Review: expose folder permissions & edit-dialog fields on folder search

  • Read the diff (backend Java, OpenAPI, tests, Postman)
  • Verify permission-computation logic in FolderAPIImpl.searchFolders
  • Verify the perPage cap can't be bypassed
  • Verify parity claim (EDIT vs WRITE) against the table's contentDriveView
  • Verify detail fields are already loaded (no extra DB cost)
  • Verify OpenAPI regeneration and test coverage

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

  • READ-implicit optimization is sound. FolderAPIImpl:819-826 filters the pre-pagination set by PERMISSION_READ, and page is a sublist of that filtered list, so every folder in page genuinely holds READ. Unconditionally adding READ in permissionNames (FolderAPIImpl:889) is correct.
  • EDIT vs WRITE parity holds. The table's DotFolderTransformerImpl.contentDriveView emits names via Type.findById(integer).name(). PERMISSION_EDIT == PERMISSION_WRITE == 2, and findById(2) returns the first enum with type==2 in declaration order — EDIT precedes WRITE (PermissionAPI.java:54-55), so the table emits "EDIT". This PR emits Type.EDIT.name()"EDIT". They match, and using getCanonicalTypes() (which holds WRITE) would indeed have diverged. The landmine comment is accurate.
  • The cap cannot be bypassed with perPage <= 0. The resource checks the raw perPage > cap (FolderResource.java:598-606), and PaginationUtil.getPage normalizes perPage <= 0perPageDefault (default 10, PaginationUtil.java:213), which can never exceed the cap. The limit < 0 → all results branch in FolderAPIImpl:825 is therefore unreachable from this endpoint, so an unbounded page is not reachable through the flag.
  • Detail fields cost nothing extra. FolderFactoryImpl.searchFolders already runs SELECT folder.* (FolderFactoryImpl:1189); every new field (getTitle, getSortOrder, getFilesMasks, getDefaultFileType, isShowOnMenu) is read off the already-loaded Folder.
  • Flag-off is a true no-op. filterIdsByPermission returns Set.of() without issuing a query when includePermissions is false (FolderAPIImpl:876-878), and permissions is set to null (FolderAPIImpl:849-851). The test_searchFolders_perPageOverCap_withoutFlag_succeeds test asserts the cap does not apply with the flag off.
  • BadRequestException varargs usage is correct. The message uses %s placeholders passed as varargs (FolderResource.java:601-606); HttpStatusCodeException runs String.format over the message, so pre-formatting would double-format — the comment and the code agree.
  • Test coverage is meaningful. Gating tests run as a limited non-admin user (filterCollection short-circuits for CMS Admin at PermissionBitAPIImpl.java), covering all-5-granted, READ+EDIT-only with the other three asserted absent, page-2 pagination, and exact string assertions. The cap has at-cap / over-cap / over-cap-without-flag / config-driven cases.

Minor (non-blocking)

  • 🟡 Medium: FolderAPIImpl.java:836-838 — the three filterIdsByPermission batch calls execute even when page is empty (e.g. an offset past the end of the result set). Each is a no-op filterCollection over an empty list, so at worst three cheap short-circuit calls; not a correctness problem, only a trivial waste. Worth a guard only if it shows up in profiling.

No security, transaction, replay-safety, or convention issues found. Config.getIntProperty is used correctly for the cap, Logger/APILocator conventions are respected, the change is purely additive to the REST contract, and the OpenAPI yaml was regenerated with the concrete ResponseEntityFolderSearchView schema and the includePermissions parameter.

No blocking issues found.
· issue-36595-sidebar-folder-context-menu-be

@github-actions github-actions Bot added the Area : Backend PR changes Java/Maven backend code label Aug 4, 2026
@mergify

mergify Bot commented Aug 5, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Backend PR changes Java/Maven backend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants