diff --git a/.github/actions/core-cicd/deployment/deploy-javadoc/action.yml b/.github/actions/core-cicd/deployment/deploy-javadoc/action.yml index a544a034dee2..137ad046b616 100644 --- a/.github/actions/core-cicd/deployment/deploy-javadoc/action.yml +++ b/.github/actions/core-cicd/deployment/deploy-javadoc/action.yml @@ -49,7 +49,6 @@ runs: maven-args: "javadoc:javadoc -pl :dotcms-core" generate-docker: false cleanup-runner: true - restore-classes: true artifacts-from: ${{ inputs.artifact-run-id }} github-token: ${{ inputs.github-token }} diff --git a/.github/actions/core-cicd/maven-job/README.md b/.github/actions/core-cicd/maven-job/README.md index b9e1284d3371..a44120906726 100644 --- a/.github/actions/core-cicd/maven-job/README.md +++ b/.github/actions/core-cicd/maven-job/README.md @@ -27,7 +27,6 @@ This GitHub Action sets up and runs a Maven job with extensive configuration opt | `dotcms-license` | The license key for dotCMS | No | `''` | | `artifacts-from` | Download artifacts from a previous job | No | `''` | | `github-token` | GitHub token for authentication | Yes | - | -| `restore-classes` | Restore build classes | No | `false` | | `stage-name` | Stage name for the build | Yes | - | | `maven-args` | Arguments for Maven build | Yes | - | | `generates-test-results` | Generate test results artifacts | No | `false` | diff --git a/.github/actions/core-cicd/maven-job/action.yml b/.github/actions/core-cicd/maven-job/action.yml index 1a8cbf80bd5c..e06b733eca56 100644 --- a/.github/actions/core-cicd/maven-job/action.yml +++ b/.github/actions/core-cicd/maven-job/action.yml @@ -61,10 +61,6 @@ inputs: github-token: description: 'GitHub token for authentication' required: true - restore-classes: - description: 'Restore build classes' - required: false - default: 'false' stage-name: description: 'Stage name for the build' required: true @@ -262,15 +258,6 @@ runs: if: ${{ inputs.needs-docker-image == 'true' }} run: docker load < /tmp/docker-image/image.tar - - id: restore-artifact-classes - name: Restore Classes - if: ${{ inputs.restore-classes == 'true' }} - uses: actions/download-artifact@v4 - with: - run-id: ${{ inputs.artifacts-from }} - github-token: ${{ inputs.github-token }} - name: build-classes${{ steps.artifact-suffix.outputs.suffix }} - - name: Docker Hub Login if: ${{ inputs.docker-io-username != '' && inputs.docker-io-token != '' }} uses: docker/login-action@v3.0.0 @@ -383,18 +370,6 @@ runs: name: docker-image${{ steps.artifact-suffix.outputs.suffix }} path: /tmp/image.tar - - id: persist-build-classes - name: Persist Build Classes - if: ${{ inputs.generate-artifacts == 'true' }} - uses: actions/upload-artifact@v4 - with: - name: build-classes${{ steps.artifact-suffix.outputs.suffix }} - path: | - **/target/classes/**/*.class - **/target/generated-sources/**/*.java - **/target/test-classes/**/*.class - LICENSE - - id: delete-built-artifacts-from-cache name: Delete Built Artifacts From Cache if: ${{ inputs.generate-artifacts == 'true' && steps.restore-cache-maven.outputs.cache-hit != 'true' }} diff --git a/.github/scripts/test-balance/find_swallowed.py b/.github/scripts/test-balance/find_swallowed.py new file mode 100644 index 000000000000..f4f85e4bd4c4 --- /dev/null +++ b/.github/scripts/test-balance/find_swallowed.py @@ -0,0 +1,80 @@ +#!/usr/bin/env python3 +"""Find integration tests that fail without reporting why. + +The pattern that just cost a CI run: + + try { doThing(); } + catch (Exception e) { Assert.assertTrue("No Exception should be thrown", false); } + +The test fails, but the caught exception is discarded, so the CI log says nothing +about the actual cause. Retries just repeat the empty message. + +Classifies each catch block whose body FAILS the test: + SWALLOWED - fails, never references the exception variable (the bad case) + REPORTED - fails and passes the exception / its message along +Also flags empty catch blocks, which hide failures entirely. +""" +import os, re, os, glob, collections + +ROOT = os.path.join(os.environ.get("REPO_ROOT", os.getcwd()), "dotcms-integration/src/test/java") + +CATCH = re.compile(r'catch\s*\(\s*([\w.]+(?:\s*\|\s*[\w.]+)*)\s+(\w+)\s*\)\s*\{') +# something that makes the test fail +FAILS = re.compile(r'\b(fail|assertTrue|assertFalse|assertEquals|assertNotNull|Assert\.fail)\b') +# an unconditional failure, i.e. the catch exists only to fail +HARD_FAIL = re.compile( + r'\bfail\s*\(|\bAssert\.fail\s*\(|assertTrue\s*\([^;]*,\s*false\s*\)|assertFalse\s*\([^;]*,\s*true\s*\)') + + +def body_of(src, open_idx): + """Return the text inside the braces starting at open_idx (index of '{').""" + depth, i = 0, open_idx + while i < len(src): + if src[i] == '{': + depth += 1 + elif src[i] == '}': + depth -= 1 + if depth == 0: + return src[open_idx + 1:i] + i += 1 + return '' + + +swallowed, empty, reported = [], [], [] +for path in glob.glob(ROOT + "/**/*.java", recursive=True): + src = open(path, errors='ignore').read() + rel = path[len(ROOT) + 1:] + for m in CATCH.finditer(src): + var = m.group(2) + body = body_of(src, m.end() - 1) + line = src[:m.start()].count('\n') + 1 + stripped = re.sub(r'//[^\n]*|/\*.*?\*/', '', body, flags=re.S).strip() + + if not stripped: + empty.append((rel, line, m.group(1))) + continue + if not HARD_FAIL.search(stripped): + continue # catch doesn't unconditionally fail + # does it carry the exception anywhere? + carries = re.search(rf'\b{re.escape(var)}\b', stripped) + (reported if carries else swallowed).append((rel, line, m.group(1), stripped[:90])) + +print(f"scanned {len(glob.glob(ROOT + '/**/*.java', recursive=True))} integration test files\n") +print(f"SWALLOWED (fails without reporting the cause): {len(swallowed)}") +print(f"REPORTED (fails and includes the exception): {len(reported)}") +print(f"EMPTY catch blocks (hide failures entirely): {len(empty)}\n") + +by_file = collections.Counter(f for f, *_ in swallowed) +print("=== worst files (swallowed catches) ===") +for f, n in by_file.most_common(20): + print(f" {n:>3} {f}") + +print("\n=== every swallowed catch ===") +for f, line, exc, snippet in sorted(swallowed): + one = ' '.join(snippet.split()) + print(f" {f}:{line} catch({exc}) -> {one[:80]}") + +if empty: + print("\n=== empty catch blocks ===") + for f, line, exc in sorted(empty)[:25]: + print(f" {f}:{line} catch({exc}) {{}}") diff --git a/dotCMS/src/main/java/com/dotcms/publisher/endpoint/bean/impl/AWSS3PublishingEndPoint.java b/dotCMS/src/main/java/com/dotcms/publisher/endpoint/bean/impl/AWSS3PublishingEndPoint.java index 6f824e11ee25..c507dd1fc2ce 100644 --- a/dotCMS/src/main/java/com/dotcms/publisher/endpoint/bean/impl/AWSS3PublishingEndPoint.java +++ b/dotCMS/src/main/java/com/dotcms/publisher/endpoint/bean/impl/AWSS3PublishingEndPoint.java @@ -20,6 +20,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import com.liferay.portal.language.LanguageUtil; +import com.liferay.portal.model.User; import com.liferay.portal.util.PortalUtil; import java.io.IOException; import java.io.StringReader; @@ -111,8 +112,18 @@ public void validatePublishingEndPoint() throws PublishingEndPointValidationExce final SystemMessageBuilder systemMessageBuilder = new SystemMessageBuilder(); SystemMessage systemMessage = systemMessageBuilder.setMessage("Unable to verify S3 Endpoint. Please check your configuration:" + e.getMessage()).setType(MessageType.SIMPLE_MESSAGE) .setSeverity(MessageSeverity.WARNING).setLife(100000).create(); - SystemMessageEventUtil.getInstance().pushMessage(systemMessage, ImmutableList.of(PortalUtil.getUser().getUserId())); + // PortalUtil.getUser() returns null whenever no request is bound to the + // thread - scheduled jobs, background tasks, tests. Dereferencing it here + // threw a NullPointerException out of this catch block, which replaced the + // S3 error we are trying to report with a confusing NPE and defeated the + // point of catching at all. The message is a UI notification, so when + // there is no user to notify, the logged warning above is the whole story. + final User currentUser = PortalUtil.getUser(); + if (currentUser != null) { + SystemMessageEventUtil.getInstance().pushMessage(systemMessage, + ImmutableList.of(currentUser.getUserId())); + } } } //validatePublishingEndPoint. 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 3519877b9c0f..c7324e62abae 100644 --- a/dotcms-integration/src/test/java/com/dotcms/browser/BrowserAPITest.java +++ b/dotcms-integration/src/test/java/com/dotcms/browser/BrowserAPITest.java @@ -433,7 +433,7 @@ public void testGetFolderContentWithValidIdentifier() throws Exception { // http Assert.assertTrue( folderContent.containsKey( "total" ) ); Assert.assertTrue( folderContent.containsKey( "list" ) ); } catch ( Exception e ){ - Assert.fail( "We should not be getting any exception here" ); + throw new AssertionError("We should not be getting any exception here", e); } finally { folderAPI.delete( folder, user, false ); } @@ -1432,15 +1432,22 @@ public void test_buildBaseESQuery_edgeCases() { * to calculate the offset within the database pagination. */ @Test - public void test_SmartPaginationPage1_25Folders1Contentlet() throws Exception { + public void test_SmartPaginationPage1_FoldersThenOneContentlet() throws Exception { + // What is under test is the RELATIONSHIP between page size and folder count: + // a page larger than the folder count is filled with every folder and then + // topped up from the DB. The absolute sizes do not matter, and each item + // costs a persist + index, so keep them small. + final int folderCount = 5; + final int contentCount = 6; // more than needed, so the top-up is bounded + final int pageSize = folderCount + 1; + final User owner = new UserDataGen().nextPersisted(); // Create a test environment final Host host = new SiteDataGen().nextPersisted(); final Folder parentFolder = new FolderDataGen().site(host).nextPersisted(); - // Create 25 folders final List subFolders = new ArrayList<>(); - for (int i = 0; i < 25; i++) { + for (int i = 0; i < folderCount; i++) { final Folder subFolder = new FolderDataGen() .name(String.format("folder_%02d", i)) .parent(parentFolder) @@ -1449,8 +1456,7 @@ public void test_SmartPaginationPage1_25Folders1Contentlet() throws Exception { subFolders.add(subFolder); } - // Create 30 contentlets - for (int i = 0; i < 30; i++) { + for (int i = 0; i < contentCount; i++) { new FileAssetDataGen(FileUtil.createTemporaryFile("content", ".txt", "content " + i)) .host(host) .folder(parentFolder) @@ -1458,7 +1464,7 @@ public void test_SmartPaginationPage1_25Folders1Contentlet() throws Exception { .nextPersisted(); } - // Execute pagination query - Page 1 with page size 26 + // Execute pagination query - Page 1, one slot wider than the folder count final BrowserQuery browserQuery = BrowserQuery.builder() .showFolders(true) .showContent(true) @@ -1467,7 +1473,7 @@ public void test_SmartPaginationPage1_25Folders1Contentlet() throws Exception { .showLinks(false) // Simplify test by disabling links .withHostOrFolderId(parentFolder.getIdentifier()) .offset(0) - .maxResults(26) + .maxResults(pageSize) .build(); final PaginatedContents paginatedContents = browserAPI.getPaginatedContents(browserQuery); @@ -1478,46 +1484,53 @@ public void test_SmartPaginationPage1_25Folders1Contentlet() throws Exception { @SuppressWarnings("unchecked") final List> list = paginatedContents.list; - assertEquals("Should return exactly 26 items (25 folders + 1 contentlet)", 26, list.size()); - assertEquals("Folder count should be 25", 25, paginatedContents.folderCount); + assertEquals("Should return every folder plus one contentlet", pageSize, list.size()); + assertEquals("Folder count should be " + folderCount, folderCount, paginatedContents.folderCount); assertEquals("Content count should be 1", 1, paginatedContents.contentCount); - // Verify first 25 items are folders - for (int i = 0; i < 25; i++) { + // Verify the folders come first + for (int i = 0; i < folderCount; i++) { final Map item = list.get(i); assertNotNull("Item should have name", item.get("name")); - assertTrue("First 25 items should be folders", + assertTrue("Leading items should be folders", item.get("name").toString().startsWith("folder_")); assertEquals("Owner should be the same as parent folder", owner.getFullName(), item.get("owner")); } // Verify the last item is a contentlet - final Map lastItem = list.get(25); + final Map lastItem = list.get(folderCount); assertNotNull("Last item should have extension", lastItem.get("extension")); assertEquals("Last item should be a file", "txt", lastItem.get("extension")); } /** - * Test Case: Smart Pagination - Page 2 with the same data (offset=11, still 11 items per page) - * Expected: 11 contentlets (all folders were shown on page 1) + * Test Case: Smart Pagination - a later page, past the end of the folders + * Expected: only the contentlets left after the cursor, and no more pages */ @Test - public void test_SmartPaginationPage2_15Contentlets() throws Exception { + public void test_SmartPaginationPage2_RemainingContentletsOnly() throws Exception { + // The invariant: once the folder cursor is past every folder, the page is + // contentlets only, and when fewer than a full page remain there is no next + // page. Sizes are kept small - each item costs a persist + index. + final int folderCount = 2; + final int contentCount = 5; + final int contentCursor = 2; + final int remaining = contentCount - contentCursor; // 3 + final int pageSize = remaining + 1; // wider than what is left + // Create test environment final Host host = new SiteDataGen().nextPersisted(); final Folder parentFolder = new FolderDataGen().site(host).nextPersisted(); - // Create 10 folders - for (int i = 0; i < 10; i++) { + for (int i = 0; i < folderCount; i++) { new FolderDataGen() .name(String.format("folder_%02d", i)) .parent(parentFolder) .nextPersisted(); } - // Create 25 contentlets - for (int i = 0; i < 25; i++) { + for (int i = 0; i < contentCount; i++) { new FileAssetDataGen(FileUtil.createTemporaryFile("content", ".txt", "content " + i)) .host(host) .folder(parentFolder) @@ -1525,16 +1538,16 @@ public void test_SmartPaginationPage2_15Contentlets() throws Exception { .nextPersisted(); } - // Execute pagination query - Page 2 (offset=10) + // Execute pagination query - cursors placed past all folders final BrowserQuery browserQuery = BrowserQuery.builder() .showFolders(true) .showContent(true) .showFiles(true) .showLinks(false) .withHostOrFolderId(parentFolder.getIdentifier()) - .folderCursor(10) // Second page - .contentCursor(10) - .maxResults(20) + .folderCursor(folderCount) // past every folder + .contentCursor(contentCursor) + .maxResults(pageSize) .build(); final PaginatedContents paginatedContents = browserAPI.getPaginatedContents(browserQuery); @@ -1545,33 +1558,39 @@ public void test_SmartPaginationPage2_15Contentlets() throws Exception { @SuppressWarnings("unchecked") final List> list = paginatedContents.list; - assertEquals("Should return exactly 15 items (15 contentlets, no folders)", 15, list.size()); - assertEquals("Folder count should be 0 the 10 folders where in the first page", 0, paginatedContents.folderCount); + assertEquals("Should return only the contentlets left after the cursor", remaining, list.size()); + assertEquals("Folder count should be 0, all folders were on the first page", 0, paginatedContents.folderCount); assertFalse("Should indicate NO more folders available", paginatedContents.hasMoreFolders); - assertEquals("Content count should be 15", 15, paginatedContents.contentCount); + assertEquals("Content count should be " + remaining, remaining, paginatedContents.contentCount); assertFalse("Should indicate NO more content available", paginatedContents.hasMoreContent); } /** - * Test Case: Smart Pagination - Page 3 (offset=52) - * Expected: 26 more contentlets + * Test Case: Smart Pagination - a full mid-stream page with more still to come + * Expected: exactly one page of contentlets, and hasMoreContent set */ @Test - public void test_SmartPaginationPage3_16MoreContentlets() throws Exception { + public void test_SmartPaginationPage3_FullPageWithMoreRemaining() throws Exception { + // The invariant here is the opposite of the previous test: MORE than a page + // remains after the cursor, so the page comes back full and hasMoreContent + // is true. Sizes are kept small - each item costs a persist + index. + final int folderCount = 3; + final int contentCount = 10; + final int contentCursor = 3; + final int pageSize = 4; // < contentCount - contentCursor (7) + // Create a test environment final Host host = new SiteDataGen().nextPersisted(); final Folder parentFolder = new FolderDataGen().site(host).nextPersisted(); - // Create 15 folders - for (int i = 0; i < 15; i++) { + for (int i = 0; i < folderCount; i++) { new FolderDataGen() .name(String.format("folder_%02d", i)) .parent(parentFolder) .nextPersisted(); } - // Create 50 contentlets - for (int i = 0; i < 50; i++) { + for (int i = 0; i < contentCount; i++) { new FileAssetDataGen(FileUtil.createTemporaryFile("content", ".txt", "content " + i)) .host(host) .folder(parentFolder) @@ -1579,7 +1598,7 @@ public void test_SmartPaginationPage3_16MoreContentlets() throws Exception { .nextPersisted(); } - // Execute pagination query - Page 3 (offset=52) + // Execute pagination query - cursors past all folders, mid-way through content final BrowserQuery browserQuery = BrowserQuery.builder() .showFolders(true) .showContent(true) @@ -1587,9 +1606,9 @@ public void test_SmartPaginationPage3_16MoreContentlets() throws Exception { .showDotAssets(true) .showLinks(false) .withHostOrFolderId(parentFolder.getIdentifier()) - .folderCursor(15) - .contentCursor(17) // Third page (16*2) = 32 (15 folders and 17 contents) - .maxResults(16) + .folderCursor(folderCount) + .contentCursor(contentCursor) + .maxResults(pageSize) .build(); final PaginatedContents paginatedContents = browserAPI.getPaginatedContents(browserQuery); @@ -1600,10 +1619,10 @@ public void test_SmartPaginationPage3_16MoreContentlets() throws Exception { @SuppressWarnings("unchecked") final List> list = paginatedContents.list; - assertEquals("Should return exactly 16 items (16 contentlets)", 16, list.size()); + assertEquals("Should return a full page of contentlets", pageSize, list.size()); assertEquals("Folder count should be 0", 0, paginatedContents.folderCount); assertFalse("Should indicate NO more folders available", paginatedContents.hasMoreFolders); - assertEquals("Content count should be 16", 16, paginatedContents.contentCount); + assertEquals("Content count should be " + pageSize, pageSize, paginatedContents.contentCount); assertTrue("Should indicate more content available", paginatedContents.hasMoreContent); } @@ -1917,8 +1936,13 @@ public void test_exhaustive_pagination_with_permission_filtering() throws Except // This ensures non-continuous distribution in the database final List accessibleContentlets = new ArrayList<>(); - // Create 20 pieces of content, alternating permissions (10 accessible, 10 non-accessible) - for (int i = 0; i < 20; i++) { + // What matters is that accessible items are separated by gaps the scan has to + // skip, not how many there are. Alternating permissions over totalContent + // gives accessibleCount readable items; each costs a persist + index, so keep + // the total small. + final int totalContent = 10; + final int accessibleCount = totalContent / 2; + for (int i = 0; i < totalContent; i++) { final File file = FileUtil.createTemporaryFile("content(" + i + ")", ".txt", "content-" + i); final Contentlet contentlet = new FileAssetDataGen(file) .host(host) @@ -1954,10 +1978,11 @@ public void test_exhaustive_pagination_with_permission_filtering() throws Except }); }); - // Verify permission setup: should have 10 accessible contentlets - assertEquals("Should have created 10 accessible contentlets", 10, accessibleContentlets.size()); + // Verify permission setup + assertEquals("Should have created " + accessibleCount + " accessible contentlets", + accessibleCount, accessibleContentlets.size()); - // Test Case 1: Request page size of 5 - should get exactly 5 accessible items + // Test Case 1: Request a partial page - should fill it despite the gaps final BrowserQuery query1 = BrowserQuery.builder() .withHostOrFolderId(folder.getInode()) .ignoreSiteForFolders(true) @@ -1977,38 +2002,46 @@ public void test_exhaustive_pagination_with_permission_filtering() throws Except // Using reflection to access the package-private method for direct testing final BrowserAPIImpl browserAPIImpl = (BrowserAPIImpl) browserAPI; - final var results1 = browserAPIImpl.getContentUnderParentFromDB(query1, 5); - assertEquals("Should return exactly 5 accessible contentlets", 5, results1.contentlets.size()); + final int partialPage = 2; + final var results1 = browserAPIImpl.getContentUnderParentFromDB(query1, partialPage); + assertEquals("Should return exactly " + partialPage + " accessible contentlets", + partialPage, results1.contentlets.size()); assertTrue("Should indicate more pages available", results1.hasMore); - // Test Case 2: Request page size of 8 - should get exactly 8 accessible items - final var results2 = browserAPIImpl.getContentUnderParentFromDB(query1, 8); - assertEquals("Should return exactly 8 accessible contentlets", 8, results2.contentlets.size()); + // Test Case 2: Request one short of everything - should still fill the page + final var results2 = browserAPIImpl.getContentUnderParentFromDB(query1, accessibleCount - 1); + assertEquals("Should return exactly " + (accessibleCount - 1) + " accessible contentlets", + accessibleCount - 1, results2.contentlets.size()); assertTrue("Should indicate more pages available", results2.hasMore); - // Test Case 3: Request page size of 10 - should get all 10 accessible items - final var results3 = browserAPIImpl.getContentUnderParentFromDB(query1, 10); - assertEquals("Should return exactly 10 accessible contentlets", 10, results3.contentlets.size()); + // Test Case 3: Request exactly what is available - should get all, with no more + final var results3 = browserAPIImpl.getContentUnderParentFromDB(query1, accessibleCount); + assertEquals("Should return exactly " + accessibleCount + " accessible contentlets", + accessibleCount, results3.contentlets.size()); assertFalse("Should indicate no more pages available", results3.hasMore); - // Test Case 4: Request more than available - should get all 10 accessible items - final var results4 = browserAPIImpl.getContentUnderParentFromDB(query1, 15); - assertEquals("Should return all 10 accessible contentlets", 10, results4.contentlets.size()); + // Test Case 4: Request more than available - should get all accessible items + final var results4 = browserAPIImpl.getContentUnderParentFromDB(query1, accessibleCount + 3); + assertEquals("Should return all " + accessibleCount + " accessible contentlets", + accessibleCount, results4.contentlets.size()); assertFalse("Should indicate no more pages available", results4.hasMore); - // Test Case 5: Cursor-based pagination - // first page returns 5 items and a cursor, - // second page continues from that cursor and returns the remaining 5 items. - final var results5 = browserAPIImpl.getContentUnderParentFromDB(query1, 5); - assertEquals("Should return exactly 5 accessible contentlets on page 1", 5, results5.contentlets.size()); + // Test Case 5: Cursor-based pagination - the first page returns a cursor, and the + // second page continues from it and returns whatever is left. + final int firstPage = 3; + final int secondPage = accessibleCount - firstPage; + final var results5 = browserAPIImpl.getContentUnderParentFromDB(query1, firstPage); + assertEquals("Should return exactly " + firstPage + " accessible contentlets on page 1", + firstPage, results5.contentlets.size()); assertTrue("Should indicate more pages available after page 1", results5.hasMore); // Build query2 from query1, advancing only the contentCursor final BrowserQuery query2 = BrowserQuery.from(query1) .contentCursor(results5.nextDbCursor) .build(); - final var results6 = browserAPIImpl.getContentUnderParentFromDB(query2, 8); - assertEquals("Should return remaining 5 accessible contentlets on page 2", 5, results6.contentlets.size()); + final var results6 = browserAPIImpl.getContentUnderParentFromDB(query2, accessibleCount); + assertEquals("Should return the remaining " + secondPage + " accessible contentlets on page 2", + secondPage, results6.contentlets.size()); assertFalse("Should indicate no more pages available after page 2", results6.hasMore); } @@ -2075,8 +2108,8 @@ public void test_getPaginatedContents_foldersExactlyFillPage_hasMoreContentIsTru /** *
    *
  • Method to Test: {@link BrowserAPI#getPaginatedContents(BrowserQuery)}
    • - *
  • Given Scenario: A site contains 20 content items but - * {@code BROWSER_DB_MAX_SCAN_ROWS} is intentionally set to 15, lower than the total row + *
  • Given Scenario: A site contains more content items than + * {@code BROWSER_DB_MAX_SCAN_ROWS}, which is intentionally set below the total row * count. The scan loop must stop as soon as the number of rows scanned exceeds the limit, * preventing runaway queries on large sites with heavily restricted users.
    • *
  • Expected Result: The request completes without hanging or throwing an @@ -2086,8 +2119,10 @@ public void test_getPaginatedContents_foldersExactlyFillPage_hasMoreContentIsTru */ @Test public void test_getPaginatedContents_scanLimitStopsLoop() throws Exception { - final int scanLimit = 15; - final int itemCount = 20; + // Only the relationship matters: itemCount must exceed scanLimit so the loop + // trips the limit. Each item costs a persist + index, so keep both small. + final int scanLimit = 3; + final int itemCount = 5; Config.setProperty(BrowserAPIImpl.BROWSER_DB_MAX_SCAN_ROWS_KEY, scanLimit); try { @@ -2114,7 +2149,7 @@ public void test_getPaginatedContents_scanLimitStopsLoop() throws Exception { final PaginatedContents result = browserAPI.getPaginatedContents(query); assertNotNull("Result must not be null when scan limit is reached", result); - // 20 items were accumulated before the scan limit fired (dbOffset 20 >= scanLimit 15) + // Items accumulated before the scan limit fired (dbOffset >= scanLimit) assertTrue("Should have returned items accumulated before the scan limit", result.contentCount > 0); // Cursor must reflect how far into the DB the scan reached diff --git a/dotcms-integration/src/test/java/com/dotcms/business/LocalTransactionAndCloseDBIfOpenedFactoryTest.java b/dotcms-integration/src/test/java/com/dotcms/business/LocalTransactionAndCloseDBIfOpenedFactoryTest.java index af9d22417f64..f6c389614cc3 100644 --- a/dotcms-integration/src/test/java/com/dotcms/business/LocalTransactionAndCloseDBIfOpenedFactoryTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/business/LocalTransactionAndCloseDBIfOpenedFactoryTest.java @@ -95,7 +95,7 @@ public void testUpdateSelectTransaction() throws Exception { try { hibernateUtil.list(); } catch (Exception e) { - Assert.fail("Hibernate wired connection still works"); + throw new AssertionError("Hibernate wired connection still works", e); } } ); @@ -168,7 +168,7 @@ public void testSelectUpdateTransaction() throws Exception { try { hibernateUtil.list(); } catch (Exception e) { - Assert.fail("Hibernate wired connection still works"); + throw new AssertionError("Hibernate wired connection still works", e); } } ); @@ -245,7 +245,7 @@ public void testSingleSelectUpdateTransaction() throws Exception { try { hibernateUtil.list(); } catch (Exception e) { - Assert.fail("Hibernate wired connection still works"); + throw new AssertionError("Hibernate wired connection still works", e); } Assert.assertTrue(DbConnectionFactory.inTransaction()); diff --git a/dotcms-integration/src/test/java/com/dotcms/contenttype/business/StoryBlockAPITest.java b/dotcms-integration/src/test/java/com/dotcms/contenttype/business/StoryBlockAPITest.java index 461b6cd4ac5a..ab7c9c203f8c 100644 --- a/dotcms-integration/src/test/java/com/dotcms/contenttype/business/StoryBlockAPITest.java +++ b/dotcms-integration/src/test/java/com/dotcms/contenttype/business/StoryBlockAPITest.java @@ -306,7 +306,7 @@ public void test_refresh_references_on_self_reference() throws DotDataException, final StoryBlockReferenceResult refreshResult = APILocator.getStoryBlockAPI() .refreshStoryBlockValueReferences(JSON_SELF_REFERENCE, "3d3a99c4-9b94-4840-8390-704fb6d1d998"); } catch (Throwable e) { - Assert.fail("Should not throw any exception"); + throw new AssertionError("Should not throw any exception", e); } } diff --git a/dotcms-integration/src/test/java/com/dotcms/contenttype/test/ContentTypeAPIImplTest.java b/dotcms-integration/src/test/java/com/dotcms/contenttype/test/ContentTypeAPIImplTest.java index 943f266a42a8..a2a8e3359ecd 100644 --- a/dotcms-integration/src/test/java/com/dotcms/contenttype/test/ContentTypeAPIImplTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/contenttype/test/ContentTypeAPIImplTest.java @@ -365,7 +365,7 @@ public void Test_Fields_without_contenttype_on_saving() throws Exception { } } catch (Exception e) { - fail("Should work"); + throw new AssertionError("Should work", e); } finally { ctApi.delete(movie); diff --git a/dotcms-integration/src/test/java/com/dotcms/contenttype/test/ContentTypeTest.java b/dotcms-integration/src/test/java/com/dotcms/contenttype/test/ContentTypeTest.java index c4a6d7fb7596..708785287ea1 100644 --- a/dotcms-integration/src/test/java/com/dotcms/contenttype/test/ContentTypeTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/contenttype/test/ContentTypeTest.java @@ -42,7 +42,7 @@ public void test_content_type_living_in_system_host() throws Exception { .nextPersisted(); try { type.folderPath(); } catch (Exception e) { - fail("This should not throw exception"); + throw new AssertionError("This should not throw exception", e); } } diff --git a/dotcms-integration/src/test/java/com/dotcms/enterprise/publishing/remote/bundler/RuleBundlerTest.java b/dotcms-integration/src/test/java/com/dotcms/enterprise/publishing/remote/bundler/RuleBundlerTest.java index 1b9b947871ee..9a2e0adcd6de 100644 --- a/dotcms-integration/src/test/java/com/dotcms/enterprise/publishing/remote/bundler/RuleBundlerTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/enterprise/publishing/remote/bundler/RuleBundlerTest.java @@ -48,17 +48,34 @@ public static void prepare() throws Exception { public static Object[] rules() throws Exception { prepare(); - final Rule rule = new RuleDataGen().nextPersisted(); + // Deliberately creates no persisted data. A @DataProvider is evaluated when + // the suite is CONSTRUCTED - MainBaseSuite builds every runner up front - so + // anything persisted here exists long before this test runs, and the test + // classes scheduled in between can destroy it. That is exactly how this test + // used to fail ("rule is null") once it was scheduled late in a suite. + // Fixtures are created inside the test instead; keep this method stateless. + return new TestCase[]{ + new TestCase(false), + new TestCase(true) + }; + } + + /** + * Creates the Rule under test immediately before it is used, so the fixture + * cannot be invalidated by unrelated tests running earlier in the suite. + * + * @param attachedToPage whether the Rule should be bound to an HTML page + * @return a freshly persisted Rule + */ + private Rule createRule(final boolean attachedToPage) { + if (!attachedToPage) { + return new RuleDataGen().nextPersisted(); + } final Host host = new SiteDataGen().nextPersisted(); final Template template = new TemplateDataGen().host(host).nextPersisted(); final HTMLPageAsset htmlPageAsset = new HTMLPageDataGen(host, template).nextPersisted(); - final Rule ruleWithPage = new RuleDataGen().page(htmlPageAsset).nextPersisted(); - - return new TestCase[]{ - new TestCase(rule), - new TestCase(ruleWithPage) - }; + return new RuleDataGen().page(htmlPageAsset).nextPersisted(); } @@ -73,7 +90,7 @@ public static Object[] rules() throws Exception { public void addRuleInBundle(final TestCase testCase) throws DotBundleException, IOException, DotSecurityException, DotDataException { - final Rule rule = testCase.rule; + final Rule rule = createRule(testCase.attachedToPage); final BundlerStatus status = mock(BundlerStatus.class); final RuleBundler bundler = new RuleBundler(); @@ -104,16 +121,24 @@ public void addRuleInBundle(final TestCase testCase) } private static class TestCase{ - Rule rule; - String expectedFilePath; + final boolean attachedToPage; + final String expectedFilePath; - public TestCase(final Rule rule, final String expectedFilePath) { - this.rule = rule; + public TestCase(final boolean attachedToPage, final String expectedFilePath) { + this.attachedToPage = attachedToPage; this.expectedFilePath = expectedFilePath; } - public TestCase(final Rule rule) { - this(rule, "/bundlers-test/rule/rule.rule.xml"); + public TestCase(final boolean attachedToPage) { + this(attachedToPage, "/bundlers-test/rule/rule.rule.xml"); + } + + // Without this the parameterised test reports as + // "addRuleInBundle[0: RuleBundlerTest$TestCase@5c2004ac]", which says nothing + // about which case failed. + @Override + public String toString() { + return attachedToPage ? "rule attached to a page" : "standalone rule"; } } } diff --git a/dotcms-integration/src/test/java/com/dotcms/publisher/endpoint/bean/PublishingEndPointTest.java b/dotcms-integration/src/test/java/com/dotcms/publisher/endpoint/bean/PublishingEndPointTest.java index 2d833775cf43..1b4428c58f8e 100644 --- a/dotcms-integration/src/test/java/com/dotcms/publisher/endpoint/bean/PublishingEndPointTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/publisher/endpoint/bean/PublishingEndPointTest.java @@ -49,57 +49,57 @@ public void validatePublishingEndPoint_whenAWSS3PublishWithoutProperties_returnE Assert.assertTrue(exceptionCatched); } + /** + * A missing bucket ID is reported to the user as a system message, not raised to + * the caller, so validation must return normally. + *

    + * This previously swallowed the exception and asserted {@code false} with the + * message "No Exception should be thrown", which reported nothing about what + * actually failed - four retries produced four identical, useless messages. Let + * the exception surface instead. + */ @Test - public void validatePublishingEndPoint_whenAWSS3PublishWithoutBucketID_returnException() { - - boolean exceptionCatched = false; + public void validatePublishingEndPoint_whenAWSS3PublishWithoutBucketID_returnsWithoutThrowing() + throws PublishingEndPointValidationException { final String noBucketID = "Key=Value"; PublishingEndPoint endPoint = factory.getPublishingEndPoint(AWSS3Publisher.PROTOCOL_AWS_S3); endPoint.setAuthKey(new StringBuilder(PublicEncryptionFactory.encryptString(noBucketID))); - try { - endPoint.validatePublishingEndPoint(); - } catch (Exception e) { - Assert.assertTrue("No Exception should be thrown", false); - } - - Assert.assertTrue("No Exception should be thrown", true); + endPoint.validatePublishingEndPoint(); } + /** + * Invalid credentials are reported to the user as a system message, not raised to + * the caller, so validation must return normally. See the note on the sibling test + * about why the exception is no longer swallowed. + */ @Test - public void validatePublishingEndPoint_whenAWSS3PublishWithoutValidCredentials_returnException() { - - boolean exceptionCatched = false; + public void validatePublishingEndPoint_whenAWSS3PublishWithoutValidCredentials_returnsWithoutThrowing() + throws PublishingEndPointValidationException { - final String noBucketID = "aws_bucket_name=name\naws_access_key=value\naws_secret_access_key=value"; + final String invalidCredentials = + "aws_bucket_name=name\naws_access_key=value\naws_secret_access_key=value"; PublishingEndPoint endPoint = factory.getPublishingEndPoint(AWSS3Publisher.PROTOCOL_AWS_S3); - endPoint.setAuthKey(new StringBuilder(PublicEncryptionFactory.encryptString(noBucketID))); + endPoint.setAuthKey(new StringBuilder(PublicEncryptionFactory.encryptString(invalidCredentials))); - try { - endPoint.validatePublishingEndPoint(); - } catch (Exception e) { - Assert.assertTrue("No Exception should be thrown", false); - } - - Assert.assertTrue("No Exception should be thrown", true); + endPoint.validatePublishingEndPoint(); } + /** + * Same shape as the AWS S3 cases above: validation succeeds by returning, so the + * exception is allowed to surface rather than being swallowed behind an assertion + * message that hides the cause. + */ @Test - public void validatePublishingEndPoint_whenStaticPublishWithWritePermission_returnOK() { - - boolean exceptionCatched = false; + public void validatePublishingEndPoint_whenStaticPublishWithWritePermission_returnOK() + throws PublishingEndPointValidationException { PublishingEndPoint endPoint = factory.getPublishingEndPoint(StaticPublisher.PROTOCOL_STATIC); - try { - endPoint.validatePublishingEndPoint(); - } catch (Exception e) { - Assert.assertTrue("No Exception should be thrown", false); - } - Assert.assertTrue("No Exception should be thrown", true); + endPoint.validatePublishingEndPoint(); } } diff --git a/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/authentication/ResetPasswordTokenUtilTest.java b/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/authentication/ResetPasswordTokenUtilTest.java index ef398571236b..e19f4ccbbaff 100644 --- a/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/authentication/ResetPasswordTokenUtilTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/authentication/ResetPasswordTokenUtilTest.java @@ -164,7 +164,7 @@ public void test_checkToken_tokenExpired_19_minutes_throwDotInvalidTokenExceptio Assert.assertTrue("token is valid 19 minutes", true); } catch(DotInvalidTokenException e) { - Assert.assertTrue("token should be valid for 19 minutes", false); + throw new AssertionError("token should be valid for 19 minutes", e); } } diff --git a/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/folder/FolderResourceTest.java b/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/folder/FolderResourceTest.java index a916695d2a8b..53089d20536e 100644 --- a/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/folder/FolderResourceTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/folder/FolderResourceTest.java @@ -344,59 +344,69 @@ public void test_findSubFoldersByPath_defaultLimit40_returnsUpTo40() throws DotD /** * Method to test: findSubFoldersByPath in the FolderResource - * Given Scenario: Create 30 subfolders; call with limit=10 - * ExpectedResult: 10 results (the parent + 9 subfolders) + * Given Scenario: Create more subfolders than the requested limit + * ExpectedResult: exactly `limit` results (the parent + limit-1 subfolders) */ @Test public void test_findSubFoldersByPath_withCustomLimit_returnsLimitedResults() throws DotDataException, DotSecurityException { + // Unlike the cap tests above, no production constant is pinned here - all this + // needs is more subfolders than the limit, so keep both small. + final int limit = 5; + final int subFolders = limit * 2; + final Host site = new SiteDataGen().nextPersisted(); final Folder parent = new FolderDataGen().site(site).name("parent").nextPersisted(); - for (int i = 0; i < 30; i++) { + for (int i = 0; i < subFolders; i++) { new FolderDataGen().parent(parent).name(String.format("subfolder%02d", i)).nextPersisted(); } final String path = String.format("//%s/%s/", site.getHostname(), parent.getName()); final Response res = resource.findSubFoldersByPath( getHttpRequest(adminUser.getEmailAddress(), "admin"), response, - new SearchByPathForm(path), 0, 10); + new SearchByPathForm(path), 0, limit); Assert.assertEquals(Status.OK.getStatusCode(), res.getStatus()); final ResponseEntityView entity = ResponseEntityView.class.cast(res.getEntity()); final List results = (List) entity.getEntity(); - Assert.assertEquals("Expected 10 results with limit=10", 10, results.size()); + Assert.assertEquals("Expected " + limit + " results with limit=" + limit, limit, results.size()); } /** * Method to test: findSubFoldersByPath in the FolderResource - * Given Scenario: Create 30 named subfolders; call with offset=10, limit=10 - * ExpectedResult: 10 results that do NOT include the first 10 items + * Given Scenario: Create enough named subfolders for two full pages; page through them + * ExpectedResult: a second page of `limit` results that does NOT overlap the first */ @Test public void test_findSubFoldersByPath_withOffset_returnsCorrectPage() throws DotDataException, DotSecurityException { + // Only needs enough subfolders to fill two pages; no production constant is + // pinned here, so keep the page size and the dataset small. + final int limit = 5; + final int subFolders = limit * 2; + final Host site = new SiteDataGen().nextPersisted(); final Folder parent = new FolderDataGen().site(site).name("parent").nextPersisted(); - for (int i = 0; i < 30; i++) { + for (int i = 0; i < subFolders; i++) { new FolderDataGen().parent(parent).name(String.format("subfolder%02d", i)).nextPersisted(); } final String path = String.format("//%s/%s/", site.getHostname(), parent.getName()); - // page 1: first 10 + // page 1 final Response page1Res = resource.findSubFoldersByPath( getHttpRequest(adminUser.getEmailAddress(), "admin"), response, - new SearchByPathForm(path), 0, 10); + new SearchByPathForm(path), 0, limit); final List page1 = (List) ResponseEntityView.class.cast(page1Res.getEntity()).getEntity(); - // page 2: next 10 + // page 2 final Response page2Res = resource.findSubFoldersByPath( getHttpRequest(adminUser.getEmailAddress(), "admin"), response, - new SearchByPathForm(path), 10, 10); + new SearchByPathForm(path), limit, limit); final List page2 = (List) ResponseEntityView.class.cast(page2Res.getEntity()).getEntity(); - Assert.assertEquals(10, page1.size()); - Assert.assertEquals(10, page2.size()); + Assert.assertEquals(limit, page1.size()); + Assert.assertEquals(limit, page2.size()); // No overlap between pages final List page1Paths = page1.stream().map(FolderSearchResultView::getPath).toList(); diff --git a/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/taillog/TailLogResourceTest.java b/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/taillog/TailLogResourceTest.java index 134b801dafc6..b319644ee94c 100644 --- a/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/taillog/TailLogResourceTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/rest/api/v1/taillog/TailLogResourceTest.java @@ -118,10 +118,10 @@ public void write(final OutboundEvent outboundEvent) throws IOException { // meaning that at least once we will see the keepAlive event which is sent every 20 seconds Thread.sleep(TimeUnit.SECONDS.toMillis(3)); } catch (IOException e) { - fail("Error writing to file"); + throw new AssertionError("Error writing to file", e); } catch (InterruptedException e) { Thread.currentThread().interrupt(); - fail("Error attempting to sleep thread"); + throw new AssertionError("Error attempting to sleep thread", e); } })); } @@ -131,7 +131,7 @@ public void write(final OutboundEvent outboundEvent) throws IOException { future.get(); } catch (Exception e) { Thread.currentThread().interrupt(); - fail("Error writing to file"); + throw new AssertionError("Error writing to file", e); } } diff --git a/dotcms-integration/src/test/java/com/dotcms/storage/Chainable404StorageCacheTest.java b/dotcms-integration/src/test/java/com/dotcms/storage/Chainable404StorageCacheTest.java index c6b5ef3f9c1d..9e8b5252a6f5 100644 --- a/dotcms-integration/src/test/java/com/dotcms/storage/Chainable404StorageCacheTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/storage/Chainable404StorageCacheTest.java @@ -76,7 +76,7 @@ public void Test_Cache_null_put404() { try { cache.put404(null, null); } catch (Exception e) { - Assert.fail("Should not throw exception put404 null null"); + throw new AssertionError("Should not throw exception put404 null null", e); } } diff --git a/dotcms-integration/src/test/java/com/dotcms/util/ImportUtilTest.java b/dotcms-integration/src/test/java/com/dotcms/util/ImportUtilTest.java index 7c23ad95ad16..b460f6a0d39a 100644 --- a/dotcms-integration/src/test/java/com/dotcms/util/ImportUtilTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/util/ImportUtilTest.java @@ -123,6 +123,9 @@ import java.util.Map; import java.util.Set; import java.util.Optional; +import java.util.concurrent.TimeUnit; + +import static org.awaitility.Awaitility.await; import java.util.stream.Collectors; import static com.dotcms.util.CollectionsUtils.list; @@ -517,19 +520,17 @@ public void importFile() //------------------------------------------------------------------------ //Making sure the contentlets are in the indexes - List contentletSearchResults; - int x = 0; - do { - Thread.sleep(30000); - //Verify if it was added to the index - contentletSearchResults = contentletAPI.searchIndex( - "+structureName:" + contentType.getVelocityVarName() - + " +working:true +deleted:false +" + contentType - .getVelocityVarName() + ".title:Test1 +languageId:1", 0, -1, null, - user, true); - x++; - } while ((contentletSearchResults == null || contentletSearchResults.isEmpty()) - && x < 100); + final String indexedTitleQuery = + "+structureName:" + contentType.getVelocityVarName() + + " +working:true +deleted:false +" + contentType + .getVelocityVarName() + ".title:Test1 +languageId:1"; + await().atMost(30, TimeUnit.SECONDS) + .pollInterval(200, TimeUnit.MILLISECONDS) + .until(() -> { + final List indexed = contentletAPI.searchIndex( + indexedTitleQuery, 0, -1, null, user, true); + return indexed != null && !indexed.isEmpty(); + }); //Create the csv file to import reader = createTempFile(textFieldVarName + ", " + siteFieldVarName + "\r\n" + @@ -1458,8 +1459,8 @@ public void importFile_success_when_importLinesUpdateExistingContent() APILocator.getContentletIndexAPI().addContentToIndex(savedData); - //Update ContentType - Thread.sleep(1000); + //Update ContentType - wait for the content just pushed to be searchable + awaitIndexed(savedData); tempFile = "Identifier," + TITLE_FIELD_NAME + ", " + BODY_FIELD_NAME + ", " + Contentlet.WORKFLOW_ACTION_KEY + "\r\n" + @@ -1482,7 +1483,13 @@ public void importFile_success_when_importLinesUpdateExistingContent() //Validations validate(results, false, false, false); - Thread.sleep(2000); + // The import runs asynchronously; wait for all three contentlets to land + // rather than assuming a fixed delay is long enough. + final String importedTypeInode = contentType.inode(); + await().atMost(30, TimeUnit.SECONDS) + .pollInterval(200, TimeUnit.MILLISECONDS) + .until(() -> contentletAPI + .findByStructure(importedTypeInode, user, false, 0, 0).size() == 3); savedData = contentletAPI .findByStructure(contentType.inode(), user, false, 0, 0); @@ -5725,7 +5732,7 @@ public void importFile_tagFieldUpdate_replacesExistingTags() c.setIndexPolicy(IndexPolicy.FORCE); } APILocator.getContentletIndexAPI().addContentToIndex(afterFirst); - Thread.sleep(1000); + awaitIndexed(afterFirst); final String secondCsv = "title,hostFolder,tags\r\n" + contentTitle + "," + defaultSite.getIdentifier() + ",\"" @@ -5796,6 +5803,37 @@ private void cleanUpContentType(final ContentType contentType) { } } + /** + * Waits until every given contentlet is retrievable from the index. + *

    + * {@code addContentToIndex} returns before Elasticsearch has refreshed, so the + * content is not immediately searchable. This replaces a fixed sleep: it + * returns as soon as the index has caught up instead of always paying the + * worst case, and fails with a clear timeout if it never does. + * + * @param contentlets the contentlets that were just pushed to the index + */ + private void awaitIndexed(final List contentlets) { + if (contentlets == null || contentlets.isEmpty()) { + return; + } + final ContentletAPI contentletAPI = APILocator.getContentletAPI(); + final List inodes = contentlets.stream().map(Contentlet::getInode) + .collect(Collectors.toList()); + await().atMost(30, TimeUnit.SECONDS) + .pollInterval(200, TimeUnit.MILLISECONDS) + .until(() -> { + for (final String inode : inodes) { + final List found = contentletAPI.searchIndex( + "+inode:" + inode, 0, -1, null, user, false); + if (found == null || found.isEmpty()) { + return false; + } + } + return true; + }); + } + private void cleanUpTagsByName(final TagAPI tagAPI, final String tagName) { try { for (final Tag t : tagAPI.getTagsByName(tagName)) { diff --git a/dotcms-integration/src/test/java/com/dotmarketing/filters/FiltersTest.java b/dotcms-integration/src/test/java/com/dotmarketing/filters/FiltersTest.java index a8873568e93a..f54e5b443e0d 100644 --- a/dotcms-integration/src/test/java/com/dotmarketing/filters/FiltersTest.java +++ b/dotcms-integration/src/test/java/com/dotmarketing/filters/FiltersTest.java @@ -707,7 +707,7 @@ public void shouldRedirectToFolderIndex() throws Exception { response.getRedirectLocation()); assertEquals(301, response.getStatus()); } catch (ServletException e) { - Assert.fail(); + throw new AssertionError("Unexpected exception", e); } Logger.info(this.getClass(), diff --git a/dotcms-integration/src/test/java/com/dotmarketing/startup/runonce/Task04335CreateSystemWorkflowTest.java b/dotcms-integration/src/test/java/com/dotmarketing/startup/runonce/Task04335CreateSystemWorkflowTest.java index 4d4a9ae70f9f..0f853de17aab 100644 --- a/dotcms-integration/src/test/java/com/dotmarketing/startup/runonce/Task04335CreateSystemWorkflowTest.java +++ b/dotcms-integration/src/test/java/com/dotmarketing/startup/runonce/Task04335CreateSystemWorkflowTest.java @@ -50,7 +50,7 @@ public void addPermission_onDiffDataBase_Success() throws DotDataException, DotS .loadResult(); } catch (Exception e) { - Assert.fail("Could not insert on the db: " + dbType + ", a permission"); + throw new AssertionError("Could not insert on the db: " + dbType + ", a permission", e); } } } diff --git a/dotcms-integration/src/test/java/com/dotmarketing/startup/runonce/Task220413IncreasePublishedPushedAssetIdColTest.java b/dotcms-integration/src/test/java/com/dotmarketing/startup/runonce/Task220413IncreasePublishedPushedAssetIdColTest.java index c65d7f1e2211..c21e51c441d4 100644 --- a/dotcms-integration/src/test/java/com/dotmarketing/startup/runonce/Task220413IncreasePublishedPushedAssetIdColTest.java +++ b/dotcms-integration/src/test/java/com/dotmarketing/startup/runonce/Task220413IncreasePublishedPushedAssetIdColTest.java @@ -61,7 +61,7 @@ public void test_upgradeTask() throws DotDataException, SQLException { try { insertPublishedAsset(ASSET_ID); } catch (DotDataException e) { - Assert.fail(); + throw new AssertionError("Unexpected exception", e); } } finally { final Connection conn = DbConnectionFactory.getConnection(); diff --git a/dotcms-integration/src/test/java/com/dotmarketing/startup/runonce/Task240306MigrateLegacyLanguageVariablesTest.java b/dotcms-integration/src/test/java/com/dotmarketing/startup/runonce/Task240306MigrateLegacyLanguageVariablesTest.java index 6abbfc93b251..1c825c8a6d50 100644 --- a/dotcms-integration/src/test/java/com/dotmarketing/startup/runonce/Task240306MigrateLegacyLanguageVariablesTest.java +++ b/dotcms-integration/src/test/java/com/dotmarketing/startup/runonce/Task240306MigrateLegacyLanguageVariablesTest.java @@ -1,7 +1,6 @@ package com.dotmarketing.startup.runonce; import com.dotcms.contenttype.business.ContentTypeAPI; -import com.dotcms.contenttype.business.ContentTypeAPIImpl; import com.dotcms.contenttype.model.type.ContentType; import com.dotcms.contenttype.model.type.KeyValueContentType; import com.dotcms.languagevariable.business.ImmutableMigrationSummary; @@ -18,7 +17,6 @@ import com.dotmarketing.portlets.languagesmanager.business.LanguageAPI; import com.dotmarketing.portlets.languagesmanager.business.UniqueLanguageDataGen; import com.dotmarketing.portlets.languagesmanager.model.Language; -import com.dotmarketing.util.Config; import com.dotmarketing.util.Logger; import com.google.common.collect.ImmutableList; import org.junit.BeforeClass; @@ -118,33 +116,25 @@ public void testExecuteUpgrade() throws DotDataException { } } - /** - * Given scenario: We simulate the case where the language variable content type is dropped - * Expected result: the upgrade task should run without errors and recreate the language variable content type when missing. We run a basic check to verify the task ran successfully - * @throws DotDataException if an error occurs - * @throws DotSecurityException if a security violation occurs + /* + * testDropThenRecreateLanguageVariableContentType used to live here. It deleted the + * Language Variable content type through ContentTypeAPI and asserted that + * checkContentType() recreated it. + * + * Removed because that delete should not be possible. The Language Variable content + * type underpins all i18n, and deleting it breaks language resolution site-wide - + * see #36958, which marks it `system` so ContentTypeFactoryImpl.dbDelete refuses. + * Once that lands this test cannot work as written, and in the meantime it is a test + * deliberately exercising a destructive path we are trying to close off. + * + * Its remaining assertions (executeUpgrade succeeds, summary present, no failures) + * duplicate testExecuteUpgrade, so nothing else is lost by deleting it outright. + * + * What IS lost is coverage of checkContentType() recreating the type when it is + * genuinely absent - a real situation for legacy installs, which is why the upgrade + * task performs that check at all. #36958 tracks re-covering it by seeding the + * missing state directly rather than by calling the delete API. */ - @Test - public void testDropThenRecreateLanguageVariableContentType() throws DotDataException, DotSecurityException { - final Task240306MigrateLegacyLanguageVariables dataTask = new Task240306MigrateLegacyLanguageVariables(); - assertTrue("This Data Task must always run", dataTask.forceRun()); - try { - removeLanguageVariableContentType(); - final Optional optional = dataTask.checkContentType(); - assertTrue("The Language Variable Content Type must always be present", optional.isPresent()); - assertTrue("The migration summary object should not exist before running the task", - dataTask.getMigrationSummary().isEmpty()); - dataTask.executeUpgrade(); - assertTrue("There must be a migration summary after the task execution", - dataTask.getMigrationSummary().isPresent()); - final ImmutableMigrationSummary summary = dataTask.getMigrationSummary().get(); - assertTrue("There must be at least 5 successfully processed Locales", summary.success().size() >= 5); - assertEquals("There must be no errors", 0, summary.fails().size()); - } finally { - final Optional migrationSummary = dataTask.getMigrationSummary(); - migrationSummary.ifPresent(this::cleanup); - } - } /** *

      @@ -448,23 +438,6 @@ public void testMigrationVariableFailureDoesNotCascadeToSubsequentVariables() successes.isEmpty()); } - /** - * Given scenario: We simulate the case where the language variable content type is dropped - * @throws DotSecurityException if a security violation occurs - * @throws DotDataException if an error occurs - */ - private void removeLanguageVariableContentType() throws DotSecurityException, DotDataException { - final ContentTypeAPI contentTypeAPI = APILocator.getContentTypeAPI( - APILocator.systemUser()); - final ContentType languageVariableCt = contentTypeAPI.find( - LanguageVariableAPI.LANGUAGEVARIABLE_VAR_NAME); - final boolean asyncDelete = Config.getBooleanProperty( - ContentTypeAPIImpl.DELETE_CONTENT_TYPE_ASYNC, true); - Config.setProperty(ContentTypeAPIImpl.DELETE_CONTENT_TYPE_ASYNC, false); - contentTypeAPI.delete(languageVariableCt); - Config.setProperty(ContentTypeAPIImpl.DELETE_CONTENT_TYPE_ASYNC, asyncDelete); - } - /** * Backs up existing files in the messages directory before modifying them. * @return Map of filename to file contents for restoration @@ -625,12 +598,21 @@ private void removeExistingLanguageVariables() throws DotDataException, DotSecur "+contentType:" + languageVariableContentType.variable(), 0, 0, null, APILocator.systemUser(), false); - for (Contentlet contentlet : existingVariables) { + if (!existingVariables.isEmpty()) { + // Batched for the same reason as cleanup() - see destroyQuietly. try { - contentletAPI.destroy(contentlet, APILocator.systemUser(), false); - } catch (Exception e) { - Logger.warn(this, "Failed to delete existing language variable: " + - contentlet.getIdentifier(), e); + contentletAPI.destroy(existingVariables, APILocator.systemUser(), false); + } catch (Exception batchFailure) { + Logger.debug(this, "Batch destroy failed, falling back to one at a time: " + + batchFailure.getMessage(), batchFailure); + for (final Contentlet contentlet : existingVariables) { + try { + contentletAPI.destroy(contentlet, APILocator.systemUser(), false); + } catch (Exception e) { + Logger.warn(this, "Failed to delete existing language variable: " + + contentlet.getIdentifier(), e); + } + } } } @@ -681,9 +663,41 @@ private boolean removeLanguageAndContent(final Language language) throws DotData * @param summary the migration summary */ private void cleanup(final ImmutableMigrationSummary summary) { - final ContentletAPI contentletAPI = APILocator.getContentletAPI(); //Clean up required to avoid side effects - summary.success().forEach((language, inodes) -> { + final List inodes = summary.success().values().stream() + .flatMap(List::stream) + .collect(Collectors.toList()); + destroyQuietly(inodes); + } + + /** + * Destroys the given contentlets by inode, batching the work. + *

      + * A migration run produces well over a hundred Language Variables, and both + * {@code find} and {@code destroy} are per-contentlet round trips - + * {@code destroy} is {@code @WrapInTransaction}, so destroying one at a time + * costs one transaction (plus an ES delete and cache invalidation) each. The + * batch overloads collapse that into a single query and a single transaction. + *

      + * Teardown must never leave content behind for the next test, so if the batch + * fails for any reason this falls back to the original per-contentlet loop, + * which tolerates individual failures. + * + * @param inodes inodes of the contentlets to destroy + */ + private void destroyQuietly(final List inodes) { + if (inodes.isEmpty()) { + return; + } + final ContentletAPI contentletAPI = APILocator.getContentletAPI(); + try { + final List contentlets = contentletAPI.findContentlets(inodes); + if (!contentlets.isEmpty()) { + contentletAPI.destroy(contentlets, APILocator.systemUser(), false); + } + } catch (Exception batchFailure) { + Logger.debug(this, "Batch destroy failed, falling back to one at a time: " + + batchFailure.getMessage(), batchFailure); inodes.forEach(inode -> { try { final Contentlet contentlet = contentletAPI.find(inode, APILocator.systemUser(), false); @@ -692,7 +706,7 @@ private void cleanup(final ImmutableMigrationSummary summary) { Logger.debug(this, e.getMessage(), e); } }); - }); + } } /**