OFBiz › OFBiz - Commits

[ofbiz-framework] branch trunk updated (8aea160 -> 03b69b3)

_‹ Previous Topic Next Topic _›

Classic

List

Threaded

5 messages Options

mthl

[ofbiz-framework] branch trunk updated (8aea160 -> 03b69b3)

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

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

from 8aea160 Fixed: Getting policy error while editing html text data using cms (OFBIZ-11265)
new 1317703 Implemented: Show dependency resolution algorithm problem (OFBIZ-11275)
new 7044af8 Implemented: Add a generic directed graph utilitary class (OFBIZ-11275)
new 3d3533c Fixed: Remove dependency management from ‘ComponentContainer’ (OFBIZ-11275)
new 03b69b3 Fixed: Fix linting issues (OFBIZ-11265)

The 4 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.

Summary of changes:
build.gradle | 4 +-
config/checkstyle/checkstyle.xml | 5 +-
.../base/component/AlreadyLoadedException.java | 44 -------
.../ofbiz/base/component/ComponentConfig.java | 61 +++++++++-
.../ofbiz/base/container/ComponentContainer.java | 133 ++++-----------------
.../java/org/apache/ofbiz/base/util/Digraph.java | 118 ++++++++++++++++++
.../base/container/ComponentContainerTest.java | 82 +++++++++++++
.../org/apache/ofbiz/base/util/DiGraphTest.java | 93 ++++++++++++++
.../org/apache/ofbiz/base/util/UtilCodecTests.java | 3 +-
.../org/apache/ofbiz/service/ModelService.java | 4 +-
.../applications/accounting}/ofbiz-component.xml | 11 +-
.../applications/order}/ofbiz-component.xml | 12 +-
12 files changed, 401 insertions(+), 169 deletions(-)
delete mode 100644 framework/base/src/main/java/org/apache/ofbiz/base/component/AlreadyLoadedException.java
create mode 100644 framework/base/src/main/java/org/apache/ofbiz/base/util/Digraph.java
create mode 100644 framework/base/src/test/java/org/apache/ofbiz/base/container/ComponentContainerTest.java
create mode 100644 framework/base/src/test/java/org/apache/ofbiz/base/util/DiGraphTest.java
copy {applications/securityext => testsdata/ComponentContainerTest/applications/accounting}/ofbiz-component.xml (78%)
copy {framework/minilang => testsdata/ComponentContainerTest/applications/order}/ofbiz-component.xml (79%)

mthl

[ofbiz-framework] 01/04: Implemented: Show dependency resolution algorithm problem (OFBIZ-11275)

mthl

[ofbiz-framework] 02/04: Implemented: Add a generic directed graph utilitary class (OFBIZ-11275)

In reply to this post by mthl

mthl

[ofbiz-framework] 03/04: Fixed: Remove dependency management from ‘ComponentContainer’ (OFBIZ-11275)

In reply to this post by 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

commit 3d3533cf5e04b303e5d7a4b0f178c54f10ddf620
Author: Samuel Trégouët <[hidden email]>
AuthorDate: Wed Oct 30 17:22:06 2019 +0100

Fixed: Remove dependency management from ‘ComponentContainer’
(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.
---
.../base/component/AlreadyLoadedException.java | 44 --------
.../ofbiz/base/component/ComponentConfig.java | 61 ++++++++++-
.../ofbiz/base/container/ComponentContainer.java | 120 ++-------------------
.../base/container/ComponentContainerTest.java | 2 -
4 files changed, 68 insertions(+), 159 deletions(-)

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
deleted file mode 100644
index 4485ccf..0000000
--- a/framework/base/src/main/java/org/apache/ofbiz/base/component/AlreadyLoadedException.java
+++ /dev/null
@@ -1,44 +0,0 @@
-/*******************************************************************************
- * 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 4a39885..c1a1ada 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,6 +18,9 @@
*******************************************************************************/
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;
@@ -40,6 +43,7 @@ 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;
@@ -91,6 +95,16 @@ 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();
}
@@ -681,6 +695,8 @@ 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);
@@ -701,8 +717,49 @@ public final class ComponentConfig {
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 {
+ 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);
+ }
}
}

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 3f14707..31f6e60 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,13 +26,7 @@ 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;
@@ -44,7 +38,6 @@ 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
@@ -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 {
@@ -89,11 +79,12 @@ 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 = readyComponents.stream()
+ List<Classpath> componentsClassPath = ComponentConfig.components()
.peek(cmpnt -> Debug.logInfo(String.format(fmt, cmpnt.getComponentName()), module))
.map(cmpnt -> buildClasspathFromComponentConfig(cmpnt))
.collect(Collectors.toList());
@@ -134,19 +125,15 @@ 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, 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;
}
}
@@ -157,9 +144,8 @@ 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, 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);
@@ -205,56 +191,15 @@ public class ComponentContainer implements Container {
* 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));
}
}

@@ -279,53 +224,6 @@ 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 358f0bc..57be05a 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,7 +35,6 @@ 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;

@@ -61,7 +60,6 @@ public class ComponentContainerTest {
}
}

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

mthl

[ofbiz-framework] 04/04: Fixed: Fix linting issues (OFBIZ-11265)

In reply to this post by 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

commit 03b69b30a510bb92dfb01a478de3a2ad1083ec70
Author: Mathieu Lirzin <[hidden email]>
AuthorDate: Fri Nov 8 18:08:42 2019 +0100

Fixed: Fix linting issues
(OFBIZ-11265)
---
build.gradle | 2 +-
.../base/src/test/java/org/apache/ofbiz/base/util/UtilCodecTests.java | 3 ++-
.../service/src/main/java/org/apache/ofbiz/service/ModelService.java | 4 ++--
3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/build.gradle b/build.gradle
index a4d9466..0395b8c 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 = 37780
+ 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
diff --git a/framework/base/src/test/java/org/apache/ofbiz/base/util/UtilCodecTests.java b/framework/base/src/test/java/org/apache/ofbiz/base/util/UtilCodecTests.java
index 06c366d..f5d3f97 100644
--- a/framework/base/src/test/java/org/apache/ofbiz/base/util/UtilCodecTests.java
+++ b/framework/base/src/test/java/org/apache/ofbiz/base/util/UtilCodecTests.java
@@ -99,7 +99,8 @@ public class UtilCodecTests {
public void testCheckStringForHtmlSafe() {
String xssVector = "<script>alert('XSS vector');</script>";
List<String> errorList = new ArrayList<>();
- String canonicalizedXssVector = UtilCodec.checkStringForHtmlSafe("fieldName", xssVector, errorList,new Locale("test"), true);
+ String canonicalizedXssVector = UtilCodec.checkStringForHtmlSafe("fieldName", xssVector, errorList,
+ new Locale("test"), true);
assertEquals("<script>alert('XSS vector');</script>", canonicalizedXssVector);
assertEquals(1, errorList.size());
assertEquals("In field [fieldName] by our input policy, your input has not been accepted for security reason. "
diff --git a/framework/service/src/main/java/org/apache/ofbiz/service/ModelService.java b/framework/service/src/main/java/org/apache/ofbiz/service/ModelService.java
index b44c07f..82d3b3b 100644
--- a/framework/service/src/main/java/org/apache/ofbiz/service/ModelService.java
+++ b/framework/service/src/main/java/org/apache/ofbiz/service/ModelService.java
@@ -614,8 +614,8 @@ public class ModelService extends AbstractMap<String, Object> implements Seriali
if ("none".equals(modelParam.allowHtml)) {
UtilCodec.checkStringForHtmlStrictNone(modelParam.name, value, errorMessageList, (Locale) context.get("locale"));
} else if ("safe".equals(modelParam.allowHtml)) {
- UtilCodec.checkStringForHtmlSafe(modelParam.name, value, errorMessageList,
- (Locale) context.get("locale"),
+ UtilCodec.checkStringForHtmlSafe(modelParam.name, value, errorMessageList,
+ (Locale) context.get("locale"),
EntityUtilProperties.getPropertyAsBoolean("owasp", "sanitizer.enable", true));
}
}