36991 support showlinks in the content drive search api apiv1drivesearch - #37112
Conversation
Adds menu Links as a third paginated source (alongside folders and contentlets) in POST /api/v1/drive/search, gated behind an opt-in showLinks flag with its own linkCursor/hasMoreLinks/linkCount contract, matching legacy /api/v1/browser behavior for permissions and live/archived filtering.
There was a problem hiding this comment.
Pull request overview
Adds opt-in support for returning menu Links from the Content Drive Search API (POST /api/v1/drive/search) by treating links as a third paginated source alongside folders and contentlets.
Changes:
- Introduces
showLinks(defaultfalse) andlinkCursorrequest fields, and threadslinkCursorthroughBrowserQuery. - Extends
BrowserAPIImpl.getPaginatedContents()to page links independently (withlinkCount/hasMoreLinks/nextLinkCursor) and merge them into the response list. - Adds automated coverage via Postman collection updates and a new integration test suite entry.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| dotCMS/src/main/java/com/dotcms/rest/api/v1/drive/ContentDriveHelper.java | Enables showLinks/linkCursor support when building the BrowserQuery for drive search and logs link-related info. |
| dotCMS/src/main/java/com/dotcms/rest/api/v1/drive/AbstractDriveRequestForm.java | Adds request contract/documentation for showLinks and linkCursor. |
| dotCMS/src/main/java/com/dotcms/browser/BrowserQuery.java | Adds linkCursor to the query object + builder and includes it in toString(). |
| dotCMS/src/main/java/com/dotcms/browser/BrowserAPIImpl.java | Implements link paging + response metadata, and adds link-to-drive map transformations. |
| dotCMS/src/main/java/com/dotcms/browser/BrowserAPI.java | Updates Javadoc to reflect multi-source cursor pagination (folders/links/contentlets). |
| dotcms-postman/src/main/resources/postman/ContentDriveResource.postman_collection.json | Adds Postman tests covering showLinks flag behavior and link cursor contract. |
| dotcms-integration/src/test/java/com/dotcms/rest/api/v1/drive/ContentDriveLinksTest.java | New integration test validating opt-in behavior, permissions, paging, suppression rules, ordering, and legacy parity. |
| dotcms-integration/src/test/java/com/dotcms/MainSuite3a.java | Registers the new integration test in the main suite. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Claude finished @nicobytes's task in 2m 46s —— View job Code Review —
|
How to use
|
| Field | Type | Default | Purpose |
|---|---|---|---|
showLinks |
boolean | false |
Include the menu Links directly under assetPath |
linkCursor |
int | 0 |
Index to start paging links from |
The false default is what keeps every current consumer (Content Drive, AssetPicker) behaving exactly as it does today.
Response
{
"entity": {
"list": [ /* folders, links and contentlets merged, ordered by sortBy */ ],
"folderCount": 2, "hasMoreFolders": false, "nextFolderCursor": 2,
"linkCount": 3, "hasMoreLinks": true, "nextLinkCursor": 3,
"contentCount": 0, "hasMoreContent": true, "nextContentCursor": 0
},
"errors": [], "messages": [], "i18nMessagesMap": {}
}Links are a third symmetric pagination source: their own cursor, count and hasMore flag, mirroring the folder slice.
1. Basic — links alongside everything else
curl -u admin@dotcms.com:admin \
-X POST 'http://localhost:8080/api/v1/drive/search' \
-H 'Content-Type: application/json' \
-d '{
"assetPath": "//demo.dotcms.com/about-us/",
"showLinks": true,
"maxResults": 20
}'2. The redirect_custom_field_new.vtl case — links + pages
This is the direct translation of the legacy browser flags that motivated the issue:
curl -u admin@dotcms.com:admin \
-X POST 'http://localhost:8080/api/v1/drive/search' \
-H 'Content-Type: application/json' \
-d '{
"assetPath": "//demo.dotcms.com/",
"showLinks": true,
"baseTypes": ["HTMLPAGE"],
"showFolders": false,
"live": true,
"archived": false,
"sortBy": "modDate:desc",
"maxResults": 20
}'3. Links only
Links are not a BaseContentType, so showLinks is orthogonal to baseTypes. An empty baseTypes array disables the content query:
curl -u admin@dotcms.com:admin \
-X POST 'http://localhost:8080/api/v1/drive/search' \
-H 'Content-Type: application/json' \
-d '{
"assetPath": "//demo.dotcms.com/about-us/",
"showLinks": true,
"showFolders": false,
"baseTypes": [],
"maxResults": 20
}'The three documented outcomes:
| Request | Result |
|---|---|
showLinks: true, baseTypes omitted |
links plus content of every base type |
showLinks: true, baseTypes: ["HTMLPAGE"] |
links plus pages |
showLinks: true, baseTypes: [], showFolders: false |
links only |
4. Paging links to exhaustion
Feed nextLinkCursor back as linkCursor and keep offset at 0:
# page 1
curl -u admin@dotcms.com:admin -X POST 'http://localhost:8080/api/v1/drive/search' \
-H 'Content-Type: application/json' \
-d '{"assetPath":"//demo.dotcms.com/about-us/","showLinks":true,
"showFolders":false,"baseTypes":[],"maxResults":2,"linkCursor":0}'
# -> linkCount: 2, hasMoreLinks: true, nextLinkCursor: 2
# page 2
curl ... -d '{... "maxResults":2, "linkCursor":2}'Stop when hasMoreLinks: false; from then on send showLinks: false to skip the query entirely.
Budget order is folders -> links -> contentlets. If folders fill maxResults, you get linkCount: 0 with hasMoreLinks: true and an unadvanced nextLinkCursor — that is the signal that links remain, not a bug.
Link shape
Identify links by mimeType === "application/dotlink" (or type === "links").
{
"identifier": "9f2c...",
"type": "links",
"title": "Contact Us Redirect",
"name": "Contact Us Redirect",
"mimeType": "application/dotlink",
"extension": "link",
"__icon__": "linkIcon",
"url": "www.google.com",
"protocol": "https://",
"target": "_blank",
"linkType": "EXTERNAL",
"permissions": ["READ", "WRITE"],
"modDate": "2026-08-19T17:41:28.000Z",
"owner": "Admin User"
}Note two Content Drive conventions, consistent with the folders this endpoint already returns: no inode, and permissions as role-type names rather than raw integer ids. The legacy includeLinks() used by /api/v1/browser is untouched.
Three filters suppress links
Sending showLinks: true together with any of these yields linkCount: 0, because a Link cannot satisfy them:
mimeTypes— a Link has no file MIME typeworkflow— a Link carries no workflow state (folders are already dropped here for the same reason)userSearchable— resolves against a single content type; a Link has no fields
Two inherited limits
Both match legacy /api/v1/browser behaviour, which is what the AC asked for:
- Links are direct children only of the resolved
assetPath— no recursion into subfolders, even at site root (where contentlets are gathered recursively). filters.textmatches link titles only, applied in memory, because links are not indexed in Elasticsearch.
- Document the userSearchable suppression on showLinks; the Javadoc listed only mimeTypes and workflow, so it disagreed with the implementation. - Build the BrowserQuery once and log it instead of the pre-override locals. The workflow and userSearchable branches can flip showLinks/showFolders after they are computed, so the old debug line could misreport what actually ran. - Add mimeTypes to BrowserQuery.toString() so the query log keeps the filter detail the previous hand-rolled message carried. - Clarify in PaginatedContents that a next*Cursor is only meaningful while its hasMore* is true; a cursor past the end of a source is echoed back unchanged rather than clamped, for folders and links alike. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tent-drive-search-api-apiv1drivesearch
…oss join Addresses @oidacra's review on #37112. live:true + showLinks:true returned every link many times over. FolderFactoryImpl's getChildrenClass appends the version table to the FROM list with no join predicate of its own, so the only correlation it ever produces is the incidental one from `working_inode = links_1_.inode`. BrowserAPIImpl passed showWorking straight through as that flag, and showWorking=false flips the predicate to `<>` — which correlates nothing and degenerates into a cross product against every link version in the installation. Duplicates, an archived filter that silently stops applying, and non-deterministic truncation at the factory's 1000-row ceiling. It is the one combination the motivating consumer actually sends: redirect_custom_field_new.vtl passes showWorking:false, which the drive maps to live:true. The defect is pre-existing and equally present in legacy /api/v1/browser; both paths share BrowserAPIImpl.getLinks, so both are fixed here. - getLinks now always asks FolderAPI for the working links and resolves "live" by keeping the ones carrying a published version. Only the live:true+archived:false combination changes behaviour — BrowserQuery ORs showArchived into showWorking, so the other two combinations already passed working=true and are untouched. - Clamp contentCursor, folderCursor, linkCursor and offset at zero in BrowserQuery.Builder. A negative cursor survived Math.min and reached List.subList, surfacing as a 500 rather than an empty page. Mirrors the clamp maxResults already does in the same builder. - Document that links page in title order independent of sortBy (as folders and contentlets already do), and that filters.filterFolders does not gate link titles. Tests, all confirmed failing before the fix: - testLiveOnlyReturnsPublishedLinks pins the meaning of live:true rather than agreement with another code path. - testLinksHonourLiveAndArchivedLikeLegacyBrowser now compares lists instead of sets. The HashSet erased the duplicates by construction, which is why it passed against the bug. - testFilterTextNarrowsLinksByTitle / testFilterTextWithNoMatchReturnsNoLinks cover the in-memory title filter, the one piece of new logic that had no test. - testNegativeLinkCursorIsTreatedAsZero covers the clamp. - @afterclass cleanup, plus the permission test's site, role and user hoisted into statics so they are torn down too; the class is in MainSuite3a and was leaking its fixtures. - Two Postman requests for the filters.text and live:true combinations. openapi.yaml is unchanged by design: ContentDriveResource.search is @hidden, so /v1/drive has no generated schema. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review feedback addressed —
|
| Thread | Outcome |
|---|---|
| Negative cursor → 500 | Clamped contentCursor, folderCursor, linkCursor and offset at zero in BrowserQuery.Builder, matching the clamp maxResults already does there. Fixes the pre-existing folder/offset cases too. |
sortBy vs pre-slice order |
Documented, per your own reading — folders and contentlets slice the same way, so AC9 holds by consistency. Noted in both showLinks() and PaginatedContents. |
Missing @AfterClass |
Added, mirroring ContentDriveKeywordSearchTest.cleanup(). The permission test's site, role and user are hoisted into statics so they are torn down too. |
| Untested title filter | Three cases added, including the upper-cased term for the toLowerCase() branch. The filterFolders asymmetry is intentional and now documented. |
Copilot: userSearchable in Javadoc |
Already fixed in 243523a29b. |
Test evidence
ContentDriveLinksTest is 14 → 18 tests, all green. The four new ones were confirmed failing against the pre-fix code:
testLiveOnlyReturnsPublishedLinks AssertionError: expected:<35> but was:<5>
testLinksHonourLiveAndArchivedLikeLegacyBrowser AssertionError: expected:<35> but was:<5>
testNegativeLinkCursorIsTreatedAsZero IndexOutOfBoundsException: fromIndex = -1
Worth noting why the existing live/archived test could not have caught this: it collapsed ids into a HashSet, which erases duplicates by construction, and asserted parity against the legacy path, which shares the defect. Both halves had to change — it now compares lists with an explicit no-duplicates assertion.
Postman got two more requests (filters.text and live:true), but weaker ones by necessity: menu links cannot be created over REST, so that collection has no link fixtures and can only pin that the combinations are accepted and the contract survives. Said so in the request descriptions.
On AC10 (openapi.yaml)
Nothing to commit, and that is correct rather than an omission: ContentDriveResource.search is @Hidden, so /v1/drive has no generated schema at all. The single showLinks in the yaml belongs to BrowserQueryForm (the legacy browser), untouched here. Verified with ./mvnw compile -pl :dotcms-core — the file comes back unchanged.
| linkCount = linkEnd - linkStart; | ||
| maxResults -= linkCount; | ||
| nextLinkCursor = linkEnd; | ||
| hasMoreLinks = linkEnd < totalLinks; |
There was a problem hiding this comment.
I think you should used PaginationUtil for pagination here
There was a problem hiding this comment.
I dug into this and I don't think PaginationUtil fits here — but it's a fair thing to ask, so here's what I found rather than just a no.
com.dotcms.util.PaginationUtil is REST-layer infrastructure, and the codebase is unanimous about it: of the 25 new PaginationUtil(...) sites in dotCMS/src/main/java, all 25 are under com.dotcms.rest.*. Not one business-layer *APIImpl uses it. That isn't an accident of style:
- It imports
javax.ws.rs.core.Responseandjavax.servlet.http.HttpServletRequest/Response, and builds the JAX-RSResponseitself, settingX-Pagination-Per-Page,X-Pagination-Current-Page,X-Pagination-Total-Entriesand an RFC-5988Linkheader. - All five
getPage(...)overloads are@Deprecated. The replacement,getPageView(PaginationUtilParams), unconditionally dereferencesutilParams.request().getRequestURI()andutilParams.response().setHeader(...)— passing null NPEs.
BrowserAPIImpl.getPaginatedContents(BrowserQuery) has no request, no response and no request URI. It returns the PaginatedContents POJO.
Beyond the layer, the model doesn't line up either:
- One source, not three.
Paginator<T>.getItems(user, limit, offset, params)returns onePaginatedArrayList<T>with onetotalResults. This block runs three heterogeneous sources against one decrementingmaxResultsbudget and returns nine pagination scalars (three counts, threehasMore, three cursors). There's no seam in thePaginatorcontract for a second source, and nowhere inPaginatedArrayListto carry ahasMore. - Page numbers, not cursors.
getMinIndex(currentPage, perPage) = (currentPage - 1) * perPage— the offset is derived from a page number, so you can't hand it three independent cursors. Grepping the whole pagination package for cursor semantics returns nothing. Meanwhile this contract says "keepoffsetat 0 on every request; only the cursors change." - It requires a total we deliberately don't compute.
lastPage = ceil(totalRecords / perPage).PaginatedContentshas nototalfield by design — thehasMore*flags exist precisely to avoid counting. WithtotalResultsunset,PaginationUtilwould advertiselastpage 0 and never emit anextrel: actively wrong output.
FolderSearchPaginator is the correctly-scoped example in-repo — single source, single window, delegating to FolderAPI.searchFolders. That shape works; this one doesn't.
The adjacent point I think you may actually be reaching for is real, though, and I'd rather name it than let it hide behind my "no": ContentDriveResource returns new ResponseEntityView<>(...) rather than ResponseEntityPaginatedDataView, so drive responses carry no pagination envelope and no X-Pagination-* headers — the pagination state lives entirely in the entity body. That's a legitimate discussion about the resource's response shape. But it's a change to the /api/v1/drive/search contract, which is already on main with folderCount/contentCount/hasMore*/next*Cursor and has consumers — so not something a showLinks PR should do. And even there, Pagination's currentPage/perPage/totalEntries triple still can't represent three cursors.
| hasMoreLinks = linkEnd < totalLinks; | ||
| } | ||
| // else: linkCursor is past the end — all links already shown, add nothing | ||
| } |
There was a problem hiding this comment.
can we do this pagination in the database query?
There was a problem hiding this comment.
I looked into this properly, and the answer is no — but for a reason that surprised me and is worth writing down, because the premise doesn't hold.
DotConnect does not paginate in the database at all. In dotCMS/src/main/java/com/dotmarketing/common/db/DotConnect.java:
:698→// statement.setMaxRows(maxRows);is commented out. On thecreateStatement()branch it's never called either. The JDBC driver receives no row hint whatsoever.:790-792→for (int i = 0; i < startRow; i++) { rs.next(); }. The offset is a client-side skip over theResultSet, not SQLOFFSET.:797→while (rs.next() && (maxRows <= 0 || i < maxRows)). The limit is client-side too.
So FolderFactoryImpl.getChildrenClass already asks the DB for every link under the parent today; its offset/limit args only cap how many Java objects get built. Threading them through would satisfy the wording of the request while delivering nothing in the database. I didn't want to make that change and report back "done, it paginates in the DB now."
And real SQL LIMIT/OFFSET still wouldn't be correct here, because four filters shrink the set after the query: the READ filterCollection in FolderAPIImpl, the live filter, the in-memory title filter (links aren't in Elasticsearch), and the per-link getPermissionIdsFromRoles check. A DB page of 40 could return 3 rows with hasMoreLinks: false — silently lost links. The Java sort also runs after filtering, so a SQL ORDER BY would order a different set than the one the cursor indexes into, breaking the cursor invariant outright.
This question has been adjudicated twice in this codebase already, in writing:
FolderAPIImpl.searchFolders(:812) — the newest paginatedFolderAPImethod, written for Content Drive: "Load all matching folders (no LIMIT — pagination happens in Java so that permission filtering does not produce short pages for limited users)".BrowserAPIImpl:184— "Pagination is applied after permission filtering. Applying DB-level pagination before filtering produces empty or sparse pages for restricted users."
Permissions can't move into the WHERE clause either: permission_reference is a lazily and asynchronously populated cache (PERMISSION_REFERENCES_UPDATE_ASYNC defaults to true), so a freshly created link has no row until someone reads it. Joining it would omit readable links — a correctness bug, worse than a short page. That's also why content.drive.folder.search.permissions.max.per.page exists to cap page size with a 400, rather than to paginate by permission.
Contentlets do use real LIMIT ? OFFSET ?, but only inside the getContentByChunks over-fetch machinery (BrowserAPIImpl:248-320) that exists precisely because SQL paging plus post-hoc permission filtering is unsound on its own. Worth it for contentlets (unbounded cardinality); not for links, which are the direct children of one folder.
Lastly, folders do exactly the same in-memory slice today (getFolders + allFolders.subList, :1636-1650) — the link branch is a line-for-line mirror of it. Paginating only links in the DB would make the two asymmetric without buying correctness.
Where I think your instinct is pointing at something real: the convenience overloads of getChildrenClass hardcode a 1000-row ceiling on a result set with no ORDER BY, so a folder with more than 1000 links is truncated non-deterministically. That's a genuine limitation of this path and I documented it in linksDefaultView's javadoc rather than leave it implicit. Fixing it properly means a searchLinks(...)-style method modelled on searchFolders — permission-filter first, then slice, with a total order — not SQL pagination. Happy to file that if you think it's worth queueing.
…tent-drive-search-api-apiv1drivesearch
…ink distinction Addresses @jcastro-dotcms's review on #37112. The previous commit resolved "live" for links by fetching the working links and filtering them in memory with hasLiveVersion. That compensated in BrowserAPIImpl for something that belongs to FolderAPI, and the reviewer was right to push back. FolderAPI.getLiveLinks(Folder, User, boolean) already existed and already emits the correct SQL: cond.live=true produces `live_inode = links_1_.inode`, the correlating `=` branch, so it never hits the cross join that `working=false` does. The only gap was the Host overload for the site-root case. - Add FolderAPI/FolderAPIImpl.getLiveLinks(Host, User, boolean), mirroring the existing getLinks(Host, working, deleted, ...) with cond.live=true and cond.deleted=false. - BrowserAPIImpl.getLinks now picks between getLinks and getLiveLinks per parent type. The hasLiveVersion stream and the javadoc paragraph explaining the workaround are both gone; what remains is one line on why working=false is not "live". getLiveLinks pins deleted=false, which is all this path needs: BrowserQuery ORs showArchived into showWorking, so the live branch is only ever reached with archived=false. Behavioural note: getLiveLinks returns the live version rows, where the previous code returned the working rows of links that have a live version. For live:true the former is the more correct answer — the caller asked for published content, not for the draft of something published. ContentDriveLinksTest is unchanged and still 18/18 green, which is the evidence the refactor preserves observable behaviour. openapi.yaml unchanged (the endpoint is @hidden). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second review round —
|
This pull request enhances the pagination and retrieval of menu links in the Content Drive browser API. It introduces independent, cursor-based paging for menu links (in addition to folders and contentlets), ensuring more consistent and scalable navigation when browsing assets under a folder. The changes also unify how links are mapped and returned, improving both API clarity and client-side handling.
Pagination and API enhancements:
Added independent cursor-based pagination for menu links, alongside folders and contentlets, in the
getPaginatedContentsmethod ofBrowserAPIImpl, including new fields (linkCursor,linkCount,hasMoreLinks,nextLinkCursor) in both the request (BrowserQuery) and response (PaginatedContents). [1] [2] [3] [4] [5] [6] [7] [8]Modified the documentation and method contracts in both
BrowserAPIandBrowserAPIImplto describe the new paging behavior for links, clarifying the contract for clients and the order in which page slots are filled (folders, then links, then contentlets). [1] [2]Link retrieval and mapping improvements:
Implemented the
linksDefaultViewmethod to retrieve, filter, and deterministically order menu links directly under a parent, supporting stable paging and matching legacy endpoint behavior.Standardized link mapping in API responses by using a shared constant for the MIME type and providing additional metadata (permissions, owner name, etc.) in the returned map, aligning with folder conventions. [1] [2]
Supporting changes:
StreamandUserLocalManagerUtil. [1] [2] [3]These updates collectively provide a more robust, flexible, and user-friendly API for browsing assets, especially when dealing with large numbers of menu links.
#36991
Fixes #36991