Issue #8222 - Addressing NPE with JvmArgs and non-existent modules. (#8223)

* Issue #8222 - Addressing NPE with JvmArgs and non-existent modules.

+ Better variable naming.
+ Simpler use case, without need for new StartArgs method
+ Correct naming in StartArgs.getEnabledModules
+ Introduce StartArgs.getSelectedModules
+ Change javadoc and usage everywhere else
* Stabilize flaky test by using UTC properly

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>

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

@@ -29,6 +29,7 @@ import java.net.Socket;
 import java.net.SocketTimeoutException;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Locale;
 import java.util.stream.Collectors;
@@ -187,7 +188,7 @@ public class Main
 
     public void invokeMain(ClassLoader classloader, StartArgs args) throws IllegalAccessException, InvocationTargetException, NoSuchMethodException, IOException
     {
-        if (args.getEnabledModules().isEmpty())
+        if (args.getSelectedModules().isEmpty())
         {
             if (Files.exists(getBaseHome().getBasePath("start.jar")))
                 StartLog.error("Do not start with ${jetty.base} == ${jetty.home}!");
@@ -325,12 +326,22 @@ public class Main
         modules.registerAll();
 
         // 4) Active Module Resolution
-        for (String enabledModule : modules.getSortedNames(args.getEnabledModules()))
+        List<String> selectedModules = args.getSelectedModules();
+        List<String> sortedSelectedModules = modules.getSortedNames(selectedModules);
+        List<String> unknownModules = new ArrayList<>(selectedModules);
+        unknownModules.removeAll(sortedSelectedModules);
+        if (unknownModules.size() >= 1)
         {
-            for (String source : args.getSources(enabledModule))
+            throw new UsageException(UsageException.ERR_UNKNOWN, "Unknown module%s=[%s] List available with --list-modules",
+                unknownModules.size() > 1 ? 's' : "",
+                String.join(", ", unknownModules));
+        }
+        for (String selectedModule : sortedSelectedModules)
+        {
+            for (String source : args.getSources(selectedModule))
             {
                 String shortForm = baseHome.toShortForm(source);
-                modules.enable(enabledModule, shortForm);
+                modules.enable(selectedModule, shortForm);
             }
         }
 
@@ -470,8 +481,7 @@ public class Main
             CommandLineBuilder cmd = args.getMainArgs(StartArgs.ALL_PARTS);
             cmd.debug();
 
-            List<String> execModules = args.getEnabledModules().stream()
+            List<String> execModules = args.getAllModules().getEnabled().stream()
-                .map(name -> args.getAllModules().get(name))
                 // Keep only the forking modules.
                 .filter(module -> !module.getJvmArgs().isEmpty())
                 .map(Module::getName)

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

@@ -395,7 +395,6 @@ public class Modules implements Iterable<Module>
                 order.add(name);
             }
         }
-
         return order;
     }
 

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

@@ -646,7 +646,29 @@ public class StartArgs
         return classpath;
     }
 
+    /**
+     * @deprecated use {@link #getSelectedModules()} instead
+     */
+    @Deprecated
     public List<String> getEnabledModules()
+    {
+        return getSelectedModules();
+    }
+
+    /**
+     * <p>
+     * The list of selected Modules to enable based on configuration
+     * obtained from {@code start.d/*.ini}, {@code start.ini}, and command line.
+     * </p>
+     *
+     * <p>
+     *     For full list of enabled modules, use {@link Modules#getEnabled()}
+     * </p>
+     *
+     * @return the list of selected modules (by name) that the configuration has.
+     * @see Modules#getEnabled()
+     */
+    public List<String> getSelectedModules()
     {
         return this.modules;
     }
@@ -1315,11 +1337,11 @@ public class StartArgs
             return;
         }
 
-        // Enable a module
+        // Select a module to eventually be enabled
         if (arg.startsWith("--module="))
         {
             List<String> moduleNames = Props.getValues(arg);
-            enableModules(source, moduleNames);
+            selectModules(source, moduleNames);
             return;
         }
 
@@ -1464,7 +1486,7 @@ public class StartArgs
         setProperty(key, value, source);
     }
 
-    private void enableModules(String source, List<String> moduleNames)
+    private void selectModules(String source, List<String> moduleNames)
     {
         for (String moduleName : moduleNames)
         {

jetty-start/src/test/java/org/eclipse/jetty/start/fileinits/MavenMetadataTest.java

@@ -71,7 +71,7 @@ public class MavenMetadataTest
     @Test
     public void testIsExpiredTimestampNow()
     {
-        LocalDateTime now = LocalDateTime.now();
+        LocalDateTime now = LocalDateTime.now(ZoneId.of("UTC"));
         String timestamp = getTimestampFormatter().format(now);
         assertFalse(MavenMetadata.isExpiredTimestamp(timestamp), "Timestamp should NOT be stale: " + timestamp);
     }

jetty-start/src/test/java/org/eclipse/jetty/start/usecases/BasicTest.java

@@ -26,6 +26,7 @@ import java.util.List;
 import java.util.Set;
 
 import org.eclipse.jetty.start.Props;
+import org.eclipse.jetty.start.UsageException;
 import org.eclipse.jetty.toolchain.test.FS;
 import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
 import org.eclipse.jetty.toolchain.test.PathAssert;
@@ -34,10 +35,12 @@ import org.junit.jupiter.api.Test;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.startsWith;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 public class BasicTest extends AbstractUseCase
 {
@@ -110,6 +113,59 @@ public class BasicTest extends AbstractUseCase
         assertThat("System.getProperty(jetty.base)", System.getProperty("jetty.base"), is(not(startsWith("file:"))));
     }
 
+    @Test
+    public void testAddModuleDoesNotExist() throws Exception
+    {
+        setupDistHome();
+
+        Files.write(baseDir.resolve("start.ini"),
+            List.of(
+                "--module=main",
+                "--module=does-not-exist"
+            ),
+            StandardCharsets.UTF_8);
+
+        // === Execute Main
+        List<String> runArgs = new ArrayList<>();
+        runArgs.add("--create-files");
+        UsageException usage = assertThrows(UsageException.class, () ->
+        {
+            ExecResults results = exec(runArgs, true);
+            if (results.exception != null)
+            {
+                throw results.exception;
+            }
+        });
+        assertThat(usage.getMessage(), containsString("Unknown module=[does-not-exist]"));
+    }
+
+    @Test
+    public void testAddModuleDoesNotExistMultiple() throws Exception
+    {
+        setupDistHome();
+
+        Files.write(baseDir.resolve("start.ini"),
+            List.of(
+                "--module=main",
+                "--module=does-not-exist",
+                "--module=also-not-present"
+            ),
+            StandardCharsets.UTF_8);
+
+        // === Execute Main
+        List<String> runArgs = new ArrayList<>();
+        runArgs.add("--create-files");
+        UsageException usage = assertThrows(UsageException.class, () ->
+        {
+            ExecResults results = exec(runArgs, true);
+            if (results.exception != null)
+            {
+                throw results.exception;
+            }
+        });
+        assertThat(usage.getMessage(), containsString("Unknown modules=[does-not-exist, also-not-present]"));
+    }
+
     @Test
     public void testProvidersUsingDefault() throws Exception
     {