Description
FolderFactoryImpl.getChildrenClass(Identifier, Class, ChildrenCondition, String, int, int) appends the version table to the FROM list without any join predicate of its own:
https://github.com/dotCMS/core/blob/main/dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderFactoryImpl.java#L1310-L1343
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 — LinkTransformer is a 1:1 map and PermissionBitAPIImpl.filterCollection only 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
deleted predicate 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 = 1000 and orderBy = 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:
https://github.com/dotCMS/core/blob/main/dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderAPIImpl.java#L985-L997
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 #37112 (comment), 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 pass cond.live = true, not cond.working = false. Some may need updating.
- The missing
ORDER BY. limit is applied client-side in DotConnect (statement.setMaxRows is 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
Priority
Medium
Additional Context
Relevant files:
dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderFactoryImpl.java
dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderAPIImpl.java
dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/ChildrenCondition.java
dotCMS/src/main/java/com/dotcms/browser/BrowserAPIImpl.java (holds the current workaround)
Description
FolderFactoryImpl.getChildrenClass(Identifier, Class, ChildrenCondition, String, int, int)appends the version table to theFROMlist without any join predicate of its own:https://github.com/dotCMS/core/blob/main/dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderFactoryImpl.java#L1310-L1343
The only correlation the query ever produces is the incidental one from the
=variants: since the version table's primary key isidentifier,working_inode = X_1_.inodehappens to pin exactly one version row. Flip either flag tofalseand the predicate becomes<>, which correlates nothing — the query degenerates into a cross product against every version row in the installation.For
Link.classwithworking=false, deleted=falsethe emitted SQL is:Three consequences:
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.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.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/.deletedare boxedBooleanand the gate is!= null, not truthiness.FolderAPIImpl.getLinks(parent, working, deleted, user, respectFrontEndRoles)takes primitivebooleans, soworkingis never null on that path:https://github.com/dotCMS/core/blob/main/dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderAPIImpl.java#L985-L997
That is how
POST /api/v1/browserwithshowWorking: falsereaches the broken branch.Not a regression, and already worked around in one place
Found while reviewing #37112 (
showLinkssupport in/api/v1/drive/search, issue #36991) — see #37112 (comment), raised by @oidacra.That PR worked around it rather than fixing it:
BrowserAPIImpl.getLinks(BrowserQuery)now always asksFolderAPIforworking=trueand resolves "live" in memory viahasLiveVersion. 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 passingworking=false/live=false, now or later, still hits the cross join.Suggested fix
Emit the correlating clause whenever the version table is added:
This is a no-op for every current
=-variant caller (the incidental pin is already equivalent) and makes the<>anddeleted-only variants correct without needingDISTINCT.Worth deciding alongside it:
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.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.classandContentlet.classgo through this method, so this needs its own PR with tests rather than riding along on a feature branch.Acceptance Criteria
getChildrenClassworking=false/live=false/ deleted-only conditions return each asset at most oncedeletedpredicate applies for every combination of the other flagsPOST /api/v1/browserwithshowWorking: falsereturns no duplicate linksworking=falseare audited against the corrected semanticsChildrenConditionflag combination, for bothLinkandContentletPriority
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)