From e2ecab5ca4e86f3f5ed88eae89cbc54b3f3dd336 Mon Sep 17 00:00:00 2001 From: Jose Castro Date: Tue, 9 Jun 2026 15:57:22 -0600 Subject: [PATCH 1/4] Creating draft PR with potential fix --- .../business/ESContentletAPIImpl.java | 81 ++++++++++--------- .../business/ESMappingAPIImpl.java | 16 ++-- .../dotmarketing/factories/TreeFactory.java | 56 +++++++++++-- 3 files changed, 100 insertions(+), 53 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java index e7b0faf1efe5..f55c63c4619d 100644 --- a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java @@ -4592,38 +4592,35 @@ public void deleteRelatedContent(final Contentlet contentlet, final Relationship + " cannot edit Contentlet with identifier " + contentlet.getIdentifier()); } - List rels = APILocator.getRelationshipAPI() + final List relationshipsFromContent = APILocator.getRelationshipAPI() .byContentType(contentlet.getContentType()); - if (!rels.contains(relationship)) { + if (!relationshipsFromContent.contains(relationship)) { throw new DotContentletStateException( "Error deleting existing relationships in contentlet: " + (contentlet != null ? contentlet.getInode() : "Unknown")); } - List cons = relationshipAPI + List relatedContents = relationshipAPI .dbRelatedContent(relationship, contentlet, hasParent); - cons = permissionAPI - .filterCollection(cons, PermissionAPI.PERMISSION_READ, respectFrontendRoles, user); + relatedContents = permissionAPI + .filterCollection(relatedContents, PermissionAPI.PERMISSION_READ, respectFrontendRoles, user); - for (final Contentlet relatedContent : cons) { - if (hasParent) { - TreeFactory.deleteTreesByParentAndChildAndRelationType(contentlet.getIdentifier(), - relatedContent.getIdentifier(), relationship.getRelationTypeValue()); - } else { - TreeFactory.deleteTreesByParentAndChildAndRelationType( - relatedContent.getIdentifier(), - contentlet.getIdentifier(), relationship.getRelationTypeValue()); - } + if (hasParent) { + TreeFactory.deleteTreesByParentAndRelationType(contentlet.getIdentifier(), + relationship.getRelationTypeValue()); + } else { + TreeFactory.deleteTreesByChildAndRelationType(contentlet.getIdentifier(), + relationship.getRelationTypeValue()); } final List identifiersToBeRelated = contentletsToBeRelated.stream().map( - Contentlet::getIdentifier).collect(Collectors.toList()); + Contentlet::getIdentifier).toList(); // We need to refresh related parents, because currently the system does not - // update the contentlets that lost the relationship (when the user remove a relationship). - if (cons != null) { - for (final Contentlet relatedContentlet : cons) { - //Only deleted parents will be reindexed + // update the contentlets that lost the relationship (when the user removes a relationship). + if (relatedContents != null) { + for (final Contentlet relatedContentlet : relatedContents) { + //Only deleted parents will be re-indexed if (!hasParent && !identifiersToBeRelated .contains(relatedContentlet.getIdentifier())) { relatedContentlet.setIndexPolicy(contentlet.getIndexPolicyDependencies()); @@ -4977,45 +4974,49 @@ public void relateContent(final Contentlet contentlet, respectFrontendRoles, related.getRecords()); Tree newTree; - Set uniqueRelationshipSet = new HashSet<>(); - - List conRels = getRelatedContentFromIndex(contentlet, relationship, - related.isHasParent(), user, respectFrontendRoles); - - int treePosition = (conRels != null && conRels.size() != 0) ? conRels.size() : 1; + final Set uniqueRelationshipSet = new HashSet<>(); + + final String countSQL = related.isHasParent() + ? "SELECT COUNT(*) as count FROM tree WHERE parent = ? AND relation_type = ?" + : "SELECT COUNT(*) as count FROM tree WHERE child = ? AND relation_type = ?"; + final List> countResult = new DotConnect().setSQL(countSQL) + .addParam(contentlet.getIdentifier()) + .addParam(relationship.getRelationTypeValue()) + .loadObjectResults(); + final int existingCount = countResult.isEmpty() + ? 0 + : Integer.parseInt(null != countResult.getFirst() + ? countResult.getFirst().get("count").toString() + : "0"); + + int treePosition = existingCount > 0 ? existingCount : 1; int positionInParent = 1; - - for (Contentlet c : related.getRecords()) { + final List treesToInsert = new ArrayList<>(); + for (final Contentlet relatedContent : related.getRecords()) { if (child) { - for (Tree currentTree : contentParents) { + for (final Tree currentTree : contentParents) { if (currentTree.getRelationType() - .equals(relationship.getRelationTypeValue()) && c.getIdentifier() + .equals(relationship.getRelationTypeValue()) && relatedContent.getIdentifier() .equals(currentTree.getParent())) { positionInParent = currentTree.getTreeOrder(); } } - newTree = new Tree(c.getIdentifier(), contentlet.getIdentifier(), + newTree = new Tree(relatedContent.getIdentifier(), contentlet.getIdentifier(), relationship.getRelationTypeValue(), positionInParent); } else { - newTree = new Tree(contentlet.getIdentifier(), c.getIdentifier(), + newTree = new Tree(contentlet.getIdentifier(), relatedContent.getIdentifier(), relationship.getRelationTypeValue(), treePosition); } positionInParent = positionInParent + 1; if (uniqueRelationshipSet.add(newTree)) { - final int newTreePosition = newTree.getTreeOrder(); - final Tree treeToUpdate = TreeFactory.getTree(newTree); - treeToUpdate.setTreeOrder(newTreePosition); - - TreeFactory.saveTree(treeToUpdate != null && UtilMethods.isSet( - treeToUpdate.getRelationType()) ? treeToUpdate : newTree); - + treesToInsert.add(newTree); treePosition++; } - invalidateRelatedContentCache(c, relationship, !related.isHasParent()); + invalidateRelatedContentCache(relatedContent, relationship, !related.isHasParent()); } - + TreeFactory.insertTrees(treesToInsert); //If relationship field, related content cache must be invalidated invalidateRelatedContentCache(contentlet, relationship, related.isHasParent()); diff --git a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESMappingAPIImpl.java b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESMappingAPIImpl.java index c2cc01919731..ef265bfffccf 100644 --- a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESMappingAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESMappingAPIImpl.java @@ -52,6 +52,7 @@ import com.dotmarketing.business.VersionableAPI; import com.dotmarketing.cache.FieldsCache; import com.dotmarketing.common.db.DotConnect; +import com.dotmarketing.common.model.ContentletSearch; import com.dotmarketing.exception.DotCorruptedDataException; import com.dotmarketing.exception.DotDataException; import com.dotmarketing.exception.DotSecurityException; @@ -1245,7 +1246,7 @@ public List dependenciesLeftToReindex(final Contentlet contentlet) throw final List> relatedContentlets = db.loadResults(); - if(relatedContentlets.size()>0) { + if(!relatedContentlets.isEmpty()) { final List relationships = relationshipAPI .byContentType(contentlet.getContentType()); @@ -1256,13 +1257,12 @@ public List dependenciesLeftToReindex(final Contentlet contentlet) throw final List oldRelatedIds = new ArrayList<>(); final List newRelatedIds = new ArrayList<>(); - oldDocs = contentletAPI.getRelatedContent(contentlet, relationship, - userAPI.getSystemUser(), false); - - if(oldDocs.size() > 0) { - for(Contentlet oldDoc : oldDocs) { - oldRelatedIds.add(oldDoc.getIdentifier()); - } + final List oldSearchResults = contentletAPI.searchIndex( + "+" + relationship.getRelationTypeValue() + + ":" + contentlet.getIdentifier(), + -1, 0, null, userAPI.getSystemUser(), false); + for (final ContentletSearch result : oldSearchResults) { + oldRelatedIds.add(result.getIdentifier()); } relatedContentlets.stream().filter(map -> map.get(ESMappingConstants.RELATION_TYPE) diff --git a/dotCMS/src/main/java/com/dotmarketing/factories/TreeFactory.java b/dotCMS/src/main/java/com/dotmarketing/factories/TreeFactory.java index db3831eb9015..000eb7d6c62a 100644 --- a/dotCMS/src/main/java/com/dotmarketing/factories/TreeFactory.java +++ b/dotCMS/src/main/java/com/dotmarketing/factories/TreeFactory.java @@ -6,9 +6,13 @@ import com.dotmarketing.beans.Tree; import com.dotmarketing.business.DotStateException; import com.dotmarketing.common.db.DotConnect; +import com.dotmarketing.db.DbConnectionFactory; import com.dotmarketing.exception.DotDataException; import com.dotmarketing.util.Logger; +import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.SQLException; import java.util.ArrayList; import java.util.Iterator; import java.util.List; @@ -349,10 +353,52 @@ public static void insertTree(Tree tree) { } } - - - + public static void deleteTreesByParentAndRelationType(final String parentId, + final String relationType) { + try { + new DotConnect() + .setSQL("DELETE FROM tree WHERE parent = ? AND relation_type = ?") + .addParam(parentId) + .addParam(relationType) + .loadResult(); + } catch (final DotDataException e) { + throw new DotStateException(e); + } + } + + public static void deleteTreesByChildAndRelationType(final String childId, + final String relationType) { + try { + new DotConnect() + .setSQL("DELETE FROM tree WHERE child = ? AND relation_type = ?") + .addParam(childId) + .addParam(relationType) + .loadResult(); + } catch (final DotDataException e) { + throw new DotStateException(e); + } + } + + public static void insertTrees(final List trees) { + if (trees == null || trees.isEmpty()) { + return; + } + try { + final Connection conn = DbConnectionFactory.getConnection(); + try (final PreparedStatement ps = conn.prepareStatement( + "INSERT INTO tree (child, parent, relation_type, tree_order) VALUES (?,?,?,?)")) { + for (final Tree tree : trees) { + ps.setString(1, tree.getChild()); + ps.setString(2, tree.getParent()); + ps.setString(3, tree.getRelationType()); + ps.setInt(4, tree.getTreeOrder()); + ps.addBatch(); + } + ps.executeBatch(); + } + } catch (final SQLException e) { + throw new DotStateException(e); + } + } - } - From 80cd1c9af026c6f03d6c999face85bcf71f1fb62 Mon Sep 17 00:00:00 2001 From: Jose Castro Date: Thu, 11 Jun 2026 09:51:53 -0600 Subject: [PATCH 2/4] Adding more potential changes --- .../business/ESContentletAPIImpl.java | 242 +++++++++++++++--- .../business/ESMappingAPIImpl.java | 76 +++--- .../dotmarketing/factories/TreeFactory.java | 148 +++++++++-- .../src/test/java/com/dotcms/MainSuite3a.java | 6 +- .../business/ESContentletAPIImplTest.java | 129 ++++++++++ .../business/ESMappingAPITest.java | 102 ++++++++ .../factories/TreeFactoryTest.java | 192 ++++++++++++++ 7 files changed, 803 insertions(+), 92 deletions(-) create mode 100644 dotcms-integration/src/test/java/com/dotmarketing/factories/TreeFactoryTest.java diff --git a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java index f55c63c4619d..fd31e1b2a1c9 100644 --- a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java @@ -4600,43 +4600,212 @@ public void deleteRelatedContent(final Contentlet contentlet, final Relationship ? contentlet.getInode() : "Unknown")); } - List relatedContents = relationshipAPI - .dbRelatedContent(relationship, contentlet, hasParent); - relatedContents = permissionAPI - .filterCollection(relatedContents, PermissionAPI.PERMISSION_READ, respectFrontendRoles, user); + final Set identifiersToBeRelated = contentletsToBeRelated.stream() + .map(Contentlet::getIdentifier).collect(Collectors.toSet()); + + if (isExemptFromPermissionFiltering(user)) { + deleteAllRelatedContent(contentlet, relationship, hasParent, identifiersToBeRelated); + } else { + deleteReadableRelatedContent(contentlet, relationship, hasParent, user, + respectFrontendRoles, identifiersToBeRelated); + } + + // Refresh the parent only if the contentlet is not already in the checkin + if (!contentlet.getBoolProperty(CHECKIN_IN_PROGRESS)) { + refreshNoDeps(contentlet); + } + } + + /** + * Mirrors the fast path of + * {@link PermissionAPI#filterCollection(List, int, boolean, User)}: for the system user and + * CMS Admins the READ-permission filter never removes related content, so the much cheaper + * identifier-only deletion path can be used instead of hydrating every related contentlet. + * + * @param user The {@link User} performing the deletion + * @return If the user is exempt from permission filtering, {@code true} + */ + private boolean isExemptFromPermissionFiltering(final User user) throws DotDataException { + if (user == null) { + return false; + } + final var roleAPI = APILocator.getRoleAPI(); + return APILocator.systemUser().getUserId().equals(user.getUserId()) + || roleAPI.doesUserHaveRole(user, roleAPI.loadCMSAdminRole()); + } + + /** + * Deletes ALL the tree rows for the given relationship and direction without hydrating the + * related contentlets: related identifiers are read straight from the tree table, the rows + * are removed with a single bulk delete, and only the parents that actually lost the + * relationship — and therefore must be re-indexed — get loaded. + * + * @param contentlet The {@link Contentlet} whose related content is deleted + * @param relationship The {@link Relationship} being cleared + * @param hasParent If the contentlet is the parent side, {@code true} + * @param identifiersToBeRelated Identifiers that will be re-related right after this call, + * which therefore do NOT need re-indexing + */ + private void deleteAllRelatedContent(final Contentlet contentlet, + final Relationship relationship, final boolean hasParent, + final Set identifiersToBeRelated) + throws DotDataException, DotSecurityException { + + final String relationTypeValue = relationship.getRelationTypeValue(); + final List relatedIds = (hasParent + ? TreeFactory.getRelatedIdsByParentAndRelationType(contentlet.getIdentifier(), + relationTypeValue) + : TreeFactory.getRelatedIdsByChildAndRelationType(contentlet.getIdentifier(), + relationTypeValue)) + .stream().distinct().toList(); if (hasParent) { TreeFactory.deleteTreesByParentAndRelationType(contentlet.getIdentifier(), - relationship.getRelationTypeValue()); + relationTypeValue); } else { TreeFactory.deleteTreesByChildAndRelationType(contentlet.getIdentifier(), - relationship.getRelationTypeValue()); + relationTypeValue); } - final List identifiersToBeRelated = contentletsToBeRelated.stream().map( - Contentlet::getIdentifier).toList(); + // We need to refresh related parents, because currently the system does not + // update the contentlets that lost the relationship (when the user removes a relationship) + if (!hasParent) { + final List removedParentIds = relatedIds.stream() + .filter(identifier -> !identifiersToBeRelated.contains(identifier)) + .toList(); + for (final Contentlet removedParent : findAllWorkingVersions(removedParentIds)) { + removedParent.setIndexPolicy(contentlet.getIndexPolicyDependencies()); + removedParent.setIndexPolicyDependencies(contentlet.getIndexPolicyDependencies()); + refreshNoDeps(removedParent); + } + } + + //If relationship field, related content cache must be invalidated + for (final String relatedId : relatedIds) { + invalidateRelatedContentCache(relatedId, relationship, !hasParent); + } + } + + /** + * Deletes the tree rows for the given relationship and direction preserving the legacy + * permission semantics: only relationships to content the user can READ are removed. This + * path must hydrate the related contentlets in order to evaluate their permissions. + * + * @param contentlet The {@link Contentlet} whose related content is deleted + * @param relationship The {@link Relationship} being cleared + * @param hasParent If the contentlet is the parent side, {@code true} + * @param user The {@link User} performing the deletion + * @param respectFrontendRoles If front-end roles must be validated, {@code true} + * @param identifiersToBeRelated Identifiers that will be re-related right after this call, + * which therefore do NOT need re-indexing + */ + private void deleteReadableRelatedContent(final Contentlet contentlet, + final Relationship relationship, final boolean hasParent, final User user, + final boolean respectFrontendRoles, final Set identifiersToBeRelated) + throws DotDataException, DotSecurityException { + + final List allRelatedContents = relationshipAPI + .dbRelatedContent(relationship, contentlet, hasParent); + final List relatedContents = permissionAPI + .filterCollection(allRelatedContents, PermissionAPI.PERMISSION_READ, + respectFrontendRoles, user); + + // When the permission filter removed nothing, a single bulk delete per direction is + // safe; otherwise the delete must be scoped to the readable identifiers so the + // remaining rows are preserved + if (relatedContents.size() == allRelatedContents.size()) { + if (hasParent) { + TreeFactory.deleteTreesByParentAndRelationType(contentlet.getIdentifier(), + relationship.getRelationTypeValue()); + } else { + TreeFactory.deleteTreesByChildAndRelationType(contentlet.getIdentifier(), + relationship.getRelationTypeValue()); + } + } else { + final List readableRelatedIds = relatedContents.stream() + .map(Contentlet::getIdentifier).distinct().toList(); + if (hasParent) { + TreeFactory.deleteTreesByParentAndChildrenAndRelationType( + contentlet.getIdentifier(), readableRelatedIds, + relationship.getRelationTypeValue()); + } else { + TreeFactory.deleteTreesByChildAndParentsAndRelationType( + contentlet.getIdentifier(), readableRelatedIds, + relationship.getRelationTypeValue()); + } + } // We need to refresh related parents, because currently the system does not - // update the contentlets that lost the relationship (when the user removes a relationship). - if (relatedContents != null) { - for (final Contentlet relatedContentlet : relatedContents) { - //Only deleted parents will be re-indexed - if (!hasParent && !identifiersToBeRelated - .contains(relatedContentlet.getIdentifier())) { - relatedContentlet.setIndexPolicy(contentlet.getIndexPolicyDependencies()); - relatedContentlet - .setIndexPolicyDependencies( - contentlet.getIndexPolicyDependencies()); - refreshNoDeps(relatedContentlet); - } - //If relationship field, related content cache must be invalidated - invalidateRelatedContentCache(relatedContentlet, relationship, !hasParent); + // update the contentlets that lost the relationship (when the user removes a relationship) + for (final Contentlet relatedContentlet : relatedContents) { + //Only deleted parents will be re-indexed + if (!hasParent && !identifiersToBeRelated + .contains(relatedContentlet.getIdentifier())) { + relatedContentlet.setIndexPolicy(contentlet.getIndexPolicyDependencies()); + relatedContentlet + .setIndexPolicyDependencies( + contentlet.getIndexPolicyDependencies()); + refreshNoDeps(relatedContentlet); } + //If relationship field, related content cache must be invalidated + invalidateRelatedContentCache(relatedContentlet, relationship, !hasParent); } + } - // Refresh the parent only if the contentlet is not already in the checkin - if (!contentlet.getBoolProperty(CHECKIN_IN_PROGRESS)) { - refreshNoDeps(contentlet); + private static final int VERSION_INFO_LOOKUP_CHUNK_SIZE = 500; + + /** + * Loads the working version of every language/variant of the given identifiers using batched + * queries against {@code contentlet_version_info} instead of one lookup per identifier. + * + * @param identifiers The identifiers of the contentlets to load + * @return The working version {@link Contentlet} objects, one per existing version row + */ + private List findAllWorkingVersions(final List identifiers) + throws DotDataException, DotSecurityException { + if (identifiers.isEmpty()) { + return List.of(); + } + final List workingInodes = new ArrayList<>(); + for (int from = 0; from < identifiers.size(); from += VERSION_INFO_LOOKUP_CHUNK_SIZE) { + final List chunk = identifiers.subList(from, + Math.min(from + VERSION_INFO_LOOKUP_CHUNK_SIZE, identifiers.size())); + final DotConnect dc = new DotConnect().setSQL( + "SELECT working_inode FROM contentlet_version_info" + + " WHERE working_inode IS NOT NULL AND identifier IN (" + + DotConnect.createParametersPlaceholder(chunk.size()) + ")"); + chunk.forEach(dc::addParam); + for (final Map row : dc.loadObjectResults()) { + workingInodes.add((String) row.get("working_inode")); + } + } + return findContentlets(workingInodes); + } + + /** + * Identifier-only counterpart of + * {@link #invalidateRelatedContentCache(Contentlet, Relationship, boolean)} for callers that + * never hydrated the related contentlet. Clearing the per-instance related-content map is + * not needed in that case — readers always resolve related content through the API, so + * evicting the relationship cache entry is what keeps them consistent. + * + * @param contentletIdentifier Identifier of the contentlet whose cache entry is evicted + * @param relationship The {@link Relationship} that changed + * @param hasParent If the contentlet is the parent side, {@code true} + */ + private void invalidateRelatedContentCache(final String contentletIdentifier, + final Relationship relationship, final boolean hasParent) { + if (!relationship.isRelationshipField()) { + return; + } + if (relationshipAPI.sameParentAndChild(relationship)) { + if (relationship.getParentRelationName() != null + || relationship.getChildRelationName() != null) { + CacheLocator.getRelationshipCache().removeRelatedContentMap(contentletIdentifier); + } + } else if ((!hasParent && relationship.getParentRelationName() != null) + || (hasParent && relationship.getChildRelationName() != null)) { + CacheLocator.getRelationshipCache().removeRelatedContentMap(contentletIdentifier); } } @@ -4976,20 +5145,19 @@ public void relateContent(final Contentlet contentlet, Tree newTree; final Set uniqueRelationshipSet = new HashSet<>(); - final String countSQL = related.isHasParent() - ? "SELECT COUNT(*) as count FROM tree WHERE parent = ? AND relation_type = ?" - : "SELECT COUNT(*) as count FROM tree WHERE child = ? AND relation_type = ?"; - final List> countResult = new DotConnect().setSQL(countSQL) + // Tree rows can survive the delete above: relationships to content the user cannot + // READ are preserved by deleteRelatedContent. New rows are appended after the + // highest surviving position so their relative order never collides with them; + // when nothing survived, positions simply start at 1 + final String nextPositionSQL = related.isHasParent() + ? "SELECT COALESCE(MAX(tree_order), 0) + 1 AS next_position FROM tree" + + " WHERE parent = ? AND relation_type = ?" + : "SELECT COALESCE(MAX(tree_order), 0) + 1 AS next_position FROM tree" + + " WHERE child = ? AND relation_type = ?"; + int treePosition = new DotConnect().setSQL(nextPositionSQL) .addParam(contentlet.getIdentifier()) .addParam(relationship.getRelationTypeValue()) - .loadObjectResults(); - final int existingCount = countResult.isEmpty() - ? 0 - : Integer.parseInt(null != countResult.getFirst() - ? countResult.getFirst().get("count").toString() - : "0"); - - int treePosition = existingCount > 0 ? existingCount : 1; + .getInt("next_position"); int positionInParent = 1; final List treesToInsert = new ArrayList<>(); for (final Contentlet relatedContent : related.getRecords()) { diff --git a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESMappingAPIImpl.java b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESMappingAPIImpl.java index ef265bfffccf..17f472298767 100644 --- a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESMappingAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESMappingAPIImpl.java @@ -80,6 +80,7 @@ import com.dotmarketing.util.Config; import com.dotmarketing.util.InodeUtils; import com.dotmarketing.util.Logger; +import com.dotmarketing.util.PaginatedArrayList; import com.dotmarketing.util.ThreadSafeSimpleDateFormat; import com.dotmarketing.util.UtilMethods; import com.fasterxml.jackson.databind.ObjectMapper; @@ -97,9 +98,11 @@ import java.time.ZoneId; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.Date; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -1253,25 +1256,48 @@ public List dependenciesLeftToReindex(final Contentlet contentlet) throw for(final Relationship relationship : relationships) { - final List oldDocs; - final List oldRelatedIds = new ArrayList<>(); - final List newRelatedIds = new ArrayList<>(); - - final List oldSearchResults = contentletAPI.searchIndex( - "+" + relationship.getRelationTypeValue() - + ":" + contentlet.getIdentifier(), - -1, 0, null, userAPI.getSystemUser(), false); + // Both sides are collected into Sets of identifiers: the index holds one + // document per language/variant of the same identifier, and only deduped + // collections make the disjunction below a true symmetric difference + final Collection oldRelatedIds = new LinkedHashSet<>(); + final Collection newRelatedIds = new LinkedHashSet<>(); + + final String relatedQuery = "+" + relationship.getRelationTypeValue() + + ":" + contentlet.getIdentifier(); + List oldSearchResults = contentletAPI.searchIndex( + relatedQuery, ESContentletAPIImpl.MAX_LIMIT, 0, null, + userAPI.getSystemUser(), false); + if (oldSearchResults instanceof PaginatedArrayList + && ((PaginatedArrayList) oldSearchResults).getTotalResults() + > oldSearchResults.size()) { + // More documents reference this contentlet than a single search page can + // return; a limit above MAX_LIMIT makes searchIndex switch to a scroll + // search that returns them all + Logger.debug(this, () -> "More than " + ESContentletAPIImpl.MAX_LIMIT + + " index documents reference '" + contentlet.getIdentifier() + + "' through '" + relationship.getRelationTypeValue() + + "'. Falling back to a scroll search"); + oldSearchResults = contentletAPI.searchIndex(relatedQuery, + ESContentletAPIImpl.MAX_LIMIT + 1, 0, null, + userAPI.getSystemUser(), false); + } for (final ContentletSearch result : oldSearchResults) { oldRelatedIds.add(result.getIdentifier()); } relatedContentlets.stream().filter(map -> map.get(ESMappingConstants.RELATION_TYPE) - .equals(relationship.getRelationTypeValue())).forEach( - entry -> replaceExistingRelatedContent(entry, contentlet, - oldRelatedIds, newRelatedIds)); - - //Taking the disjunction of both collections will give the old list of dependencies that need to be removed from the - //re-indexation and the list of new dependencies no re-indexed yet + .equals(relationship.getRelationTypeValue())).forEach(entry -> { + final String childId = entry.get(ESMappingConstants.CHILD); + final String parentId = entry.get(ESMappingConstants.PARENT); + newRelatedIds.add(contentlet.getIdentifier().equalsIgnoreCase(childId) + ? parentId : childId); + }); + + // The symmetric difference of both Sets is the actual dependency delta: the + // contents that lost the relationship (their index document still references + // this contentlet) plus the ones that just gained it (not indexed yet). + // Contents whose relationship did NOT change are excluded — re-indexing them + // on every save is what made saves with heavily related content so expensive dependenciesToReindex.addAll( CollectionUtils.disjunction(oldRelatedIds, newRelatedIds)); } @@ -1279,28 +1305,6 @@ public List dependenciesLeftToReindex(final Contentlet contentlet) throw return dependenciesToReindex; } - /** - * - * @param relatedEntry - * @param con - * @param oldRelatedIds - * @param newRelatedIds - */ - private void replaceExistingRelatedContent(final Map relatedEntry, - final Contentlet con, final List oldRelatedIds, - final List newRelatedIds) { - - final String childId = relatedEntry.get(ESMappingConstants.CHILD); - final String parentId = relatedEntry.get(ESMappingConstants.PARENT); - if (con.getIdentifier().equalsIgnoreCase(childId)) { - newRelatedIds.add(parentId); - oldRelatedIds.remove(parentId); - } else { - newRelatedIds.add(childId); - oldRelatedIds.remove(childId); - } - } - /** * @deprecated Use {@link ESMappingAPIImpl#loadRelationshipFields(Contentlet, Map, StringWriter)} instead * @param contentlet diff --git a/dotCMS/src/main/java/com/dotmarketing/factories/TreeFactory.java b/dotCMS/src/main/java/com/dotmarketing/factories/TreeFactory.java index 000eb7d6c62a..c56b93c5a900 100644 --- a/dotCMS/src/main/java/com/dotmarketing/factories/TreeFactory.java +++ b/dotCMS/src/main/java/com/dotmarketing/factories/TreeFactory.java @@ -6,16 +6,15 @@ import com.dotmarketing.beans.Tree; import com.dotmarketing.business.DotStateException; import com.dotmarketing.common.db.DotConnect; -import com.dotmarketing.db.DbConnectionFactory; +import com.dotmarketing.common.db.Params; import com.dotmarketing.exception.DotDataException; import com.dotmarketing.util.Logger; -import java.sql.Connection; -import java.sql.PreparedStatement; -import java.sql.SQLException; import java.util.ArrayList; import java.util.Iterator; +import java.util.LinkedHashMap; import java.util.List; +import java.util.Map; /** * @@ -379,24 +378,139 @@ public static void deleteTreesByChildAndRelationType(final String childId, } } + /** + * Returns the identifiers of the children related to the given parent under the given + * relation type, ordered by tree order. Unlike content-returning lookups, this method never + * hydrates contentlets — it reads the tree table only. + * + * @param parentId Identifier of the parent content + * @param relationType The relationship type value + * @return The child identifiers, in tree order + */ + public static List getRelatedIdsByParentAndRelationType(final String parentId, + final String relationType) { + return getRelatedIds("child", "parent", parentId, relationType); + } + + /** + * Returns the identifiers of the parents related to the given child under the given relation + * type, ordered by tree order. Unlike content-returning lookups, this method never hydrates + * contentlets — it reads the tree table only. + * + * @param childId Identifier of the child content + * @param relationType The relationship type value + * @return The parent identifiers, in tree order + */ + public static List getRelatedIdsByChildAndRelationType(final String childId, + final String relationType) { + return getRelatedIds("parent", "child", childId, relationType); + } + + private static List getRelatedIds(final String selectColumn, final String whereColumn, + final String id, final String relationType) { + try { + return new DotConnect() + .setSQL("SELECT " + selectColumn + " FROM tree WHERE " + whereColumn + + " = ? AND relation_type = ? ORDER BY tree_order") + .addParam(id) + .addParam(relationType) + .loadObjectResults().stream() + .map(row -> (String) row.get(selectColumn)) + .toList(); + } catch (final DotDataException e) { + throw new DotStateException(e); + } + } + + /** + * Deletes the tree rows for the given parent and relation type whose child is in the given + * list. Unlike {@link #deleteTreesByParentAndRelationType(String, String)}, rows pointing to + * children NOT included in the list are preserved. + * + * @param parentId Identifier of the parent content + * @param childIds Identifiers of the child contents whose rows can be deleted + * @param relationType The relationship type value + */ + public static void deleteTreesByParentAndChildrenAndRelationType(final String parentId, + final List childIds, final String relationType) { + deleteTreesScopedByRelationType("parent", "child", parentId, childIds, relationType); + } + + /** + * Deletes the tree rows for the given child and relation type whose parent is in the given + * list. Unlike {@link #deleteTreesByChildAndRelationType(String, String)}, rows pointing to + * parents NOT included in the list are preserved. + * + * @param childId Identifier of the child content + * @param parentIds Identifiers of the parent contents whose rows can be deleted + * @param relationType The relationship type value + */ + public static void deleteTreesByChildAndParentsAndRelationType(final String childId, + final List parentIds, final String relationType) { + deleteTreesScopedByRelationType("child", "parent", childId, parentIds, relationType); + } + + private static final int DELETE_TREES_CHUNK_SIZE = 500; + + private static void deleteTreesScopedByRelationType(final String fixedColumn, + final String scopedColumn, final String fixedId, final List scopedIds, + final String relationType) { + if (scopedIds == null || scopedIds.isEmpty()) { + return; + } + try { + for (int from = 0; from < scopedIds.size(); from += DELETE_TREES_CHUNK_SIZE) { + final List chunk = scopedIds.subList(from, + Math.min(from + DELETE_TREES_CHUNK_SIZE, scopedIds.size())); + final DotConnect dc = new DotConnect().setSQL( + "DELETE FROM tree WHERE " + fixedColumn + " = ? AND relation_type = ? AND " + + scopedColumn + " IN (" + + DotConnect.createParametersPlaceholder(chunk.size()) + ")"); + dc.addParam(fixedId); + dc.addParam(relationType); + chunk.forEach(dc::addParam); + dc.loadResult(); + } + } catch (final DotDataException e) { + throw new DotStateException(e); + } + } + + /** + * Saves the given trees in batch, preserving the upsert semantics of + * {@link #saveTree(Tree)} (delete-then-insert): duplicates by the {@code (child, parent, + * relation_type)} primary key collapse to the last occurrence, and pre-existing rows with + * the same key are replaced instead of triggering a constraint violation. + * + * @param trees The {@link Tree} rows to save + */ public static void insertTrees(final List trees) { if (trees == null || trees.isEmpty()) { return; } + final Map uniqueTrees = new LinkedHashMap<>(); + for (final Tree tree : trees) { + uniqueTrees.put(tree.getChild() + "|" + tree.getParent() + "|" + + tree.getRelationType(), tree); + } + + final List deleteParams = new ArrayList<>(uniqueTrees.size()); + final List insertParams = new ArrayList<>(uniqueTrees.size()); + for (final Tree tree : uniqueTrees.values()) { + deleteParams.add(new Params(tree.getChild(), tree.getParent(), + tree.getRelationType())); + insertParams.add(new Params(tree.getChild(), tree.getParent(), + tree.getRelationType(), tree.getTreeOrder())); + } + try { - final Connection conn = DbConnectionFactory.getConnection(); - try (final PreparedStatement ps = conn.prepareStatement( - "INSERT INTO tree (child, parent, relation_type, tree_order) VALUES (?,?,?,?)")) { - for (final Tree tree : trees) { - ps.setString(1, tree.getChild()); - ps.setString(2, tree.getParent()); - ps.setString(3, tree.getRelationType()); - ps.setInt(4, tree.getTreeOrder()); - ps.addBatch(); - } - ps.executeBatch(); - } - } catch (final SQLException e) { + new DotConnect().executeBatch( + "DELETE FROM tree WHERE child = ? AND parent = ? AND relation_type = ?", + deleteParams); + new DotConnect().executeBatch( + "INSERT INTO tree (child, parent, relation_type, tree_order) VALUES (?,?,?,?)", + insertParams); + } catch (final DotDataException e) { throw new DotStateException(e); } } diff --git a/dotcms-integration/src/test/java/com/dotcms/MainSuite3a.java b/dotcms-integration/src/test/java/com/dotcms/MainSuite3a.java index 818feff9bf2f..94924a5796d8 100644 --- a/dotcms-integration/src/test/java/com/dotcms/MainSuite3a.java +++ b/dotcms-integration/src/test/java/com/dotcms/MainSuite3a.java @@ -8,7 +8,6 @@ import com.dotcms.junit.MainBaseSuite; import com.dotcms.rest.api.v1.drive.ContentDriveHelperContentletAPIComparisonTest; import com.dotcms.rest.api.v1.drive.ContentDriveWorkflowFilterTest; -import com.dotmarketing.portlets.contentlet.action.ImportContentletsActionSmokeTest; import com.dotcms.security.apps.AppsAPIImplTest; import com.dotcms.telemetry.collectors.MetricTimeoutTest; import com.dotcms.telemetry.collectors.experiment.CountPagesWithAllEndedExperimentsMetricTypeTest; @@ -25,9 +24,11 @@ import com.dotcms.util.TimeMachineUtilTest; import com.dotmarketing.business.DeterministicIdentifierAPITest; import com.dotmarketing.business.SecondaryCategoryPermissionTest; +import com.dotmarketing.factories.TreeFactoryTest; +import com.dotmarketing.fixtask.tasks.FixTask00090RecreateMissingFoldersInParentPathTest; +import com.dotmarketing.portlets.contentlet.action.ImportContentletsActionSmokeTest; import com.dotmarketing.portlets.rules.RuleAPITest; import com.dotmarketing.startup.runonce.Task230630CreateRunningIdsExperimentFieldIntegrationTest; -import com.dotmarketing.fixtask.tasks.FixTask00090RecreateMissingFoldersInParentPathTest; import com.dotmarketing.startup.runonce.Task250604UpdateFolderInodesTest; import com.dotmarketing.startup.runonce.Task250826AddIndexesToUniqueFieldsTableTest; import com.dotmarketing.startup.runonce.Task251103AddStylePropertiesColumnInMultiTreeTest; @@ -83,6 +84,7 @@ Task260505AddPluginsPortletToMenuTest.class, Task260407AddBaseTypeColumnToIdentifierTest.class, ImportContentletsActionSmokeTest.class, + TreeFactoryTest.class }) public class MainSuite3a { diff --git a/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImplTest.java b/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImplTest.java index ac9790582c95..93957c8f43e5 100644 --- a/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImplTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImplTest.java @@ -31,6 +31,7 @@ import com.dotcms.datagen.FolderDataGen; import com.dotcms.datagen.HTMLPageDataGen; import com.dotcms.datagen.LanguageDataGen; +import com.dotcms.datagen.PermissionUtilTest; import com.dotcms.datagen.RoleDataGen; import com.dotcms.datagen.SiteDataGen; import com.dotcms.datagen.TemplateDataGen; @@ -59,6 +60,7 @@ import com.dotmarketing.business.APILocator; import com.dotmarketing.business.CacheLocator; import com.dotmarketing.business.FactoryLocator; +import com.dotmarketing.business.PermissionAPI; import com.dotmarketing.business.PermissionLevel; import com.dotmarketing.business.Permissionable; import com.dotmarketing.business.RelationshipAPI; @@ -71,6 +73,7 @@ import com.dotmarketing.exception.DotRuntimeException; import com.dotmarketing.exception.DotSecurityException; import com.dotmarketing.exception.WebAssetException; +import com.dotmarketing.factories.TreeFactory; import com.dotmarketing.portlets.contentlet.business.ContentletAPI; import com.dotmarketing.portlets.contentlet.business.DotContentletValidationException; import com.dotmarketing.portlets.contentlet.model.Contentlet; @@ -943,6 +946,132 @@ public void selfRelatedContents() throws DotDataException { } + /** + * Method to test: + * {@link ContentletAPI#deleteRelatedContent(Contentlet, Relationship, boolean, User, boolean)} + * When: a limited user with EDIT permission on the parent contentlet, READ permission on one + * related child but NO permission on another related child deletes the related content + * Should: remove only the relationship to the readable child; the relationship to the + * unreadable child must be preserved + */ + @Test + public void deleteRelatedContentWithLimitedUserPreservesUnreadableRelationships() + throws DotDataException, DotSecurityException { + final Host host = new SiteDataGen().nextPersisted(); + final ContentType parentType = new ContentTypeDataGen().host(host).nextPersisted(); + final ContentType childType = new ContentTypeDataGen().host(host).nextPersisted(); + + final Relationship relationship = new FieldRelationshipDataGen() + .parent(parentType) + .child(childType) + .nextPersisted(); + + final Contentlet parent = new ContentletDataGen(parentType).host(host).nextPersisted(); + final Contentlet readableChild = new ContentletDataGen(childType).host(host) + .nextPersisted(); + final Contentlet unreadableChild = new ContentletDataGen(childType).host(host) + .nextPersisted(); + + contentletAPI.relateContent(parent, relationship, + list(readableChild, unreadableChild), user, false); + + final User limitedUser = new UserDataGen().roles(TestUserUtils.getBackendRole()).nextPersisted(); + PermissionUtilTest.addPermission(parent, limitedUser, + PermissionAPI.INDIVIDUAL_PERMISSION_TYPE, + PermissionAPI.PERMISSION_READ, PermissionAPI.PERMISSION_EDIT); + PermissionUtilTest.addPermission(readableChild, limitedUser, + PermissionAPI.INDIVIDUAL_PERMISSION_TYPE, PermissionAPI.PERMISSION_READ); + + contentletAPI.deleteRelatedContent(parent, relationship, true, limitedUser, false); + + final List remainingChildren = TreeFactory.getRelatedIdsByParentAndRelationType( + parent.getIdentifier(), relationship.getRelationTypeValue()); + assertEquals("Only the readable relationship must be deleted", 1, + remainingChildren.size()); + assertEquals("The relationship to the unreadable child must be preserved", + unreadableChild.getIdentifier(), remainingChildren.get(0)); + } + + /** + * Method to test: + * {@link ContentletAPI#relateContent(Contentlet, Relationship, List, User, boolean)} + * When: the records list contains the same related contentlet more than once — e.g. a REST + * payload listing an identifier twice, or two language versions of the same content + * Should: save without a primary key violation and keep a single relationship row + */ + @Test + public void relateContentWithDuplicateRecordsSavesSingleRelationship() + throws DotDataException, DotSecurityException { + final Host host = new SiteDataGen().nextPersisted(); + final ContentType parentType = new ContentTypeDataGen().host(host).nextPersisted(); + final ContentType childType = new ContentTypeDataGen().host(host).nextPersisted(); + + final Relationship relationship = new FieldRelationshipDataGen() + .parent(parentType) + .child(childType) + .nextPersisted(); + + final Contentlet parent = new ContentletDataGen(parentType).host(host).nextPersisted(); + final Contentlet child = new ContentletDataGen(childType).host(host).nextPersisted(); + + contentletAPI.relateContent(parent, relationship, list(child, child), user, false); + + final List relatedChildren = TreeFactory.getRelatedIdsByParentAndRelationType( + parent.getIdentifier(), relationship.getRelationTypeValue()); + assertEquals("Duplicate records must collapse into a single relationship row", 1, + relatedChildren.size()); + assertEquals(child.getIdentifier(), relatedChildren.get(0)); + } + + /** + * Method to test: + * {@link ContentletAPI#relateContent(Contentlet, Relationship, List, User, boolean)} + * When: a limited user relates new content to a parent that is already related to content + * the user cannot READ + * Should: preserve the unreadable relationship and append the new relationship after it in + * the tree order, so the surviving rows keep their position + */ + @Test + public void relateContentWithLimitedUserAppendsAfterPreservedRelationships() + throws DotDataException, DotSecurityException { + final Host host = new SiteDataGen().nextPersisted(); + final ContentType parentType = new ContentTypeDataGen().host(host).nextPersisted(); + final ContentType childType = new ContentTypeDataGen().host(host).nextPersisted(); + + final Relationship relationship = new FieldRelationshipDataGen() + .parent(parentType) + .child(childType) + .nextPersisted(); + + final Contentlet parent = new ContentletDataGen(parentType).host(host).nextPersisted(); + final Contentlet readableChild = new ContentletDataGen(childType).host(host) + .nextPersisted(); + final Contentlet unreadableChild = new ContentletDataGen(childType).host(host) + .nextPersisted(); + + // The existing relationship points to content the limited user cannot READ + contentletAPI.relateContent(parent, relationship, list(unreadableChild), user, false); + + final User limitedUser = new UserDataGen().roles(TestUserUtils.getBackendRole()).nextPersisted(); + PermissionUtilTest.addPermission(parent, limitedUser, + PermissionAPI.INDIVIDUAL_PERMISSION_TYPE, + PermissionAPI.PERMISSION_READ, PermissionAPI.PERMISSION_EDIT); + PermissionUtilTest.addPermission(readableChild, limitedUser, + PermissionAPI.INDIVIDUAL_PERMISSION_TYPE, PermissionAPI.PERMISSION_READ); + + contentletAPI.relateContent(parent, relationship, list(readableChild), limitedUser, + false); + + final List relatedChildren = TreeFactory.getRelatedIdsByParentAndRelationType( + parent.getIdentifier(), relationship.getRelationTypeValue()); + assertEquals("Both the preserved and the new relationship must exist", 2, + relatedChildren.size()); + assertEquals("The preserved relationship must keep its position", + unreadableChild.getIdentifier(), relatedChildren.get(0)); + assertEquals("The new relationship must be appended after the preserved one", + readableChild.getIdentifier(), relatedChildren.get(1)); + } + /** * Method to test: {@link ContentletAPI#checkin(Contentlet, User, boolean)} } * When: diff --git a/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ESMappingAPITest.java b/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ESMappingAPITest.java index 035d2d550c83..884f9e2094c6 100644 --- a/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ESMappingAPITest.java +++ b/dotcms-integration/src/test/java/com/dotcms/content/elasticsearch/business/ESMappingAPITest.java @@ -40,12 +40,16 @@ import com.dotcms.datagen.ContentTypeDataGen; import com.dotcms.datagen.ContentletDataGen; import com.dotcms.datagen.FieldDataGen; +import com.dotcms.datagen.FieldRelationshipDataGen; import com.dotcms.datagen.FileAssetDataGen; +import com.dotcms.datagen.LanguageDataGen; import com.dotcms.datagen.SiteDataGen; import com.dotcms.datagen.TestDataUtils; import com.dotcms.datagen.TestDataUtils.TestFile; import com.dotcms.util.IntegrationTestInitService; import com.dotmarketing.beans.Host; +import com.dotmarketing.beans.Tree; +import com.dotmarketing.factories.TreeFactory; import com.dotmarketing.beans.Identifier; import com.dotmarketing.business.APILocator; import com.dotmarketing.business.IdentifierAPI; @@ -711,6 +715,104 @@ public void testLoadRelationshipFields_whenUsingLegacyRelationships_shouldSucces } } + /** + * Method to test: {@link ESMappingAPIImpl#dependenciesLeftToReindex(Contentlet)} + * When: a child contentlet is related to several parents in the index (the state previous + * to a save) and the database relationships then change — one parent removed, a new parent + * added, and another parent untouched + * Should: return only the removed and the added parents. The unchanged parent must NOT be + * marked for re-indexing + */ + @Test + public void dependenciesLeftToReindexReturnsOnlyAddedAndRemovedParents() throws Exception { + final Host host = new SiteDataGen().nextPersisted(); + final ContentType parentType = new ContentTypeDataGen().host(host).nextPersisted(); + final ContentType childType = new ContentTypeDataGen().host(host).nextPersisted(); + + final Relationship relationship = new FieldRelationshipDataGen() + .parent(parentType) + .child(childType) + .nextPersisted(); + + final Contentlet child = new ContentletDataGen(childType).host(host).nextPersisted(); + final Contentlet removedParent = new ContentletDataGen(parentType).host(host) + .nextPersisted(); + final Contentlet unchangedParent = new ContentletDataGen(parentType).host(host) + .nextPersisted(); + final Contentlet addedParent = new ContentletDataGen(parentType).host(host) + .nextPersisted(); + + contentletAPI.relateContent(removedParent, relationship, list(child), user, false); + contentletAPI.relateContent(unchangedParent, relationship, list(child), user, false); + + // Make sure the parent documents, including their relationship field, are searchable + // in the index before the relationships change + removedParent.setIndexPolicy(IndexPolicy.FORCE); + contentletAPI.refresh(removedParent); + unchangedParent.setIndexPolicy(IndexPolicy.FORCE); + contentletAPI.refresh(unchangedParent); + + // Change the DB state only, reproducing the mid-save state where the index still holds + // the previous relationships: one parent is removed and another one is added + TreeFactory.deleteTreesByChildAndParentsAndRelationType(child.getIdentifier(), + list(removedParent.getIdentifier()), relationship.getRelationTypeValue()); + TreeFactory.insertTrees(list(new Tree(addedParent.getIdentifier(), + child.getIdentifier(), relationship.getRelationTypeValue(), 2))); + + final List dependencies = new ESMappingAPIImpl() + .dependenciesLeftToReindex(child); + + assertEquals("Only the removed and the added parents must be re-indexed. Got: " + + dependencies, 2, dependencies.size()); + assertTrue("The removed parent must be re-indexed", + dependencies.contains(removedParent.getIdentifier())); + assertTrue("The added parent must be re-indexed", + dependencies.contains(addedParent.getIdentifier())); + } + + /** + * Method to test: {@link ESMappingAPIImpl#dependenciesLeftToReindex(Contentlet)} + * When: a parent related to the child has several language versions — so the index holds + * more than one document for the same parent identifier — and the relationships did NOT + * change + * Should: return no dependencies. Without deduping the identifiers coming from the index, + * the duplicate documents would force a spurious re-index of the unchanged parent on every + * save of the child + */ + @Test + public void dependenciesLeftToReindexDedupesMultilingualParents() throws Exception { + final Host host = new SiteDataGen().nextPersisted(); + final ContentType parentType = new ContentTypeDataGen().host(host).nextPersisted(); + final ContentType childType = new ContentTypeDataGen().host(host).nextPersisted(); + + final Relationship relationship = new FieldRelationshipDataGen() + .parent(parentType) + .child(childType) + .nextPersisted(); + + final Contentlet child = new ContentletDataGen(childType).host(host).nextPersisted(); + final Contentlet parent = new ContentletDataGen(parentType).host(host).nextPersisted(); + + contentletAPI.relateContent(parent, relationship, list(child), user, false); + + parent.setIndexPolicy(IndexPolicy.FORCE); + contentletAPI.refresh(parent); + + // A second language version of the parent adds a second index document for the same + // identifier; the relationship rows in the DB are identifier-scoped and stay the same + final Language secondLanguage = new LanguageDataGen().nextPersisted(); + final Contentlet parentInSecondLanguage = ContentletDataGen.checkout(parent); + parentInSecondLanguage.setLanguageId(secondLanguage.getId()); + parentInSecondLanguage.setIndexPolicy(IndexPolicy.FORCE); + contentletAPI.checkin(parentInSecondLanguage, user, false); + + final List dependencies = new ESMappingAPIImpl() + .dependenciesLeftToReindex(child); + + assertTrue("Unchanged multilingual parents must not be re-indexed. Got: " + dependencies, + dependencies.isEmpty()); + } + @Test public void testLoadRelationshipFields_whenUsingSelfRelationships_shouldSuccess() throws DotDataException, DotSecurityException { diff --git a/dotcms-integration/src/test/java/com/dotmarketing/factories/TreeFactoryTest.java b/dotcms-integration/src/test/java/com/dotmarketing/factories/TreeFactoryTest.java new file mode 100644 index 000000000000..05b71a170dbf --- /dev/null +++ b/dotcms-integration/src/test/java/com/dotmarketing/factories/TreeFactoryTest.java @@ -0,0 +1,192 @@ +package com.dotmarketing.factories; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import com.dotcms.util.IntegrationTestInitService; +import com.dotmarketing.beans.Tree; +import com.dotmarketing.util.UUIDGenerator; +import java.util.ArrayList; +import java.util.List; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Integration tests for the bulk and scoped {@link TreeFactory} operations used when saving + * contentlets with relationships. The tree table has no foreign keys, so these tests work with + * synthetic identifiers and a unique relation type per test to stay isolated. + */ +public class TreeFactoryTest { + + @BeforeClass + public static void prepare() throws Exception { + IntegrationTestInitService.getInstance().init(); + } + + private static String newId() { + return UUIDGenerator.generateUuid(); + } + + private static String newRelationType() { + return "testRel" + UUIDGenerator.generateUuid().replace("-", ""); + } + + /** + * Method to test: {@link TreeFactory#insertTrees(List)} + * When: the list contains two trees with the same {@code (parent, child, relation_type)} + * primary key but different tree order — e.g. the same related content submitted twice, or + * in two languages + * Should: NOT fail with a primary key violation, and keep the last occurrence, matching the + * legacy {@link TreeFactory#saveTree(Tree)} delete-then-insert behavior + */ + @Test + public void insertTreesCollapsesDuplicateKeysToLastOccurrence() { + final String parent = newId(); + final String child = newId(); + final String relationType = newRelationType(); + + TreeFactory.insertTrees(List.of( + new Tree(parent, child, relationType, 1), + new Tree(parent, child, relationType, 5))); + + final List relatedIds = TreeFactory + .getRelatedIdsByParentAndRelationType(parent, relationType); + assertEquals("Duplicates by PK must collapse into a single row", 1, relatedIds.size()); + assertEquals(child, relatedIds.get(0)); + + final Tree saved = TreeFactory.getTree(parent, child, relationType); + assertEquals("The last occurrence must win", 5, saved.getTreeOrder()); + } + + /** + * Method to test: {@link TreeFactory#insertTrees(List)} + * When: a row with the same {@code (parent, child, relation_type)} primary key already + * exists in the table — e.g. a relationship row preserved by the permission-scoped delete + * Should: replace the pre-existing row instead of failing with a primary key violation + */ + @Test + public void insertTreesReplacesPreExistingRows() { + final String parent = newId(); + final String child = newId(); + final String relationType = newRelationType(); + + TreeFactory.saveTree(new Tree(parent, child, relationType, 1)); + TreeFactory.insertTrees(List.of(new Tree(parent, child, relationType, 9))); + + final List relatedIds = TreeFactory + .getRelatedIdsByParentAndRelationType(parent, relationType); + assertEquals(1, relatedIds.size()); + + final Tree saved = TreeFactory.getTree(parent, child, relationType); + assertEquals("The new row must replace the pre-existing one", 9, saved.getTreeOrder()); + } + + /** + * Method to test: {@link TreeFactory#deleteTreesByParentAndChildrenAndRelationType(String, List, String)} + * When: only a subset of the children related to a parent is passed in + * Should: delete only the rows pointing to the listed children, preserving the rest + */ + @Test + public void scopedDeleteByParentPreservesUnlistedChildren() { + final String parent = newId(); + final String child1 = newId(); + final String child2 = newId(); + final String child3 = newId(); + final String relationType = newRelationType(); + + TreeFactory.insertTrees(List.of( + new Tree(parent, child1, relationType, 1), + new Tree(parent, child2, relationType, 2), + new Tree(parent, child3, relationType, 3))); + + TreeFactory.deleteTreesByParentAndChildrenAndRelationType(parent, + List.of(child1, child3), relationType); + + final List remaining = TreeFactory + .getRelatedIdsByParentAndRelationType(parent, relationType); + assertEquals(1, remaining.size()); + assertEquals("The unlisted child must be preserved", child2, remaining.get(0)); + } + + /** + * Method to test: {@link TreeFactory#deleteTreesByChildAndParentsAndRelationType(String, List, String)} + * When: only a subset of the parents related to a child is passed in + * Should: delete only the rows pointing to the listed parents, preserving the rest + */ + @Test + public void scopedDeleteByChildPreservesUnlistedParents() { + final String child = newId(); + final String parent1 = newId(); + final String parent2 = newId(); + final String relationType = newRelationType(); + + TreeFactory.insertTrees(List.of( + new Tree(parent1, child, relationType, 1), + new Tree(parent2, child, relationType, 2))); + + TreeFactory.deleteTreesByChildAndParentsAndRelationType(child, + List.of(parent1), relationType); + + final List remaining = TreeFactory + .getRelatedIdsByChildAndRelationType(child, relationType); + assertEquals(1, remaining.size()); + assertEquals("The unlisted parent must be preserved", parent2, remaining.get(0)); + } + + /** + * Method to test: {@link TreeFactory#deleteTreesByParentAndChildrenAndRelationType(String, List, String)} + * When: the list of identifiers exceeds the internal chunk size (500) + * Should: delete every listed row across multiple chunked statements + */ + @Test + public void scopedDeleteHandlesMoreIdsThanChunkSize() { + final String parent = newId(); + final String relationType = newRelationType(); + final int total = 501; + + final List trees = new ArrayList<>(total); + final List childIds = new ArrayList<>(total); + for (int i = 0; i < total; i++) { + final String child = newId(); + childIds.add(child); + trees.add(new Tree(parent, child, relationType, i + 1)); + } + TreeFactory.insertTrees(trees); + assertEquals(total, TreeFactory + .getRelatedIdsByParentAndRelationType(parent, relationType).size()); + + TreeFactory.deleteTreesByParentAndChildrenAndRelationType(parent, childIds, relationType); + + assertTrue("All listed rows must be deleted across chunks", TreeFactory + .getRelatedIdsByParentAndRelationType(parent, relationType).isEmpty()); + } + + /** + * Method to test: {@link TreeFactory#getRelatedIdsByParentAndRelationType(String, String)} + * and {@link TreeFactory#getRelatedIdsByChildAndRelationType(String, String)} + * When: rows are inserted with non-sequential tree order values + * Should: return the identifiers sorted by tree order, without hydrating contentlets + */ + @Test + public void getRelatedIdsReturnsIdentifiersInTreeOrder() { + final String parent = newId(); + final String childA = newId(); + final String childB = newId(); + final String childC = newId(); + final String relationType = newRelationType(); + + TreeFactory.insertTrees(List.of( + new Tree(parent, childC, relationType, 1), + new Tree(parent, childA, relationType, 2), + new Tree(parent, childB, relationType, 3))); + + final List children = TreeFactory + .getRelatedIdsByParentAndRelationType(parent, relationType); + assertEquals(List.of(childC, childA, childB), children); + + final List parents = TreeFactory + .getRelatedIdsByChildAndRelationType(childA, relationType); + assertEquals(List.of(parent), parents); + } + +} From f1946e1344ec7e596a05e5640da522e789fbdab5 Mon Sep 17 00:00:00 2001 From: Jose Castro Date: Thu, 11 Jun 2026 15:53:40 -0600 Subject: [PATCH 3/4] Adding more potential changes --- .../business/ESContentletAPIImpl.java | 78 +++++------- .../business/ESMappingAPIImpl.java | 68 ++++++----- .../business/VersionableFactory.java | 22 +++- .../business/VersionableFactoryImpl.java | 37 ++++-- .../dotmarketing/factories/TreeFactory.java | 112 +++++++++++++++++- 5 files changed, 229 insertions(+), 88 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java index fd31e1b2a1c9..a2aaba0c1b01 100644 --- a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java @@ -1,12 +1,5 @@ package com.dotcms.content.elasticsearch.business; -import static com.dotcms.exception.ExceptionUtil.bubbleUpException; -import static com.dotcms.exception.ExceptionUtil.getLocalizedMessageOrDefault; -import static com.dotmarketing.business.PermissionAPI.PERMISSION_CAN_ADD_CHILDREN; -import static com.dotmarketing.portlets.contentlet.model.Contentlet.URL_MAP_FOR_CONTENT_KEY; -import static com.dotmarketing.portlets.personas.business.PersonaAPI.DEFAULT_PERSONA_NAME_KEY; -import static com.liferay.util.StringPool.BLANK; - import com.dotcms.api.system.event.ContentletSystemEventUtil; import com.dotcms.api.web.HttpServletRequestThreadLocal; import com.dotcms.business.CloseDBIfOpened; @@ -22,6 +15,7 @@ import com.dotcms.content.elasticsearch.constants.ESMappingConstants; import com.dotcms.content.elasticsearch.util.PaginationUtil; import com.dotcms.content.index.IndexContentletScroll; +import com.dotcms.content.index.domain.ContentSearchResponse; import com.dotcms.contenttype.business.BaseTypeToContentTypeStrategy; import com.dotcms.contenttype.business.BaseTypeToContentTypeStrategyResolver; import com.dotcms.contenttype.business.ContentTypeAPI; @@ -199,6 +193,16 @@ import io.vavr.Lazy; import io.vavr.Tuple2; import io.vavr.control.Try; +import org.apache.commons.beanutils.BeanUtils; +import org.apache.commons.io.FileUtils; +import org.apache.commons.lang.StringUtils; +import org.elasticsearch.action.search.SearchPhaseExecutionException; +import org.elasticsearch.action.search.SearchResponse; + +import javax.activation.MimeType; +import javax.annotation.Nullable; +import javax.servlet.http.HttpServletRequest; +import javax.validation.constraints.NotNull; import java.io.BufferedOutputStream; import java.io.File; import java.io.IOException; @@ -228,16 +232,13 @@ import java.util.regex.Pattern; import java.util.stream.Collectors; import java.util.stream.Stream; -import javax.activation.MimeType; -import javax.annotation.Nullable; -import javax.servlet.http.HttpServletRequest; -import javax.validation.constraints.NotNull; -import org.apache.commons.beanutils.BeanUtils; -import org.apache.commons.io.FileUtils; -import org.apache.commons.lang.StringUtils; -import org.elasticsearch.action.search.SearchPhaseExecutionException; -import com.dotcms.content.index.domain.ContentSearchResponse; -import org.elasticsearch.action.search.SearchResponse; + +import static com.dotcms.exception.ExceptionUtil.bubbleUpException; +import static com.dotcms.exception.ExceptionUtil.getLocalizedMessageOrDefault; +import static com.dotmarketing.business.PermissionAPI.PERMISSION_CAN_ADD_CHILDREN; +import static com.dotmarketing.portlets.contentlet.model.Contentlet.URL_MAP_FOR_CONTENT_KEY; +import static com.dotmarketing.portlets.personas.business.PersonaAPI.DEFAULT_PERSONA_NAME_KEY; +import static com.liferay.util.StringPool.BLANK; /** @@ -312,8 +313,6 @@ public enum QueryType { search, suggest, moreLike, Facets } - ; - private static final Supplier ND_SUPPLIER = () -> "N/D"; public static boolean getFeatureFlagDbUniqueFieldValidation() { @@ -4711,7 +4710,7 @@ private void deleteReadableRelatedContent(final Contentlet contentlet, respectFrontendRoles, user); // When the permission filter removed nothing, a single bulk delete per direction is - // safe; otherwise the delete must be scoped to the readable identifiers so the + // safe; otherwise, the delete operation must be scoped to the readable identifiers so the // remaining rows are preserved if (relatedContents.size() == allRelatedContents.size()) { if (hasParent) { @@ -4752,11 +4751,10 @@ private void deleteReadableRelatedContent(final Contentlet contentlet, } } - private static final int VERSION_INFO_LOOKUP_CHUNK_SIZE = 500; - /** - * Loads the working version of every language/variant of the given identifiers using batched - * queries against {@code contentlet_version_info} instead of one lookup per identifier. + * Loads the working version of every language/variant of the given identifiers using the + * batched {@link com.dotmarketing.business.VersionableFactory} lookup instead of one query + * per identifier. * * @param identifiers The identifiers of the contentlets to load * @return The working version {@link Contentlet} objects, one per existing version row @@ -4766,19 +4764,11 @@ private List findAllWorkingVersions(final List identifiers) if (identifiers.isEmpty()) { return List.of(); } - final List workingInodes = new ArrayList<>(); - for (int from = 0; from < identifiers.size(); from += VERSION_INFO_LOOKUP_CHUNK_SIZE) { - final List chunk = identifiers.subList(from, - Math.min(from + VERSION_INFO_LOOKUP_CHUNK_SIZE, identifiers.size())); - final DotConnect dc = new DotConnect().setSQL( - "SELECT working_inode FROM contentlet_version_info" - + " WHERE working_inode IS NOT NULL AND identifier IN (" - + DotConnect.createParametersPlaceholder(chunk.size()) + ")"); - chunk.forEach(dc::addParam); - for (final Map row : dc.loadObjectResults()) { - workingInodes.add((String) row.get("working_inode")); - } - } + final List workingInodes = FactoryLocator.getVersionableFactory() + .findAllContentletVersionInfos(identifiers).stream() + .map(ContentletVersionInfo::getWorkingInode) + .filter(UtilMethods::isSet) + .toList(); return findContentlets(workingInodes); } @@ -5149,15 +5139,11 @@ public void relateContent(final Contentlet contentlet, // READ are preserved by deleteRelatedContent. New rows are appended after the // highest surviving position so their relative order never collides with them; // when nothing survived, positions simply start at 1 - final String nextPositionSQL = related.isHasParent() - ? "SELECT COALESCE(MAX(tree_order), 0) + 1 AS next_position FROM tree" - + " WHERE parent = ? AND relation_type = ?" - : "SELECT COALESCE(MAX(tree_order), 0) + 1 AS next_position FROM tree" - + " WHERE child = ? AND relation_type = ?"; - int treePosition = new DotConnect().setSQL(nextPositionSQL) - .addParam(contentlet.getIdentifier()) - .addParam(relationship.getRelationTypeValue()) - .getInt("next_position"); + int treePosition = related.isHasParent() + ? TreeFactory.getNextTreeOrderByParentAndRelationType( + contentlet.getIdentifier(), relationship.getRelationTypeValue()) + : TreeFactory.getNextTreeOrderByChildAndRelationType( + contentlet.getIdentifier(), relationship.getRelationTypeValue()); int positionInParent = 1; final List treesToInsert = new ArrayList<>(); for (final Contentlet relatedContent : related.getRecords()) { diff --git a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESMappingAPIImpl.java b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESMappingAPIImpl.java index 17f472298767..62ae02b89b66 100644 --- a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESMappingAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESMappingAPIImpl.java @@ -1,20 +1,5 @@ package com.dotcms.content.elasticsearch.business; -import static com.dotcms.content.elasticsearch.business.ESIndexAPI.INDEX_OPERATIONS_TIMEOUT_IN_MS; -import static com.dotcms.content.elasticsearch.constants.ESMappingConstants.CREATION_DATE; -import static com.dotcms.content.elasticsearch.constants.ESMappingConstants.PERSONA_KEY_TAG; -import static com.dotcms.content.elasticsearch.constants.ESMappingConstants.SYS_PUBLISH_USER; -import static com.dotcms.contenttype.model.field.LegacyFieldTypes.CUSTOM_FIELD; -import static com.dotcms.contenttype.model.type.PersonaContentType.PERSONA_KEY_TAG_FIELD_VAR; -import static com.dotcms.util.DotPreconditions.checkNotEmpty; -import static com.dotmarketing.business.PermissionAPI.PERMISSION_PUBLISH; -import static com.dotmarketing.business.PermissionAPI.PERMISSION_READ; -import static com.dotmarketing.business.PermissionAPI.PERMISSION_WRITE; -import static com.dotmarketing.util.UtilMethods.isNotSet; -import static com.liferay.util.StringPool.BLANK; -import static com.liferay.util.StringPool.COMMA; -import static com.liferay.util.StringPool.PERIOD; - import com.dotcms.business.CloseDBIfOpened; import com.dotcms.cdi.CDIUtils; import com.dotcms.content.business.ContentMappingAPI; @@ -89,6 +74,9 @@ import com.liferay.portal.model.User; import io.vavr.Lazy; import io.vavr.control.Try; +import org.apache.commons.collections.CollectionUtils; +import org.apache.commons.lang.time.FastDateFormat; + import java.io.IOException; import java.io.StringWriter; import java.text.DecimalFormat; @@ -111,8 +99,20 @@ import java.util.TimeZone; import java.util.function.Supplier; import java.util.stream.Collectors; -import org.apache.commons.collections.CollectionUtils; -import org.apache.commons.lang.time.FastDateFormat; + +import static com.dotcms.content.elasticsearch.constants.ESMappingConstants.CREATION_DATE; +import static com.dotcms.content.elasticsearch.constants.ESMappingConstants.PERSONA_KEY_TAG; +import static com.dotcms.content.elasticsearch.constants.ESMappingConstants.SYS_PUBLISH_USER; +import static com.dotcms.contenttype.model.field.LegacyFieldTypes.CUSTOM_FIELD; +import static com.dotcms.contenttype.model.type.PersonaContentType.PERSONA_KEY_TAG_FIELD_VAR; +import static com.dotcms.util.DotPreconditions.checkNotEmpty; +import static com.dotmarketing.business.PermissionAPI.PERMISSION_PUBLISH; +import static com.dotmarketing.business.PermissionAPI.PERMISSION_READ; +import static com.dotmarketing.business.PermissionAPI.PERMISSION_WRITE; +import static com.dotmarketing.util.UtilMethods.isNotSet; +import static com.liferay.util.StringPool.BLANK; +import static com.liferay.util.StringPool.COMMA; +import static com.liferay.util.StringPool.PERIOD; /** * Implementation class for the {@link ContentMappingAPI}. @@ -1237,25 +1237,37 @@ public String toJsonString(Map map) throws IOException{ return mapper.writeValueAsString(map); } + /** + * Returns the identifiers of the contentlets whose search index documents became stale + * because of changes to the given contentlet's relationships — i.e., the contents that must + * be re-indexed along with it. + * + *

The method compares the relationships referencing this contentlet in the search index + * (the state previous to the current save) against the rows in the {@code tree} table (the + * just-saved state). The symmetric difference of both sets — relationships that were removed + * plus relationships that were added — is the complete set of stale documents: only PARENT + * documents store relationship references (see + * {@link #loadRelationshipFields(Contentlet, Map, StringWriter)}), so contents whose + * relationship to this contentlet did not change never need re-indexing.

+ * + * @param contentlet The {@link Contentlet} being re-indexed + * @return Identifiers of the related contents that must be re-indexed along with it + */ @CloseDBIfOpened public List dependenciesLeftToReindex(final Contentlet contentlet) throws DotStateException, DotDataException, DotSecurityException { final List dependenciesToReindex = new ArrayList<>(); - - final String relatedSQL = "select tree.* from tree where child = ? order by tree_order"; final DotConnect db = new DotConnect(); db.setSQL(relatedSQL); db.addParam(contentlet.getIdentifier()); final List> relatedContentlets = db.loadResults(); - - if(!relatedContentlets.isEmpty()) { + if (!relatedContentlets.isEmpty()) { final List relationships = relationshipAPI .byContentType(contentlet.getContentType()); - for(final Relationship relationship : relationships) { - + for (final Relationship relationship : relationships) { // Both sides are collected into Sets of identifiers: the index holds one // document per language/variant of the same identifier, and only deduped // collections make the disjunction below a true symmetric difference @@ -1273,7 +1285,7 @@ public List dependenciesLeftToReindex(final Contentlet contentlet) throw // More documents reference this contentlet than a single search page can // return; a limit above MAX_LIMIT makes searchIndex switch to a scroll // search that returns them all - Logger.debug(this, () -> "More than " + ESContentletAPIImpl.MAX_LIMIT + Logger.warn(this, () -> "More than " + ESContentletAPIImpl.MAX_LIMIT + " index documents reference '" + contentlet.getIdentifier() + "' through '" + relationship.getRelationTypeValue() + "'. Falling back to a scroll search"); @@ -1285,12 +1297,14 @@ public List dependenciesLeftToReindex(final Contentlet contentlet) throw oldRelatedIds.add(result.getIdentifier()); } - relatedContentlets.stream().filter(map -> map.get(ESMappingConstants.RELATION_TYPE) - .equals(relationship.getRelationTypeValue())).forEach(entry -> { + relatedContentlets.stream() + .filter(map -> map.get(ESMappingConstants.RELATION_TYPE).equals(relationship.getRelationTypeValue())) + .forEach(entry -> { final String childId = entry.get(ESMappingConstants.CHILD); final String parentId = entry.get(ESMappingConstants.PARENT); newRelatedIds.add(contentlet.getIdentifier().equalsIgnoreCase(childId) - ? parentId : childId); + ? parentId + : childId); }); // The symmetric difference of both Sets is the actual dependency delta: the diff --git a/dotCMS/src/main/java/com/dotmarketing/business/VersionableFactory.java b/dotCMS/src/main/java/com/dotmarketing/business/VersionableFactory.java index c98a4d81f2f9..eff3a445027f 100644 --- a/dotCMS/src/main/java/com/dotmarketing/business/VersionableFactory.java +++ b/dotCMS/src/main/java/com/dotmarketing/business/VersionableFactory.java @@ -1,15 +1,16 @@ package com.dotmarketing.business; import com.dotcms.variant.model.Variant; -import java.util.List; -import java.util.Map; -import java.util.Optional; - import com.dotmarketing.beans.Identifier; import com.dotmarketing.beans.VersionInfo; import com.dotmarketing.exception.DotDataException; import com.dotmarketing.portlets.contentlet.model.ContentletVersionInfo; +import java.util.Collection; +import java.util.List; +import java.util.Map; +import java.util.Optional; + /** * Provides access to information regarding the different versions (working, * live, and version info) of a dotCMS content object -i.e., contents, @@ -277,6 +278,19 @@ protected abstract List findAllContentletVersionInfos(Str protected abstract List findAllContentletVersionInfos(String identifier, String variantName) throws DotDataException, DotStateException ; + /** + * Returns ALL the {@link ContentletVersionInfo} rows — every language and variant — for the + * given set of identifiers, reading them from the database in batches of bound parameters. + * Unlike {@link #findAllContentletVersionInfos(String)}, results are NOT cached: this method + * is meant for bulk read-only operations, such as collecting the working versions of related + * content that must be re-indexed. + * + * @param identifiers Identifiers of the contentlets whose version info rows are returned + * @return The {@link ContentletVersionInfo} rows found for the given identifiers + */ + public abstract List findAllContentletVersionInfos( + Collection identifiers) throws DotDataException; + /** * Return all versions of the {@link com.dotmarketing.portlets.contentlet.model.Contentlet} * for the specific {@link com.dotcms.variant.model.Variant} no matter the {@Link Language} diff --git a/dotCMS/src/main/java/com/dotmarketing/business/VersionableFactoryImpl.java b/dotCMS/src/main/java/com/dotmarketing/business/VersionableFactoryImpl.java index b5467bf9711a..3c8ada89c1fb 100644 --- a/dotCMS/src/main/java/com/dotmarketing/business/VersionableFactoryImpl.java +++ b/dotCMS/src/main/java/com/dotmarketing/business/VersionableFactoryImpl.java @@ -1,11 +1,7 @@ package com.dotmarketing.business; -import static com.dotcms.util.CollectionsUtils.set; -import static com.dotcms.variant.VariantAPI.DEFAULT_VARIANT; - import com.dotcms.concurrent.DotConcurrentFactory; import com.dotcms.concurrent.lock.IdentifierStripedLock; -import com.google.common.annotations.VisibleForTesting; import com.dotcms.util.transform.TransformerLocator; import com.dotcms.variant.model.Variant; import com.dotmarketing.beans.Identifier; @@ -16,7 +12,6 @@ import com.dotmarketing.db.HibernateUtil; import com.dotmarketing.exception.DotDataException; import com.dotmarketing.exception.DotRuntimeException; -import com.dotmarketing.exception.DotSecurityException; import com.dotmarketing.portlets.containers.business.ContainerAPI; import com.dotmarketing.portlets.containers.model.Container; import com.dotmarketing.portlets.contentlet.model.ContentletVersionInfo; @@ -26,8 +21,12 @@ import com.dotmarketing.util.Logger; import com.dotmarketing.util.UUIDGenerator; import com.dotmarketing.util.UtilMethods; +import com.google.common.annotations.VisibleForTesting; import com.liferay.portal.model.User; +import org.apache.commons.beanutils.BeanUtils; + import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.Date; import java.util.List; @@ -35,10 +34,10 @@ import java.util.Objects; import java.util.Optional; import java.util.Set; -import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; -import org.apache.commons.beanutils.BeanUtils; +import static com.dotcms.util.CollectionsUtils.set; +import static com.dotcms.variant.VariantAPI.DEFAULT_VARIANT; /** * Implementation class for the {@link VersionableFactory} class. @@ -460,6 +459,30 @@ private List findContentletVersionInfosInDB(final String : versionInfos; } + private static final int VERSION_INFO_LOOKUP_CHUNK_SIZE = 500; + + @Override + public List findAllContentletVersionInfos( + final Collection identifiers) throws DotDataException { + if (identifiers == null || identifiers.isEmpty()) { + return Collections.emptyList(); + } + final List idList = List.copyOf(identifiers); + final List versionInfos = new ArrayList<>(); + for (int from = 0; from < idList.size(); from += VERSION_INFO_LOOKUP_CHUNK_SIZE) { + final List chunk = idList.subList(from, + Math.min(from + VERSION_INFO_LOOKUP_CHUNK_SIZE, idList.size())); + final DotConnect dotConnect = new DotConnect().setSQL( + "SELECT * FROM contentlet_version_info WHERE identifier IN (" + + DotConnect.createParametersPlaceholder(chunk.size()) + ")"); + chunk.forEach(dotConnect::addParam); + versionInfos.addAll(TransformerLocator + .createContentletVersionInfoTransformer(dotConnect.loadObjectResults()) + .asList()); + } + return versionInfos; + } + @Override protected List findAllContentletVersionInfos(final String identifier) throws DotDataException, DotStateException { diff --git a/dotCMS/src/main/java/com/dotmarketing/factories/TreeFactory.java b/dotCMS/src/main/java/com/dotmarketing/factories/TreeFactory.java index c56b93c5a900..45a24ba1f36c 100644 --- a/dotCMS/src/main/java/com/dotmarketing/factories/TreeFactory.java +++ b/dotCMS/src/main/java/com/dotmarketing/factories/TreeFactory.java @@ -17,12 +17,27 @@ import java.util.Map; /** - * + * Provides low-level, direct database access to the {@code tree} table, which stores + * parent-child associations between dotCMS objects through the {@code parent}, {@code child}, + * {@code relation_type}, and {@code tree_order} columns. + * + *

Its most important use is persisting content relationships: each row links a parent + * contentlet identifier to a child contentlet identifier under the relationship's type value, + * with {@code tree_order} preserving the ordering of the related records. The table is also used + * by other legacy object associations, such as categories and menu links.

+ * + *

The table has no foreign keys and both sides of a row are plain strings, so callers — + * typically {@code ESContentletAPIImpl} and the relationship factories — are responsible for + * referential consistency and permission enforcement. In particular, the bulk delete operations + * remove rows regardless of user permissions; see the individual method docs for their + * contracts.

+ * * @author maria + * @since Mar 22nd, 2022 */ public class TreeFactory { - + private static final int DELETE_TREES_CHUNK_SIZE = 500; public static Tree getTree(final Inode parent, final Inode child) { return getTree(parent.getInode(), child.getInode()); @@ -352,6 +367,16 @@ public static void insertTree(Tree tree) { } } + /** + * Deletes ALL the tree rows for the given parent and relation type in a single statement, + * regardless of which children they point to. Callers are responsible for enforcing READ + * permissions BEFORE calling this method: when only some of the relationships may be + * removed, use {@link #deleteTreesByParentAndChildrenAndRelationType(String, List, String)} + * instead. + * + * @param parentId Identifier of the parent content + * @param relationType The relationship type value + */ public static void deleteTreesByParentAndRelationType(final String parentId, final String relationType) { try { @@ -365,6 +390,16 @@ public static void deleteTreesByParentAndRelationType(final String parentId, } } + /** + * Deletes ALL the tree rows for the given child and relation type in a single statement, + * regardless of which parents they point to. Callers are responsible for enforcing READ + * permissions BEFORE calling this method: when only some of the relationships may be + * removed, use {@link #deleteTreesByChildAndParentsAndRelationType(String, List, String)} + * instead. + * + * @param childId Identifier of the child content + * @param relationType The relationship type value + */ public static void deleteTreesByChildAndRelationType(final String childId, final String relationType) { try { @@ -406,6 +441,17 @@ public static List getRelatedIdsByChildAndRelationType(final String chil return getRelatedIds("parent", "child", childId, relationType); } + /** + * Shared lookup for the public identifier methods: returns the values of the + * {@code selectColumn} for the tree rows matching the given {@code whereColumn} value and + * relation type, ordered by tree order. + * + * @param selectColumn Column to return: {@code parent} or {@code child} + * @param whereColumn Column to filter by: the opposite side of {@code selectColumn} + * @param id Identifier to match in the {@code whereColumn} + * @param relationType The relationship type value + * @return The identifiers found, in tree order + */ private static List getRelatedIds(final String selectColumn, final String whereColumn, final String id, final String relationType) { try { @@ -422,6 +468,52 @@ private static List getRelatedIds(final String selectColumn, final Strin } } + /** + * Returns the next available tree order for the children of the given parent under the + * given relation type: the highest existing tree order plus one, or 1 when no rows exist. + * + * @param parentId Identifier of the parent content + * @param relationType The relationship type value + * @return The next tree order value + */ + public static int getNextTreeOrderByParentAndRelationType(final String parentId, + final String relationType) { + return getNextTreeOrder("parent", parentId, relationType); + } + + /** + * Returns the next available tree order for the parents of the given child under the given + * relation type: the highest existing tree order plus one, or 1 when no rows exist. + * + * @param childId Identifier of the child content + * @param relationType The relationship type value + * @return The next tree order value + */ + public static int getNextTreeOrderByChildAndRelationType(final String childId, + final String relationType) { + return getNextTreeOrder("child", childId, relationType); + } + + /** + * Shared lookup for the public next-tree-order methods: returns the highest + * {@code tree_order} plus one among the rows matching the given column value and relation + * type, or 1 when no rows exist. + * + * @param whereColumn Column to filter by: {@code parent} or {@code child} + * @param id Identifier to match in the {@code whereColumn} + * @param relationType The relationship type value + * @return The next tree order value + */ + private static int getNextTreeOrder(final String whereColumn, final String id, + final String relationType) { + return new DotConnect() + .setSQL("SELECT COALESCE(MAX(tree_order), 0) + 1 AS next_position FROM tree" + + " WHERE " + whereColumn + " = ? AND relation_type = ?") + .addParam(id) + .addParam(relationType) + .getInt("next_position"); + } + /** * Deletes the tree rows for the given parent and relation type whose child is in the given * list. Unlike {@link #deleteTreesByParentAndRelationType(String, String)}, rows pointing to @@ -450,8 +542,20 @@ public static void deleteTreesByChildAndParentsAndRelationType(final String chil deleteTreesScopedByRelationType("child", "parent", childId, parentIds, relationType); } - private static final int DELETE_TREES_CHUNK_SIZE = 500; - + /** + * Shared implementation of the scoped deletes: removes the tree rows whose + * {@code fixedColumn} matches the given identifier under the given relation type, but only + * when their {@code scopedColumn} value is included in the {@code scopedIds} list. Rows + * pointing to identifiers NOT in the list are preserved. The list is processed in chunks of + * {@code DELETE_TREES_CHUNK_SIZE} (500) identifiers, keeping the number of bound parameters + * per statement within safe database limits. + * + * @param fixedColumn Column matched against {@code fixedId}: {@code parent} or {@code child} + * @param scopedColumn The opposite side, matched against the {@code scopedIds} list + * @param fixedId Identifier of the content whose relationships are being deleted + * @param scopedIds Identifiers whose rows may be deleted + * @param relationType The relationship type value + */ private static void deleteTreesScopedByRelationType(final String fixedColumn, final String scopedColumn, final String fixedId, final List scopedIds, final String relationType) { From 48bc34639ad23a846e53152b641633bb71782b51 Mon Sep 17 00:00:00 2001 From: Jose Castro Date: Fri, 12 Jun 2026 11:36:50 -0600 Subject: [PATCH 4/4] Implementing feedback from Claude Bot. --- .../business/ESContentletAPIImpl.java | 5 +- .../dotmarketing/factories/TreeFactory.java | 52 ++++++++++++------- .../factories/TreeFactoryTest.java | 19 +++++++ 3 files changed, 55 insertions(+), 21 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java index a2aaba0c1b01..4d8be84c11b2 100644 --- a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java @@ -5139,11 +5139,12 @@ public void relateContent(final Contentlet contentlet, // READ are preserved by deleteRelatedContent. New rows are appended after the // highest surviving position so their relative order never collides with them; // when nothing survived, positions simply start at 1 + // The next position is only needed when this contentlet is the parent side: the + // child branch below positions its rows via positionInParent instead int treePosition = related.isHasParent() ? TreeFactory.getNextTreeOrderByParentAndRelationType( contentlet.getIdentifier(), relationship.getRelationTypeValue()) - : TreeFactory.getNextTreeOrderByChildAndRelationType( - contentlet.getIdentifier(), relationship.getRelationTypeValue()); + : 1; int positionInParent = 1; final List treesToInsert = new ArrayList<>(); for (final Contentlet relatedContent : related.getRecords()) { diff --git a/dotCMS/src/main/java/com/dotmarketing/factories/TreeFactory.java b/dotCMS/src/main/java/com/dotmarketing/factories/TreeFactory.java index 45a24ba1f36c..0cffa90a99b9 100644 --- a/dotCMS/src/main/java/com/dotmarketing/factories/TreeFactory.java +++ b/dotCMS/src/main/java/com/dotmarketing/factories/TreeFactory.java @@ -7,7 +7,9 @@ import com.dotmarketing.business.DotStateException; import com.dotmarketing.common.db.DotConnect; import com.dotmarketing.common.db.Params; +import com.dotmarketing.db.LocalTransaction; import com.dotmarketing.exception.DotDataException; +import com.dotmarketing.exception.DotSecurityException; import com.dotmarketing.util.Logger; import java.util.ArrayList; @@ -441,6 +443,20 @@ public static List getRelatedIdsByChildAndRelationType(final String chil return getRelatedIds("parent", "child", childId, relationType); } + /** + * Guards the private methods that interpolate tree column names into SQL: only the + * {@code parent} and {@code child} columns are legal. Any other value is a programming + * error and is rejected before it can reach the database. + * + * @param columnName The column name to validate + */ + private static void assertTreeColumn(final String columnName) { + if (!"parent".equals(columnName) && !"child".equals(columnName)) { + throw new IllegalArgumentException("Invalid tree column name: " + columnName + + ". Only 'parent' and 'child' are allowed"); + } + } + /** * Shared lookup for the public identifier methods: returns the values of the * {@code selectColumn} for the tree rows matching the given {@code whereColumn} value and @@ -454,6 +470,8 @@ public static List getRelatedIdsByChildAndRelationType(final String chil */ private static List getRelatedIds(final String selectColumn, final String whereColumn, final String id, final String relationType) { + assertTreeColumn(selectColumn); + assertTreeColumn(whereColumn); try { return new DotConnect() .setSQL("SELECT " + selectColumn + " FROM tree WHERE " + whereColumn @@ -481,18 +499,6 @@ public static int getNextTreeOrderByParentAndRelationType(final String parentId, return getNextTreeOrder("parent", parentId, relationType); } - /** - * Returns the next available tree order for the parents of the given child under the given - * relation type: the highest existing tree order plus one, or 1 when no rows exist. - * - * @param childId Identifier of the child content - * @param relationType The relationship type value - * @return The next tree order value - */ - public static int getNextTreeOrderByChildAndRelationType(final String childId, - final String relationType) { - return getNextTreeOrder("child", childId, relationType); - } /** * Shared lookup for the public next-tree-order methods: returns the highest @@ -506,6 +512,7 @@ public static int getNextTreeOrderByChildAndRelationType(final String childId, */ private static int getNextTreeOrder(final String whereColumn, final String id, final String relationType) { + assertTreeColumn(whereColumn); return new DotConnect() .setSQL("SELECT COALESCE(MAX(tree_order), 0) + 1 AS next_position FROM tree" + " WHERE " + whereColumn + " = ? AND relation_type = ?") @@ -559,6 +566,8 @@ public static void deleteTreesByChildAndParentsAndRelationType(final String chil private static void deleteTreesScopedByRelationType(final String fixedColumn, final String scopedColumn, final String fixedId, final List scopedIds, final String relationType) { + assertTreeColumn(fixedColumn); + assertTreeColumn(scopedColumn); if (scopedIds == null || scopedIds.isEmpty()) { return; } @@ -608,13 +617,18 @@ public static void insertTrees(final List trees) { } try { - new DotConnect().executeBatch( - "DELETE FROM tree WHERE child = ? AND parent = ? AND relation_type = ?", - deleteParams); - new DotConnect().executeBatch( - "INSERT INTO tree (child, parent, relation_type, tree_order) VALUES (?,?,?,?)", - insertParams); - } catch (final DotDataException e) { + // Both batches must be atomic for ANY caller: when a transaction is already open + // (e.g. relateContent) this joins it; standalone callers get their own one, so a + // failed insert can never leave the delete process committed on its own + LocalTransaction.wrap(() -> { + new DotConnect().executeBatch( + "DELETE FROM tree WHERE child = ? AND parent = ? AND relation_type = ?", + deleteParams); + new DotConnect().executeBatch( + "INSERT INTO tree (child, parent, relation_type, tree_order) VALUES (?,?,?,?)", + insertParams); + }); + } catch (final DotDataException | DotSecurityException e) { throw new DotStateException(e); } } diff --git a/dotcms-integration/src/test/java/com/dotmarketing/factories/TreeFactoryTest.java b/dotcms-integration/src/test/java/com/dotmarketing/factories/TreeFactoryTest.java index 05b71a170dbf..46890114b18c 100644 --- a/dotcms-integration/src/test/java/com/dotmarketing/factories/TreeFactoryTest.java +++ b/dotcms-integration/src/test/java/com/dotmarketing/factories/TreeFactoryTest.java @@ -189,4 +189,23 @@ public void getRelatedIdsReturnsIdentifiersInTreeOrder() { assertEquals(List.of(parent), parents); } + /** + * Method to test: {@link TreeFactory#getNextTreeOrderByParentAndRelationType(String, String)} + * When: no rows exist for the parent and relation type, and then a row with tree order 7 is + * inserted + * Should: return 1 first, and the highest existing tree order plus one afterwards + */ + @Test + public void getNextTreeOrderReturnsHighestOrderPlusOne() { + final String parent = newId(); + final String relationType = newRelationType(); + + assertEquals(1, + TreeFactory.getNextTreeOrderByParentAndRelationType(parent, relationType)); + + TreeFactory.insertTrees(List.of(new Tree(parent, newId(), relationType, 7))); + assertEquals(8, + TreeFactory.getNextTreeOrderByParentAndRelationType(parent, relationType)); + } + }