Skip to content

Issue 36151 implement new site/folder field component (edit contentlet)#36263

Open
freddyDOTCMS wants to merge 25 commits into
mainfrom
issue-36151-Implement-new-site/folder-field-component-(Edit-Contentlet)
Open

Issue 36151 implement new site/folder field component (edit contentlet)#36263
freddyDOTCMS wants to merge 25 commits into
mainfrom
issue-36151-Implement-new-site/folder-field-component-(Edit-Contentlet)

Conversation

@freddyDOTCMS

@freddyDOTCMS freddyDOTCMS commented Jun 22, 2026

Copy link
Copy Markdown
Member

Proposed Changes

  • Add GET /api/v1/folder/search endpoint supporting optional case-insensitive name filter (min 3 chars), path scope, recursive depth control, pagination (page/per_page), and sort (name / mod_date) — replaces the deprecated POST /byPath
  • Deprecate POST /byPath (@Deprecated(forRemoval = true)) and mark it as deprecated in the OpenAPI spec; existing callers continue to work unchanged
  • Introduce FolderSearchParams record with a fluent builder to consolidate all search parameters across FolderAPI, FolderFactory, and FolderSearchPaginator, replacing a 10-argument method signature
  • Convert FolderSearchResultView from a plain class to a Java record
  • Add PermissionAPI.filterCollection(Collection<P>, int, User, boolean) — a batch permission check that resolves the entire collection in one SQL round-trip, eliminating the N+1 permission queries that caused ~2s response times on large sites
  • Add PermissionBitFactory.getPermittedIds() as the SQL backbone for the batch check, using a UNION of direct (permission) and inherited (permission_reference) permissions chunked in batches of 500

Checklist

  • Tests (unit: FolderSearchPaginatorTest, PermissionBitAPIImplFilterCollectionTest, PermissionBitFactoryImplGetPermittedIdsTest; integration: FolderAPIImplFilterTest, FolderFactoryImplFilterTest, FolderResourceSearchTest)
  • Translations
  • Security Implications Contemplated — all search results are permission-filtered via the new batch method; admin and system-user fast-paths are maintained; siteId is required and validated server-side before any DB query runs; role IDs injected into SQL are system-generated UUIDs (not user input), consistent with the existing filterCollectionByDBPermissionReference pattern in PermissionBitFactoryImpl

Additional Info

The deprecated POST /byPath endpoint is kept intact with backward-compatible pagination parameters (offset / limit) to avoid breaking existing integrations. The new endpoint uses the standard dotCMS pagination contract (page / per_page) and returns a ResponseEntityPaginatedDataView with full pagination metadata.

The batch permission approach reduces query count from O(N) — one DB/cache lookup per folder — to O(1) regardless of result set size, which is the primary fix for the 2-second response times reported on large sites.

Screenshots

N/A — backend API changes only

This PR fixes: #36151

@claude

claude Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Claude Code is working…

I'll analyze this and get back to you.

View job run

@github-actions

github-actions Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

🤖 Bedrock Review — deepseek.v3.2

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/api/v1/folder/FolderResource.java:619 — Missing permission check for site read access in validateSiteReadAccess method when find returns null but DotSecurityException is not thrown. The catch block for DotSecurityException will not catch null site scenarios, potentially leaking "No site found" info to unauthorized users.

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/business/PermissionBitAPIImpl.java:1415 — New filterCollection method returns empty list for empty role IDs but doesn't handle the case where user has no roles (should return empty list, which it does). However, it calls permissionFactory.getPermittedIds with empty permissionIds list — the factory method should handle empty list, but the contract is unclear.

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/business/PermissionBitFactoryImpl.java:2725 — SQL uses string concatenation for rolePlaceholders and inodePlaceholders. While parameters are added via addParam, the SQL string itself is built via concatenation which could be problematic if the number of placeholders is huge. However, it uses chunking (500 items) which mitigates risk.

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderAPIImpl.java:809 — In searchFolders, pagination is done in Java after permission filtering. This could cause performance issues with large result sets (all folders loaded from DB, then filtered in memory). Should be documented as a potential scalability concern.

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderAPIImpl.java:826 — Second filterCollection call for PERMISSION_CAN_ADD_CHILDREN is performed on the paginated subset. This is correct for performance but means the canAddChildren flag is only accurate for the current page, not for all filtered items.

[🟠 High] dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderFactoryImpl.java:1196 — SQL uses LIKE with % pattern on full_path_lc. This could cause performance issues on large folder tables if the path is short (like /). Should ensure there's an index on identifier.full_path_lc.

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderFactoryImpl.java:1200 — Logic if (!("/".equals(normalizedPath) && params.recursive())) skips path condition only when searching root recursively. This means recursive search on root (path="/", recursive=true) will not filter by path at all, returning all site folders. This matches the intended behavior but should be clearly documented.

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderSearchParams.java:59 — Builder's build() method requires siteId and user to be non-null, but does not validate that siteId is a valid identifier format (could be empty string). The validation happens later in FolderResource.validateSiteReadAccess.

[🟡 Medium] dotCMS/src/test/java/com/dotcms/util/pagination/FolderSearchPaginatorTest.java:1 — Test file uses JUnit 4 (org.junit.Test) but dotCMS may have migrated to JUnit 5. Ensure consistency with project testing standards.

[🟡 Medium] dotCMS/src/test/java/com/dotcms/rest/api/v1/folder/FolderResourceSearchTest.java:1 — Integration test uses JUnit 4. Also, test test_searchFolders_unauthenticated_throws catches SecurityException but the actual endpoint throws SecurityException? The resource method doesn't declare throwing SecurityException; it's wrapped in WebResource initialization.

[🔴 Critical] dotCMS/src/test/java/com/dotmarketing/business/PermissionAPITest.java:196 — Test change casts null to Permissionable to avoid NPE in doesUserHavePermission. This is a test fix only, but indicates the method may throw NPE for null permissionable — the production code should handle null gracefully or document NPE.

[🟡 Medium] dotCMS/src/test/java/com/dotmarketing/portlets/folders/business/FolderAPIImplFilterTest.java:1 — Integration test uses JUnit 4. Also, test test_searchFolders_adminUser_returnsMatchingFolders creates a fresh site but uses instance variable folderAPI which may have cached data from previous tests.

[🟡 Medium] dotCMS/src/test/java/com/dotmarketing/portlets/folders/business/FolderFactoryImplFilterTest.java:1 — Test uses JUnit 4. Also, test test_searchFolders_matchesPartialName uses System.currentTimeMillis() for uniqueness but could collide in fast consecutive runs.

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderAPIImpl.java:1383 — Pattern matching instanceof with variable ifa is used but the variable name ifa is not descriptive. Consider fileAsset for clarity.

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderAPIImpl.java:1388 — Same pattern matching with ifa. This is a style change but doesn't affect functionality.

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderAPIImpl.java:1431 — Uses .toList() instead of Collectors.toList(). This is Java 16+ feature; ensure the project's Java version compatibility (Java 25 is specified, so it's fine).

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/api/v1/folder/FolderResource.java:365 — Deprecated endpoint findSubFoldersByPath is marked forRemoval = true with date "Jun 19th, 26". Ensure the date format is correct (should be "2026-06-19"?). The since value is a string, but non-standard format may cause issues with tooling.

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/api/v1/folder/FolderResource.java:592validateSiteReadAccess catches DotSecurityException and rethrows as ForbiddenException. However, DotSecurityException could be thrown by find if user lacks permission to see the site, which is appropriate. The method also catches DotDataException and throws BadRequestException — but DotDataException could be due to DB issues, not invalid siteId. This could mask real DB errors.

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/api/v1/folder/FolderResource.java:569searchFolders method uses switch expression with direction.toUpperCase(). If direction is null, NPE will be thrown. direction has @DefaultValue("ASC") so it should not be null, but defensive programming is missing.

[🟡 Medium] dotCMS/src/main/java/com/dotcms/util/pagination/FolderSearchPaginator.java:44orderBy switch uses null in case label case null, default. This is valid Java 17+ but ensure compatibility.

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderFactoryImpl.java:1172ALLOWED_SORT_COLUMNS is a Set but defined as private static final Set<String>. Should be Set.of(...) immutable.

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderFactoryImpl.java:1175switch on params.orderDirection() with "DESC", "desc". Should be case-insensitive match maybe using equalsIgnoreCase. The current code handles both uppercase and lowercase "desc".

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderFactoryImpl.java:1206normalizedPath logic ensures trailing slash. However, if params.path() is empty string, it becomes "/"? Actually path defaults to "/" in FolderSearchParams. Empty string would be invalid.

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderFactoryImpl.java:1209 — Condition !("/".equals(normalizedPath) && params.recursive()) might be confusing. Consider extracting to a boolean variable with comment.

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/business/PermissionBitAPIImpl.java:1432permissionFactory.getPermittedIds


Run: #28053896991 · tokens: in: 20771 · out: 2048 · total: 22819

@semgrep-code-dotcms-test

Copy link
Copy Markdown
Contributor

Semgrep found 1 CUSTOM_INJECTION-2 finding:

  • dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderFactoryImpl.java

The method identified is susceptible to injection. The input should be validated and properly
escaped.

If this is a critical or high severity finding, please also link this issue in the #security channel in Slack.

…-new-site/folder-field-component-(Edit-Contentlet)
@freddyDOTCMS freddyDOTCMS enabled auto-merge June 22, 2026 20:23
@github-actions github-actions Bot added the Area : Documentation PR changes documentation files label Jun 22, 2026
@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

🤖 Bedrock Review — us.deepseek.r1-v1:0

New Issues

  • 🟡 Medium: dotCMS/src/main/java/com/dotmarketing/business/PermissionBitAPIImpl.java:1423 — Missing frontend role inclusion when respectFrontendRoles=true. The batch permission check adds frontend roles to roleIds list but doesn't handle respectFrontendRoles flag properly. When respectFrontendRoles=false, anonymous and frontend roles should be excluded from permission checks.

Existing

  • 🟡 Medium: dotCMS/src/main/java/com/dotmarketing/business/PermissionBitAPIImpl.java:1423 — prior finding still present (unchanged or incomplete fix)

Run: #28055551152 · tokens: in: 21425 · out: 744 · total: 22169

freddyDOTCMS and others added 3 commits June 23, 2026 15:53
…Edit-Contentlet)' of https://github.com/dotCMS/core into issue-36151-Implement-new-site/folder-field-component-(Edit-Contentlet)
@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

🤖 Bedrock Review — us.deepseek.r1-v1:0

Resolved

  • dotCMS/src/main/java/com/dotmarketing/business/PermissionBitAPIImpl.java:1423 — Batch permission check now includes frontend roles when respectFrontendRoles=true

Run: #28059674484 · tokens: in: 21555 · out: 601 · total: 22156

@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

🤖 Bedrock Review — us.deepseek.r1-v1:0

New Issues

  • 🟠 High: dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderAPIImpl.java — Pagination applied in-memory after loading all results from DB. Causes memory exhaustion and poor performance with large datasets.
  • 🟠 High: dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderFactoryImpl.java — Missing LIMIT/OFFSET in SQL query. Loads entire result set into memory instead of using DB-level pagination.

Run: #28059731961 · tokens: in: 21505 · out: 1609 · total: 23114

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

Labels

Area : Backend PR changes Java/Maven backend code Area : Documentation PR changes documentation files

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Implement new site/folder field component (Edit Contentlet)

2 participants