Issue #1743 More refactoring and code tidying

Signed-off-by: Jan Bartel <janb@webtide.com>

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/AbstractWebAppMojo.java

@@ -57,8 +57,8 @@ import org.eclipse.jetty.security.LoginService;
 import org.eclipse.jetty.server.RequestLog;
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.server.handler.ContextHandler;
+import org.eclipse.jetty.util.PathWatcher;
 import org.eclipse.jetty.util.resource.Resource;
-import org.eclipse.jetty.util.resource.ResourceCollection;
 
 /**
  * AbstractWebAppMojo
@@ -196,15 +196,15 @@ public abstract class AbstractWebAppMojo extends AbstractMojo
      * List of files or directories to additionally periodically scan for changes. Optional.
      */
     @Parameter
-    protected File[] scanTargets;
+    protected List<File> scanTargets;
     
     /**
      * List of directories with ant-style &lt;include&gt; and &lt;exclude&gt; patterns
      * for extra targets to periodically scan for changes. Can be used instead of,
      * or in conjunction with &lt;scanTargets&gt;.Optional.
-     * @parameter
      */
-    protected ScanTargetPattern[] scanTargetPatterns;
+    @Parameter
+    protected List<ScanTargetPattern> scanTargetPatterns;
     
     @Parameter(defaultValue = "${reactorProjects}", readonly = true, required = true)
     protected List<MavenProject> reactorProjects;
@@ -596,47 +596,7 @@ public abstract class AbstractWebAppMojo extends AbstractMojo
     }
     
     /**
-     * Unpack all included overlays, using the unpacked
+     * Verify some basic configuration is present, supplying defaults if not.
-     * location as resource bases for the webapp.
-     */
-    protected void unpackOverlays()
-        throws Exception
-    {
-        OverlayManager overlayManager =  mavenProjectHelper.getOverlayManager();
-        
-        List<Overlay> overlays = overlayManager.getOverlays();
-        if (overlays.isEmpty())
-            return;
-
-        List<Resource> resourceBaseCollection = new ArrayList<Resource>();
-
-        for (Overlay o:overlays)
-        {
-            //can refer to the current project in list of overlays for ordering purposes
-            if (o.getConfig() != null && o.getConfig().isCurrentProject() && webApp.getBaseResource().exists())
-            {
-                resourceBaseCollection.add(webApp.getBaseResource()); 
-                continue;
-            }
-
-            Resource unpacked = overlayManager.unpackOverlay(o);
-            resourceBaseCollection.add(unpacked); //add in the selectively unpacked overlay in the correct order to the webapps resource base
-        }
-
-        if (!resourceBaseCollection.contains(webApp.getBaseResource()) && webApp.getBaseResource().exists())
-        {
-            if (webApp.getBaseAppFirst())
-                resourceBaseCollection.add(0, webApp.getBaseResource());
-            else
-                resourceBaseCollection.add(webApp.getBaseResource());
-        }
-        webApp.setBaseResource(new ResourceCollection(resourceBaseCollection.toArray(new Resource[resourceBaseCollection.size()])));
-    }
-    
-    /**
-     * Verify the configuration given in the pom.
-     * 
-     * 
      */
     protected void verifyPomConfiguration() throws MojoExecutionException
     {        
@@ -772,15 +732,23 @@ public abstract class AbstractWebAppMojo extends AbstractMojo
     {  
         if (useProvidedScope)
         {
-            return project.getArtifacts().
+            return project.getArtifacts()
-                stream().
+                .stream()
-                filter(a->Artifact.SCOPE_PROVIDED.equals(a.getScope()) && !isPluginArtifact(a)).
+                .filter(a -> Artifact.SCOPE_PROVIDED.equals(a.getScope()) && !isPluginArtifact(a))
-                map(a->a.getFile()).collect(Collectors.toList());
+                .map(a -> a.getFile()).collect(Collectors.toList());
         }
         else
             return Collections.emptyList();
     }
 
+    /**
+     * Synthesize a classpath appropriate for a forked jetty based off
+     * the artifacts associated with the jetty plugin, plus any dependencies
+     * that are marked as provided and useProvidedScope is true.
+     * 
+     * @return jetty classpath
+     * @throws Exception
+     */
     protected String getContainerClassPath() throws Exception
     {
         //Add in all the plugin artifacts
@@ -815,8 +783,6 @@ public abstract class AbstractWebAppMojo extends AbstractMojo
         return classPath.toString();
     }
 
-    
-    
     /**
      * Check to see if the given artifact is one of the dependency artifacts for this plugin.
      * 
@@ -831,7 +797,6 @@ public abstract class AbstractWebAppMojo extends AbstractMojo
         return pluginArtifacts.stream().anyMatch(pa -> pa.getGroupId().equals(artifact.getGroupId()) && pa.getArtifactId().equals(artifact.getArtifactId()));
     }
     
-    
     /**
      * Check if the goal that we're executing as is excluded or not.
      * 
@@ -874,6 +839,12 @@ public abstract class AbstractWebAppMojo extends AbstractMojo
         return projectName;
     }
 
+    /**
+     * Ensure there is a webapp, and that some basic defaults are applied
+     * if the user has not supplied them.
+     * 
+     * @throws Exception
+     */
     protected void configureWebApp()
         throws Exception
     {
@@ -902,6 +873,11 @@ public abstract class AbstractWebAppMojo extends AbstractMojo
         getLog().info("Tmp directory = " + (webApp.getTempDirectory() == null ? " determined at runtime" : webApp.getTempDirectory()));
     }
     
+    /**
+     * Configure a webapp that has not been assembled into a war. 
+     * 
+     * @throws Exception
+     */
     protected void configureUnassembledWebApp() throws Exception
     {   
         //Set up the location of the webapp.
@@ -932,7 +908,6 @@ public abstract class AbstractWebAppMojo extends AbstractMojo
         if (useTestScope && (testClassesDirectory != null))
             webApp.setTestClasses(testClassesDirectory);
 
-       
         List<File> webInfLibs = getWebInfLibArtifacts().stream()
             .map(a ->
             {
@@ -977,8 +952,9 @@ public abstract class AbstractWebAppMojo extends AbstractMojo
             }
         }
 
-        //process any overlays and the war type artifacts
+        //process any overlays and the war type artifacts, and
-        unpackOverlays(); //this sets up the base resource collection
+        //sets up the base resource collection for the webapp
+        mavenProjectHelper.getOverlayManager().applyOverlays(webApp);
         
         getLog().info("web.xml file = " + webApp.getDescriptor());       
         getLog().info("Webapp directory = " + webAppSourceDirectory.getCanonicalPath());
@@ -1014,6 +990,10 @@ public abstract class AbstractWebAppMojo extends AbstractMojo
     
     /**
      * Get a file into which to write output from jetty.
+     * 
+     * @param name
+     * @return
+     * @throws Exception
      */
     protected File getJettyOutputFile(String name) throws Exception
     {
@@ -1024,11 +1004,49 @@ public abstract class AbstractWebAppMojo extends AbstractMojo
         return outputFile;
     }
     
+    /**
+     * Configure any extra files, directories or patterns thereof for the
+     * scanner to watch for changes.
+     * 
+     * @param scanner PathWatcher that notices changes in files and dirs.
+     */
+    protected void configureScanTargetsAndPatterns (PathWatcher scanner)
+    {
+        //handle the explicit extra scan targets
+        if (scanTargets != null)
+        {
+            for (File f:scanTargets)
+            {
+                if (f.isDirectory())
+                {
+                    PathWatcher.Config config = new PathWatcher.Config(f.toPath());
+                    config.setRecurseDepth(PathWatcher.Config.UNLIMITED_DEPTH);
+                    scanner.watch(config);
+                }
+                else
+                    scanner.watch(f.toPath());
+            }
+        }
+        
+        //handle the extra scan patterns
+        if (scanTargetPatterns != null)
+        {
+            for (ScanTargetPattern p:scanTargetPatterns)
+            {
+                PathWatcher.Config config = new PathWatcher.Config(p.getDirectory().toPath());
+                config.setRecurseDepth(PathWatcher.Config.UNLIMITED_DEPTH);
+                for (String pattern:p.getExcludes())
+                    config.addExcludeGlobRelative(pattern);
+                for (String pattern:p.getIncludes())
+                    config.addIncludeGlobRelative(pattern);
+                scanner.watch(config);
+            }
+        }
+    }
+    
     /**
      * Find which dependencies are suitable for addition to the virtual
      * WEB-INF lib.
-     * 
-     * @param mavenProject this project
      */
     private Collection<Artifact> getWebInfLibArtifacts()
     {

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyEffectiveWebXml.java

@@ -57,18 +57,12 @@ public class JettyEffectiveWebXml extends AbstractWebAppMojo
         generate();
     }
 
-    /**
-     *
-     */
     @Override
     protected void startJettyForked() throws MojoExecutionException
     {
         generate();
     }
 
-    /**
-     *
-     */
     @Override
     protected void startJettyDistro() throws MojoExecutionException
     {

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyRunMojo.java

@@ -120,13 +120,9 @@ public class JettyRunMojo extends AbstractWebAppMojo
             forker = newJettyForker();
             forker.setWaitForChild(true); //we run at the command line, echo child output and wait for it
             forker.setScan(true); //have the forked child notice changes to the webapp
-            
-            startScanner();
-            
             //TODO is it ok to start the scanner before we start jetty?
-            
+            startScanner();
             forker.start(); //forks jetty instance 
-            
         }
         catch (Exception e)
         {
@@ -141,10 +137,9 @@ public class JettyRunMojo extends AbstractWebAppMojo
         {
             distroForker = newJettyDistroForker();
             distroForker.setWaitForChild(true); //we always run at the command line, echo child output and wait for it
+            //TODO is it ok to start the scanner before we start jetty?
             startScanner();
             distroForker.start(); //forks a jetty distro
-
-            //TODO is it ok to start the scanner before we start jetty?
         }
         catch (Exception e)
         {
@@ -263,37 +258,8 @@ public class JettyRunMojo extends AbstractWebAppMojo
             scanner.watch(a.getFile().toPath());
         }
         
-        //handle the explicit extra scan targets
+        //set up any extra files or dirs to watch
-        if (scanTargets != null)
+        configureScanTargetsAndPatterns(scanner);
-        {
-            for (File f:scanTargets)
-            {
-                if (f.isDirectory())
-                {
-                    PathWatcher.Config config = new PathWatcher.Config(f.toPath());
-                    config.setRecurseDepth(PathWatcher.Config.UNLIMITED_DEPTH);
-                    scanner.watch(config);
-                }
-                else
-                    scanner.watch(f.toPath());
-            }
-        }
-        
-        //handle the extra scan patterns
-        if (scanTargetPatterns != null)
-        {
-            for (ScanTargetPattern p:scanTargetPatterns)
-            {
-                PathWatcher.Config config = new PathWatcher.Config(p.getDirectory().toPath());
-                config.setRecurseDepth(PathWatcher.Config.UNLIMITED_DEPTH);
-                for (String pattern:p.getExcludes())
-                    config.addExcludeGlobRelative(pattern);
-                for (String pattern:p.getIncludes())
-                    config.addIncludeGlobRelative(pattern);
-                scanner.watch(config);
-            }
-        }
-      
 
         scanner.watch(project.getFile().toPath());
 

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyRunWarMojo.java

@@ -50,7 +50,6 @@ import org.eclipse.jetty.util.StringUtil;
 @Execute(phase = LifecyclePhase.PACKAGE)
 public class JettyRunWarMojo extends AbstractWebAppMojo
 {   
-    //Start of parameters only valid for runType=EMBED  
     /**
      * The interval in seconds to pause before checking if changes
      * have occurred and re-deploying as necessary. A value 
@@ -186,6 +185,9 @@ public class JettyRunWarMojo extends AbstractWebAppMojo
         scanner.watch(project.getFile().toPath());
         scanner.watch(war);
         
+        //set up any extra files or dirs to watch
+        configureScanTargetsAndPatterns(scanner);
+        
         scanner.addListener(new PathWatcher.EventListListener()
         {
             @Override
@@ -250,7 +252,7 @@ public class JettyRunWarMojo extends AbstractWebAppMojo
                 {
                     getLog().info("Reconfiguring scanner after change to pom.xml ...");
                     scanner.reset();
-                    warArtifacts = null; ///TODO if the pom changes for the forked case, how would we get the forked process to stop and restart?
+                    warArtifacts = null;
                     configureScanner();
                 }
                 
@@ -269,8 +271,7 @@ public class JettyRunWarMojo extends AbstractWebAppMojo
                 {
                     getLog().info("Reconfiguring scanner after change to pom.xml ...");
                     scanner.reset();
-                    warArtifacts = null; //TODO if there are any changes to the pom, then we would have to tell the
+                    warArtifacts = null;
-                    //existing forked distro process to stop, then rerun the configuration and then refork - too complicated??!
                     configureScanner();
                 }
                 configureWebApp();

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyStartMojo.java

@@ -26,15 +26,14 @@ import org.apache.maven.plugins.annotations.ResolutionScope;
 
 /**
  *  <p>
- *  This goal is similar to the jetty:run goal, EXCEPT that it is designed to be bound to an execution inside your pom, rather
+ *  This goal is similar to the jetty:run goal in that it it starts jetty on an unassembled webapp, 
- *  than being run from the command line. 
+ *  EXCEPT that it is designed to be bound to an execution inside your pom. Thus, this goal does NOT
+ *  run a parallel build cycle, so you must be careful to ensure that you bind it to a phase in 
+ *  which all necessary generated files and classes for the webapp have been created.
  *  </p>
  * <p>
- *  When using it, be careful to ensure that you bind it to a phase in which all necessary generated files and classes for the webapp
+ * This goal will NOT scan for changes in either the webapp project or any scanTargets or scanTargetPatterns.
- *  will have been created. If you run it from the command line, then also ensure that all necessary generated files and classes for
- *  the webapp ALREADY exist.
  * </p>
- *
  */
 @Mojo(name = "start", requiresDependencyResolution = ResolutionScope.TEST)
 @Execute(phase = LifecyclePhase.VALIDATE)
@@ -67,7 +66,6 @@ public class JettyStartMojo extends AbstractWebAppMojo
         }
     }
 
-
     /**
      * Start the webapp in a forked jetty process. Use the
      * jetty:stop goal to terminate.
@@ -81,7 +79,6 @@ public class JettyStartMojo extends AbstractWebAppMojo
             jetty.setWaitForChild(false); //we never wait for child
             jetty.setJettyOutputFile(getJettyOutputFile("jetty-start.out"));
             jetty.start(); //forks jetty instance
-            
         }
         catch (Exception e)
         {

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/JettyStartWarMojo.java

@@ -83,7 +83,6 @@ public class JettyStartWarMojo extends AbstractWebAppMojo
         }
     }
 
-    
     /**
      * Fork a jetty instance to run the given war.
      */
@@ -123,13 +122,11 @@ public class JettyStartWarMojo extends AbstractWebAppMojo
         }
     }
    
-    /**
-     *
-     */
     @Override
     protected void verifyPomConfiguration() throws MojoExecutionException
     {
-        //Do not verify the configuration of the webapp, as we are deploying
+        //Do nothing here, as we want the user to configure a war to deploy,
-        //a random war instead.
+        //or we default to the webapp that is running the jetty plugin, but
+        //we need to delay that decision until configureWebApp().
     }
 }

jetty-maven-plugin/src/main/java/org/eclipse/jetty/maven/plugin/OverlayManager.java

@@ -28,6 +28,8 @@ import java.util.Set;
 
 import org.apache.maven.artifact.Artifact;
 import org.eclipse.jetty.util.resource.Resource;
+import org.eclipse.jetty.util.resource.ResourceCollection;
+import org.eclipse.jetty.webapp.WebAppContext;
 
 /**
  * OverlayManager
@@ -44,6 +46,34 @@ public class OverlayManager
         this.warPlugin = warPlugin;
     }
 
+    public void applyOverlays (JettyWebAppContext webApp)
+        throws Exception
+    {
+        List<Resource> resourceBases = new ArrayList<Resource>();
+
+        for (Overlay o : getOverlays())
+        {
+            //can refer to the current project in list of overlays for ordering purposes
+            if (o.getConfig() != null && o.getConfig().isCurrentProject() && webApp.getBaseResource().exists())
+            {
+                resourceBases.add(webApp.getBaseResource()); 
+                continue;
+            }
+            //add in the selectively unpacked overlay in the correct order to the webapp's resource base
+            resourceBases.add(unpackOverlay(o));
+        }
+
+        if (!resourceBases.contains(webApp.getBaseResource()) && webApp.getBaseResource().exists())
+        {
+            if (webApp.getBaseAppFirst())
+                resourceBases.add(0, webApp.getBaseResource());
+            else
+                resourceBases.add(webApp.getBaseResource());
+        }
+        
+        webApp.setBaseResource(new ResourceCollection(resourceBases.toArray(new Resource[resourceBases.size()])));
+    }
+    
     /**
      * Generate an ordered list of overlays
      */