Skip to content

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

Open
freddyDOTCMS wants to merge 13 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 13 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

[🟠 High] dotCMS/src/main/java/com/dotcms/rest/api/v1/folder/FolderResource.java:587-589 — Missing permission check for site read access before constructing PaginationUtilParams. The validateSiteReadAccess method is called, but it only checks PERMISSION_READ on the site. However, the PaginationUtilParams.Builder is built with the user and request, and the folderSearchPaginationUtil.getPageView will internally call the paginator which uses FolderAPI.searchFolders. That method also performs permission filtering, but the initial site check is correct and should remain.

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/api/v1/folder/FolderResource.java:592 — The filter parameter passed to PaginationUtilParams.Builder.withFilter(name) may be null. The builder and paginator should handle null filters gracefully. The FolderSearchPaginator does check UtilMethods.isSet(filter) and passes null to FolderSearchParams.builder().name(...), which is acceptable. However, ensure the FolderFactoryImpl.searchFolders SQL building correctly handles a null name (it does: if (UtilMethods.isSet(params.name()))). This is fine but worth noting.

[🟠 High] dotCMS/src/main/java/com/dotmarketing/business/PermissionBitAPIImpl.java:1410-1456 — The new filterCollection method does not respect the respectFrontendRoles flag correctly for the anonymous and front-end roles. The code adds anonRole.get().getId() and APILocator.getRoleAPI().loadLoggedinSiteRole().getId() only when respectFrontendRoles is true. However, the existing filterCollection(List, int, boolean, User) overload adds these roles when respectFrontendRoles is true, but also includes them in the role list for the permission check. The new implementation matches that behavior, so it's correct. However, note that the anonRole is a lazy-loaded supplier; ensure it's thread-safe (it is). No bug.

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/business/PermissionBitFactoryImpl.java:2723-2780 — The getPermittedIds method uses string concatenation for SQL placeholders (rolePlaceholders, inodePlaceholders). While it uses DotConnect.addParam for each placeholder, which is safe from SQL injection, the chunking loop (500 items) constructs a new SQL string per chunk. This is acceptable but could be a performance concern if inodeIds is very large. However, the method is internal and called with filtered collections (already limited by pagination). It's acceptable.

[🟠 High] dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderAPIImpl.java:806-848 — The searchFolders method loads all matching folders without limit (folderFactory.searchFolders(params) returns all matches, ignoring params.limit() and params.offset()). Then it applies permission filtering on the entire result set, and finally does Java pagination via filtered.subList. This is a design issue: for a site with many folders, this will load all folders into memory, causing performance degradation and potential out-of-memory errors. The factory method should support pagination at the database level. The current implementation is inefficient and not scalable.

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderAPIImpl.java:827 — The canAddIds check uses permissionAPI.filterCollection on the page items (≤ perPage). This is a batch permission check, which is good. However, it calls filterCollection with PermissionAPI.PERMISSION_CAN_ADD_CHILDREN. Note that PERMISSION_CAN_ADD_CHILDREN is a bit mask; ensure the permission API supports this permission type for folders (it does). No bug.

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderFactoryImpl.java:1169-1220 — The searchFolders method builds SQL dynamically. The normalizedPath logic ensures a trailing slash. However, the condition if (!("/".equals(normalizedPath) && params.recursive())) skips the path condition only when searching the whole site (root + recursive). This is correct. But note: if path is "/" and recursive is false, the condition will not skip, and it will add identifier.parent_path = ? with normalizedPath (which is "/"). That will return only direct children of the root, which is correct. Good.

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderFactoryImpl.java:1195 — The SQL uses LOWER(folder.name) LIKE LOWER(?) with a % wildcard added in the parameter. This is case-insensitive search. However, the parameter is already lowercased in the code: sqlParams.add("%" + params.name().toLowerCase() + "%");. This results in double lowercasing (once in SQL, once in Java). It's redundant but harmless. However, note that LOWER(?) will lower-case the parameter, which is already lower-cased. This might affect performance on some databases. Consider using LIKE ? and lower-casing only in Java.

[🟠 High] dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderSearchParams.java:1-55 — The record is immutable and built via a builder. However, there is no validation in the builder or record constructor. For example, siteId and user could be null, which would cause NullPointerException in the API layer. The FolderAPI.searchFolders method should validate these, but the record itself doesn't enforce non-null. This is a design gap: the builder should require siteId and user (or handle null gracefully). Currently, FolderAPIImpl.searchFolders calls folderFactory.searchFolders(params) which uses params.siteId() directly; if null, SQL will fail. The resource layer validates siteId not null, but user is derived from the request and should not be null. Still, defensive checks are missing.

[🟡 Medium] dotCMS/src/main/java/com/dotcms/util/pagination/FolderSearchPaginator.java:42 — The extraParams map keys are hardcoded strings ("siteId", "path", "recursive"). These must match the keys used in FolderResource.searchFolders (line 585-587). They do: SITE_ID_PARAM, PATH_PARAM, RECURSIVE_PARAM. However, there's a risk of typo if the keys are changed in one place but not the other. Consider defining these constants in a shared location.

[🟡 Medium] dotCMS/src/main/java/com/dotcms/util/pagination/FolderSearchPaginator.java:44-47 — The orderBy switch statement defaults to DEFAULT_ORDER_BY_COLUMN for null or any other value. However, the orderBy parameter from the resource is constrained by the API annotation to "name" or "mod_date". The paginator maps "mod_date" to "folder.mod_date". This is fine, but note that the FolderFactoryImpl also has a safety check (ALLOWED_SORT_COLUMNS). The duplication is okay.

[🟠 High] dotCMS/src/test/java/com/dotcms/rest/api/v1/folder/FolderResourceSearchTest.java:217-233 — The test test_searchFolders_unauthenticated_throws expects a security exception but catches any Exception and passes. This is a weak test; it should assert the specific exception type or message. However, the test is integration and the behavior is that an unauthenticated request will be rejected by the WebResource initialization (since requiredBackendUser(true)). The test is acceptable but could be more precise.

[🟡 Medium] dotCMS/src/test/java/com/dotmarketing/portlets/folders/business/FolderAPIImplFilterTest.java:193-214 — The test test_searchFolders_pagination_returnsCorrectSlice creates 5 folders and sets limit=2, offset=2. However, because the implementation loads all folders first and then applies Java pagination, the test passes but does not actually test database-level pagination. This is a test gap: the test should verify that the factory method respects limit/offset (which it currently does not). The test should be updated once the factory supports pagination.

[🟡 Medium] dotCMS/src/test/java/com/dotmarketing/portlets/folders/business/FolderFactoryImplFilterTest.java (file not fully shown) — The tests for FolderFactoryImpl.searchFolders should verify that the SQL generated is correct and uses parameters safely. The diff shows only the beginning of the file; ensure the tests use DotConnect mocking


Run: #27986035737 · tokens: in: 20774 · out: 2048 · total: 22822

@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
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