From 1e3f3a3cbbb79d404736a7f452755d69dbd4139a Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Mon, 12 Apr 2021 10:41:51 +0200 Subject: [PATCH] Fixes #6049 - Default provider [files] section always executed (#6051) * Fixes #6049 - Default provider [files] section always executed Keeping only enabled modules when processing the modules, so that default provider modules don't get processed. Signed-off-by: Simone Bordet --- .../org/eclipse/jetty/start/BaseBuilder.java | 121 ++++++++++-------- .../java/org/eclipse/jetty/start/Modules.java | 3 +- .../org/eclipse/jetty/start/StartArgs.java | 46 +++---- .../tests/distribution/DistributionTests.java | 50 ++++++++ 4 files changed, 140 insertions(+), 80 deletions(-) diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/BaseBuilder.java b/jetty-start/src/main/java/org/eclipse/jetty/start/BaseBuilder.java index 985da7b50cb..74a35389b4b 100644 --- a/jetty-start/src/main/java/org/eclipse/jetty/start/BaseBuilder.java +++ b/jetty-start/src/main/java/org/eclipse/jetty/start/BaseBuilder.java @@ -39,7 +39,7 @@ import org.eclipse.jetty.start.fileinits.TestFileInitializer; import org.eclipse.jetty.start.fileinits.UriFileInitializer; /** - * Build a start configuration in ${jetty.base}, including + * Build a start configuration in {@code ${jetty.base}}, including * ini files, directories, and libs. Also handles License management. */ public class BaseBuilder @@ -47,7 +47,7 @@ public class BaseBuilder public static interface Config { /** - * Add a module to the start environment in ${jetty.base} + * Add a module to the start environment in {@code ${jetty.base}} * * @param module the module to add * @param props The properties to substitute into a template @@ -163,7 +163,7 @@ public class BaseBuilder } // generate the files - List files = new ArrayList(); + List files = new ArrayList<>(); AtomicReference builder = new AtomicReference<>(); AtomicBoolean modified = new AtomicBoolean(); @@ -184,18 +184,21 @@ public class BaseBuilder if (Files.exists(startd)) { // Copy start.d files into start.ini - DirectoryStream.Filter filter = new DirectoryStream.Filter() + DirectoryStream.Filter filter = new DirectoryStream.Filter<>() { - PathMatcher iniMatcher = PathMatchers.getMatcher("glob:**/start.d/*.ini"); + private final PathMatcher iniMatcher = PathMatchers.getMatcher("glob:**/start.d/*.ini"); + @Override - public boolean accept(Path entry) throws IOException + public boolean accept(Path entry) { return iniMatcher.matches(entry); } }; List paths = new ArrayList<>(); for (Path path : Files.newDirectoryStream(startd, filter)) + { paths.add(path); + } paths.sort(new NaturalSort.Paths()); // Read config from start.d @@ -212,12 +215,16 @@ public class BaseBuilder try (FileWriter out = new FileWriter(startini.toFile(), true)) { for (String line : startLines) + { out.append(line).append(System.lineSeparator()); + } } // delete start.d files for (Path path : paths) + { Files.delete(path); + } Files.delete(startd); } } @@ -264,56 +271,66 @@ public class BaseBuilder StartLog.warn("Use of both %s and %s is deprecated", getBaseHome().toShortForm(startd), getBaseHome().toShortForm(startini)); builder.set(useStartD ? new StartDirBuilder(this) : new StartIniBuilder(this)); - newlyAdded.stream().map(modules::get).forEach(module -> - { - String ini = null; - try - { - if (module.isSkipFilesValidation()) - { - StartLog.debug("Skipping [files] validation on %s", module.getName()); - } - else - { - // if (explicitly added and ini file modified) - if (startArgs.getStartModules().contains(module.getName())) - { - ini = builder.get().addModule(module, startArgs.getProperties()); - if (ini != null) - modified.set(true); - } - for (String file : module.getFiles()) - { - files.add(new FileArg(module, startArgs.getProperties().expand(file))); - } - } - } - catch (Exception e) - { - throw new RuntimeException(e); - } - if (module.isDynamic()) + // Collect the filesystem operations to perform, + // only for those modules that are enabled. + newlyAdded.stream() + .map(modules::get) + .filter(Module::isEnabled) + .forEach(module -> { - for (String s : module.getEnableSources()) + String ini = null; + try { - StartLog.info("%-15s %s", module.getName(), s); + if (module.isSkipFilesValidation()) + { + StartLog.debug("Skipping [files] validation on %s", module.getName()); + } + else + { + // if (explicitly added and ini file modified) + if (startArgs.getStartModules().contains(module.getName())) + { + ini = builder.get().addModule(module, startArgs.getProperties()); + if (ini != null) + modified.set(true); + } + for (String file : module.getFiles()) + { + files.add(new FileArg(module, startArgs.getProperties().expand(file))); + } + } + } + catch (Exception e) + { + throw new RuntimeException(e); + } + + if (module.isDynamic()) + { + for (String s : module.getEnableSources()) + { + StartLog.info("%-15s %s", module.getName(), s); + } + } + else if (module.isTransitive()) + { + if (module.hasIniTemplate()) + { + StartLog.info("%-15s transitively enabled, ini template available with --add-module=%s", + module.getName(), + module.getName()); + } + else + { + StartLog.info("%-15s transitively enabled", module.getName()); + } } - } - else if (module.isTransitive()) - { - if (module.hasIniTemplate()) - StartLog.info("%-15s transitively enabled, ini template available with --add-module=%s", - module.getName(), - module.getName()); else - StartLog.info("%-15s transitively enabled", module.getName()); - } - else - { - StartLog.info("%-15s initialized in %s", module.getName(), ini); - } - }); + { + StartLog.info("%-15s initialized in %s", module.getName(), ini); + } + }); files.addAll(startArgs.getFiles()); if (!files.isEmpty() && processFileResources(files)) @@ -370,7 +387,7 @@ public class BaseBuilder * @param files the list of {@link FileArg}s to process * @return true if base directory modified, false if left untouched */ - private boolean processFileResources(List files) throws IOException + private boolean processFileResources(List files) { if ((files == null) || (files.isEmpty())) { diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/Modules.java b/jetty-start/src/main/java/org/eclipse/jetty/start/Modules.java index 17bc231e8d5..37074630866 100644 --- a/jetty-start/src/main/java/org/eclipse/jetty/start/Modules.java +++ b/jetty-start/src/main/java/org/eclipse/jetty/start/Modules.java @@ -18,7 +18,6 @@ import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; -import java.util.Collections; import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; @@ -552,7 +551,7 @@ public class Modules implements Iterable Set providers = _provided.get(name); StartLog.debug("Providers of [%s] are %s", name, providers); if (providers == null || providers.isEmpty()) - return Collections.emptySet(); + return Set.of(); providers = new HashSet<>(providers); diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java b/jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java index 8516c0573d4..46b629b14e5 100644 --- a/jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java +++ b/jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java @@ -52,14 +52,8 @@ import org.eclipse.jetty.util.ManifestUtils; public class StartArgs { public static final String VERSION; - public static final Set ALL_PARTS = Collections.unmodifiableSet(new HashSet<>(Arrays.asList( - "java", - "opts", - "path", - "main", - "args"))); - public static final Set ARG_PARTS = Collections.unmodifiableSet(new HashSet<>(Arrays.asList( - "args"))); + public static final Set ALL_PARTS = Set.of("java", "opts", "path", "main", "args"); + public static final Set ARG_PARTS = Set.of("args"); static { @@ -126,12 +120,12 @@ public class StartArgs /** * List of enabled modules */ - private List modules = new ArrayList<>(); + private final List modules = new ArrayList<>(); /** * List of modules to skip [files] section validation */ - private Set skipFileValidationModules = new HashSet<>(); + private final Set skipFileValidationModules = new HashSet<>(); /** * Map of enabled modules to the source of where that activation occurred @@ -141,56 +135,56 @@ public class StartArgs /** * List of all active [files] sections from enabled modules */ - private List files = new ArrayList<>(); + private final List files = new ArrayList<>(); /** * List of all active [lib] sections from enabled modules */ - private Classpath classpath; + private final Classpath classpath; /** * List of all active [xml] sections from enabled modules */ - private List xmls = new ArrayList<>(); + private final List xmls = new ArrayList<>(); /** * List of all active [jpms] sections for enabled modules */ - private Set jmodAdds = new LinkedHashSet<>(); - private Map> jmodPatch = new LinkedHashMap<>(); - private Map> jmodOpens = new LinkedHashMap<>(); - private Map> jmodExports = new LinkedHashMap<>(); - private Map> jmodReads = new LinkedHashMap<>(); + private final Set jmodAdds = new LinkedHashSet<>(); + private final Map> jmodPatch = new LinkedHashMap<>(); + private final Map> jmodOpens = new LinkedHashMap<>(); + private final Map> jmodExports = new LinkedHashMap<>(); + private final Map> jmodReads = new LinkedHashMap<>(); /** * JVM arguments, found via command line and in all active [exec] sections from enabled modules */ - private List jvmArgs = new ArrayList<>(); + private final List jvmArgs = new ArrayList<>(); /** * List of all xml references found directly on command line or start.ini */ - private List xmlRefs = new ArrayList<>(); + private final List xmlRefs = new ArrayList<>(); /** * List of all property references found directly on command line or start.ini */ - private List propertyFileRefs = new ArrayList<>(); + private final List propertyFileRefs = new ArrayList<>(); /** * List of all property files */ - private List propertyFiles = new ArrayList<>(); + private final List propertyFiles = new ArrayList<>(); - private Props properties = new Props(); - private Map systemPropertySource = new HashMap<>(); - private List rawLibs = new ArrayList<>(); + private final Props properties = new Props(); + private final Map systemPropertySource = new HashMap<>(); + private final List rawLibs = new ArrayList<>(); // jetty.base - build out commands /** * --add-module=[module,[module]] */ - private List startModules = new ArrayList<>(); + private final List startModules = new ArrayList<>(); // module inspection commands /** diff --git a/tests/test-distribution/src/test/java/org/eclipse/jetty/tests/distribution/DistributionTests.java b/tests/test-distribution/src/test/java/org/eclipse/jetty/tests/distribution/DistributionTests.java index 0924c38af36..97d940c3ea6 100644 --- a/tests/test-distribution/src/test/java/org/eclipse/jetty/tests/distribution/DistributionTests.java +++ b/tests/test-distribution/src/test/java/org/eclipse/jetty/tests/distribution/DistributionTests.java @@ -859,4 +859,54 @@ public class DistributionTests extends AbstractJettyHomeTest } } } + + @Test + public void testDefaultLoggingProviderNotActiveWhenExplicitProviderIsPresent() throws Exception + { + String jettyVersion = System.getProperty("jettyVersion"); + JettyHomeTester distribution1 = JettyHomeTester.Builder.newInstance() + .jettyVersion(jettyVersion) + .mavenLocalRepository(System.getProperty("mavenRepoPath")) + .build(); + + String[] args1 = { + "--approve-all-licenses", + "--add-modules=logging-logback,http" + }; + + try (JettyHomeTester.Run run1 = distribution1.start(args1)) + { + assertTrue(run1.awaitFor(10, TimeUnit.SECONDS)); + assertEquals(0, run1.getExitValue()); + + Path jettyBase = run1.getConfig().getJettyBase(); + + assertTrue(Files.exists(jettyBase.resolve("resources/logback.xml"))); + // The jetty-logging.properties should be absent. + assertFalse(Files.exists(jettyBase.resolve("resources/jetty-logging.properties"))); + } + + JettyHomeTester distribution2 = JettyHomeTester.Builder.newInstance() + .jettyVersion(jettyVersion) + .mavenLocalRepository(System.getProperty("mavenRepoPath")) + .build(); + + // Try the modules in reverse order, since it may execute a different code path. + String[] args2 = { + "--approve-all-licenses", + "--add-modules=http,logging-logback" + }; + + try (JettyHomeTester.Run run2 = distribution2.start(args2)) + { + assertTrue(run2.awaitFor(1000, TimeUnit.SECONDS)); + assertEquals(0, run2.getExitValue()); + + Path jettyBase = run2.getConfig().getJettyBase(); + + assertTrue(Files.exists(jettyBase.resolve("resources/logback.xml"))); + // The jetty-logging.properties should be absent. + assertFalse(Files.exists(jettyBase.resolve("resources/jetty-logging.properties"))); + } + } }