Re: [PR] SLING-12265 - Improve node type registration in JCR_OAK [sling-org-apache-sling-testing-sling-mock]

["stefanseifert (via GitHub)" <gi...@apache.org>]

stefanseifert commented on code in PR #37:
URL: https://github.com/apache/sling-org-apache-sling-testing-sling-mock/pull/37#discussion_r1557767226


##########
core/src/main/java/org/apache/sling/testing/mock/sling/NodeTypeDefinitionScanner.java:
##########
@@ -161,50 +160,75 @@ private void registerNodeTypes(Session session, List<String> nodeTypeResources)
       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);
+      for (NodeTypeTemplate template : nodeTypes.values()) {
+          ensureNtBase(template, nodeTypes, nodeTypeManager);
       }
+
+      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();
+            }
+            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);
             }
-            catch (Throwable ex) {
-                if (logError) {
-                    log.warn("Unable to register node type: " + nodeTypeResource, ex);
+            return result;
+        } catch (Throwable ex) {
+            log.warn("Failed to parse CND resource: " + resource, ex);
+            return Map.of();
+        }
+    }
+
+    /**
+     * Add an implied nt:base supertype explicitly to the node type definition.
+     * @param nodeTypeTemplate The definition to update.
+     * @param templates The mapping of all definitions that are going to be added.
+     * @param nodeTypeManager Node type manager of the target repository, for looking up existing types.
+     * @throws RepositoryException If any issues happen while querying type information from the repository.
+     */
+    private static void ensureNtBase(NodeTypeTemplate nodeTypeTemplate, Map<String, NodeTypeTemplate> templates,

Review Comment:
   i do not understand the need for the code of this method. what does it do/improve?
   i deleted it locally and all works expected without it - the unit tests in this project, and also the unit tests e.g. from AEM Mocks seem to work without it code. what should it achieve? are you trying to fix something else here not related to this PR?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org