From cb636d1140bbbaada081e6b86471e9b7771423ea Mon Sep 17 00:00:00 2001 From: Nana Insaidoo Date: Fri, 28 May 2021 09:16:38 +0100 Subject: [PATCH] MNT-22385 Cmis query GetTotalNumItems is returning wrong value (#504) * Changes made to correct the value of totalItems when performing a TMDQ * Fixes after review - Slight change was made to NodePermissionAssessor to log when permission limits are exceeded * Now pre-computing maxPermissionChecks value as per review suggestion --- .../querymodel/impl/db/DBQueryEngine.java | 40 ++++++++++++++----- .../impl/db/NodePermissionAssessor.java | 27 +++++++++---- .../resources/alfresco/repository.properties | 1 + .../Search/common-search-context.xml | 3 ++ .../querymodel/impl/db/DBQueryEngineTest.java | 15 ++++++- 5 files changed, 67 insertions(+), 19 deletions(-) diff --git a/repository/src/main/java/org/alfresco/repo/search/impl/querymodel/impl/db/DBQueryEngine.java b/repository/src/main/java/org/alfresco/repo/search/impl/querymodel/impl/db/DBQueryEngine.java index 2e47cf5f49..612f958a65 100644 --- a/repository/src/main/java/org/alfresco/repo/search/impl/querymodel/impl/db/DBQueryEngine.java +++ b/repository/src/main/java/org/alfresco/repo/search/impl/querymodel/impl/db/DBQueryEngine.java @@ -112,6 +112,8 @@ public class DBQueryEngine implements QueryEngine private long maxPermissionCheckTimeMillis; + private boolean maxPermissionCheckEnabled; + protected EntityLookupCache nodesCache; private List> stores; @@ -122,7 +124,7 @@ public class DBQueryEngine implements QueryEngine { this.aclCrudDAO = aclCrudDAO; } - + public void setMaxPermissionChecks(int maxPermissionChecks) { this.maxPermissionChecks = maxPermissionChecks; @@ -132,6 +134,11 @@ public class DBQueryEngine implements QueryEngine { this.maxPermissionCheckTimeMillis = maxPermissionCheckTimeMillis; } + + public void setMaxPermissionCheckEnabled(boolean maxPermissionCheckEnabled) + { + this.maxPermissionCheckEnabled = maxPermissionCheckEnabled; + } public void setTemplate(SqlSessionTemplate template) { @@ -329,13 +336,7 @@ public class DBQueryEngine implements QueryEngine @Override public void handleResult(ResultContext context) { - doHandleResult(permissionAssessor, nodes, requiredNodes, context); - } - - private void doHandleResult(NodePermissionAssessor permissionAssessor, List nodes, - int requiredNodes, ResultContext context) - { - if (nodes.size() >= requiredNodes) + if (!maxPermissionCheckEnabled && nodes.size() >= requiredNodes) { context.stop(); return; @@ -344,7 +345,7 @@ public class DBQueryEngine implements QueryEngine Node node = context.getResultObject(); addStoreInfo(node); - boolean shouldCache = nodes.size() >= options.getSkipCount(); + boolean shouldCache = shouldCache(options, nodes, requiredNodes); if(shouldCache) { logger.debug("- selected node "+nodes.size()+": "+node.getUuid()+" "+node.getId()); @@ -357,7 +358,14 @@ public class DBQueryEngine implements QueryEngine if (permissionAssessor.isIncluded(node)) { - nodes.add(shouldCache ? node : null); + if (nodes.size() > requiredNodes) + { + nodes.add(node); + } + else + { + nodes.add(shouldCache ? node : null); + } } if (permissionAssessor.shouldQuitChecks()) @@ -366,6 +374,18 @@ public class DBQueryEngine implements QueryEngine return; } } + + private boolean shouldCache(QueryOptions options, List nodes, int requiredNodes) + { + if (nodes.size() > requiredNodes) + { + return false; + } + else + { + return nodes.size() >= options.getSkipCount(); + } + } }); int numberFound = nodes.size(); diff --git a/repository/src/main/java/org/alfresco/repo/search/impl/querymodel/impl/db/NodePermissionAssessor.java b/repository/src/main/java/org/alfresco/repo/search/impl/querymodel/impl/db/NodePermissionAssessor.java index 534ff1b459..1381789fca 100644 --- a/repository/src/main/java/org/alfresco/repo/search/impl/querymodel/impl/db/NodePermissionAssessor.java +++ b/repository/src/main/java/org/alfresco/repo/search/impl/querymodel/impl/db/NodePermissionAssessor.java @@ -41,9 +41,13 @@ import org.alfresco.service.cmr.repository.datatype.DefaultTypeConverter; import org.alfresco.service.cmr.security.PermissionService; import org.alfresco.service.namespace.QName; import org.alfresco.util.EqualsHelper; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; public class NodePermissionAssessor { + protected static final Log logger = LogFactory.getLog(NodePermissionAssessor.class); + private final boolean isSystemReading; private final boolean isAdminReading; private final boolean isNullReading; @@ -138,24 +142,31 @@ public class NodePermissionAssessor public void setMaxPermissionChecks(int maxPermissionChecks) { - this.maxPermissionChecks = maxPermissionChecks; + if (maxPermissionChecks == Integer.MAX_VALUE) + { + this.maxPermissionChecks = maxPermissionChecks; + } + else + { + this.maxPermissionChecks = maxPermissionChecks + 1; + } } public boolean shouldQuitChecks() { - boolean result = false; - if (checksPerformed >= maxPermissionChecks) { - result = true; + logger.warn("Maximum permission checks exceeded (" + maxPermissionChecks + ")"); + return true; } - + if ((System.currentTimeMillis() - startTime) >= maxPermissionCheckTimeMillis) { - result = true; + logger.warn("Maximum permission checks time exceeded (" + maxPermissionCheckTimeMillis + ")"); + return true; } - - return result; + + return false; } public void setMaxPermissionCheckTimeMillis(long maxPermissionCheckTimeMillis) diff --git a/repository/src/main/resources/alfresco/repository.properties b/repository/src/main/resources/alfresco/repository.properties index 6012d8c185..ef6bade8a1 100644 --- a/repository/src/main/resources/alfresco/repository.properties +++ b/repository/src/main/resources/alfresco/repository.properties @@ -153,6 +153,7 @@ system.cache.parentAssocs.limitFactor=8 system.acl.maxPermissionCheckTimeMillis=10000 # The maximum number of search results to perform permission checks against system.acl.maxPermissionChecks=1000 +system.acl.maxPermissionCheckEnabled=false # The maximum number of filefolder list results system.filefolderservice.defaultListMaxResults=5000 diff --git a/repository/src/main/resources/alfresco/subsystems/Search/common-search-context.xml b/repository/src/main/resources/alfresco/subsystems/Search/common-search-context.xml index 1421de0b3a..7410e27f2b 100644 --- a/repository/src/main/resources/alfresco/subsystems/Search/common-search-context.xml +++ b/repository/src/main/resources/alfresco/subsystems/Search/common-search-context.xml @@ -126,6 +126,9 @@ ${system.acl.maxPermissionCheckTimeMillis} + + ${system.acl.maxPermissionCheckEnabled} + diff --git a/repository/src/test/java/org/alfresco/repo/search/impl/querymodel/impl/db/DBQueryEngineTest.java b/repository/src/test/java/org/alfresco/repo/search/impl/querymodel/impl/db/DBQueryEngineTest.java index de25ef22c2..5c42c56223 100644 --- a/repository/src/test/java/org/alfresco/repo/search/impl/querymodel/impl/db/DBQueryEngineTest.java +++ b/repository/src/test/java/org/alfresco/repo/search/impl/querymodel/impl/db/DBQueryEngineTest.java @@ -167,7 +167,20 @@ public class DBQueryEngineTest assertNodePresent(6, result); assertNodePresent(7, result); } - + + @Test + public void shouldResultSetLengthMatchTheAmountOfAllAccessibleNodesWhenMaxPermissionCheckEnabled() + { + withMaxItems(5); + prepareTemplate(dbQuery, createNodes(10)); + when(assessor.isIncluded(any(Node.class))).thenReturn(true); + + engine.setMaxPermissionCheckEnabled(true); + FilteringResultSet result = engine.acceleratedNodeSelection(options, dbQuery, assessor); + + assertEquals(10, result.length()); + } + @Test public void shouldNotConsiderInaccessibleNodesInResultSetWhenSkippingNodes() {