Issue #4628 - Non-Required Module Dependency Support

Signed-off-by: Joakim Erdfelt <[email protected]>

jetty-start/src/main/java/org/eclipse/jetty/start/Module.java

@@ -20,7 +20,6 @@
 
 import java.io.BufferedReader;
 import java.io.BufferedWriter;
-import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.PrintWriter;
 import java.nio.charset.StandardCharsets;
@@ -154,12 +153,12 @@ public class Module implements Comparable<Module>
     private final List<String> _license = new ArrayList<>();
 
     /**
-     * Dependencies
+     * Dependencies from {@code [depends]} section
      */
     private final List<String> _depends = new ArrayList<>();
 
     /**
-     * Optional
+     * Optional dependencies from {@code [optional]} section are structural in nature.
      */
     private final Set<String> _optional = new HashSet<>();
 
@@ -196,6 +195,18 @@ public Module(BaseHome basehome, Path path) throws IOException
         process(basehome);
     }
 
+    public static boolean isRequiredDependency(String depends)
+    {
+        return (depends != null) && (depends.charAt(0) != '?');
+    }
+
+    public static String normalizeModuleName(String name)
+    {
+        if (!isRequiredDependency(name))
+            return name.substring(1);
+        return name;
+    }
+
     public String getName()
     {
         return _name;

jetty-start/src/main/java/org/eclipse/jetty/start/ModuleGraphWriter.java

@@ -247,6 +247,7 @@ private void writeRelationships(PrintWriter out, Iterable<Module> modules, List<
         {
             for (String depends : module.getDepends())
             {
+                depends = Module.normalizeModuleName(depends);
                 out.printf("    \"%s\" -> \"%s\";%n", module.getName(), depends);
             }
             for (String optional : module.getOptional())

jetty-start/src/main/java/org/eclipse/jetty/start/Modules.java

@@ -20,6 +20,7 @@
 
 import java.io.FileInputStream;
 import java.io.IOException;
+import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.Collections;
@@ -132,7 +133,12 @@ public void dump(List<String> tags)
                     label = "     Depend: %s";
                     for (String parent : module.getDepends())
                     {
+                        parent = Module.normalizeModuleName(parent);
                         System.out.printf(label, parent);
+                        if (!Module.isRequiredDependency(parent))
+                        {
+                            System.out.print(" [not-required]");
+                        }
                         label = ", %s";
                     }
                     System.out.println();
@@ -352,45 +358,59 @@ private void enable(Set<String> newlyEnabled, Module module, String enabledFrom,
 
         // Process module dependencies (always processed as may be dynamic)
         StartLog.debug("Enabled module %s depends on %s", module.getName(), module.getDepends());
-        for (String dependsOn : module.getDepends())
+        for (String dependsOnRaw : module.getDepends())
         {
+            boolean isRequired = Module.isRequiredDependency(dependsOnRaw);
+            // Final to allow lambda's below to use name
+            final String dependentModule = Module.normalizeModuleName(dependsOnRaw);
+
             // Look for modules that provide that dependency
-            Set<Module> providers = getAvailableProviders(dependsOn);
+            Set<Module> providers = getAvailableProviders(dependentModule);
 
-            StartLog.debug("Module %s depends on %s provided by %s", module, dependsOn, providers);
+            StartLog.debug("Module %s depends on %s provided by %s", module, dependentModule, providers);
 
             // If there are no known providers of the module
             if (providers.isEmpty())
             {
                 // look for a dynamic module
-                if (dependsOn.contains("/"))
+                if (dependentModule.contains("/"))
                 {
-                    Path file = _baseHome.getPath("modules/" + dependsOn + ".mod");
-                    registerModule(file).expandDependencies(_args.getProperties());
-                    providers = _provided.get(dependsOn);
-                    if (providers == null || providers.isEmpty())
-                        throw new UsageException("Module %s does not provide %s", _baseHome.toShortForm(file), dependsOn);
+                    Path file = _baseHome.getPath("modules/" + dependentModule + ".mod");
+                    if (isRequired || Files.exists(file))
+                    {
+                        registerModule(file).expandDependencies(_args.getProperties());
+                        providers = _provided.get(dependentModule);
+                        if (providers == null || providers.isEmpty())
+                            throw new UsageException("Module %s does not provide %s", _baseHome.toShortForm(file), dependentModule);
 
-                    enable(newlyEnabled, providers.stream().findFirst().get(), "dynamic dependency of " + module.getName(), true);
+                        enable(newlyEnabled, providers.stream().findFirst().get(), "dynamic dependency of " + module.getName(), true);
+                        continue;
+                    }
+                }
+                // is this a non-required module
+                if (!isRequired)
+                {
+                    StartLog.debug("Skipping non-required module [%s]: doesn't exist", dependentModule);
                     continue;
                 }
-                throw new UsageException("No module found to provide %s for %s", dependsOn, module);
+                // throw an exception (not a dynamic module and a required dependency)
+                throw new UsageException("No module found to provide %s for %s", dependentModule, module);
             }
 
             // If a provider is already enabled, then add a transitive enable
-            if (providers.stream().filter(Module::isEnabled).count() > 0)
-                providers.stream().filter(m -> m.isEnabled() && !m.equals(module)).forEach(m -> enable(newlyEnabled, m, "transitive provider of " + dependsOn + " for " + module.getName(), true));
+            if (providers.stream().anyMatch(Module::isEnabled))
+                providers.stream().filter(m -> m.isEnabled() && !m.equals(module)).forEach(m -> enable(newlyEnabled, m, "transitive provider of " + dependentModule + " for " + module.getName(), true));
             else
             {
                 // Is there an obvious default?
                 Optional<Module> dftProvider = (providers.size() == 1)
                     ? providers.stream().findFirst()
-                    : providers.stream().filter(m -> m.getName().equals(dependsOn)).findFirst();
+                    : providers.stream().filter(m -> m.getName().equals(dependentModule)).findFirst();
 
                 if (dftProvider.isPresent())
-                    enable(newlyEnabled, dftProvider.get(), "transitive provider of " + dependsOn + " for " + module.getName(), true);
+                    enable(newlyEnabled, dftProvider.get(), "transitive provider of " + dependentModule + " for " + module.getName(), true);
                 else if (StartLog.isDebugEnabled())
-                    StartLog.debug("Module %s requires a %s implementation from one of %s", module, dependsOn, providers);
+                    StartLog.debug("Module %s requires a %s implementation from one of %s", module, dependentModule, providers);
             }
         }
     }

jetty-start/src/test/java/org/eclipse/jetty/start/ModulesTest.java

@@ -36,6 +36,8 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.greaterThanOrEqualTo;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.nullValue;
 
 @ExtendWith(WorkDirExtension.class)
 public class ModulesTest
@@ -202,6 +204,152 @@ public void testResolveServerHttp() throws IOException
         assertThat("Resolved XMLs: " + actualXmls, actualXmls, contains(expectedXmls.toArray()));
     }
 
+    @Test
+    public void testResolveNotRequiredModuleNotFound() throws IOException
+    {
+        // Test Env
+        File homeDir = MavenTestingUtils.getTestResourceDir("non-required-deps");
+        File baseDir = testdir.getEmptyPathDir().toFile();
+        String[] cmdLine = new String[]{"bar.type=cannot-find-me"};
+
+        // Configuration
+        CommandLineConfigSource cmdLineSource = new CommandLineConfigSource(cmdLine);
+        ConfigSources config = new ConfigSources();
+        config.add(cmdLineSource);
+        config.add(new JettyHomeConfigSource(homeDir.toPath()));
+        config.add(new JettyBaseConfigSource(baseDir.toPath()));
+
+        // Initialize
+        BaseHome basehome = new BaseHome(config);
+
+        StartArgs args = new StartArgs(basehome);
+        args.parse(config);
+
+        // Test Modules
+        Modules modules = new Modules(basehome, args);
+        modules.registerAll();
+
+        // Enable module
+        modules.enable("bar", TEST_SOURCE);
+
+        // Collect active module list
+        List<Module> active = modules.getEnabled();
+
+        // Assert names are correct, and in the right order
+        List<String> expectedNames = new ArrayList<>();
+        expectedNames.add("foo");
+        expectedNames.add("bar");
+
+        List<String> actualNames = new ArrayList<>();
+        for (Module actual : active)
+        {
+            actualNames.add(actual.getName());
+        }
+
+        assertThat("Resolved Names: " + actualNames, actualNames, contains(expectedNames.toArray()));
+
+        Props props = args.getProperties();
+        assertThat(props.getString("bar.name"), is(nullValue()));
+    }
+
+    @Test
+    public void testResolveNotRequiredModuleFound() throws IOException
+    {
+        // Test Env
+        File homeDir = MavenTestingUtils.getTestResourceDir("non-required-deps");
+        File baseDir = testdir.getEmptyPathDir().toFile();
+        String[] cmdLine = new String[]{"bar.type=dive"};
+
+        // Configuration
+        CommandLineConfigSource cmdLineSource = new CommandLineConfigSource(cmdLine);
+        ConfigSources config = new ConfigSources();
+        config.add(cmdLineSource);
+        config.add(new JettyHomeConfigSource(homeDir.toPath()));
+        config.add(new JettyBaseConfigSource(baseDir.toPath()));
+
+        // Initialize
+        BaseHome basehome = new BaseHome(config);
+
+        StartArgs args = new StartArgs(basehome);
+        args.parse(config);
+
+        // Test Modules
+        Modules modules = new Modules(basehome, args);
+        modules.registerAll();
+
+        // Enable module
+        modules.enable("bar", TEST_SOURCE);
+
+        // Collect active module list
+        List<Module> active = modules.getEnabled();
+
+        // Assert names are correct, and in the right order
+        List<String> expectedNames = new ArrayList<>();
+        expectedNames.add("foo");
+        expectedNames.add("bar");
+        expectedNames.add("bar-dive");
+
+        List<String> actualNames = new ArrayList<>();
+        for (Module actual : active)
+        {
+            actualNames.add(actual.getName());
+        }
+
+        assertThat("Resolved Names: " + actualNames, actualNames, contains(expectedNames.toArray()));
+
+        Props props = args.getProperties();
+        assertThat(props.getString("bar.name"), is("dive"));
+    }
+
+    @Test
+    public void testResolveNotRequiredModuleFoundDynamic() throws IOException
+    {
+        // Test Env
+        File homeDir = MavenTestingUtils.getTestResourceDir("non-required-deps");
+        File baseDir = testdir.getEmptyPathDir().toFile();
+        String[] cmdLine = new String[]{"bar.type=dynamic"};
+
+        // Configuration
+        CommandLineConfigSource cmdLineSource = new CommandLineConfigSource(cmdLine);
+        ConfigSources config = new ConfigSources();
+        config.add(cmdLineSource);
+        config.add(new JettyHomeConfigSource(homeDir.toPath()));
+        config.add(new JettyBaseConfigSource(baseDir.toPath()));
+
+        // Initialize
+        BaseHome basehome = new BaseHome(config);
+
+        StartArgs args = new StartArgs(basehome);
+        args.parse(config);
+
+        // Test Modules
+        Modules modules = new Modules(basehome, args);
+        modules.registerAll();
+
+        // Enable module
+        modules.enable("bar", TEST_SOURCE);
+
+        // Collect active module list
+        List<Module> active = modules.getEnabled();
+
+        // Assert names are correct, and in the right order
+        List<String> expectedNames = new ArrayList<>();
+        expectedNames.add("foo");
+        expectedNames.add("bar");
+        expectedNames.add("impls/bar-dynamic");
+
+        List<String> actualNames = new ArrayList<>();
+        for (Module actual : active)
+        {
+            actualNames.add(actual.getName());
+        }
+
+        assertThat("Resolved Names: " + actualNames, actualNames, contains(expectedNames.toArray()));
+
+        Props props = args.getProperties();
+        assertThat(props.getString("bar.name"), is("dynamic"));
+    }
+
     private List<String> normalizeLibs(List<Module> active)
     {
         List<String> libs = new ArrayList<>();

jetty-start/src/test/resources/non-required-deps/modules/bar-dive.mod

@@ -0,0 +1,2 @@
+[ini]
+bar.name=dive

jetty-start/src/test/resources/non-required-deps/modules/bar.mod

@@ -0,0 +1,6 @@
+# Top level mod
+
+[depends]
+foo
+?bar-${bar.type}
+?impls/bar-${bar.type}

jetty-start/src/test/resources/non-required-deps/modules/foo.mod

@@ -0,0 +1 @@
+# nothing here

jetty-start/src/test/resources/non-required-deps/modules/impls/bar-dynamic.mod

@@ -0,0 +1,2 @@
+[ini]
+bar.name=dynamic