fix(multitree): reject save when empty payload would wipe all page content

[dsilvam]

Summary

Adds a net-loss guard in `MultiTreeAPIImpl.overridesMultitreesByPersonalization()` to prevent a stale Edit Mode session from silently deleting all contentlets on a page.

• Guard is off by default — enabled by setting `MULTITREE_EMPTY_SAVE_GUARD_ENABLED=true` in dotmarketing-config.properties.
• When enabled: if the incoming `multiTrees` payload is empty and the page already has existing `multi_tree` rows, throw `DotDataException` with a message instructing the user to refresh and try again.
• No behaviour change when the page is genuinely empty (0 existing rows) — those saves proceed normally.

Root Cause (see #36097)

For TRADITIONAL pages, the editor's `$pageData` signal is frozen at session-open time and never refreshed. A user who opened the page while empty will post an empty `containers` payload after other users have built up content. `overridesMultitreesByPersonalization()` does a page-wide DELETE + reinsert, so an empty post wipes all concurrent work with no warning.

Test Plan

• Set `MULTITREE_EMPTY_SAVE_GUARD_ENABLED=true`
• Open a TRADITIONAL page in Edit Mode while it has no content
• Have a second user add contentlets and save
• From the first session, trigger a save — confirm an error is returned instead of a silent wipe
• Verify pages with genuinely empty content (0 existing rows) still save normally
• Verify guard is inactive when property is not set (default behaviour unchanged)
• Run `MultiTreeAPIImplTest` integration tests

Links

• Fixes Page content silently wiped when saving with a stale edit session (concurrent editing, TRADITIONAL pages) #36097
• FD-36897: https://dotcms.freshdesk.com/a/tickets/36897

🤖 Generated with Claude Code

[claude]

Claude finished @dsilvam's task in 3m 14s —— View job

---

🔍 dotCMS Backend Review

• Gather context and load conventions
• Get Java diff
• Run specialized sub-agent reviews (Security, Database, Java Standards, REST API)
• Consolidate findings
• Post review comment

Review posted at #36098 (comment)

Summary: Java Standards and REST API sub-agents returned NO_FINDINGS. Security and Database reviews surfaced 4 medium-severity findings — all in MultiTreeAPIImpl.java. No Critical or High issues. PR approval was skipped (tool requires elevated permissions).

[claude]

🔍 dotCMS Backend Review

[🔴 Critical] dotCMS/src/main/java/com/dotcms/rest/api/v1/page/PageResource.java:~835

Missing @ApiResponse(responseCode = "409", ...) on the addContent method. The method now returns HTTP 409 CONFLICT via the new DotDataException catch block, but the @ApiResponses annotation has no 409 entry. The auto-generated openapi.yaml will omit this response code, breaking the published API contract — the CI verifies the committed yaml matches the build output.

@ApiResponses(value = {
    @ApiResponse(responseCode = "200", ...),
    @ApiResponse(responseCode = "400", ...),
    // ← 409 CONFLICT returned in code but never declared
})

💡 Add @ApiResponse(responseCode = "409", description = "Conflict — page content was modified concurrently; refresh and retry"), then regenerate openapi.yaml with ./mvnw compile -pl :dotcms-core -DskipTests and commit the updated file.

---

[🔴 Critical] dotCMS/src/main/java/com/dotcms/rest/api/v1/page/PageResourceHelper.java:~171

Logger.debug(PageResourceHelper.class, ...) uses the class literal instead of the instance this. All other logger calls in this class use this; using the class literal is inconsistent and bypasses the instance-based dispatch contract.

Logger.debug(PageResourceHelper.class, () -> String.format(
        "Page content save: pageId='%s' user='%s' ...

💡 Change to Logger.debug(this, () -> String.format(...

---

[🔴 Critical] dotCMS/src/main/java/com/dotcms/rest/api/v1/page/PageResourceHelper.java:~220

Logger.warn(PageResourceHelper.class, ...) uses the class literal instead of this. Same issue as above — inconsistent with all surrounding logger calls in this class.

Logger.warn(PageResourceHelper.class, String.format(
        "Empty contentlet payload for page '%s', ...

💡 Change to Logger.warn(this, String.format(...

---

[🔴 Critical] dotCMS/src/main/java/com/dotmarketing/factories/MultiTreeAPIImpl.java:~726

Logger.warn(MultiTreeAPIImpl.class, ...) uses the class literal instead of this. The existing Logger.info(this, ...) call in the same method demonstrates the expected style — this must be used for bean-instantiated classes.

Logger.warn(MultiTreeAPIImpl.class, String.format(
        "Empty save payload would wipe %d existing contentlet(s) ...

💡 Change to Logger.warn(this, String.format(...

---

[🟠 High] dotCMS/src/main/java/com/dotcms/rest/api/v1/page/PageResource.java:~887

The catch (DotDataException e) block is overly broad. DotDataException is the base class for all data-layer errors — DB constraint violations, connection timeouts, serialization errors. Any such failure inside saveContent() is absorbed here and returned as HTTP 409 CONFLICT with a misleading "modified by another user" message, masking the true cause from callers and operators.

} catch (DotDataException e) {
    return ExceptionMapperUtil.createResponse(
        new DotDataException("Page content may have been modified by another user ..."),
        Response.Status.CONFLICT);
}

💡 Introduce a dedicated StalePageSaveException extends DotDataException thrown only by the guard, and catch it narrowly as 409. Let unrelated DotDataException subtypes fall through to the outer handler.

---

[🟠 High] dotCMS/src/main/java/com/dotmarketing/factories/MultiTreeAPIImpl.java:~732

The DotDataException thrown by the guard embeds existing.size() (internal row count) and the raw pageId in its message. Although the current call path wraps this in a generic message before returning it to the HTTP client, DotDataException is caught broadly in many other call sites. A future caller passing it directly to ExceptionMapperUtil would expose internal data to the HTTP response.

throw new DotDataException(String.format(
        "Save rejected: empty payload would delete %d existing contentlet(s) from page '%s'. ...",
        existing.size(), pageId));

💡 Keep the count and pageId in the server-side Logger.warn (already present). Throw a generic message: "Save rejected: empty payload would wipe existing page content. The page may have been modified by another user — please refresh and try again."

---

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/factories/MultiTreeAPIImpl.java:~721

The guard SELECT (getOriginalContentlets) runs unconditionally on every empty-save call, even when MULTITREE_EMPTY_SAVE_GUARD_ENABLED=false (the default). The feature flag is checked after the DB query. This pays a round-trip per stale-session request with no functional effect when the guard is disabled.

if (multiTrees.isEmpty()) {
    final Set<String> existing = ... // DB SELECT always runs
    if (!existing.isEmpty()) {
        if (Config.getBooleanProperty("MULTITREE_EMPTY_SAVE_GUARD_ENABLED", false)) {  // flag checked after

💡 Check the flag first: if (multiTrees.isEmpty() && Config.getBooleanProperty("MULTITREE_EMPTY_SAVE_GUARD_ENABLED", false)). If the warn-only path is intentional even when the guard is off, document that explicitly and consider whether the DB cost is justified.

---

Next steps

• 🔴 / 🟠 Fix locally and push — these need your judgment
• 🟡 You can ask me to handle mechanical fixes inline: @claude fix <issue description> in <File.java>
• Every new push triggers a fresh review automatically

[claude]

🔍 dotCMS Backend Review

[🔴 Critical] dotCMS/src/main/java/com/dotmarketing/factories/MultiTreeAPIImpl.java:~729

Guard SELECT scope does not match the downstream DELETE scope. When DEFAULT_CONTENT_TO_DEFAULT_LANGUAGE=true and the requested language is not the default, the guard queries only single-language rows via getOriginalContentlets(..., languageIdOpt.get()). But the downstream delete (lines 778–793) calls deleteEntriesByRequestedAndDefaultLanguage, which deletes rows for both the requested language and the default language. Contentlets stored only in the default language are completely invisible to the guard — they are not counted in netLoss, not listed in wipedIds, and not logged. A save that passes the threshold check may still silently wipe an arbitrary number of default-language-only rows, defeating the guard's protective purpose entirely for the most common multilingual configuration.

// Guard — always single-language scope:
final Set<String> existing = languageIdOpt.isPresent()
        ? this.getOriginalContentlets(pageId, ContainerUUID.UUID_DEFAULT_VALUE, personalization, variantId, languageIdOpt.get())
        : this.getOriginalContentlets(pageId, ContainerUUID.UUID_DEFAULT_VALUE, personalization, variantId);
// But downstream delete uses two-language scope when DEFAULT_CONTENT_TO_DEFAULT_LANGUAGE=true

💡 Mirror the downstream branching logic: read DEFAULT_CONTENT_TO_DEFAULT_LANGUAGE before computing existing, and when it is true and the requested language differs from the default, use the two-language overload getOriginalContentlets(pageId, personalization, variantId, languageIdOpt.get(), defaultLanguageId) — identical to what the delete path does.

---

[🔴 Critical] dotCMS/src/main/java/com/dotcms/rest/api/v1/page/PageResource.java:~891

ExceptionMapperUtil.createResponse(Throwable, Status) is used for an intentional user-facing 409. In dev mode this overload appends a stacktrace field to the JSON response, leaking internal class names, method signatures, and line numbers to the HTTP caller. Additionally, the caught StalePageSaveException e is discarded and a brand-new DotDataException is constructed purely to be passed into this overload — wasteful, and the constructed exception carries a misleading stack trace. Use createResponse(String, Status) instead: it always emits only {"message": "..."} with no stack trace path.

return ExceptionMapperUtil.createResponse(
        new DotDataException("Save rejected: net content loss exceeds the configured threshold..."),
        Response.Status.CONFLICT);

💡 Replace with: return ExceptionMapperUtil.createResponse("Save rejected: net content loss exceeds the configured threshold. Please refresh and try again.", Response.Status.CONFLICT);

---

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/factories/MultiTreeAPIImpl.java:~741

When the empty-payload branch fires, the full existing set of contentlet UUIDs is logged at WARN level. WARN logs are typically shipped to centralised log aggregation systems. Logging a page's complete content inventory on every stale-session attempt (which could be triggered deliberately) exposes the full content topology unnecessarily.

Logger.warn(this, String.format(
        "... Contentlets at risk: %s",
        existing.size(), pageId, ..., existing));   // full ID set dumped at WARN

💡 Either downgrade this block to DEBUG (where full IDs are appropriate), or at WARN level replace existing with existing.size() + " IDs" and add a Logger.debug call for the full set.

---

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/factories/MultiTreeAPIImpl.java:~749

Same concern: when the threshold is exceeded, the full incomingIds and wipedIds sets are dumped into the WARN log — exposing the entire content topology of the page at a log level that goes to centralised aggregation.

Logger.warn(this, String.format(
        "... Incoming IDs: %s — Wiped IDs: %s",
        ..., incomingIds, wipedIds));   // full UUID sets at WARN

💡 At WARN level log only counts (incomingIds.size() / wipedIds.size()); emit full sets at DEBUG.

---

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/api/v1/page/PageResource.java:~885

The StalePageSaveException catch is correctly scoped to saveContent(...). However, the catch is physically placed inside the outer try-block alongside other calls (validateContainerEntries, checkPermission) that also declare throws DotDataException. Since StalePageSaveException extends DotDataException, any future code added before the inner try could throw StalePageSaveException and escape the 409 guard, propagating to the JAX-RS container as a 500. There is no active bug today, but the pattern is a maintenance trap.

// Calls before the inner try declare throws DotDataException (which StalePageSaveException extends)
this.validateContainerEntries(pageContainerForm.getContainerEntries());
final List<ContentView> savedContent;
try {
    savedContent = pageResourceHelper.saveContent(...);
} catch (StalePageSaveException e) { ... }  // only covers saveContent()

💡 Add an inline comment: // Intentionally narrow — covers saveContent() only; other DotDataException sources in this block must not be mapped to 409.

---

Next steps

• 🔴 / 🟠 Fix locally and push — these need your judgment
• 🟡 You can ask me to handle mechanical fixes inline: @claude fix <issue description> in <File.java>
• Every new push triggers a fresh review automatically

[claude]

🔍 dotCMS Backend Review

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/factories/MultiTreeAPIImpl.java:761

The full existing set of contentlet UUIDs is dumped into the WARN log. Contentlet identifiers are internal object references that can be used in other API calls (direct contentlet fetch or edit endpoints). WARN-level messages are typically forwarded to centralised log aggregators and retained longer than DEBUG messages, unnecessarily broadening their exposure.

Logger.warn(this, String.format(
        "... Contentlets at risk: %s",
        existing.size(), pageId, ..., existing));  // full UUID set at WARN

💡 Log only the count at WARN level and move the full ID set to DEBUG: Logger.debug(this, () -> "Contentlets at risk: " + existing);

---

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/factories/MultiTreeAPIImpl.java:769

Same concern: both incomingIds and wipedIds (full contentlet UUID sets) are dumped at WARN level on threshold violation. Any deliberate stale-session trigger would cause the full page content topology to be emitted to aggregated logs.

Logger.warn(this, String.format(
        "... Incoming IDs: %s — Wiped IDs: %s",
        ..., incomingIds, wipedIds));  // full UUID sets at WARN

💡 Log only counts at WARN; emit the full sets at DEBUG: Logger.debug(this, () -> String.format("Incoming IDs: %s — Wiped IDs: %s", incomingIds, wipedIds));

---

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/factories/MultiTreeAPIImpl.java:728

When MULTITREE_NET_LOSS_THRESHOLD=-1 (default, guard disabled) and the payload is empty, the guard condition multiTrees.isEmpty() || threshold >= 0 still fires and issues a getOriginalContentlets() SELECT. The query result is used only for the warn-only log path; the threshold check on line 768 (threshold >= 0) is always false. Every empty-payload call at default configuration pays an extra DB round-trip with no functional effect.

// threshold == -1 (disabled), multiTrees is empty → SELECT runs for logging only
if (multiTrees.isEmpty() || threshold >= 0) {
    existing = this.getOriginalContentlets(...); // paid unconditionally
    if (threshold >= 0 && netLoss > threshold)  // never true here

💡 Only issue the SELECT when at least one consumer will use it. Check threshold >= 0 first so the DB round-trip is skipped when the guard is fully disabled and the payload is empty.

---

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/factories/MultiTreeAPIImpl.java:737

APILocator.getLanguageAPI().getDefaultLanguage() is called inside the guard block (line 737) and again inside the downstream delete block (line 799), both within the same transaction. getDefaultLanguage() is annotated @CloseDBIfOpened, so on a cache miss it opens and closes a DB connection twice per request. Hoisting the call to a single variable shared by both blocks would halve the cost on cold-cache paths.

// line 737 — guard block
final long defaultLanguageId = APILocator.getLanguageAPI().getDefaultLanguage().getId();
// line 799 — delete block (same transaction, no shared variable)
final long defaultLanguageId = APILocator.getLanguageAPI().getDefaultLanguage().getId();

💡 Hoist getDefaultLanguage().getId() to a single local variable before the guard block and pass it to both the guard and the delete logic.

---

Next steps

• 🔴 / 🟠 Fix locally and push — these need your judgment
• 🟡 You can ask me to handle mechanical fixes inline: @claude fix <issue description> in <File.java>
• Every new push triggers a fresh review automatically