Skip to content

FolderFactoryImpl.getChildrenClass cross joins the version table when working/live is false #37133

Description

@nicobytes

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:

  1. 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.
  2. 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.
  3. 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

  • 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 deleted predicate applies for every combination of the other flags
  • POST /api/v1/browser with showWorking: false returns no duplicate links
  • Callers relying on working=false are audited against the corrected semantics
  • Integration coverage for each ChildrenCondition flag combination, for both Link and Contentlet

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)

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Status
    New

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions