SLING-12265 - Improve node type registration in JCR_OAK (#37)

Instead of calling CndImporter (and triggering one commit per CND file),
parse each CND file separately and register them all at once. This
doesn't just reduce the number of commits, but also takes care of
inter-type dependencies.
diff --git a/core/src/main/java/org/apache/sling/testing/mock/sling/NodeTypeDefinitionScanner.java b/core/src/main/java/org/apache/sling/testing/mock/sling/NodeTypeDefinitionScanner.java
index d0c1370..8783e19 100644
--- a/core/src/main/java/org/apache/sling/testing/mock/sling/NodeTypeDefinitionScanner.java
+++ b/core/src/main/java/org/apache/sling/testing/mock/sling/NodeTypeDefinitionScanner.java
@@ -22,8 +22,9 @@
 import java.io.InputStreamReader;
 import java.io.Reader;
 import java.util.ArrayList;
-import java.util.Iterator;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 import javax.jcr.NamespaceRegistry;
 import javax.jcr.RepositoryException;
@@ -41,7 +42,6 @@
 import javax.jcr.nodetype.PropertyDefinition;
 import javax.jcr.nodetype.PropertyDefinitionTemplate;
 
-import org.apache.jackrabbit.commons.cnd.CndImporter;
 import org.apache.jackrabbit.commons.cnd.CompactNodeTypeDefReader;
 import org.apache.jackrabbit.commons.cnd.DefinitionBuilderFactory;
 import org.apache.jackrabbit.commons.cnd.TemplateBuilderFactory;
@@ -58,8 +58,6 @@
 
     private static final NodeTypeDefinitionScanner SINGLETON = new NodeTypeDefinitionScanner();
 
-    private static final int MAX_ITERATIONS = 5;
-
     private static final Logger log = LoggerFactory.getLogger(NodeTypeDefinitionScanner.class);
 
     private final List<String> nodeTypeDefinitions;
@@ -161,50 +159,40 @@
       NodeTypeManager nodeTypeManager = workspace.getNodeTypeManager();
       NamespaceRegistry namespaceRegistry = workspace.getNamespaceRegistry();
       ValueFactory valueFactory = session.getValueFactory();
+      DefinitionBuilderFactory<NodeTypeTemplate, NamespaceRegistry> factory =
+            new TemplateBuilderFactory(nodeTypeManager, valueFactory, namespaceRegistry);
 
-      // try registering node types multiple times because the exact order is not known
-      int iteration = 0;
-      List<String> remainingNodeTypeResources = new ArrayList<String>(nodeTypeResources);
-      while (!remainingNodeTypeResources.isEmpty()) {
-          registerNodeTypesAndRemoveSucceeds(remainingNodeTypeResources, classLoader, nodeTypeManager, namespaceRegistry, valueFactory, false);
-          iteration++;
-          if (iteration >= MAX_ITERATIONS) {
-              break;
-          }
+      Map<String, NodeTypeTemplate> nodeTypes = new HashMap<>();
+      for (String resource : nodeTypeResources) {
+          nodeTypes.putAll(parseNodeTypesFromResource(resource, classLoader, factory));
       }
-      if (!remainingNodeTypeResources.isEmpty()) {
-          registerNodeTypesAndRemoveSucceeds(remainingNodeTypeResources, classLoader, nodeTypeManager, namespaceRegistry, valueFactory, true);
-      }
+
+      nodeTypeManager.registerNodeTypes(nodeTypes.values().toArray(new NodeTypeTemplate[0]), true);
     }
 
     /**
-     * Register node types found in classpath in JCR repository, and remove those that succeeded to register from the list.
-     * @param nodeTypeResources List of nodetype classpath resources
-     * @param classLoader
-     * @param nodeTypeManager
-     * @param namespaceRegistry
-     * @param valueFactory
-     * @param logError if true, and error is logged if node type registration failed. Otherwise it is ignored.
+     * Parses a CND file present on the classpath and returns the node types found within.
+     * @param resource The resource name.
+     * @param classLoader The classloader to load resources with.
+     * @param factory The factory to build node type definitions with.
+     * @return A mapping from node type names to node type definitions.
      */
-    private void registerNodeTypesAndRemoveSucceeds(List<String> nodeTypeResources, ClassLoader classLoader,
-            NodeTypeManager nodeTypeManager, NamespaceRegistry namespaceRegistry, ValueFactory valueFactory,
-            boolean logError) {
-        Iterator<String> nodeTypeResourcesIterator = nodeTypeResources.iterator();
-        while (nodeTypeResourcesIterator.hasNext()) {
-            String nodeTypeResource = nodeTypeResourcesIterator.next();
-            try (InputStream is = classLoader.getResourceAsStream(nodeTypeResource)) {
-                if (is == null) {
-                    continue;
-                }
-                Reader reader = new InputStreamReader(is);
-                CndImporter.registerNodeTypes(reader, nodeTypeResource, nodeTypeManager, namespaceRegistry, valueFactory, true);
-                nodeTypeResourcesIterator.remove();
+    private Map<String, NodeTypeTemplate> parseNodeTypesFromResource(String resource, ClassLoader classLoader,
+            DefinitionBuilderFactory<NodeTypeTemplate, NamespaceRegistry> factory) {
+        try (InputStream is = classLoader.getResourceAsStream(resource)) {
+            if (is == null) {
+                return Map.of();
             }
-            catch (Throwable ex) {
-                if (logError) {
-                    log.warn("Unable to register node type: " + nodeTypeResource, ex);
-                }
+            CompactNodeTypeDefReader<NodeTypeTemplate, NamespaceRegistry> cndReader =
+                new CompactNodeTypeDefReader<>(new InputStreamReader(is), resource, factory);
+            Map<String, NodeTypeTemplate> result = new HashMap<>();
+            for (NodeTypeTemplate template : cndReader.getNodeTypeDefinitions()) {
+                result.put(template.getName(), template);
             }
+            return result;
+        } catch (Throwable ex) {
+            log.warn("Failed to parse CND resource: " + resource, ex);
+            return Map.of();
         }
     }
 
diff --git a/core/src/test/java/org/apache/sling/testing/mock/sling/NodeTypeDefinitionScannerTest.java b/core/src/test/java/org/apache/sling/testing/mock/sling/NodeTypeDefinitionScannerTest.java
index 07df1cb..44d8291 100644
--- a/core/src/test/java/org/apache/sling/testing/mock/sling/NodeTypeDefinitionScannerTest.java
+++ b/core/src/test/java/org/apache/sling/testing/mock/sling/NodeTypeDefinitionScannerTest.java
@@ -20,8 +20,14 @@
 
 import static org.junit.Assert.assertTrue;
 
+import javax.jcr.Session;
+import javax.jcr.nodetype.NodeTypeIterator;
+import java.io.StringReader;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 
+import org.apache.sling.testing.mock.jcr.MockJcr;
 import org.junit.Test;
 
 
@@ -36,4 +42,22 @@
         assertTrue(definitions.contains("SLING-INF/nodetypes/resource.cnd"));
     }
 
+    @Test
+    public void testRegistersNodeTypes() throws Exception {
+        Session session = MockJcr.newSession();
+        MockJcr.loadNodeTypeDefs(session, new StringReader(
+            "[nt:hierarchyNode] > nt:base\n" +
+            "[nt:folder] > nt:hierarchyNode"));
+
+        NodeTypeDefinitionScanner.get().register(session, NodeTypeMode.NODETYPES_REQUIRED);
+
+        Set<String> nodeTypes = new HashSet<>();
+        NodeTypeIterator nodeTypeIterator = session.getWorkspace().getNodeTypeManager().getAllNodeTypes();
+        while (nodeTypeIterator.hasNext()) {
+            nodeTypes.add(nodeTypeIterator.nextNodeType().getName());
+        }
+        assertTrue(nodeTypes.containsAll(Set.of("sling:Folder", "sling:HierarchyNode", "sling:OrderedFolder",
+            "sling:Resource", "sling:ResourceSuperType")));
+    }
+
 }
diff --git a/core/src/test/resources/META-INF/MANIFEST.MF b/core/src/test/resources/META-INF/MANIFEST.MF
index 5423d41..af35ab1 100644
--- a/core/src/test/resources/META-INF/MANIFEST.MF
+++ b/core/src/test/resources/META-INF/MANIFEST.MF
@@ -1 +1,3 @@
 Sling-Model-Packages: org.apache.sling.testing.mock.sling.context.modelsautoreg
+Sling-Nodetypes: SLING-INF/nodetypes/missing.cnd,
+ SLING-INF/nodetypes/syntax-error.cnd
diff --git a/core/src/test/resources/SLING-INF/nodetypes/syntax-error.cnd b/core/src/test/resources/SLING-INF/nodetypes/syntax-error.cnd
new file mode 100644
index 0000000..28aba1b
--- /dev/null
+++ b/core/src/test/resources/SLING-INF/nodetypes/syntax-error.cnd
@@ -0,0 +1 @@
+!!! This is a deliberately invalid CND file !!!
\ No newline at end of file