Skip to content

maintenance portlet | Implement search/replace, drop old versions, and delete pushed assets APIs #35191#35231

Open
hassandotcms wants to merge 9 commits intomainfrom
35199-maintenance-search-replace-old-versions-pushed-assets
Open

maintenance portlet | Implement search/replace, drop old versions, and delete pushed assets APIs #35191#35231
hassandotcms wants to merge 9 commits intomainfrom
35199-maintenance-search-replace-old-versions-pushed-assets

Conversation

@hassandotcms
Copy link
Copy Markdown
Member

Summary

Add 3 new REST endpoints to MaintenanceResource, replacing legacy DWR/Struts maintenance tools with modern REST APIs for the Maintenance portlet Tools tab.

New endpoints:

  • POST /api/v1/maintenance/_searchAndReplace — Database-wide find/replace across text content in contentlets, containers, templates, fields, and links. Only affects working/live versions. Flushes all caches after completion.
  • DELETE /api/v1/maintenance/_oldVersions?date=yyyy-MM-dd — Deletes all versions of versionable objects (contentlets, containers, templates, links, workflow history) older than the specified date. Uses ISO date format instead of legacy MM/dd/yyyy.
  • DELETE /api/v1/maintenance/_pushedAssets — Clears all push publishing history records, making all assets appear as "never pushed" to all endpoints.

Implementation details:

  • SearchAndReplaceForm extends Validated with @JsonCreator, validates searchString is non-empty, allows empty replaceString (delete occurrences)
  • Typed @Value.Immutable views (SearchAndReplaceResultView, DropOldVersionsResultView) with full Swagger/OpenAPI annotations
  • SecurityLogger entries on all destructive operations
  • Reuses existing assertBackendUser() pattern (requireAdmin + requiredPortlet MAINTENANCE)
  • ResponseEntityStringView reused for simple _pushedAssets response

Test plan

  • Integration tests pass: ./mvnw verify -pl :dotcms-integration -Dcoreit.test.skip=false -Dit.test=MaintenanceResourceIntegrationTest
  • POST /_searchAndReplace with valid search/replace strings returns {success: true, hasErrors: false}
  • POST /_searchAndReplace with empty searchString returns 400
  • POST /_searchAndReplace with empty replaceString succeeds (deletes occurrences)
  • DELETE /_oldVersions?date=2000-01-01 returns {deletedCount: N, success: true}
  • DELETE /_oldVersions?date=01/01/2000 returns 400 (wrong format)
  • DELETE /_oldVersions without date param returns 400
  • DELETE /_pushedAssets returns "success"
  • Non-admin users get 403 on all endpoints
  • SecurityLogger entries appear for all operations

Closes #35199, Closes #35201, Closes #35204

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 7, 2026

Claude finished @hassandotcms's task in 2m 18s —— View job


PR Review

  • Read changed files
  • Analyze for bugs, security, design issues
  • Post review

Issues Found

1. success and hasErrors are redundant and misleading — MaintenanceResource.java:583-587

return new ResponseEntitySearchAndReplaceResultView(
    SearchAndReplaceResultView.builder()
        .success(!hasErrors)
        .hasErrors(hasErrors)
        .build());

These two fields always carry the same information (success = !hasErrors). Worse, when some tables fail, the response returns HTTP 200 with success: false. A client checking success will think the entire operation failed, not that it partially succeeded. The Swagger description even acknowledges partial failure ("Returns 200 with hasErrors=true if some tables failed") but success=false contradicts a 200. Either drop success and let clients read hasErrors, or use a named field like partialFailure.

Fix this →


2. deleteOldAssetVersions partial commits are silently swallowed — MaintenanceResource.java:655-661

CMSMaintenanceFactory.deleteOldAssetVersions iterates in 30-day chunks, each committed independently. If chunk N fails, it returns -1, but chunks 1 through N-1 are already committed. The resource maps -1 to a 500 with a message implying total failure ("Failed to delete old asset versions before [date]"). The caller has no way to know that, say, 3000 records were actually deleted before the error. This can lead to confusion during incident investigation.

Consider logging the partial count before throwing, or surfacing it in the error response.


3. No upper-bound guard on date parameter — MaintenanceResource.java:634-646

Any valid ISO date including far-future dates (e.g., 2099-01-01) is accepted and will delete all existing versions. Given this is an irreversible destructive operation, a sanity check that the date is in the past (or at least not beyond "today") would prevent accidental wipe-outs. A floor-date guard (e.g., refuse dates less than some minimum like 1990-01-01) would also prevent obvious mistakes.

Fix this →


4. MaintenanceUtil.DBSearchAndReplace logs "Returning" but doesn't return — MaintenanceUtil.java:451-456

if(!UtilMethods.isSet(textToSearchFor)){
    Logger.info(MaintenanceUtil.class, "Returning because text to search for is null or empty");
    // ← NO RETURN STATEMENT
}
if(textToReplaceWith == null){
    Logger.info(MaintenanceUtil.class, "Returning because text to replace is null");
    // ← NO RETURN STATEMENT
}

This is a pre-existing bug (not introduced by this PR), but it's worth noting because the new REST layer is the only safety net. If replaceString: null somehow bypassed the form's @NotNull check (e.g., called internally), the SQL would be executed with null parameters, causing a DB error that surfaces only as hasErrors: true. Defensive return statements here would isolate the blast radius.


5. @JSONP annotation missing on all three new endpoints

Every existing endpoint in this class has @JSONP. The three new endpoints don't. If this is intentional (JSONP deprecated for new endpoints), it should be documented. If not, it's an omission.


6. _pushedAssets returns no count

deletePushedAssets returns the string "success" with no indication of how many records were deleted. This is operationally blind — admins have no way to tell if the call did nothing (empty table) vs. deleted thousands of records. A count in the response would cost nothing and would be useful for audit purposes. The other two endpoints return typed views with counts/status — this one is the odd one out.


7. Missing test for null replaceString

test_searchAndReplace_nullSearchString_throwsValidation exists but there is no equivalent for replaceString. Since replaceString has @NotNull (not just empty-allowed), a test verifying that new SearchAndReplaceForm("search", null) throws ValidationException is missing.


Summary: The main design concern is #1 (redundant/misleading success/hasErrors pair returning HTTP 200). Issues #2 and #3 are safety/correctness gaps on the dropOldVersions endpoint. Issues #4–7 are lower severity. The auth, SecurityLogger placement, and input validation structure are solid.

@hassandotcms hassandotcms changed the base branch from 35191-feature-maintenance-tools-rest-apis to main April 7, 2026 13:08
@github-actions github-actions bot added the Area : Backend PR changes Java/Maven backend code label Apr 7, 2026
@hassandotcms hassandotcms force-pushed the 35199-maintenance-search-replace-old-versions-pushed-assets branch from b797ebe to 65d3572 Compare April 7, 2026 14:53
…s, and delete pushed assets #35191

Add three new endpoints to MaintenanceResource:
- POST /_searchAndReplace: database-wide find/replace across content tables
- DELETE /_oldVersions: drop versionable objects older than a given date
- DELETE /_pushedAssets: clear all push publishing history

Includes Validated forms, @Value.Immutable response views, full Swagger
annotations, and SecurityLogger entries for all destructive operations.
…ions, and pushed assets #35191

Tests cover:
- _searchAndReplace: admin success, non-admin rejection, null/empty form validation,
  empty replaceString allowed
- _oldVersions: admin success, non-admin rejection, missing date, invalid date formats
- _pushedAssets: admin success, non-admin rejection
@hassandotcms hassandotcms force-pushed the 35199-maintenance-search-replace-old-versions-pushed-assets branch from 9c70839 to fb599c9 Compare April 16, 2026 15:54

if (deleted < 0) {
throw new DotRuntimeException(
"Failed to delete old asset versions — check server logs for details");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's include the value of dateStr in the message.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Backend PR changes Java/Maven backend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

[TASK] Implement delete pushed assets endpoint [TASK] Implement drop old versions endpoint [TASK] Implement search and replace endpoint

2 participants