Skip to content

**feat(folder-resource): add offset and limit pagination for sub-folder search#35618

Merged
erickgonzalez merged 3 commits into
mainfrom
issue-31934-folderresource-bypath-limit-offset
May 12, 2026
Merged

**feat(folder-resource): add offset and limit pagination for sub-folder search#35618
erickgonzalez merged 3 commits into
mainfrom
issue-31934-folderresource-bypath-limit-offset

Conversation

@erickgonzalez
Copy link
Copy Markdown
Member

@erickgonzalez erickgonzalez commented May 7, 2026

Summary

Adds offset and limit query parameters to POST /api/v1/folder/byPath to fix silent truncation of folder lists. Closes #34505.

  • Updated FolderHelper to support pagination via offset and limit parameters in findSubFoldersPathByParentPath method.
  • Removed the hardcoded subfolder cap of 20 and introduced an effective-limit model.
  • limit=-1 is treated as "unlimited" but bounded by SUB_FOLDER_UNLIMITED_SAFETY_CAP (10000) to avoid OOM on very large sites.
  • Refactored findSubfoldersUnderHost and findSubfoldersUnderFolder to accept effectiveLimit.
  • Final result slice uses subList(offset, offset+limit) wrapped in a fresh ArrayList to avoid backing-list aliasing.
  • FolderResource.findSubFoldersByPath now validates inputs (offset >= 0, limit > 0 or limit == -1) and returns 400 on bad input.
  • Overflow-safe effectiveLimit computation via a long intermediate clamped to Integer.MAX_VALUE.
  • OpenAPI parameter descriptions document the -1 sentinel and the safety cap.
  • Added 12 integration tests covering default cap, custom limit, offset paging, offset-beyond-total, limit=-1, negative offset/limit, limit=0, overflow input, and fewer-than-default-folders.

⚠️ Behavior change

The endpoint previously returned at most 20 subfolders silently (hardcoded). It now returns at most 40 by default. Existing clients that omit offset/limit will receive more results than before. This is intentional and addresses the bug described in #34505. Worth a regression sanity check on:

  • Angular Site Browser / folder picker components consuming /api/v1/folder/byPath
  • DotFolderService.getFolders() callers

Scope

This PR covers the BE side only. Follow-up tickets will wire the new pagination params into Content Drive and the Host/Folder Field in the new edit contentlet.

Test plan

  • ./mvnw verify -pl :dotcms-integration -Dcoreit.test.skip=false -Dit.test=FolderResourceTest — all 22 tests pass
  • Smoke test the Site Browser folder tree against a site with >40 subfolders
  • Smoke test Content Drive (still uses default — will show up to 40 vs. 20)

ref: #34505

This PR fixes: #31934

…er search**

- Updated `FolderHelper` to support pagination via `offset` and `limit` parameters in `findSubFoldersPathByParentPath` method.
- Modified hardcoded limit to 40 and introduced an effective limit to handle `-1` (unlimited) cases.
- Refactored `findSubfoldersUnderHost` and `findSubfoldersUnderFolder` to include pagination logic.
- Adjusted return logic to paginate results using `subList` for accurate slicing.
- Enhanced `FolderResource` API to accept `offset` and `limit` as query parameters for sub-folder search requests.
- Updated OpenAPI documentation to reflect the new pagination parameters.
- Added comprehensive integration tests in `FolderResourceTest` to validate pagination behavior, including offset handling, custom limits, and default limit scenarios.
- Verified backward compatibility for unlimited results (`limit=-1`) and ensured proper response for offset-beyond-result cases.

ref: #31934
Signed-off-by: erickgonzalez <erick.gonzalez@dotcms.com>
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 7, 2026

Claude finished @erickgonzalez's task in 3m 16s —— View job


PR Review

Solid, well-tested change overall. The validation, overflow guard, and integration test coverage are good. A few things worth a second look before merge:


1. The SUB_FOLDER_UNLIMITED_SAFETY_CAP is bypassable — clients can opt out of the safety net

FolderHelper.java:182-184

final int effectiveLimit = limit == -1
        ? SUB_FOLDER_UNLIMITED_SAFETY_CAP
        : (int) Math.min(Integer.MAX_VALUE, (long) offset + limit);

The 10 000 cap is only applied when a client passes limit=-1. A client that passes limit=1000000 (or limit=Integer.MAX_VALUE) sails right past it. Given the PR description sells this as "OOM protection on very large sites," the cap should clamp every code path, not just the sentinel. Consider:

final long requested = (limit == -1) ? SUB_FOLDER_UNLIMITED_SAFETY_CAP : (long) limit;
final long bounded = Math.min(requested, SUB_FOLDER_UNLIMITED_SAFETY_CAP);
final int effectiveLimit = (int) Math.min(Integer.MAX_VALUE, (long) offset + bounded);

This also makes the OpenAPI/Parameter description ("capped at 10000 as a safety limit") accurate for all callers, not just the -1 case.


2. "Pagination" here is in-memory slicing on top of a full-table fetch

The inner methods still call APILocator.getFolderAPI().findSubFolders(site|folder, ...), which loads every folder under the host/parent into memory along with a per-folder permission check (FolderFactoryImpl.findFoldersByHostgetSubFolders(...) + in-memory sort). The .limit(effectiveLimit) only truncates the stream that builds FolderSearchResultView objects — it doesn't reduce DB load.

Practical consequences:

  • offset=10000, limit=10 still scans the full folder list of the site.
  • The "safety cap" only protects against constructing too many response objects, not against the DB query / permission check on N rows. On the "very large sites" you're trying to protect, the bottleneck isn't there.
  • Cost is O(total folders × users-with-permission-check) per request, regardless of paging.

Not a blocker — this is the same behavior the old code had — but the PR description ("avoid OOM on very large sites") oversells it. Worth a TODO or follow-up to push pagination into the DAO if real large-site support is the goal.


3. Cross-host pagination (when siteId is null) is non-deterministic

FolderHelper.java:189-195 / :201-207

When siteId is empty, the code iterates hostAPI.findAll(user, false) and concatenates each site's filtered folders. Two things to note:

  • findAll(...) does not guarantee a stable ordering across calls. So offset=0, limit=40 followed by offset=40, limit=40 can return inconsistent or duplicated pages if the host list shuffles between requests (sites added/removed, cache reset, replica routing, etc.).
  • The merged list isn't globally sorted — it's "site A's first N, then site B's first N, …" — so a user paging through results sees per-host runs, not a coherent alphabetical list.

If cross-host paging isn't actually used by callers, fine; but worth either documenting the limitation in the Swagger description or adding a defensive sort on the merged list. The integration tests don't cover the siteId=null path, so this is untested.


4. @DefaultValue("40") and "Default 40" strings duplicate SUB_FOLDER_DEFAULT_LIMIT

FolderResource.java:394-396

You introduced FolderHelper.SUB_FOLDER_DEFAULT_LIMIT = 40 but never reference it — @DefaultValue("40") and the description string "Default 40" are hardcoded. JAX-RS @DefaultValue needs a compile-time constant String, so you'd need @DefaultValue("" + FolderHelper.SUB_FOLDER_DEFAULT_LIMIT) — workable, but if anyone ever bumps the constant they'll silently miss two places. Either use the constant everywhere or drop it (since the only reference is the @DefaultValue literal).


5. Test javadoc is stale

FolderResourceTest.java:432-454

The javadoc says "Create 25 subfolders ... All 26 results returned (parent + all 25 subfolders)" but the loop creates 50 (for (int i = 0; i < 50; i++)) and the assertion is 51. The test itself is fine; just fix the comment so future maintainers don't get confused.


6. findSubfoldersUnderFolder adds the parent before the .limit(effectiveLimit) stream

FolderHelper.java:262-273

The parent entry is appended (line 263) and then up to effectiveLimit children are added. So per-host the method can return effectiveLimit + 1 items. Combined with the cross-host accumulation guard subFolders.size() < effectiveLimit, it means a single host can push the running total a hair over the cap. The final subList(...) saves you, but it's a subtle invariant. Same applies to the system-folder branch in findSubfoldersUnderHost (line 228).

No functional bug, but if you ever change the slicing, this is a footgun.


7. Minor: name effectiveLimit is misleading in the inner methods

In findSubFoldersPathByParentPath it really means "how many to fetch before we slice" — i.e., offset + limit. Passed verbatim into findSubfoldersUnderHost/Folder, where it's used as a per-host fetch ceiling. maxToFetch or fetchUpper would convey the intent better.


Summary

Fix issue #1 (safety cap bypass) →

  • Read changed files
  • Analyze pagination logic and edge cases
  • Check input validation & overflow handling
  • Review tests for coverage gaps
  • Post review feedback
    • Branch: issue-31934-folderresource-bypath-limit-offset

…overflow guard, safety cap

Address review feedback on the byPath pagination work:

- Validate offset/limit at the REST layer: reject offset<0, limit==0, and
  limit<-1 with 400 (previously caused 500 IndexOutOfBoundsException /
  IllegalArgumentException).
- Overflow-safe effectiveLimit via long intermediate clamped to
  Integer.MAX_VALUE; prior offset+limit could overflow to a negative int.
- Cap "unlimited" (limit=-1) at SUB_FOLDER_UNLIMITED_SAFETY_CAP (10000)
  instead of Integer.MAX_VALUE to bound worst-case in-memory work.
- Return a fresh ArrayList copy of the subList slice to avoid backing-list
  aliasing.
- OpenAPI @parameter descriptions document offset>=0 and the -1 sentinel
  with its safety cap.
- Fix pre-existing typo "must be send" -> "must be sent".

Tests added: negative offset, negative limit != -1, limit==0, and
overflow input (offset=Integer.MAX_VALUE-5, limit=100). All 22
FolderResourceTest integration tests pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@erickgonzalez erickgonzalez enabled auto-merge May 12, 2026 16:26
@erickgonzalez erickgonzalez added this pull request to the merge queue May 12, 2026
Merged via the queue into main with commit a904fea May 12, 2026
51 checks passed
@erickgonzalez erickgonzalez deleted the issue-31934-folderresource-bypath-limit-offset branch May 12, 2026 19:38
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.

[EPIC] New Edit Content: Bugs, Stabilization, Telemetry and UX Issues [TASK] Add pagination support to FolderResource byPath endpoint

2 participants