Merge pull request ModeShape#1421 from hchiorean/MODE-2401

MODE-2401 Fixed the fact that queryable children which were under parents that had the noquery attribute were not taken into account by the query engine.

modeshape-jcr/src/main/java/org/modeshape/jcr/cache/document/NodeCacheIterator.java

@@ -106,17 +106,24 @@ protected final void nextNode() {
                 continue;
             }
             if (filter != null && !filter.includeNode(node, cache)) {
-                // this node is excluded by the filter, so skip it ...
-                continue;
+                // the node should not be included by the filter but check if any of its children should be
+                if (filter.continueProcessingChildren(node, cache)) {
+                    nextKey = null;
+                } else {
+                    // this node is excluded by the filter, so skip it ...
+                    continue;
+                }
             }
             // Add all of the children onto the queue ...
             Iterator<NodeKey> iter = node.getChildReferences(cache).getAllKeys();
             while (iter.hasNext()) {
                 keys.add(iter.next());
             }
-            // Set the nextNode ref and return ...
+            // Set the nextNode ref and return if we have a valid node...
             this.nextNode = nextKey;
-            return;
+            if (nextKey != null) {
+                return;
+            }
         }
     }
 
@@ -137,14 +144,26 @@ public String toString() {
 
     public static interface NodeFilter {
         /**
-         * Determine if the supplied node is to be included in the iterator. If this method returns false, then the node and all
-         * descendants will be excluded from the iterator. By default, this method always returns true.
+         * Determine if the supplied node is to be included in the iterator. If this method returns false, the code should
+         * use the {@link org.modeshape.jcr.cache.document.NodeCacheIterator.NodeFilter#continueProcessingChildren(CachedNode, NodeCache)}
+         * method to determine if any of the children of the node should be processed.
+         * By default, this method always returns true.
          * 
          * @param node the node; never null
          * @param cache the node cache; never null
          * @return true if the node is to be included; or false otherwise
          */
         public boolean includeNode( CachedNode node,
                                     NodeCache cache );
+
+        /**
+         * Determine if for a given node that isn't included by 
+         * {@link org.modeshape.jcr.cache.document.NodeCacheIterator.NodeFilter#includeNode(CachedNode, NodeCache)}, its children 
+         * should be processed or not.
+         *                                                                
+         * @return {@code true} if any children of an excluded node should be taken into account, {@code false} otherwise.
+         */
+        public boolean continueProcessingChildren(CachedNode node,
+                                                  NodeCache cache);
     }
 }

modeshape-jcr/src/main/java/org/modeshape/jcr/query/engine/QuerySources.java

@@ -94,6 +94,12 @@ public final boolean includeNode( CachedNode node,
             public String toString() {
                 return "(queryable nodes)";
             }
+
+            @Override
+            public boolean continueProcessingChildren( CachedNode node, NodeCache cache ) {
+                Name nodePrimaryType = node.getPrimaryType(cache);
+                return node.isQueryable(cache) && !nodeTypes.isQueryable(nodePrimaryType);
+            }
         };
         if (!this.includeSystemContent) {
             final String systemWorkspaceKey = repo.getSystemWorkspaceKey();
@@ -112,6 +118,14 @@ public final boolean includeNode( CachedNode node,
                 public String toString() {
                     return "(queryable nodes not in workspace)";
                 }
+
+                @Override
+                public boolean continueProcessingChildren( CachedNode node, NodeCache cache ) {
+                    Name nodePrimaryType = node.getPrimaryType(cache);
+                    return  node.isQueryable(cache) &&
+                            !node.getKey().getWorkspaceKey().equals(systemWorkspaceKey) && 
+                            !nodeTypes.isQueryable(nodePrimaryType);
+                }
             };
         } else {
             this.queryableAndNonSystemFilter = queryableFilter;
@@ -502,6 +516,16 @@ public boolean includeNode( CachedNode node,
             return true;
         }
 
+        @Override
+        public boolean continueProcessingChildren( CachedNode node, NodeCache cache ) {
+            for (NodeFilter filter : filters) {
+                if (!filter.continueProcessingChildren(node, cache)) {
+                    return false;
+                }
+            }
+            return true;
+        }
+
         @Override
         public String toString() {
             return filters.isEmpty() ? "" : filters.toString();
@@ -533,6 +557,11 @@ public boolean includeNode( CachedNode node,
                 return true;
             }
 
+            @Override
+            public boolean continueProcessingChildren( CachedNode node, NodeCache cache ) {
+                return !shareableNodeKeys.contains(node.getKey());
+            }
+
             @Override
             public String toString() {
                 return "(excludes-sharables)";

modeshape-jcr/src/test/java/org/modeshape/jcr/JcrQueryManagerTest.java

@@ -26,6 +26,7 @@
 import static org.hamcrest.core.IsNull.notNullValue;
 import static org.hamcrest.core.IsNull.nullValue;
 import static org.hamcrest.core.IsSame.sameInstance;
+import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
@@ -51,6 +52,7 @@
 import java.util.Map;
 import java.util.NoSuchElementException;
 import java.util.Set;
+import java.util.TreeSet;
 import javax.jcr.ImportUUIDBehavior;
 import javax.jcr.NamespaceRegistry;
 import javax.jcr.Node;
@@ -4765,15 +4767,30 @@ public void shouldAllowMissingSelectorColumnsInQOM() throws Exception {
     @Test
     @FixFor( "MODE-2401" )
     public void shouldNotReturnNonQueryableNodeTypes() throws Exception {
-        session.getRootNode().addNode("folder1", "test:noQueryFolder");
-        session.getRootNode().addNode("folder2", "nt:folder"); 
+        Node folder1 = session.getRootNode().addNode("folder1", "test:noQueryFolder");
+        Node folder2 = session.getRootNode().addNode("folder2", "nt:folder");
+        Node folder3 = session.getRootNode().addNode("folder3", "test:noQueryFolder");
+        Node folder31 = folder3.addNode("folder3_1", "nt:folder");
+        Node folder311 = folder31.addNode("folder3_1", "test:noQueryFolder");
+        folder311.addNode("folder3_1_1", "nt:folder");
         session.save();
 
-        String sql = "SELECT folder.[jcr:name] FROM [nt:folder] AS folder";
-        Query query = session.getWorkspace().getQueryManager().createQuery(sql, Query.JCR_SQL2);
-        NodeIterator nodes = query.execute().getNodes();
-        assertEquals(1, nodes.getSize());
-        assertEquals("folder2", nodes.nextNode().getName());
+        try {
+            String sql = "SELECT folder.[jcr:name] FROM [nt:folder] AS folder";
+            Query query = session.getWorkspace().getQueryManager().createQuery(sql, Query.JCR_SQL2);
+            NodeIterator nodes = query.execute().getNodes();
+            assertEquals(3, nodes.getSize());
+            Set<String> names = new TreeSet<>();
+            while (nodes.hasNext()) {
+                names.add(nodes.nextNode().getName());
+            }
+            assertArrayEquals(new String[] { "folder2", "folder3_1", "folder3_1_1" }, names.toArray(new String[0]));
+        } finally {
+            folder1.remove();
+            folder2.remove();
+            folder3.remove();
+            session.save();
+        }
     }
 
     private void registerNodeType( String typeName ) throws RepositoryException {

modeshape-jcr/src/test/java/org/modeshape/jcr/LocalIndexProviderTest.java

@@ -1075,17 +1075,25 @@ public void shouldNotConsiderNonQueryableNodeTypes() throws RepositoryException,
         waitForIndexes();
 
         session.getRootNode().addNode("nonQueryableFolder", typeName);
-        session.getRootNode().addNode("regularFolder", "nt:folder");
+        session.getRootNode().addNode("regularFolder1", "nt:folder");
+        Node folder2 = session.getRootNode().addNode("regularFolder2", typeName);
+        folder2.addNode("subFolder", "nt:folder");
         session.save();
 
         waitForIndexes();
-
+        final List<String> expectedResults = new ArrayList<>(Arrays.asList("/regularFolder1", "/regularFolder2/subFolder"));
         Query query = jcrSql2Query("select folder.[jcr:name] FROM [nt:folder] as folder");
         validateQuery()
-                .rowCount(1L)
+                .rowCount(2L)
                 .useIndex("typesIndex")
-                .hasNodesAtPaths("/regularFolder")
+                .onEachRow(new ValidateQuery.Predicate() {
+                    @Override
+                    public void validate( int rowNumber, Row row ) throws RepositoryException {
+                        expectedResults.remove(row.getPath());
+                    }
+                })
                 .validate(query, query.execute());
+        assertTrue("Not all expected nodes found: " + expectedResults, expectedResults.isEmpty());
     }
 
     @FixFor( "MODE-2432 ")