Repository: adobe-consulting-services/acs-aem-commons
Issue: TraversingCursor when creating ACL packages

Hi,

we're running into TraversingCursor warnings when building an ACL package:

```
31.01.2015 18:48:13.956 *WARN* [0:0:0:0:0:0:0:1 [1422724031258] POST /etc/acs-commons/packagers/test/_jcr_content.package.json HTTP/1.1] org.apache.jackrabbit.oak.spi.query.Cursors$TraversingCursor Traversed 3455000 nodes with filter Filter(query=SELECT * FROM [rep:ACL], path=*); consider creating an index or changing the query
```

Guess that comes from findResources(QUERY, QUERY_LANG) in https://github.com/Adobe-Consulting-Services/acs-aem-commons/blob/master/bundle/src/main/java/com/adobe/acs/commons/packaging/impl/ACLPackagerServletImpl.java#L224

What is the recommended solution? Create an index and fingers crossed that the query finishes faster? :)

What is the reason for query on `path=*` ?


Changed files:
bundle/src/test/java/com/adobe/acs/commons/packaging/impl/ACLPackagerServletImplTest.java
bundle/src/main/java/com/adobe/acs/commons/packaging/impl/ACLPackagerServletImpl.java

diff --git a/bundle/src/main/java/com/adobe/acs/commons/packaging/impl/ACLPackagerServletImpl.java b/bundle/src/main/java/com/adobe/acs/commons/packaging/impl/ACLPackagerServletImpl.java
index ebb2e8f77..606cfd353 100644
--- a/bundle/src/main/java/com/adobe/acs/commons/packaging/impl/ACLPackagerServletImpl.java
+++ b/bundle/src/main/java/com/adobe/acs/commons/packaging/impl/ACLPackagerServletImpl.java
@@ -21,6 +21,7 @@
 package com.adobe.acs.commons.packaging.impl;
 
 import com.adobe.acs.commons.packaging.PackageHelper;
+import com.adobe.acs.commons.util.AemCapabilityHelper;
 import com.day.cq.wcm.api.Page;
 import com.day.cq.wcm.api.PageManager;
 import com.day.jcr.vault.fs.api.PathFilterSet;
@@ -105,16 +106,26 @@ public class ACLPackagerServletImpl extends SlingAllMethodsServlet {
 
     private static final boolean DEFAULT_INCLUDE_CONFIGURATION = false;
 
-    private static final String QUERY = "SELECT * FROM [rep:ACL]";
-
     private static final String QUERY_LANG = Query.JCR_SQL2;
 
+    private static final String CQ5_QUERY = "SELECT * FROM [rep:ACL]";
+
+    private static final String[] CQ5_QUERIES = new String[] {CQ5_QUERY};
+
+    // rep:ACE covers rep:GrantACE and rep:DenyACE
+    private static final String AEM6_QUERY_ACE = "SELECT * FROM [rep:ACE] where [rep:principalName] is not null";
+
+    private static final String[] AEM6_QUERIES = new String[] {AEM6_QUERY_ACE};
+
     private static final String ACL_PACKAGE_THUMBNAIL_RESOURCE_PATH =
             "/apps/acs-commons/components/utilities/packager/acl-packager/definition/package-thumbnail.png";
 
     @Reference
     private PackageHelper packageHelper;
 
+    @Reference
+    private AemCapabilityHelper aemCapabilityHelper;
+
     @Override
     public final void doPost(final SlingHttpServletRequest request,
                              final SlingHttpServletResponse response) throws IOException {
@@ -217,36 +228,60 @@ public class ACLPackagerServletImpl extends SlingAllMethodsServlet {
     private List<PathFilterSet> findResources(final ResourceResolver resourceResolver,
                                               final List<String> principalNames,
                                               final List<Pattern> includePatterns) {
+        boolean isOak = true;
+        try {
+            isOak = aemCapabilityHelper.isOak();
+        } catch (RepositoryException e) {
+            isOak = true;
+        }
 
         final Set<Resource> resources = new TreeSet<Resource>(resourceComparator);
         final List<PathFilterSet> pathFilterSets = new ArrayList<PathFilterSet>();
 
-        final Iterator<Resource> repPolicies = resourceResolver.findResources(QUERY, QUERY_LANG);
+        String[] queries = CQ5_QUERIES;
+        if (isOak) {
+            queries = AEM6_QUERIES;
+        }
 
-        while (repPolicies.hasNext()) {
-            final Resource repPolicy = repPolicies.next();
+        for (final String query : queries) {
+            final Iterator<Resource> hits = resourceResolver.findResources(query, QUERY_LANG);
+
+            while (hits.hasNext()) {
+                final Resource hit = hits.next();
+                Resource repPolicy = null;
+
+                if (isOak) {
+                    // If Oak, get the parent node since the query is for the Grant/Deny nodes
+                    if (hit.getParent() != null) {
+                        repPolicy = hit.getParent();
+                    }
+                } else {
+                    // If not Oak, then the rep:ACL is the hit
+                    repPolicy = hit;
+                }
 
-            if (this.isIncluded(repPolicy, includePatterns)) {
-                log.debug("Included by pattern [ {} ]", repPolicy.getPath());
-            } else {
-                continue;
-            }
+                if (this.isIncluded(repPolicy, includePatterns)) {
+                    log.debug("Included by pattern [ {} ]", repPolicy.getPath());
+                } else {
+                    continue;
+                }
 
-            final Iterator<Resource> aces = repPolicy.listChildren();
+                final Iterator<Resource> aces = repPolicy.listChildren();
 
-            while (aces.hasNext()) {
-                final Resource ace = aces.next();
-                final ValueMap props = ace.adaptTo(ValueMap.class);
-                final String repPrincipalName = props.get("rep:principalName", String.class);
+                while (aces.hasNext()) {
+                    final Resource ace = aces.next();
+                    final ValueMap props = ace.adaptTo(ValueMap.class);
+                    final String repPrincipalName = props.get("rep:principalName", String.class);
 
-                if (principalNames == null
-                        || principalNames.isEmpty()
-                        || principalNames.contains(repPrincipalName)) {
+                    if (principalNames == null
+                            || principalNames.isEmpty()
+                            || principalNames.contains(repPrincipalName)) {
 
-                    resources.add(repPolicy);
+                        resources.add(repPolicy);
 
-                    log.debug("Included by principal [ {} ]", repPolicy.getPath());
-                    break;
+                        log.debug("Included by principal [ {} ]", repPolicy.getPath());
+                        break;
+                    }
                 }
             }
         }
@@ -317,14 +352,25 @@ public class ACLPackagerServletImpl extends SlingAllMethodsServlet {
      * @return true if the resource's path matches any of the include patterns
      */
     private boolean isIncluded(final Resource resource, final List<Pattern> includePatterns) {
-        if (includePatterns == null || includePatterns.isEmpty()) {
-            return true;
+        if (resource == null) {
+            // Resource is null; so dont accept this
+            return false;
+        } else if (!resource.isResourceType("rep:ACL")) {
+            // ONLY accept the resource is a rep:ACL node
+           return false;
         }
 
-        for (final Pattern pattern : includePatterns) {
-            final Matcher matcher = pattern.matcher(resource.getPath());
-            if (matcher.matches()) {
-                return true;
+        if (includePatterns == null || includePatterns.isEmpty()) {
+            // If patterns are empty then accept everything!
+            return true;
+        } else {
+            // Else check the patterns
+            for (final Pattern pattern : includePatterns) {
+                final Matcher matcher = pattern.matcher(resource.getPath());
+                if (matcher.matches()) {
+                    // Accept the resource on the first match
+                    return true;
+                }
             }
         }
 
diff --git a/bundle/src/test/java/com/adobe/acs/commons/packaging/impl/ACLPackagerServletImplTest.java b/bundle/src/test/java/com/adobe/acs/commons/packaging/impl/ACLPackagerServletImplTest.java
index 5667bb5f7..b525811ed 100644
--- a/bundle/src/test/java/com/adobe/acs/commons/packaging/impl/ACLPackagerServletImplTest.java
+++ b/bundle/src/test/java/com/adobe/acs/commons/packaging/impl/ACLPackagerServletImplTest.java
@@ -21,6 +21,7 @@
 package com.adobe.acs.commons.packaging.impl;
 
 import com.adobe.acs.commons.packaging.PackageHelper;
+import com.adobe.acs.commons.util.AemCapabilityHelper;
 import com.day.jcr.vault.packaging.JcrPackage;
 import com.day.jcr.vault.packaging.JcrPackageManager;
 import com.day.jcr.vault.packaging.Version;
@@ -63,22 +64,39 @@ public class ACLPackagerServletImplTest {
     static final String CONTENT_RESOURCE_PATH = "/etc/acs-commons/packages/acl-packager-test/jcr:content";
     static final String CONTENT_RESOURCE_TYPE = "acs-commons/components/utilities/packager/acl-packager";
 
-    SuccessMockResourceResolver resourceResolver = new SuccessMockResourceResolver();
+    Crx2SuccessMockResourceResolver crx2ResourceResolver = new Crx2SuccessMockResourceResolver();
+    OakSuccessMockResourceResolver oakResourceResolver = new OakSuccessMockResourceResolver();
+
     ErrorMockResourceResolver errorResourceResolver = new ErrorMockResourceResolver();
 
     @Spy
-    MockResource contentResource = new MockResource(resourceResolver, CONTENT_RESOURCE_PATH, CONTENT_RESOURCE_TYPE);
+    MockResource crx2ContentResource = new MockResource(crx2ResourceResolver, CONTENT_RESOURCE_PATH, CONTENT_RESOURCE_TYPE);
+
+    @Spy
+    MockResource crx2ConfigurationResource = new MockResource(crx2ResourceResolver,
+            CONTENT_RESOURCE_PATH + "/configuration", "");
+
+
+    @Spy
+    MockResource oakContentResource = new MockResource(oakResourceResolver, CONTENT_RESOURCE_PATH,
+            CONTENT_RESOURCE_TYPE);
 
     @Spy
-    MockResource configurationResource = new MockResource(resourceResolver,
+    MockResource oakConfigurationResource = new MockResource(oakResourceResolver,
             CONTENT_RESOURCE_PATH + "/configuration", "");
 
+
     @Mock
     PackageHelper packageHelper;
 
-    @Mock JcrPackage jcrPackage;
+    @Mock
+    JcrPackage jcrPackage;
+
+    @Mock
+    AemCapabilityHelper aemCapabilityHelper;
 
-    @Mock Session session;
+    @Mock
+    Session session;
 
     @InjectMocks
     ACLPackagerServletImpl aclPackagerServlet;
@@ -95,10 +113,14 @@ public class ACLPackagerServletImplTest {
 
         ValueMap properties = new ValueMapDecorator(map);
 
-        doReturn(properties).when(configurationResource).adaptTo(ValueMap.class);
+        doReturn(properties).when(crx2ConfigurationResource).adaptTo(ValueMap.class);
+        doReturn(properties).when(oakConfigurationResource).adaptTo(ValueMap.class);
 
-        resourceResolver.addResource(contentResource);
-        resourceResolver.addResource(configurationResource);
+        crx2ResourceResolver.addResource(crx2ContentResource);
+        crx2ResourceResolver.addResource(crx2ConfigurationResource);
+
+        oakResourceResolver.addResource(oakContentResource);
+        oakResourceResolver.addResource(oakConfigurationResource);
 
         /* PackageHelper Mocks */
         when(packageHelper.getNextVersion(any(JcrPackageManager.class), any(String.class), any(String.class),
@@ -111,6 +133,7 @@ public class ACLPackagerServletImplTest {
         when(packageHelper.getSuccessJSON(any(JcrPackage.class))).thenReturn("{\\"status\\": \\"success\\"}");
         when(packageHelper.getErrorJSON(any(String.class))).thenReturn("{\\"status\\": \\"error\\"}");
         when(packageHelper.getPathFilterSetPreviewJSON(any(Collection.class))).thenReturn("{\\"status\\": \\"preview\\"}");
+
     }
 
     @After
@@ -119,7 +142,9 @@ public class ACLPackagerServletImplTest {
     }
 
     @Test
-    public void testDoPost_success() throws Exception {
+    public void testDoPost_crx2_success() throws Exception {
+        when(aemCapabilityHelper.isOak()).thenReturn(false);
+
         final String resourcePath = "/etc/acs-commons/packages/acl-packager-test/jcr:content";
         final String selectors = "package";
         final String extension = "json";
@@ -128,13 +153,40 @@ public class ACLPackagerServletImplTest {
 
         MockSlingHttpServletRequest request = new MockSlingHttpServletRequest(resourcePath, selectors,
                 extension, suffix, queryString);
-        request.setResourceResolver(resourceResolver);
+        request.setResourceResolver(crx2ResourceResolver);
         request.setMethod("POST");
 
         MockSlingHttpServletResponse response = new MockSlingHttpServletResponse();
 
-        request.setResourceResolver(resourceResolver);
-        request.setResource(contentResource);
+        request.setResourceResolver(crx2ResourceResolver);
+        request.setResource(crx2ContentResource);
+
+        aclPackagerServlet.doPost(request, response);
+
+        final JSONObject actual = new JSONObject(response.getOutput().toString());
+        assertEquals("success", actual.optString("status", "error"));
+    }
+
+
+    @Test
+    public void testDoPost_oak_success() throws Exception {
+        when(aemCapabilityHelper.isOak()).thenReturn(true);
+
+        final String resourcePath = "/etc/acs-commons/packages/acl-packager-test/jcr:content";
+        final String selectors = "package";
+        final String extension = "json";
+        final String suffix = "";
+        final String queryString = "";
+
+        MockSlingHttpServletRequest request = new MockSlingHttpServletRequest(resourcePath, selectors,
+                extension, suffix, queryString);
+        request.setResourceResolver(oakResourceResolver);
+        request.setMethod("POST");
+
+        MockSlingHttpServletResponse response = new MockSlingHttpServletResponse();
+
+        request.setResourceResolver(oakResourceResolver);
+        request.setResource(oakContentResource);
 
         aclPackagerServlet.doPost(request, response);
 
@@ -157,8 +209,8 @@ public class ACLPackagerServletImplTest {
 
         MockSlingHttpServletResponse response = new MockSlingHttpServletResponse();
 
-        request.setResourceResolver(resourceResolver);
-        request.setResource(contentResource);
+        request.setResourceResolver(crx2ResourceResolver);
+        request.setResource(crx2ContentResource);
 
         aclPackagerServlet.doPost(request, response);
         String tmp = response.getOutput().toString();
@@ -184,7 +236,7 @@ public class ACLPackagerServletImplTest {
         MockSlingHttpServletResponse response = new MockSlingHttpServletResponse();
 
         request.setResourceResolver(errorResourceResolver);
-        request.setResource(contentResource);
+        request.setResource(crx2ContentResource);
 
         aclPackagerServlet.doPost(request, response);
 
@@ -193,18 +245,18 @@ public class ACLPackagerServletImplTest {
     }
 
 
-    private class SuccessMockResourceResolver extends MockResourceResolver {
+    private class Crx2SuccessMockResourceResolver extends MockResourceResolver {
         final List<Resource> results;
 
-        public SuccessMockResourceResolver() {
+        public Crx2SuccessMockResourceResolver() {
             super();
 
             results = new LinkedList<Resource>();
-            results.add(new MockResource(this, "/content/dam/rep:policy", ""));
-            results.add(new MockResource(this, "/content/acs-commons/rep:policy", ""));
-            results.add(new MockResource(this, "/etc/workflow/packages/rep:policy", ""));
-            results.add(new MockResource(this, "/var/audit/1/2/3/rep:policy", ""));
-            results.add(new MockResource(this, "/home/groups/authors/rep:policy", ""));
+            results.add(new MockResource(this, "/content/dam/rep:policy", "rep:ACL"));
+            results.add(new MockResource(this, "/content/acs-commons/rep:policy", "rep:ACL"));
+            results.add(new MockResource(this, "/etc/workflow/packages/rep:policy", "rep:ACL"));
+            results.add(new MockResource(this, "/var/audit/1/2/3/rep:policy", "rep:ACL"));
+            results.add(new MockResource(this, "/home/groups/authors/rep:policy", "rep:ACL"));
 
             for(Resource resource : results) {
                 this.addResource(resource);
@@ -258,6 +310,71 @@ public class ACLPackagerServletImplTest {
         }
     }
 
+    private class OakSuccessMockResourceResolver extends MockResourceResolver {
+        final List<Resource> results;
+
+        public OakSuccessMockResourceResolver() {
+            super();
+
+            results = new LinkedList<Resource>();
+            results.add(new MockResource(this, "/content/dam/rep:policy/allow0", ""));
+            results.add(new MockResource(this, "/content/dam/rep:policy/allow1", ""));
+            results.add(new MockResource(this, "/content/acs-commons/rep:policy/deny", ""));
+            results.add(new MockResource(this, "/etc/workflow/packages/rep:policy/allow", ""));
+            results.add(new MockResource(this, "/var/audit/1/2/3/rep:policy/deny", ""));
+            results.add(new MockResource(this, "/home/groups/authors/rep:policy/allow", ""));
+
+            for(Resource resource : results) {
+                this.addResource(resource);
+            }
+
+            this.addResource(new MockResource(this, "/content/dam/rep:policy", "rep:ACL"));
+            this.addResource(new MockResource(this, "/content/acs-commons/rep:policy", "rep:ACL"));
+            this.addResource(new MockResource(this, "/etc/workflow/packages/rep:policy", "rep:ACL"));
+            this.addResource(new MockResource(this, "/var/audit/1/2/3/rep:policy", "rep:ACL"));
+            this.addResource(new MockResource(this, "/home/groups/authors/rep:policy", "rep:ACL"));
+        }
+
+        @Override
+        public Iterable<Resource> getChildren(final Resource resource) {
+            return null;
+        }
+
+        public Iterator<Resource> findResources(String query, String language) {
+            return this.results.iterator();
+        }
+
+        @Override
+        public void delete(final Resource resource) throws PersistenceException {
+
+        }
+
+        @Override
+        public Resource create(final Resource resource, final String s, final Map<String, Object> stringObjectMap) throws PersistenceException {
+            return null;
+        }
+
+        @Override
+        public void revert() {
+
+        }
+
+        @Override
+        public void commit() throws PersistenceException {
+
+        }
+
+        @Override
+        public boolean hasChanges() {
+            return false;
+        }
+
+        @SuppressWarnings("unchecked")
+        public <AdapterType> AdapterType adaptTo(Class<AdapterType> type) {
+            return (AdapterType) session;
+        }
+    }
+
     private class ErrorMockResourceResolver extends MockResourceResolver {
         final List<Resource> results;
 
@@ -265,11 +382,11 @@ public class ACLPackagerServletImplTest {
             super();
 
             results = new LinkedList<Resource>();
-            results.add(new MockResource(this, "/content/dam/rep:policy", ""));
-            results.add(new MockResource(this, "/content/acs-commons/rep:policy", ""));
-            results.add(new MockResource(this, "/etc/workflow/packages/rep:policy", ""));
-            results.add(new MockResource(this, "/var/audit/1/2/3/rep:policy", ""));
-            results.add(new MockResource(this, "/home/groups/authors/rep:policy", ""));
+            results.add(new MockResource(this, "/content/dam/rep:policy", "rep:ACL"));
+            results.add(new MockResource(this, "/content/acs-commons/rep:policy", "rep:ACL"));
+            results.add(new MockResource(this, "/etc/workflow/packages/rep:policy", "rep:ACL"));
+            results.add(new MockResource(this, "/var/audit/1/2/3/rep:policy", "rep:ACL"));
+            results.add(new MockResource(this, "/home/groups/authors/rep:policy", "rep:ACL"));
 
             for(Resource resource : results) {
                 this.addResource(resource);