diff --git a/dotCMS/src/main/java/com/dotcms/browser/BrowserAPIImpl.java b/dotCMS/src/main/java/com/dotcms/browser/BrowserAPIImpl.java index cec5b06df168..0b030e0d9ff4 100644 --- a/dotCMS/src/main/java/com/dotcms/browser/BrowserAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/browser/BrowserAPIImpl.java @@ -2008,31 +2008,67 @@ private SelectQuery selectQuery(final BrowserQuery browserQuery) { final String workingLiveInode = browserQuery.showWorking || browserQuery.showArchived ? "working_inode" : "live_inode"; - final StringBuilder selectQuery = new StringBuilder(buildSelectBaseQuery(browserQuery, workingLiveInode)); - final List parameters = new ArrayList<>(); + // issue #37229: fold folder (+ per-case host_inode + fileName) scoping into a materialized + // CTE, resolved BEFORE this query joins out to contentlet_version_info/structure/ + // contentlet -- instead of joining the full `identifier` table first and filtering + // afterward, which is the source of the unstable-planner behavior on large folders + // (FR-002). Scoped ONLY to the folder-scoped case this fix targets: this shared method's + // behavior is byte-identical to before for every caller that does not scope by folder + // (folder == null, or skipFolder=true) -- forcing materialization of the full identifier + // table with no scoping predicate would be a regression, not a fix, for those callers. + // NOT validated against EXPLAIN ANALYZE with the real predicate set (FR-010) -- flagged + // as an explicit, developer-accepted risk; see PR description. + final boolean useFolderCte = browserQuery.folder != null && !browserQuery.skipFolder; + // Handle site filtering based on ignoreSiteForFolders flag + final boolean shouldApplySiteFiltering = !browserQuery.ignoreSiteForFolders && browserQuery.folder != null; + final boolean fileNameHandledByDb = !browserQuery.useElasticsearchFiltering + && UtilMethods.isSet(browserQuery.fileName); + + String candidatesCte = BLANK; + if (useFolderCte) { + final StringBuilder candidatesPredicates = new StringBuilder(); + appendFolderQuery(candidatesPredicates, browserQuery.folder.getPath(), parameters); + if (shouldApplySiteFiltering) { + if (browserQuery.site != null) { + appendSiteQuery(candidatesPredicates, browserQuery.site.getIdentifier(), + browserQuery.forceSystemHost, parameters); + } else if (browserQuery.forceSystemHost) { + appendSystemHostQuery(candidatesPredicates); + } + } + if (fileNameHandledByDb) { + appendFileNameQuery(candidatesPredicates, browserQuery.fileName, parameters); + } + candidatesCte = "with candidates as materialized (select * from identifier id where 1=1 " + + candidatesPredicates + ") "; + } + + final StringBuilder selectQuery = new StringBuilder( + buildSelectBaseQuery(browserQuery, workingLiveInode, candidatesCte)); + if (!browserQuery.languageIds.isEmpty()) { appendLanguageQuery(selectQuery, browserQuery.languageIds, browserQuery.showDefaultLangItems); } - // Handle site filtering based on ignoreSiteForFolders flag - final boolean shouldApplySiteFiltering = !browserQuery.ignoreSiteForFolders && browserQuery.folder != null; - - if (shouldApplySiteFiltering) { - if (browserQuery.site != null) { - appendSiteQuery(selectQuery, browserQuery.site.getIdentifier(), - browserQuery.forceSystemHost, parameters); - } else { - if (browserQuery.forceSystemHost) { - appendSystemHostQuery(selectQuery); + if (!useFolderCte) { + // Pre-existing shape, unchanged: no folder scopes this request (or skipFolder=true), + // so there is nothing for the CTE above to target -- site/host filtering (independent + // of skipFolder) still applies directly against `identifier` exactly as before this + // fix. (The folder predicate itself is never appended here: useFolderCte's negation + // means folder == null || skipFolder, the same condition that gated it originally.) + if (shouldApplySiteFiltering) { + if (browserQuery.site != null) { + appendSiteQuery(selectQuery, browserQuery.site.getIdentifier(), + browserQuery.forceSystemHost, parameters); + } else { + if (browserQuery.forceSystemHost) { + appendSystemHostQuery(selectQuery); + } } } } - //This property allows the exclusion of the folder in the base query - if (browserQuery.folder != null && !browserQuery.skipFolder) { - appendFolderQuery(selectQuery, browserQuery.folder.getPath(), parameters); - } // Detect archive-target steps once per request (cached WorkflowAPI lookups, never per row). // Only step-pinned entries can be archive-target; scheme-only entries always stay live-only. // Skipped when archived rows are already admitted, so the archive-step logic must not run @@ -2055,7 +2091,11 @@ private SelectQuery selectQuery(final BrowserQuery browserQuery) { if (UtilMethods.isSet(browserQuery.filter)) { appendFilterQuery(selectQuery, browserQuery.filter, parameters); } - if (UtilMethods.isSet(browserQuery.fileName)) { + // fileNameHandledByDb is true under the exact same condition this block already + // guards (isSet(fileName), not using ES) -- when useFolderCte, it was already folded + // into the candidates CTE above (resolved scoping decision, research.md); appending + // it again here would be redundant, not incorrect, but is skipped for clarity. + if (fileNameHandledByDb && !useFolderCte) { appendFileNameQuery(selectQuery, browserQuery.fileName, parameters); } } @@ -2083,7 +2123,7 @@ private SelectQuery selectQuery(final BrowserQuery browserQuery) { appendMIMETypeQuery(selectQuery, browserQuery.mimeTypes); } if (null != browserQuery.sortBy) { - appendOrderByQuery(selectQuery, browserQuery.sortByDesc); + appendOrderByQuery(selectQuery, browserQuery.sortByDesc, useFolderCte); } Logger.debug(this, "Select Query: " + selectQuery); @@ -2108,17 +2148,28 @@ static class SelectQuery { * * @param browserQuery The {@link BrowserQuery} object specifying the filtering criteria. * @param workingLiveInode The identifier of the working live inode. + * @param candidatesCte Issue #37229: when set, a {@code with candidates as materialized + * (...)} clause that pre-resolves the folder-scoped candidate set + * (parent_path, and per-case host_inode/fileName) before this query + * joins out to {@code contentlet_version_info}/{@code structure}/ + * {@code contentlet} -- see {@link #selectQuery(BrowserQuery)}. When + * blank, the query joins directly against {@code identifier} exactly + * as before this fix (every non-folder-scoped caller is unaffected). * @return The base SQL SELECT query string. */ - private String buildSelectBaseQuery(final BrowserQuery browserQuery, final String workingLiveInode) { + private String buildSelectBaseQuery(final BrowserQuery browserQuery, final String workingLiveInode, + final String candidatesCte) { - final String baseClause = " from contentlet_version_info cvi, identifier id, structure struc, contentlet c " + final String identifierSource = UtilMethods.isSet(candidatesCte) ? "candidates" : "identifier"; + + final String baseClause = " from contentlet_version_info cvi, " + identifierSource + + " id, structure struc, contentlet c " + " where cvi.identifier = id.id and struc.velocity_var_name = id.asset_subtype and " + " c.inode = cvi." + workingLiveInode + " and cvi.variant_id='" + DEFAULT_VARIANT.name() + "' "; - final StringBuilder baseQuery = new StringBuilder( - "select cvi." + workingLiveInode + " as inode " + baseClause); + final StringBuilder baseQuery = new StringBuilder(candidatesCte) + .append("select cvi.").append(workingLiveInode).append(" as inode ").append(baseClause); final boolean showAllBaseTypes = browserQuery.baseTypes.contains(BaseContentType.ANY); if (!showAllBaseTypes) { @@ -2688,12 +2739,24 @@ private void appendExcludeArchivedQuery(StringBuilder sqlQuery) { * @param sqlQuery * @param orderByDesc */ - private void appendOrderByQuery(StringBuilder sqlQuery, boolean orderByDesc) { + private void appendOrderByQuery(StringBuilder sqlQuery, boolean orderByDesc, boolean useFolderCte) { + // issue #37229 (FR-001): `mod_date` alone has no tiebreaker, so rows sharing the same + // mod_date get an unspecified, planner-dependent order today (~1.2% of rows per #37148). + // `id.id` (the identifier row's own primary key, already joined/in scope -- no new join) + // makes tied-row order -- and the pagination cursor derived from it -- a deterministic, + // reproducible-run-to-run guarantee. This is a NEW guarantee, not a reproduction of + // whatever arbitrary order those tied rows happened to return before this fix. + // + // FR-001 scopes this to folder-scoped requests only ("every folder-scoped listing + // request"), matching useFolderCte exactly -- every other caller's ORDER BY stays + // byte-identical to before (found in review: this was previously unconditional for any + // caller with sortBy set, silently changing tie order and pagination cursors for + // non-folder-scoped callers too). sqlQuery.append(" order by "); if (orderByDesc) { - sqlQuery.append(" c.mod_date desc"); + sqlQuery.append(" c.mod_date desc").append(useFolderCte ? ", id.id desc" : ""); } else { - sqlQuery.append(" c.mod_date asc"); + sqlQuery.append(" c.mod_date asc").append(useFolderCte ? ", id.id asc" : ""); } } diff --git a/dotcms-integration/src/test/java/com/dotcms/browser/BrowserAPITest.java b/dotcms-integration/src/test/java/com/dotcms/browser/BrowserAPITest.java index b32e4d32cb37..1be7492fe56c 100644 --- a/dotcms-integration/src/test/java/com/dotcms/browser/BrowserAPITest.java +++ b/dotcms-integration/src/test/java/com/dotcms/browser/BrowserAPITest.java @@ -2531,4 +2531,275 @@ private static File fileNamed(final String name) throws IOException { FileUtils.writeStringToFile(file, "this is a test!", StandardCharsets.UTF_8); return file; } + + // ------------------------------------------------------------------------------------------ + // issue #37229 -- folder-scoped candidate-scan CTE + ORDER BY tiebreaker. + // + // UNVALIDATED against FR-010's EXPLAIN ANALYZE gate (no live Postgres/reference dataset in + // this environment) -- see specs/37229-content-drive-folder-cte/tasks.md T004/T023. These + // tests cover correctness (result-set identity, tiebreaker determinism, permission scoping), + // not the query-plan/latency claim itself, which only EXPLAIN ANALYZE against real data can + // confirm. + + /** + * + */ + @Test + public void test_getPaginatedContents_tiedModDate_orderIsDeterministicAcrossRepeatedCalls() + throws Exception { + final Host site = new SiteDataGen().nextPersisted(); + final Folder folder = new FolderDataGen().site(site).nextPersisted(); + + final Contentlet first = new FileAssetDataGen(FileUtil.createTemporaryFile("tie-a", ".txt", "a")) + .folder(folder).setPolicy(IndexPolicy.WAIT_FOR).nextPersisted(); + final Contentlet second = new FileAssetDataGen(FileUtil.createTemporaryFile("tie-b", ".txt", "b")) + .folder(folder).setPolicy(IndexPolicy.WAIT_FOR).nextPersisted(); + + // Force an identical mod_date on both working inodes so the pre-tiebreaker ORDER BY + // (mod_date alone) has nothing to disambiguate them by. + final java.sql.Timestamp sharedModDate = new java.sql.Timestamp(System.currentTimeMillis()); + new DotConnect().executeUpdate("update contentlet set mod_date = ? where inode = ?", + sharedModDate, first.getInode()); + new DotConnect().executeUpdate("update contentlet set mod_date = ? where inode = ?", + sharedModDate, second.getInode()); + + final BrowserQuery query = BrowserQuery.builder() + .withUser(APILocator.systemUser()) + .withHostOrFolderId(folder.getIdentifier()) + .ignoreSiteForFolders(true) + .showFiles(true) + .build(); + + final List firstRunOrder = browserAPI.getPaginatedContents(query).list.stream() + .map(row -> (String) row.get("identifier")) + .filter(id -> id.equals(first.getIdentifier()) || id.equals(second.getIdentifier())) + .collect(Collectors.toList()); + final List secondRunOrder = browserAPI.getPaginatedContents(query).list.stream() + .map(row -> (String) row.get("identifier")) + .filter(id -> id.equals(first.getIdentifier()) || id.equals(second.getIdentifier())) + .collect(Collectors.toList()); + + assertEquals("Both tied rows must be present", 2, firstRunOrder.size()); + assertEquals("Order among tied mod_date rows must be reproducible run-to-run", + firstRunOrder, secondRunOrder); + } + + /** + *
    + *
  • Given Scenario: FR-010/R4 -- the folder-scoping predicate the new CTE relies + * on must still be backed by an index on {@code identifier(parent_path, asset_name, + * host_inode)}, whatever that index is named under the current install lineage (fresh + * install: {@code identifier_parent_path_asset_name_host_inode_key}; upgraded via + * {@code Task00785DataModelChanges}: {@code identifier_unique_key}).
    • + *
  • Expected Result: Querying Postgres catalog tables directly (not the index + * name, not `EXPLAIN` text) confirms an index exists covering exactly those three + * columns, in that order, on {@code identifier}.
    • + *
+ */ + @Test + public void test_identifierTable_hasIndexOnParentPathAssetNameHostInode_lineageIndependent() + throws Exception { + final DotConnect dc = new DotConnect(); + dc.setSQL( + "select i.relname as index_name, " + // string_agg (not array_agg) so the result maps to a plain String via + // JDBC's generic result mapping -- no java.sql.Array unwrapping needed. + + "string_agg(a.attname, ',' order by array_position(ix.indkey, a.attnum)) as columns " + + "from pg_class t " + + "join pg_index ix on t.oid = ix.indrelid " + + "join pg_class i on i.oid = ix.indexrelid " + + "join pg_attribute a on a.attrelid = t.oid and a.attnum = any(ix.indkey) " + + "where t.relname = 'identifier' " + + "group by i.relname"); + @SuppressWarnings("unchecked") + final List> rows = dc.loadObjectResults(); + + final String expectedColumns = "parent_path,asset_name,host_inode"; + final boolean hasExpectedIndex = rows.stream() + .anyMatch(row -> expectedColumns.equals(row.get("columns"))); + + assertTrue("identifier must have an index on (parent_path, asset_name, host_inode) " + + "regardless of its name (fresh-install vs. Task00785 upgrade lineage): " + rows, + hasExpectedIndex); + } + + /** + *
    + *
  • Given Scenario: An empty folder and a small folder (a handful of children), + * both now routed through the folder-scoping CTE -- SC-002/Edge Cases.
    • + *
  • Expected Result: Both resolve without error and return the exact same result + * set as before this fix (empty list / all created items respectively).
    • + *
+ */ + @Test + public void test_getPaginatedContents_emptyAndSmallFolder_noRegression() throws Exception { + final Host site = new SiteDataGen().nextPersisted(); + final Folder emptyFolder = new FolderDataGen().site(site).nextPersisted(); + final Folder smallFolder = new FolderDataGen().site(site).nextPersisted(); + + final List expectedIdentifiers = new ArrayList<>(); + for (int i = 0; i < 5; i++) { + expectedIdentifiers.add(new FileAssetDataGen( + FileUtil.createTemporaryFile("small-" + i, ".txt", "content " + i)) + .folder(smallFolder).setPolicy(IndexPolicy.WAIT_FOR).nextPersisted() + .getIdentifier()); + } + + final PaginatedContents emptyResult = browserAPI.getPaginatedContents(BrowserQuery.builder() + .withUser(APILocator.systemUser()) + .withHostOrFolderId(emptyFolder.getIdentifier()) + .ignoreSiteForFolders(true) + .showFiles(true) + .build()); + assertTrue("Empty folder must return no results", emptyResult.list.isEmpty()); + + final PaginatedContents smallResult = browserAPI.getPaginatedContents(BrowserQuery.builder() + .withUser(APILocator.systemUser()) + .withHostOrFolderId(smallFolder.getIdentifier()) + .ignoreSiteForFolders(true) + .showFiles(true) + .build()); + final List actualIdentifiers = smallResult.list.stream() + .map(row -> (String) row.get("identifier")) + .collect(Collectors.toList()); + assertEquals(5, smallResult.list.size()); + assertTrue("All created items must be present", actualIdentifiers.containsAll(expectedIdentifiers)); + } + + /** + *
    + *
  • Given Scenario: Site/host-scoping matrix (R3, SC-003a) -- the two most + * directly reachable code paths through the public {@code BrowserQuery} builder: (1) + * explicit site, folder-scoped, {@code ignoreSiteForFolders=false} (host filter applies); + * (5) {@code ignoreSiteForFolders=true}, Content Drive's own path (no host filter).
    • + *
  • Expected Result: Both produce the same result set as before this fix.
    • + *
  • Known gap: R3 paths 2/3/4 (forced-system-host / {@code site == null} + * combinations) were not exercised here -- {@code BrowserQuery}'s builder always resolves + * {@code site} to a real {@link Host} via {@code getParents} when constructed through + * {@code withHostOrFolderId} (confirmed by reading {@code BrowserQuery.java}), so + * constructing a {@code site == null} instance requires a caller/construction path not + * identified in this pass. Flagged for the developer rather than fabricated.
    • + *
+ */ + @Test + public void test_getPaginatedContents_siteScoping_explicitSiteAndIgnoreSiteForFolders_resultsUnchanged() + throws Exception { + final Host site = new SiteDataGen().nextPersisted(); + final Folder folder = new FolderDataGen().site(site).nextPersisted(); + final String expectedIdentifier = new FileAssetDataGen( + FileUtil.createTemporaryFile("scoping", ".txt", "content")) + .folder(folder).setPolicy(IndexPolicy.WAIT_FOR).nextPersisted() + .getIdentifier(); + + // Path 1: explicit site, ignoreSiteForFolders=false -- host filter applies inside the CTE. + final PaginatedContents explicitSiteResult = browserAPI.getPaginatedContents(BrowserQuery.builder() + .withUser(APILocator.systemUser()) + .withHostOrFolderId(folder.getIdentifier()) + .ignoreSiteForFolders(false) + .showFiles(true) + .build()); + assertEquals(1, explicitSiteResult.list.size()); + assertEquals(expectedIdentifier, explicitSiteResult.list.get(0).get("identifier")); + + // Path 5: ignoreSiteForFolders=true -- no host filter, Content Drive's own behavior. + final PaginatedContents ignoreSiteResult = browserAPI.getPaginatedContents(BrowserQuery.builder() + .withUser(APILocator.systemUser()) + .withHostOrFolderId(folder.getIdentifier()) + .ignoreSiteForFolders(true) + .showFiles(true) + .build()); + assertEquals(1, ignoreSiteResult.list.size()); + assertEquals(expectedIdentifier, ignoreSiteResult.list.get(0).get("identifier")); + } + + /** + *
    + *
  • Given Scenario: A permission-restricted user browsing a folder-scoped (now + * CTE-routed) request, alongside content the user cannot read -- FR-005/SC-004.
    • + *
  • Expected Result: The restricted user sees exactly the permitted subset, same + * as before this fix -- the CTE only reshapes candidate-set resolution, not permission + * filtering, which happens afterward in {@code filterContentletsByPermissions}.
    • + *
+ */ + @Test + public void test_getPaginatedContents_folderScopedCte_permissionScopingUnchanged() throws Exception { + final Host host = new SiteDataGen().nextPersisted(true); + final Folder folder = new FolderDataGen().site(host).nextPersisted(); + // A dedicated, freshly-created user -- NOT the shared TestUserUtils.getChrisPublisherUser + // fixture, which is a singleton looked up by a hardcoded email across the whole suite. + // Granting this test's permissions on that shared user polluted a pre-existing, + // unrelated test in this same file that also uses it (found running this test). + final Role limitedRole = new RoleDataGen().nextPersisted(); + final User limitedUser = new UserDataGen() + .roles(limitedRole, TestUserUtils.getBackendRole()) + .nextPersisted(); + final PermissionAPI permissionAPI = APILocator.getPermissionAPI(); + + permissionAPI.save(new Permission(host.getPermissionId(), + APILocator.getRoleAPI().getUserRole(limitedUser).getId(), PermissionAPI.PERMISSION_READ), + host, APILocator.systemUser(), false); + permissionAPI.save(new Permission(folder.getPermissionId(), + APILocator.getRoleAPI().getUserRole(limitedUser).getId(), PermissionAPI.PERMISSION_READ), + folder, APILocator.systemUser(), false); + + // New content does not inherit READ from the folder grant above -- it needs its own + // explicit permission, same as the working pattern in + // test_exhaustive_pagination_with_permission_filtering (found running this test: the + // folder-only grant left doesUserHavePermission false on a freshly created contentlet). + final Contentlet readableContentlet = new FileAssetDataGen( + FileUtil.createTemporaryFile("readable", ".txt", "content")) + .host(host).folder(folder).setPolicy(IndexPolicy.WAIT_FOR).nextPersisted(); + permissionAPI.save(new Permission(readableContentlet.getPermissionId(), + APILocator.getRoleAPI().getUserRole(limitedUser).getId(), PermissionAPI.PERMISSION_READ), + readableContentlet, APILocator.systemUser(), false); + + // A second, otherwise-identical contentlet with NO permission granted to limitedUser -- + // an explicit permission entry for a role limitedUser doesn't have overrides folder + // inheritance, matching test_exhaustive_pagination_with_permission_filtering's proven + // pattern (the extra permissionIndividually() call tried here first did not actually + // block read access -- found running this test). + final Role noAccessRole = new RoleDataGen().nextPersisted(); + final Contentlet restrictedContentlet = new FileAssetDataGen( + FileUtil.createTemporaryFile("restricted", ".txt", "content")) + .host(host).folder(folder).setPolicy(IndexPolicy.WAIT_FOR).nextPersisted(); + permissionAPI.save(new Permission(restrictedContentlet.getPermissionId(), + noAccessRole.getId(), PermissionAPI.PERMISSION_READ), + restrictedContentlet, APILocator.systemUser(), false); + + // Diagnostic: isolate a permission-setup issue from a candidate-scan issue before + // asserting on getPaginatedContents' result. + assertTrue("Sanity check: limitedUser must have READ on readableContentlet", + permissionAPI.doesUserHavePermission(readableContentlet, PermissionAPI.PERMISSION_READ, + limitedUser, false)); + assertFalse("Sanity check: limitedUser must NOT have READ on restrictedContentlet", + permissionAPI.doesUserHavePermission(restrictedContentlet, PermissionAPI.PERMISSION_READ, + limitedUser, false)); + + final BrowserQuery query = BrowserQuery.builder() + .withHostOrFolderId(folder.getInode()) + .ignoreSiteForFolders(true) + .respectFrontEndRoles(false) + .withUser(limitedUser) + .forceSystemHost(false) + .showFiles(true) + .showWorking(true) + .build(); + + final PaginatedContents results = browserAPI.getPaginatedContents(query); + final List visibleIdentifiers = results.list.stream() + .map(row -> (String) row.get("identifier")) + .collect(Collectors.toList()); + + assertTrue("The readable contentlet must be visible", + visibleIdentifiers.contains(readableContentlet.getIdentifier())); + assertFalse("The permission-restricted contentlet must not be visible", + visibleIdentifiers.contains(restrictedContentlet.getIdentifier())); + } }