452329 - Transitive modules in start.jar --add-to-start(d) are not added if enabled already in tree

+ Modules can now be re-added.
  based on the node.selection.how == name
           and all other node selections must
            be node.selection.explicit == false

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

@@ -23,14 +23,17 @@ import java.net.URI;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 
 import org.eclipse.jetty.start.builders.StartDirBuilder;
 import org.eclipse.jetty.start.builders.StartIniBuilder;
 import org.eclipse.jetty.start.fileinits.MavenLocalRepoFileInitializer;
 import org.eclipse.jetty.start.fileinits.TestFileInitializer;
 import org.eclipse.jetty.start.fileinits.UriFileInitializer;
-import org.eclipse.jetty.start.graph.HowSetMatcher;
+import org.eclipse.jetty.start.graph.HowSetPredicate;
+import org.eclipse.jetty.start.graph.HowUniquePredicate;
 import org.eclipse.jetty.start.graph.Predicate;
 import org.eclipse.jetty.start.graph.Selection;
 
@@ -137,17 +140,19 @@ public class BaseBuilder
         String iniSource = "<add-to-start-ini>";
         Selection startDirSelection = new Selection(dirSource);
         Selection startIniSelection = new Selection(iniSource);
+        
+        Set<String> startDNames = new HashSet<>();
+        startDNames.addAll(startArgs.getAddToStartdIni());
+        Set<String> startIniNames = new HashSet<>();
+        startIniNames.addAll(startArgs.getAddToStartIni());
 
         int count = 0;
-        count += modules.selectNodes(startArgs.getAddToStartdIni(),startDirSelection);
+        count += modules.selectNodes(startDNames,startDirSelection);
-        count += modules.selectNodes(startArgs.getAddToStartIni(),startIniSelection);
+        count += modules.selectNodes(startIniNames,startIniSelection);
-
-        Predicate startDMatcher = new HowSetMatcher(dirSource);
-        Predicate startIniMatcher = new HowSetMatcher(iniSource);
 
         // look for ambiguous declaration found in both places
-        Predicate ambiguousMatcher = new HowSetMatcher(dirSource,iniSource);
+        Predicate ambiguousPredicate = new HowSetPredicate(dirSource,iniSource);
-        List<Module> ambiguous = modules.getMatching(ambiguousMatcher);
+        List<Module> ambiguous = modules.getMatching(ambiguousPredicate);
 
         if (ambiguous.size() > 0)
         {
@@ -173,11 +178,15 @@ public class BaseBuilder
         }
 
         StartLog.debug("Adding %s new module(s)",count);
-
+        
         // Acknowledge Licenses
         ackLicenses();
 
         // Collect specific modules to enable
+        // Should match 'how', with no other selections.explicit
+        Predicate startDMatcher = new HowUniquePredicate(dirSource);
+        Predicate startIniMatcher = new HowUniquePredicate(iniSource);
+
         List<Module> startDModules = modules.getMatching(startDMatcher);
         List<Module> startIniModules = modules.getMatching(startIniMatcher);
 

jetty-start/src/main/java/org/eclipse/jetty/start/graph/HowPredicate.java

@@ -0,0 +1,45 @@
+//
+//  ========================================================================
+//  Copyright (c) 1995-2014 Mort Bay Consulting Pty. Ltd.
+//  ------------------------------------------------------------------------
+//  All rights reserved. This program and the accompanying materials
+//  are made available under the terms of the Eclipse Public License v1.0
+//  and Apache License v2.0 which accompanies this distribution.
+//
+//      The Eclipse Public License is available at
+//      http://www.eclipse.org/legal/epl-v10.html
+//
+//      The Apache License v2.0 is available at
+//      http://www.opensource.org/licenses/apache2.0.php
+//
+//  You may elect to redistribute this code under either of these licenses.
+//  ========================================================================
+//
+
+package org.eclipse.jetty.start.graph;
+
+/**
+ * Predicate against a specific {@link Selection#getHow()}
+ */
+public class HowPredicate implements Predicate
+{
+    private final String how;
+
+    public HowPredicate(String how)
+    {
+        this.how = how;
+    }
+
+    @Override
+    public boolean match(Node<?> node)
+    {
+        for (Selection selection : node.getSelections())
+        {
+            if (how.equalsIgnoreCase(selection.getHow()))
+            {
+                return true;
+            }
+        }
+        return false;
+    }
+}

jetty-start/src/main/java/org/eclipse/jetty/start/graph/HowSetPredicate.java

@@ -21,11 +21,16 @@ package org.eclipse.jetty.start.graph;
 import java.util.HashSet;
 import java.util.Set;
 
-public class HowSetMatcher implements Predicate
+/**
+ * Should match against the provided set of {@link Selection#getHow()} values.
+ * <p>
+ * Incomplete set is considered to be no-match.
+ */
+public class HowSetPredicate implements Predicate
 {
     private final Set<String> howSet;
 
-    public HowSetMatcher(String... hows)
+    public HowSetPredicate(String... hows)
     {
         this.howSet = new HashSet<>();
 

jetty-start/src/main/java/org/eclipse/jetty/start/graph/HowUniquePredicate.java

@@ -0,0 +1,53 @@
+//
+//  ========================================================================
+//  Copyright (c) 1995-2014 Mort Bay Consulting Pty. Ltd.
+//  ------------------------------------------------------------------------
+//  All rights reserved. This program and the accompanying materials
+//  are made available under the terms of the Eclipse Public License v1.0
+//  and Apache License v2.0 which accompanies this distribution.
+//
+//      The Eclipse Public License is available at
+//      http://www.eclipse.org/legal/epl-v10.html
+//
+//      The Apache License v2.0 is available at
+//      http://www.opensource.org/licenses/apache2.0.php
+//
+//  You may elect to redistribute this code under either of these licenses.
+//  ========================================================================
+//
+
+package org.eclipse.jetty.start.graph;
+
+/**
+ * Match against a specific {@link Selection#getHow()}, where
+ * there are no other {@link Selection#isExplicit()} specified.
+ */
+public class HowUniquePredicate implements Predicate
+{
+    private final String how;
+
+    public HowUniquePredicate(String how)
+    {
+        this.how = how;
+    }
+
+    @Override
+    public boolean match(Node<?> node)
+    {
+        boolean ret = false;
+
+        for (Selection selection : node.getSelections())
+        {
+            if (how.equalsIgnoreCase(selection.getHow()))
+            {
+                ret = true;
+                continue; // this how is always valid.
+            }
+            if (selection.isExplicit())
+            {
+                ret = false;
+            }
+        }
+        return ret;
+    }
+}

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

@@ -18,8 +18,13 @@
 
 package org.eclipse.jetty.start;
 
+import static org.hamcrest.Matchers.*;
+import static org.junit.Assert.*;
+
 import java.io.File;
 import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.List;
 
@@ -48,6 +53,12 @@ public class DistTest
         FS.exists(file.toPath());
         return IO.readToString(file);
     }
+    
+    protected void assertNotFileExists(File basePath, String name) throws IOException
+    {
+        File file = new File(basePath, OS.separators(name));
+        assertThat("File should not exist: " + file, file.exists(), is(false));
+    }
 
     private void execMain(List<String> cmds) throws Exception
     {
@@ -82,8 +93,85 @@ public class DistTest
         execMain(cmds);
     }
     
+    /**
+     * Test for https://bugs.eclipse.org/452329
+     */
     @Test
-    @Ignore
+    public void testReAddServerModule() throws Exception
+    {
+        File basePath = testdir.getEmptyDir();
+
+        List<String> cmds = getBaseCommandLine(basePath);
+        cmds.add("--add-to-startd=http");
+        execMain(cmds);
+        
+        assertFileExists(basePath,"start.d/http.ini");
+        assertFileExists(basePath,"start.d/server.ini");
+        
+        // Delete server.ini
+        Path serverIni = basePath.toPath().resolve("start.d/server.ini");
+        Files.deleteIfExists(serverIni);
+        
+        // Attempt to re-add via 'server' module reference
+        cmds = getBaseCommandLine(basePath);
+        cmds.add("--add-to-startd=server");
+        execMain(cmds);
+        
+        assertFileExists(basePath,"start.d/server.ini");
+    }
+    
+    /**
+     * Test for https://bugs.eclipse.org/452329
+     */
+    @Test
+    public void testReAddServerViaHttpModule() throws Exception
+    {
+        File basePath = testdir.getEmptyDir();
+
+        List<String> cmds = getBaseCommandLine(basePath);
+        cmds.add("--add-to-startd=http");
+        execMain(cmds);
+        
+        assertFileExists(basePath,"start.d/http.ini");
+        assertFileExists(basePath,"start.d/server.ini");
+        
+        // Delete server.ini
+        Path serverIni = basePath.toPath().resolve("start.d/server.ini");
+        Files.deleteIfExists(serverIni);
+        
+        // Attempt to re-add via 'http' module reference
+        cmds = getBaseCommandLine(basePath);
+        cmds.add("--add-to-startd=http");
+        execMain(cmds);
+        
+        assertFileExists(basePath,"start.d/server.ini");
+    }
+    
+    /**
+     * Test for https://bugs.eclipse.org/452329
+     */
+    @Test
+    public void testReAddHttpThenDeployViaStartD() throws Exception
+    {
+        File basePath = testdir.getEmptyDir();
+
+        List<String> cmds = getBaseCommandLine(basePath);
+        cmds.add("--add-to-start=http");
+        execMain(cmds);
+        
+        assertFileExists(basePath,"start.ini");
+        
+        // Now add 'deploy' module.
+        cmds = getBaseCommandLine(basePath);
+        cmds.add("--add-to-startd=deploy");
+        execMain(cmds);
+        
+        // The following files should not exist (as its already defined in /start.ini)
+        assertNotFileExists(basePath,"start.d/server.ini");
+    }
+    
+    @Test
+    @Ignore("See https://bugs.eclipse.org/451973")
     public void testLikeDistro_SetupDemoBase() throws Exception
     {
         File basePath = testdir.getEmptyDir();

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

@@ -31,7 +31,7 @@ import org.eclipse.jetty.start.config.CommandLineConfigSource;
 import org.eclipse.jetty.start.config.ConfigSources;
 import org.eclipse.jetty.start.config.JettyBaseConfigSource;
 import org.eclipse.jetty.start.config.JettyHomeConfigSource;
-import org.eclipse.jetty.start.graph.HowSetMatcher;
+import org.eclipse.jetty.start.graph.HowSetPredicate;
 import org.eclipse.jetty.start.graph.Predicate;
 import org.eclipse.jetty.start.graph.RegexNamePredicate;
 import org.eclipse.jetty.start.graph.Selection;
@@ -462,7 +462,7 @@ public class ModulesTest
         }
 
         // Now collect the unique source list
-        List<Module> alts = modules.getMatching(new HowSetMatcher(alt));
+        List<Module> alts = modules.getMatching(new HowSetPredicate(alt));
 
         // Assert names are correct, and in the right order
         actualNames = new ArrayList<>();