fix(folders): restore new-identifier-on-rename behavior while keeping SQL-based child update

[gortiz-dotcms]

Problem

PR #35086 fixed a critical bug (#34655) where renaming a folder containing 20K+ contentlets would fail with:

ERROR: Cannot delete as this path has children
  Where: PL/pgSQL function check_child_assets() line 13 at RAISE

The root cause was that the old implementation discovered children via Elasticsearch — any unindexed content was silently skipped, leaving orphaned DB rows that caused the delete trigger to fire and fail.

PR #35086 fixed this by switching to an in-place rename (no create/delete cycle), which eliminated both the ES dependency and the delete entirely. However, this introduced an unintended behavioral change: the folder identifier (UUID) no longer changes when the folder URL changes.

This breaks the contract established by the deterministic identifier system (DeterministicIdentifierAPIImpl), where a folder's UUID is a SHA-256 hash of assetType:hostname:parentPath:folderName. After an in-place rename, the UUID stays tied to the old path, decoupling identity from URL.

Fix

This PR restores the original create-new/delete-old identity contract while keeping the SQL-based child discovery from PR #35086 — the actual fix for the root cause.

What changes

FolderFactoryImpl.renameFolder() — reverts the in-place mutation back to a create+delete cycle, but replaces the ES-based child discovery with the depth-first bulk SQL UPDATE:

1. folder.setName(newName) so getNewFolderRecord() picks up the new name
2. getNewFolderRecord() creates a new folder record — new inode + new deterministic identifier via IdentifierAPI.createNew() (URL change → identity change restored)
3. clearIdentifierCacheForSubtree(oldPath) — evict stale cache entries before mutation
4. updateChildPaths(oldPath → newPath) — depth-first bulk UPDATE identifier SET parent_path = ? sorted by path length, guaranteeing parent-before-child so the identifier_parent_path_trigger passes at every level without needing deferred constraints
5. Cache evictions (identifier, folder, nav) — unchanged from PR fix: add new approach to rename functionality (#34655) #35086
6. updateOtherFolderReferences(newInode, oldInode) — restored from pre-fix: add new approach to rename functionality (#34655) #35086 code; needed because the inode now changes (permissions, content-type structure references)
7. delete(folder) — safe because all children are moved before this runs, so check_child_assets() finds nothing
8. Mutates folder.setInode() + folder.setIdentifier() on the caller's reference so FolderAPIImpl.refreshContentUnderFolder() targets the correct new folder

Why the delete is now safe

The original bug was caused by ES-based child discovery missing unindexed items. With updateChildPaths(), child discovery is a direct SQL query against the identifier table — no ES dependency, no items skipped. Every child parent_path is updated before delete() runs, so check_child_assets() finds zero children and the trigger allows the delete.

Why no deferred constraints are needed

Depth-first ordering (sort by ascending path length = parent before child) ensures that by the time the identifier_parent_path_trigger validates each row's new parent_path, the parent folder row was already updated in the previous iteration. No DEFERRABLE constraint changes required.

Identifier semantics

	Old (broken, pre-#35086)	PR #35086 (in-place)	This PR
URL change → new UUID	Yes (via ES, unreliable)	No	Yes (via SQL, reliable)
Child discovery	Elasticsearch	Direct SQL	Direct SQL
20K+ item performance	20K round-trips	1 UPDATE/depth level	1 UPDATE/depth level
Constraint violation risk	Yes (unindexed items)	None (no delete)	None (children moved first)

Tests updated

• renameFolder() — captures old identifier before rename; asserts old identifier is deleted, sub-folder identifiers are preserved (only parent_path updated), renamed folder has new UUID
• renameFolder_updatesChildrenAndSubChildrenPaths() — asserts old parent identifier is deleted, new identifier exists with correct asset_name, sub-folder identifier is unchanged, all parent_path values are correct at every nesting level

Fixes #34655

This PR fixes: #34655

[claude]

Claude finished @gortiz-dotcms's task in 10m 15s —— View job

---

Claude finished @gortiz-dotcms's task in 9m 41s —— View job

---

🔍 dotCMS Backend Review

[🟡 Medium] dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderAPIImpl.java:680-682

When renameFolder() returns false due to a concurrent-write constraint violation (not the pre-checked collision path), the factory restores folder.setName(ident.getAssetName()) — the old name — before returning. At that point folder.getName() equals existingID.getAssetName(), so the error message reads "Could not rename folder 'oldName' to 'oldName'", hiding the intended target name.

if (!folderFactory.renameFolder(folder, folder.getName(), user, respectFrontEndPermissions)) {
    throw new DotDataException("Could not rename folder '" + existingID.getAssetName()
            + "' to '" + folder.getName() + "': a folder with that name already exists.");
}

💡 Capture the intended name before the factory call:

final String targetName = folder.getName();
if (!folderFactory.renameFolder(folder, targetName, user, respectFrontEndPermissions)) {
    throw new DotDataException("Could not rename folder '" + existingID.getAssetName()
            + "' to '" + targetName + "': a folder with that name already exists.");
}

---

Addressed from previous review
The ContentTypeCache2 eviction concern flagged in the prior two review cycles has been correctly resolved. updateOtherFolderReferences() now:

1. Issues a parameterized UPDATE structure SET folder = ? WHERE folder = ? (no string concatenation in SQL)
2. Queries ContentTypeAPI.search("folder='" + newFolderInode + "'", ...) to identify affected types — the SafeCondition parser in ContentTypeFactoryImpl validates folder against the column whitelist and converts to a parameterized folder = ? clause, so this is safe
3. Evicts those ContentType objects from ContentTypeCache2
4. Evicts permission cache for both old and new inodes via removePermissionableFromCache

No security, database transaction, Java standards, or REST API issues were found in the remaining diff.

---

Next steps

• 🟡 The misleading error message is low-risk (only visible in logs, only in the concurrent-race path) but straightforward to fix
• Every new push triggers a fresh review automatically

[claude]

Rollback Safety Analysis

Verdict: ✅ Safe to Roll Back

Analyzed the diff (2102f1b5...f87893b8) against all rollback-unsafe categories from docs/core/ROLLBACK_UNSAFE_CATEGORIES.md.

Category	Verdict
C-1 Structural Data Model Change	✅ No schema changes
C-2 Elasticsearch Mapping Change	✅ No ES mapping changes
C-3 Content JSON Model Version Bump	✅ No model version changes
C-4 DROP TABLE / DROP Column	✅ No DDL
H-1 One-way data transformation	✅ No runonce tasks or backfills
H-2 RENAME TABLE / RENAME COLUMN	✅ No schema changes
H-3 Primary key restructuring	✅ No PK changes
H-4 New ContentType field type	✅ No new field types
H-5 Storage provider change	✅ No storage changes
H-6 DROP PROCEDURE / DROP FUNCTION	✅ No procedures dropped
H-7 NOT NULL column without default	✅ No schema changes
M-1 Non-broadening column type change	✅ No schema changes
M-2 Push publishing bundle format	✅ No bundle changes
M-3 REST / GraphQL API contract change	✅ No endpoint or response schema changes
M-4 OSGi plugin API breakage	✅ No public interface signature changes

Rationale: All three changed files (FolderAPIImpl.java, FolderFactoryImpl.java, FolderAPITest.java) are pure Java application logic and test changes. There are no database schema migrations, no Elasticsearch mapping changes, no serialization model version bumps, and no REST API contract changes. Rolling back to N-1 leaves the database in a fully readable state — the folder identifier/path data written by N is still structurally valid and N-1 can read and operate on it correctly.

View job run

[claude]

🔍 dotCMS Backend Review

[🟠 High] dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderFactoryImpl.java:~639

ContentTypeCache2 is not evicted after the bulk UPDATE structure SET folder = ?. Content types cached under the old folder inode will continue serving stale folder data until natural LRU expiry. Any code path that reads contentType.folder() to resolve folder membership, security scope, or content-type-to-folder association will see the wrong inode in the window after a rename.

dc.executeUpdate(
    "UPDATE structure SET folder = ? WHERE folder = ?", newFolderInode, oldFolderInode);
// No ContentTypeCache2 eviction follows

💡 Add after the UPDATE structure line in updateOtherFolderReferences():

CacheLocator.getContentTypeCache2().clearCache();

A more targeted approach is to query the affected ContentType IDs before the UPDATE and call cache.remove(type) for each, but clearCache() is the safe low-risk option since it is a read-through cache. Fix this →

---

Next steps

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

[claude]

Rollback Safety Analysis (Updated)

Verdict: ✅ Safe to Roll Back

Analyzed the updated diff (2102f1b5...a558e286) against all rollback-unsafe categories from docs/core/ROLLBACK_UNSAFE_CATEGORIES.md.

Category	Verdict
C-1 Structural Data Model Change	✅ No schema changes
C-2 Elasticsearch Mapping Change	✅ No ES mapping changes
C-3 Content JSON Model Version Bump	✅ No model version changes
C-4 DROP TABLE / DROP Column	✅ No DDL
H-1 One-way data transformation	✅ No runonce tasks or backfills
H-2 RENAME TABLE / RENAME COLUMN	✅ No schema changes
H-3 Primary key restructuring	✅ No PK changes
H-4 New ContentType field type	✅ No new field types
H-5 Storage provider change	✅ No storage changes
H-6 DROP PROCEDURE / DROP FUNCTION	✅ No procedures dropped
H-7 NOT NULL column without default	✅ No schema changes
M-1 Non-broadening column type change	✅ No schema changes
M-2 Push publishing bundle format	✅ No bundle changes
M-3 REST / GraphQL API contract change	✅ No endpoint or response schema changes
M-4 OSGi plugin API breakage	✅ No public interface signature changes

Rationale: The new commits add permission cache evictions (CacheLocator.getPermissionCache().remove()) and reuse a single DotConnect instance in updateOtherFolderReferences — both are runtime-only changes with no database schema impact. All three changed files are pure Java application logic and test changes with no schema migrations, no ES mapping changes, no serialization model version bumps, and no REST API contract changes. Rolling back to N-1 leaves the database in a fully readable state.

View job run

[claude]

🔍 dotCMS Backend Review

[🟠 High] dotCMS/src/main/java/com/dotmarketing/portlets/folders/business/FolderFactoryImpl.java — updateOtherFolderReferences()

After UPDATE structure SET folder = ? re-points content types from the old folder inode to the new one, no ContentTypeCache2 entry is evicted. Cached ContentType objects continue to report the old folder inode until natural LRU expiry. Any code path that reads contentType.folder() — including permission resolution or ContentTypeAPIImpl.moveToSystemFolder() — will observe stale data in that window.

dc.executeUpdate(
    "UPDATE structure SET folder = ? WHERE folder = ?", newFolderInode, oldFolderInode);
// No ContentTypeCache2 eviction follows
APILocator.getPermissionAPI().removePermissionableFromCache(oldFolderInode);

💡 Add after the UPDATE structure line:

APILocator.getContentTypeAPI()
    .search("folder='" + newFolderInode + "'", "mod_date", -1, 0)
    .forEach(CacheLocator.getContentTypeCache2()::remove);

If querying after the update is not practical, CacheLocator.getContentTypeCache2().clearCache() is the safe fallback — it is a read-through cache. Fix this →

---

Next steps

• 🟠 Fix locally and push — this needs your judgment
• 🟡 You can ask me to handle mechanical fixes inline: @claude fix ContentTypeCache2 eviction in FolderFactoryImpl.java
• Every new push triggers a fresh review automatically

[claude]

Rollback Safety Analysis (Updated)

Verdict: ✅ Safe to Roll Back

Analyzed the updated diff (2102f1b5...afd5b2db) against all rollback-unsafe categories from docs/core/ROLLBACK_UNSAFE_CATEGORIES.md.

Category	Verdict
C-1 Structural Data Model Change	✅ No schema changes
C-2 Elasticsearch Mapping Change	✅ No ES mapping changes
C-3 Content JSON Model Version Bump	✅ No model version changes
C-4 DROP TABLE / DROP Column	✅ No DDL
H-1 One-way data transformation	✅ No runonce tasks; rename behavior change only
H-2 RENAME TABLE / RENAME COLUMN	✅ No schema changes
H-3 Primary key restructuring	✅ No PK changes
H-4 New ContentType field type	✅ No new field types
H-5 Storage provider change	✅ No storage changes
H-6 DROP PROCEDURE / DROP FUNCTION	✅ No procedures dropped
H-7 NOT NULL column without default	✅ No schema changes
M-1 Non-broadening column type change	✅ No schema changes
M-2 Push publishing bundle format	✅ No bundle changes
M-3 REST / GraphQL API contract change	✅ No endpoint or response schema changes
M-4 OSGi plugin API breakage	✅ No public interface signature changes

Rationale: All three changed files (FolderAPIImpl.java, FolderFactoryImpl.java, FolderAPITest.java) are pure Java application logic and test changes. The most significant behavioral change — restoring the create-new-identifier + delete-old-identifier cycle for folder renames — produces the same structural DB state (folder exists with an identifier, children have correct parent_path values) that N-1 can read and operate on correctly. There are no database schema migrations, no Elasticsearch mapping changes, no serialization model version bumps, and no REST API contract changes. Rolling back to N-1 leaves the database in a fully readable state.

View job run