[FIX JENKINS-39268] Properties are not passed to Maven command by Maven build step (#2638)

* [FIX JENKINS-39268] Properties are not passed to Maven command by Maven build step

* [FIX JENKINS-39268] Properties are not passed to Maven command by Maven build step

* [FIX JENKINS-39268] Properties are not passed to Maven command by Maven build step

* [FIX JENKINS-39268] Changed order in which properties are appended to command line: properties appended later win in case of conflicts

* [FIX JENKINS-39268] More precise messages in some assertions

* [FIX JENKINS-39268] Cleanup unused imports

* [FIX JENKINS-39268] Added functional tests for Maven task.

(cherry picked from commit 138ce3d8)

core/src/main/java/hudson/tasks/Maven.java

@@ -340,13 +340,17 @@ public class Maven extends Builder {
                 }
             }
 
+            Set<String> sensitiveVars = build.getSensitiveBuildVariables();
+
+            // Inject environment variables only if chosen to do so
             if (isInjectBuildVariables()) {
-                Set<String> sensitiveVars = build.getSensitiveBuildVariables();
-                args.addKeyValuePairs("-D",build.getBuildVariables(),sensitiveVars);
-                final VariableResolver<String> resolver = new Union<String>(new ByMap<String>(env), vr);
-                args.addKeyValuePairsFromPropertyString("-D",this.properties,resolver,sensitiveVars);
+                args.addKeyValuePairs("-D", build.getBuildVariables(), sensitiveVars);
             }
 
+            // Add properties from builder configuration, AFTER the injected build variables.
+            final VariableResolver<String> resolver = new Union<String>(new ByMap<String>(env), vr);
+            args.addKeyValuePairsFromPropertyString("-D", this.properties, resolver, sensitiveVars);
+
             if (usesPrivateRepository())
                 args.add("-Dmaven.repo.local=" + build.getWorkspace().child(".repository"));
             args.addTokenized(normalizedTarget);

test/src/test/java/hudson/tasks/MavenTest.java

@@ -45,10 +45,8 @@ import hudson.tasks.Maven.MavenInstallation.DescriptorImpl;
 import hudson.tools.ToolProperty;
 import hudson.tools.ToolPropertyDescriptor;
 import hudson.tools.InstallSourceProperty;
-import hudson.tools.ToolInstallation;
 import hudson.util.DescribableList;
 
-import java.io.IOException;
 import java.util.Collections;
 
 import javax.xml.transform.Source;
@@ -63,13 +61,11 @@ import hudson.model.PasswordParameterDefinition;
 import org.jvnet.hudson.test.Issue;
 import static org.junit.Assert.*;
 
-import org.apache.tools.ant.filters.TokenFilter.ContainsString;
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.ExtractResourceSCM;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.ToolInstallations;
-import org.jvnet.hudson.test.SingleFileSCM;
 
 /**
  * @author Kohsuke Kawaguchi
@@ -323,21 +319,41 @@ public class MavenTest {
         FreeStyleProject p = j.createFreeStyleProject();
         p.updateByXml((Source) new StreamSource(getClass().getResourceAsStream("MavenTest/doPassBuildVariablesOptionally.xml")));
         String log = j.buildAndAssertSuccess(p).getLog();
-        assertTrue(p.getBuildersList().get(Maven.class).isInjectBuildVariables());
-        assertTrue("Build variables are injected", log.contains("-DNAME=VALUE"));
+        assertTrue("Build variables injection should be enabled by default when loading from XML", p.getBuildersList().get(Maven.class).isInjectBuildVariables());
+        assertTrue("Build variables should be injected by default when loading from XML", log.contains("-DNAME=VALUE"));
 
         p.getBuildersList().clear();
         p.getBuildersList().add(new Maven("--help", maven.getName(), null, null, null, false, null, null, false/*do not inject*/));
 
         log = j.buildAndAssertSuccess(p).getLog();
-        assertFalse("Build variables are not injected", log.contains("-DNAME=VALUE"));
+        assertFalse("Build variables should not be injected", log.contains("-DNAME=VALUE"));
 
         p.getBuildersList().clear();
         p.getBuildersList().add(new Maven("--help", maven.getName(), null, null, null, false, null, null, true/*do inject*/));
 
         log = j.buildAndAssertSuccess(p).getLog();
-        assertTrue("Build variables are injected", log.contains("-DNAME=VALUE"));
+        assertTrue("Build variables should be injected", log.contains("-DNAME=VALUE"));
 
-        assertFalse(new Maven("", "").isInjectBuildVariables());
+        assertFalse("Build variables injection should be disabled by default", new Maven("", "").isInjectBuildVariables());
+    }
+
+    @Test public void doAlwaysPassProperties() throws Exception {
+        MavenInstallation maven = ToolInstallations.configureMaven3();
+
+        FreeStyleProject p = j.createFreeStyleProject();
+        String properties = "TEST_PROP1=VAL1\nTEST_PROP2=VAL2";
+
+        p.getBuildersList().add(new Maven("--help", maven.getName(), null, properties, null, false, null,
+                null, false/*do not inject build variables*/));
+        String log = j.buildAndAssertSuccess(p).getLog();
+        assertTrue("Properties should always be injected, even when build variables injection is disabled",
+                log.contains("-DTEST_PROP1=VAL1") && log.contains("-DTEST_PROP2=VAL2"));
+
+        p.getBuildersList().clear();
+        p.getBuildersList().add(new Maven("--help", maven.getName(), null, properties, null, false, null,
+                null, true/*do inject build variables*/));
+        log = j.buildAndAssertSuccess(p).getLog();
+        assertTrue("Properties should always be injected, even when build variables injection is enabled",
+                log.contains("-DTEST_PROP1=VAL1") && log.contains("-DTEST_PROP2=VAL2"));
     }
 }