FolderFactoryImpl.getChildrenClass cross joins the version table when working/live is false
Nobody has claimed this yet.
- Dominant language
- Java
- Stars
- 970
- Forks
- 486
- Avg merge
- 3d 33m
- Merged PRs (30d)
- 170
Description
Description
FolderFactoryImpl.getChildrenClass(Identifier, Class, ChildrenCondition, String, int, int) appends the version table to the FROM list without any join predicate of its own:
if (cond != null && versionTable != null && (cond.deleted != null || cond.working != null || cond.live != null)) {
sql += ", " + versionTable; // <-- no ON / no correlating WHERE clause
}
...
sql += versionTable + ".working_inode" + (cond.working ? "=" : "<>") + tableName + "_1_.inode and ";
sql += versionTable + ".live_inode" + (cond.live ? "=" : "<>") + tableName + "_1_.inode and ";
The only correlation the query ever produces is the incidental one from the = variants: since the version table's primary key is identifier, working_inode = X_1_.inode happens to pin exactly one version row. Flip either flag to false and the predicate becomes <>, which correlates nothing — the query degenerates into a cross product against every version row in the installation.
For Link.class with working=false, deleted=false the emitted SQL is:
SELECT links.* from links links, inode links_1_, identifier links_2_ , link_version_info
where links_2_.parent_path = ? and links.identifier = links_2_.id and links_1_.inode = links.inode
and link_version_info.deleted='false'
and link_version_info.working_inode <> links_1_.inode
and links_1_.type = 'links' and links_2_.host_inode = ?
Three consequences:
- Duplicates. No
DISTINCT, and no dedupe downstream —LinkTransformeris a 1:1 map andPermissionBitAPIImpl.filterCollectiononly removes on permission. Reproduced in an integration DB: five links under a folder came back 35 times, then 20 times on a second run. - The
deletedpredicate stops applying. With nothing correlating the version row,link_version_info.deleted='false'degenerates to "there exists any non-archived link anywhere", so the archived filter is a no-op in this branch. - Non-deterministic truncation. The convenience overloads pass
limit = 1000andorderBy = null, so once the installation's version-row count approaches 1000 the window can be consumed by duplicates of one link and other links in the folder become invisible, differently on each call.
Reachable from
ChildrenCondition.working / .live / .deleted are boxed Boolean and the gate is != null, not truthiness. FolderAPIImpl.getLinks(parent, working, deleted, user, respectFrontEndRoles) takes primitive booleans, so working is never null on that path:
That is how POST /api/v1/browser with showWorking: false reaches the broken branch.
Not a regression, and already worked around in one place
Found while reviewing #37112 (showLinks support in /api/v1/drive/search, issue #36991) — see https://github.com/dotCMS/core/pull/37112#discussion_r3815730314, raised by @oidacra.
That PR worked around it rather than fixing it: BrowserAPIImpl.getLinks(BrowserQuery) now always asks FolderAPI for working=true and resolves "live" in memory via hasLiveVersion. That covers both the drive path and the legacy browser path, because they share that method. The factory itself is untouched, so any other caller passing working=false / live=false, now or later, still hits the cross join.
Suggested fix
Emit the correlating clause whenever the version table is added:
if (cond != null && versionTable != null && (cond.deleted != null || cond.working != null || cond.live != null)) {
sql += ", " + versionTable;
}
...
// alongside the existing identifier joins in the WHERE clause
sql += versionTable + ".identifier = " + tableName + "_2_.id and ";
This is a no-op for every current =-variant caller (the incidental pin is already equivalent) and makes the <> and deleted-only variants correct without needing DISTINCT.
Worth deciding alongside it:
- Semantics of
working=false. Once correlated, that predicate means "versions of this asset that are not the working one", which is not the same as "the live version". Callers wanting live content should passcond.live = true, notcond.working = false. Some may need updating. - The missing
ORDER BY.limitis applied client-side inDotConnect(statement.setMaxRowsis commented out) over an unordered result set, so truncation at 1000 rows is arbitrary. A deterministic order would make the ceiling predictable, which matters for any index-based paging built on top.
Both Link.class and Contentlet.class go through this method, so this needs its own PR with tests rather than riding along on a feature branch.
Acceptance Criteria
- The version table is correlated on identifier whenever it is joined in
getChildrenClass -
working=false/live=false/ deleted-only conditions return each asset at most once - The
deletedpredicate applies for every combination of the other flags -
POST /api/v1/browserwithshowWorking: falsereturns no duplicate links - Callers relying on
working=falseare audited against the corrected semantics - Integration coverage for each
ChildrenConditionflag combination, for bothLinkandContentlet
Priority
Medium
Additional Context
Relevant files:
dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderFactoryImpl.javadotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderAPIImpl.javadotCMS/src/main/java/com/dotmarketing/portlets/folders/business/ChildrenCondition.javadotCMS/src/main/java/com/dotcms/browser/BrowserAPIImpl.java(holds the current workaround)
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderFactoryImpl.java at getChildrenClass, then read ChildrenCondition.java and the callers in FolderAPIImpl.java. Review BrowserAPIImpl.java for the existing workaround, audit the working/live semantics, and add integration coverage for Link and Contentlet flag combinations; done means correlated results, correct deleted filtering, and no duplicate browser links.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- api, backend, database, testing
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100