Skip to content

Commit

Permalink
Fixed: Remove dependency management from ‘ComponentContainer’
Browse files Browse the repository at this point in the history 
(OFBIZ-11275)

This fixes a bug identified by ‘testCheckDependencyForComponent’ where
the dependencies declared inside component configurations were not
impacting the component retrieval order.

Component configurations stored in the cache can now be properly
sorted based on their <depends-on> declaration.  The new
‘ComponentConfig#sortDependencies’ method returns a collection of
component configuration following a topological ordering.

In case of dependency cycle we throw an error.
  • Loading branch information
@mthl
Samuel Trégouët authored and mthl committed Nov 25, 2019
1 parent 3d555f5 commit 0003252
Show file tree
Hide file tree
Showing 5 changed files with 70 additions and 160 deletions.
2 changes: 1 addition & 1 deletion build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,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 = 37779
tasks.checkstyleMain.maxErrors = 37776
// Currently there are a lot of errors so we need to temporarily
// hide them to avoid polluting the terminal output.
showViolations = false
Expand Down
44 changes: 0 additions & 44 deletions framework/base/src/main/java/org/apache/ofbiz/base/component/AlreadyLoadedException.java
View file

This file was deleted.

62 changes: 60 additions & 2 deletions framework/base/src/main/java/org/apache/ofbiz/base/component/ComponentConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
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;
Expand Down Expand Up @@ -91,6 +92,16 @@ public static Collection<ComponentConfig> getAllComponents() {
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();
}
Expand Down Expand Up @@ -681,6 +692,8 @@ static final class ComponentConfigCache {
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);
Expand All @@ -701,8 +714,53 @@ synchronized ComponentConfig put(ComponentConfig config) {
return componentConfigs.put(globalName, config);
}

synchronized Collection<ComponentConfig> values() {
return Collections.unmodifiableList(new ArrayList<>(componentConfigs.values()));
/**
* 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 {
/* Create the dependency graph specification with a LinkedHashMap to preserve
implicit dependencies defined by `component-load.xml` files. */
Map<ComponentConfig, Collection<ComponentConfig>> graphSpec = new LinkedHashMap<>();
componentConfigs.values().forEach(cc -> {
graphSpec.put(cc, componentConfigDeps(cc));
});
try {
componentConfigsSorted = new Digraph<>(graphSpec).sort();
} catch (IllegalArgumentException | IllegalStateException e) {
throw new ComponentException("failed to initialize components", e);
}
}
}

Expand Down
120 changes: 9 additions & 111 deletions framework/base/src/main/java/org/apache/ofbiz/base/container/ComponentContainer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,7 @@
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;
Expand All @@ -44,7 +38,6 @@
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
Expand All @@ -62,9 +55,6 @@ 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 {
Expand All @@ -89,11 +79,12 @@ void init(String name, Path ofbizHome) throws ContainerException {
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 = readyComponents.stream()
List<Classpath> componentsClassPath = ComponentConfig.components()
.peek(cmpnt -> Debug.logInfo(String.format(fmt, cmpnt.getComponentName()), module))
.map(cmpnt -> buildClasspathFromComponentConfig(cmpnt))
.collect(Collectors.toList());
Expand Down Expand Up @@ -134,19 +125,15 @@ private static void loadClassPathForAllComponents(List<Classpath> componentsClas
* @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, ComponentException {
private void loadComponent(Path dir, ComponentDef component) throws IOException {
Path location = component.location.isAbsolute() ? component.location : dir.resolve(component.location);
switch (component.type) {
case COMPONENT_DIRECTORY:
loadComponentDirectory(location);
break;
case SINGLE_COMPONENT:
ComponentConfig config = retrieveComponentConfig(null, location);
if (config != null) {
loadSingleComponent(config);
}
retrieveComponentConfig(null, location);
break;
}
}
Expand All @@ -157,9 +144,8 @@ private void loadComponent(Path dir, ComponentDef component) throws IOException,
*
* @param directoryName the name of component directory to load
* @throws IOException
* @throws ComponentException
*/
private void loadComponentDirectory(Path directoryName) throws IOException, ComponentException {
private void loadComponentDirectory(Path directoryName) throws IOException {
Debug.logInfo("Auto-Loading component directory : [" + directoryName + "]", module);
if (Files.exists(directoryName) && Files.isDirectory(directoryName)) {
Path componentLoad = directoryName.resolve(ComponentLoaderConfig.COMPONENT_LOAD_XML_FILENAME);
Expand Down Expand Up @@ -205,56 +191,15 @@ private void loadComponentsInDirectoryUsingLoadFile(Path directoryPath, Path com
* for loading purposes
*
* @param directoryPath a valid absolute path of a component directory
* @throws IOException
* @throws ComponentException
* @throws IOException if an I/O error occurs when opening the directory
*/
private void loadComponentsInDirectory(Path directoryPath) throws IOException, ComponentException {
private static void loadComponentsInDirectory(Path directoryPath) throws IOException {
try (Stream<Path> paths = Files.list(directoryPath)) {
List<ComponentConfig> componentConfigs = paths.sorted()
paths.sorted()
.map(cmpnt -> directoryPath.resolve(cmpnt).toAbsolutePath().normalize())
.filter(Files::isDirectory)
.filter(dir -> Files.exists(dir.resolve(ComponentConfig.OFBIZ_COMPONENT_XML_FILENAME)))
.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);
}
}
}
.forEach(componentDir -> retrieveComponentConfig(null, componentDir));
}
}

Expand All @@ -278,53 +223,6 @@ private static ComponentConfig retrieveComponentConfig(String name, Path locatio
return config;
}

/**
* 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.
*
Expand Down
2 changes: 0 additions & 2 deletions framework/base/src/test/java/org/apache/ofbiz/base/container/ComponentContainerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
import org.apache.ofbiz.base.start.StartupException;
import org.junit.After;
import org.junit.Before;
import org.junit.Ignore;
import org.junit.Test;


Expand All @@ -61,7 +60,6 @@ public void cleanUp() throws IOException {
}
}

@Ignore("Dependency declarations do not impact the components order")
@Test
public void testCheckDependencyForComponent() throws ContainerException, StartupException, MalformedURLException {
ComponentContainer containerObj = new ComponentContainer();
Expand Down

0 comments on commit 0003252

Please sign in to comment.