[ofbiz-framework] branch trunk updated: Reverted: "Fixed: Remove dependency management from ‘ComponentContainer’" (OFBIZ-11275)

[mthl]

This is an automated email from the ASF dual-hosted git repository.

mthl pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git


The following commit(s) were added to refs/heads/trunk by this push:
     new a521de9  Reverted: "Fixed: Remove dependency management from ‘ComponentContainer’" (OFBIZ-11275)
a521de9 is described below

commit a521de9ad8a0b5a0f9ceadab348c46d7961ff89a
Author: Mathieu Lirzin <[hidden email]>
AuthorDate: Sat Nov 9 00:50:56 2019 +0100

    Reverted: "Fixed: Remove dependency management from ‘ComponentContainer’"
    (OFBIZ-11275)
   
    This reverts commit 3d3533cf5e04b303e5d7a4b0f178c54f10ddf620.
   
    ‘./gradlew cleanAll loadAll’ do not work anymore.
---
 build.gradle                                       |   2 +-
 .../base/component/AlreadyLoadedException.java     |  44 ++++++++
 .../ofbiz/base/component/ComponentConfig.java      |  61 +----------
 .../ofbiz/base/container/ComponentContainer.java   | 120 +++++++++++++++++++--
 .../base/container/ComponentContainerTest.java     |   2 +
 5 files changed, 160 insertions(+), 69 deletions(-)

diff --git a/build.gradle b/build.gradle
index 0395b8c..6c8ac6c 100644
--- a/build.gradle
+++ b/build.gradle
@@ -309,7 +309,7 @@ checkstyle {
     // the sum of errors that were present before introducing the
     // ‘checkstyle’ tool present in the framework and in the official
     // plugins.
-    tasks.checkstyleMain.maxErrors = 37776
+    tasks.checkstyleMain.maxErrors = 37779
     // Currently there are a lot of errors so we need to temporarily
     // hide them to avoid polluting the terminal output.
     showViolations = false
diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/component/AlreadyLoadedException.java b/framework/base/src/main/java/org/apache/ofbiz/base/component/AlreadyLoadedException.java
new file mode 100644
index 0000000..4485ccf
--- /dev/null
+++ b/framework/base/src/main/java/org/apache/ofbiz/base/component/AlreadyLoadedException.java
@@ -0,0 +1,44 @@
+/*******************************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *******************************************************************************/
+package org.apache.ofbiz.base.component;
+
+/**
+ * Component Already Loaded Exception
+ *
+ */
+@SuppressWarnings("serial")
+public class AlreadyLoadedException extends ComponentException {
+
+    public AlreadyLoadedException() {
+        super();
+    }
+
+    public AlreadyLoadedException(String str) {
+        super(str);
+    }
+
+    public AlreadyLoadedException(Throwable nested) {
+        super(nested);
+    }
+
+    public AlreadyLoadedException(String str, Throwable nested) {
+        super(str, nested);
+    }
+}
+
diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/component/ComponentConfig.java b/framework/base/src/main/java/org/apache/ofbiz/base/component/ComponentConfig.java
index c1a1ada..4a39885 100644
--- a/framework/base/src/main/java/org/apache/ofbiz/base/component/ComponentConfig.java
+++ b/framework/base/src/main/java/org/apache/ofbiz/base/component/ComponentConfig.java
@@ -18,9 +18,6 @@
  *******************************************************************************/
 package org.apache.ofbiz.base.component;
 
-import static java.util.function.Function.identity;
-import static java.util.stream.Collectors.toMap;
-
 import java.io.InputStream;
 import java.net.URL;
 import java.nio.file.Files;
@@ -43,7 +40,6 @@ import org.apache.ofbiz.base.container.ContainerConfig;
 import org.apache.ofbiz.base.location.FlexibleLocation;
 import org.apache.ofbiz.base.util.Assert;
 import org.apache.ofbiz.base.util.Debug;
-import org.apache.ofbiz.base.util.Digraph;
 import org.apache.ofbiz.base.util.KeyStoreUtil;
 import org.apache.ofbiz.base.util.UtilURL;
 import org.apache.ofbiz.base.util.UtilValidate;
@@ -95,16 +91,6 @@ public final class ComponentConfig {
         return componentConfigCache.values();
     }
 
-    /**
-     * Sorts the cached component configurations.
-     *
-     * @throws ComponentException when a component configuration contains an invalid dependency
-     *         or if the dependency graph contains a cycle.
-     */
-    public static void sortDependencies() throws ComponentException {
-        componentConfigCache.sort();
-    }
-
     public static Stream<ComponentConfig> components() {
         return componentConfigCache.values().stream();
     }
@@ -695,8 +681,6 @@ public final class ComponentConfig {
         private final Map<String, ComponentConfig> componentConfigs = new LinkedHashMap<>();
         // Root location mapped to global name.
         private final Map<String, String> componentLocations = new HashMap<>();
-        /** Sorted component configurations (based on depends-on declaration if ofbiz-component.xml). */
-        private List<ComponentConfig> componentConfigsSorted;
 
         private synchronized ComponentConfig fromGlobalName(String globalName) {
             return componentConfigs.get(globalName);
@@ -717,49 +701,8 @@ public final class ComponentConfig {
             return componentConfigs.put(globalName, config);
         }
 
-        /**
-         * Provides a sequence of cached component configurations.
-         *
-         * <p>the order of the sequence is arbitrary unless the {@link #sort())} has been invoked beforehand.
-         *
-         * @return the list of cached component configurations
-         */
-        synchronized List<ComponentConfig> values() {
-            // When the configurations have not been sorted use arbitrary order.
-            List<ComponentConfig> res = (componentConfigsSorted == null)
-                    ? new ArrayList<>(componentConfigs.values())
-                    : componentConfigsSorted;
-            return Collections.unmodifiableList(res);
-        }
-
-        /**
-         * Provides the stored dependencies as component configurations.
-         *
-         * <p>Handle invalid dependencies by creating dummy component configurations.
-         *
-         * @param the component configuration containing dependencies
-         * @return a collection of component configurations dependencies.
-         */
-        private Collection<ComponentConfig> componentConfigDeps(ComponentConfig cc) {
-            return cc.getDependsOn().stream()
-                    .map(dep -> {
-                        ComponentConfig storedCc = componentConfigs.get(dep);
-                        return (storedCc != null) ? storedCc : new Builder().globalName(dep).create();
-                    })
-                    .collect(Collectors.toList());
-        }
-
-        /**
-         * Sorts the component configurations based on their “depends-on” XML elements.
-         */
-        synchronized void sort() throws ComponentException {
-            Map<ComponentConfig, Collection<ComponentConfig>> graphSpec = componentConfigs.values().stream()
-                    .collect(toMap(identity(), this::componentConfigDeps));
-            try {
-                componentConfigsSorted = new Digraph<>(graphSpec).sort();
-            } catch (IllegalArgumentException | IllegalStateException e) {
-                throw new ComponentException("failed to initialize components", e);
-            }
+        synchronized Collection<ComponentConfig> values() {
+            return Collections.unmodifiableList(new ArrayList<>(componentConfigs.values()));
         }
     }
 
diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/container/ComponentContainer.java b/framework/base/src/main/java/org/apache/ofbiz/base/container/ComponentContainer.java
index 31f6e60..3f14707 100644
--- a/framework/base/src/main/java/org/apache/ofbiz/base/container/ComponentContainer.java
+++ b/framework/base/src/main/java/org/apache/ofbiz/base/container/ComponentContainer.java
@@ -26,7 +26,13 @@ import java.net.URLClassLoader;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -38,6 +44,7 @@ import org.apache.ofbiz.base.component.ComponentLoaderConfig.ComponentDef;
 import org.apache.ofbiz.base.start.Start;
 import org.apache.ofbiz.base.start.StartupCommand;
 import org.apache.ofbiz.base.util.Debug;
+import org.apache.ofbiz.base.util.UtilValidate;
 
 /**
  * ComponentContainer - StartupContainer implementation for Components
@@ -55,6 +62,9 @@ public class ComponentContainer implements Container {
 
     private String name;
     private final AtomicBoolean loaded = new AtomicBoolean(false);
+    /** The set of ready components in their inverse dependency order. */
+    private final LinkedHashSet<ComponentConfig> readyComponents = new LinkedHashSet<>();
+    private static Map<String, List<String>> toBeLoadedComponents = new ConcurrentHashMap<>();
 
     @Override
     public void init(List<StartupCommand> ofbizCommands, String name, String configFile) throws ContainerException {
@@ -79,12 +89,11 @@ public class ComponentContainer implements Container {
             for (ComponentDef def: ComponentLoaderConfig.getRootComponents()) {
                 loadComponent(ofbizHome, def);
             }
-            ComponentConfig.sortDependencies();
         } catch (IOException | ComponentException e) {
             throw new ContainerException(e);
         }
         String fmt = "Added class path for component : [%s]";
-        List<Classpath> componentsClassPath = ComponentConfig.components()
+        List<Classpath> componentsClassPath = readyComponents.stream()
                 .peek(cmpnt -> Debug.logInfo(String.format(fmt, cmpnt.getComponentName()), module))
                 .map(cmpnt -> buildClasspathFromComponentConfig(cmpnt))
                 .collect(Collectors.toList());
@@ -125,15 +134,19 @@ public class ComponentContainer implements Container {
      * @param dir  the location where the component should be loaded
      * @param component  a single component or a component directory definition
      * @throws IOException when component directory loading fails.
+     * @throws ComponentException when retrieving component configuration files fails.
      */
-    private void loadComponent(Path dir, ComponentDef component) throws IOException {
+    private void loadComponent(Path dir, ComponentDef component) throws IOException, ComponentException {
         Path location = component.location.isAbsolute() ? component.location : dir.resolve(component.location);
         switch (component.type) {
         case COMPONENT_DIRECTORY:
             loadComponentDirectory(location);
             break;
         case SINGLE_COMPONENT:
-            retrieveComponentConfig(null, location);
+            ComponentConfig config = retrieveComponentConfig(null, location);
+            if (config != null) {
+                loadSingleComponent(config);
+            }
             break;
         }
     }
@@ -144,8 +157,9 @@ public class ComponentContainer implements Container {
      *
      * @param directoryName the name of component directory to load
      * @throws IOException
+     * @throws ComponentException
      */
-    private void loadComponentDirectory(Path directoryName) throws IOException {
+    private void loadComponentDirectory(Path directoryName) throws IOException, ComponentException {
         Debug.logInfo("Auto-Loading component directory : [" + directoryName + "]", module);
         if (Files.exists(directoryName) && Files.isDirectory(directoryName)) {
             Path componentLoad = directoryName.resolve(ComponentLoaderConfig.COMPONENT_LOAD_XML_FILENAME);
@@ -191,15 +205,56 @@ public class ComponentContainer implements Container {
      * for loading purposes
      *
      * @param directoryPath a valid absolute path of a component directory
-     * @throws IOException if an I/O error occurs when opening the directory
+     * @throws IOException
+     * @throws ComponentException
      */
-    private static void loadComponentsInDirectory(Path directoryPath) throws IOException {
+    private void loadComponentsInDirectory(Path directoryPath) throws IOException, ComponentException {
         try (Stream<Path> paths = Files.list(directoryPath)) {
-            paths.sorted()
+            List<ComponentConfig> componentConfigs = paths.sorted()
                     .map(cmpnt -> directoryPath.resolve(cmpnt).toAbsolutePath().normalize())
                     .filter(Files::isDirectory)
                     .filter(dir -> Files.exists(dir.resolve(ComponentConfig.OFBIZ_COMPONENT_XML_FILENAME)))
-                    .forEach(componentDir -> retrieveComponentConfig(null, componentDir));
+                    .map(componentDir -> retrieveComponentConfig(null, componentDir))
+                    .filter(Objects::nonNull)
+                    .collect(Collectors.toList());
+
+            for (ComponentConfig cmpnt : componentConfigs) {
+                loadSingleComponent(cmpnt);
+            }
+        }
+        loadComponentWithDependency();
+    }
+
+    /**
+     * Checks dependency for unloaded components and add them into
+     * componentsClassPath
+     *
+     * @throws IOException
+     * @throws ComponentException
+     */
+    private void loadComponentWithDependency() throws IOException, ComponentException {
+        while (true) {
+            if (UtilValidate.isEmpty(toBeLoadedComponents)) {
+                return;
+            } else {
+                for (Map.Entry<String, List<String>> entries : toBeLoadedComponents.entrySet()) {
+                    ComponentConfig config = retrieveComponentConfig(entries.getKey(), null);
+                    if (config.enabled()) {
+                        List<String> dependencyList = checkDependencyForComponent(config);
+                        if (UtilValidate.isNotEmpty(dependencyList)) {
+                            toBeLoadedComponents.replace(config.getComponentName(), dependencyList);
+                            String msg = "Not loading component [" + config.getComponentName() + "] because it's dependent Component is not loaded [ " + dependencyList + "]";
+                            Debug.logInfo(msg, module);
+                        }
+                        if (UtilValidate.isEmpty(dependencyList)) {
+                            readyComponents.add(config);
+                            toBeLoadedComponents.replace(config.getComponentName(), dependencyList);
+                        }
+                    } else {
+                        Debug.logInfo("Not loading component [" + config.getComponentName() + "] because it's disabled", module);
+                    }
+                }
+            }
         }
     }
 
@@ -224,6 +279,53 @@ public class ComponentContainer implements Container {
     }
 
     /**
+     * Load a single component by adding all its classpath entries to
+     * the list of classpaths to be loaded
+     *
+     * @param config the component configuration
+     * @throws ComponentException
+     */
+    private void loadSingleComponent(ComponentConfig config) throws ComponentException {
+        if (config.enabled()) {
+            List<String> dependencyList = checkDependencyForComponent(config);
+            if (UtilValidate.isEmpty(dependencyList)) {
+                readyComponents.add(config);
+            }
+        } else {
+            Debug.logInfo("Not loading component [" + config.getComponentName() + "] because it's disabled", module);
+        }
+    }
+
+    /**
+     * Check for components loaded and Removes loaded components dependency
+     * from list of unloaded components
+     *
+     * @param config the component configuration
+     * @throws ComponentException
+     */
+    private List<String> checkDependencyForComponent(ComponentConfig config) throws ComponentException {
+        List<String> dependencyList = new ArrayList<>(config.getDependsOn());
+        if (UtilValidate.isNotEmpty(dependencyList)) {
+            Set<String> resolvedDependencyList = new HashSet<>();
+            for (String dependency : dependencyList) {
+                Debug.logInfo("Component : " + config.getComponentName() + " is Dependent on  " + dependency, module);
+                ComponentConfig componentConfig = ComponentConfig.getComponentConfig(String.valueOf(dependency));
+                if (readyComponents.contains(componentConfig)) {
+                    resolvedDependencyList.add(dependency);
+                }
+            }
+            resolvedDependencyList.forEach(resolvedDependency -> Debug.logInfo("Resolved : " + resolvedDependency + " Dependency for Component " + config.getComponentName(), module));
+            dependencyList.removeAll(resolvedDependencyList);
+            if (UtilValidate.isEmpty(dependencyList)) {
+                toBeLoadedComponents.remove(config.getComponentName());
+            } else {
+                toBeLoadedComponents.put(config.getComponentName(), dependencyList);
+            }
+        }
+        return dependencyList;
+    }
+
+    /**
      * Constructs a {@code Classpath} object for a specific component definition.
      *
      * @param config  the component configuration
diff --git a/framework/base/src/test/java/org/apache/ofbiz/base/container/ComponentContainerTest.java b/framework/base/src/test/java/org/apache/ofbiz/base/container/ComponentContainerTest.java
index 57be05a..358f0bc 100644
--- a/framework/base/src/test/java/org/apache/ofbiz/base/container/ComponentContainerTest.java
+++ b/framework/base/src/test/java/org/apache/ofbiz/base/container/ComponentContainerTest.java
@@ -35,6 +35,7 @@ import org.apache.ofbiz.base.component.ComponentConfig;
 import org.apache.ofbiz.base.start.StartupException;
 import org.junit.After;
 import org.junit.Before;
+import org.junit.Ignore;
 import org.junit.Test;
 
 
@@ -60,6 +61,7 @@ public class ComponentContainerTest {
         }
     }
 
+    @Ignore("Dependency declarations do not impact the components order")
     @Test
     public void testCheckDependencyForComponent() throws ContainerException, StartupException, MalformedURLException {
         ComponentContainer containerObj = new ComponentContainer();