[JENKINS-29537] EnvironmentContributingAction compatible with Workflow (#2975)

* [JENKINS-29537] EnvironmentContributingAction compatible with Workflow

+ Adds new method with default implementation in EnvironmentContributingAction to support Runs
+ Marks AbstractBuild as deprecated
+ Adds default implementation for deprecated method for backward
compatiblity.

* Tiny improvements in javadoc

core/src/main/java/hudson/model/AbstractBuild.java

@@ -887,7 +887,7 @@ public abstract class AbstractBuild<P extends AbstractProject<P,R>,R extends Abs
                 e.buildEnvVars(env);
 
         for (EnvironmentContributingAction a : getActions(EnvironmentContributingAction.class))
-            a.buildEnvVars(this,env);
+            a.buildEnvVars(this,env,getBuiltOn());
 
         EnvVars.resolve(env);
 

core/src/main/java/hudson/model/EnvironmentContributingAction.java

@@ -24,9 +24,15 @@
 package hudson.model;
 
 import hudson.EnvVars;
+import hudson.Util;
 import hudson.model.Queue.Task;
 import hudson.tasks.Builder;
 import hudson.tasks.BuildWrapper;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.ProtectedExternally;
+
+import javax.annotation.CheckForNull;
+import javax.annotation.Nonnull;
 
 /**
  * {@link Action} that contributes environment variables during a build.
@@ -44,13 +50,42 @@ import hudson.tasks.BuildWrapper;
  * @see BuildWrapper
  */
 public interface EnvironmentContributingAction extends Action {
+    /**
+     * Called by {@link Run} or {@link AbstractBuild} to allow plugins to contribute environment variables.
+     *
+     * @param run
+     *      The calling build. Never null.
+     * @param node
+     *      The node execute on. Can be {@code null} when the Run is not binded to the node,
+     *      e.g. in Pipeline outside the {@code node() step}
+     * @param env
+     *      Environment variables should be added to this map.
+     * @since TODO
+     */
+    default void buildEnvVars(@Nonnull Run<?, ?> run, @Nonnull EnvVars env, @CheckForNull Node node) {
+        if (run instanceof AbstractBuild
+                && Util.isOverridden(EnvironmentContributingAction.class,
+                                     getClass(), "buildEnvVars", AbstractBuild.class, EnvVars.class)) {
+            buildEnvVars((AbstractBuild) run, env);
+        }
+    }
+
     /**
      * Called by {@link AbstractBuild} to allow plugins to contribute environment variables.
      *
+     * @deprecated Use {@link #buildEnvVars(Run, EnvVars, Node)} instead
+     *
      * @param build
      *      The calling build. Never null.
      * @param env
      *      Environment variables should be added to this map.
      */
-    void buildEnvVars(AbstractBuild<?, ?> build, EnvVars env);
+    @Deprecated
+    @Restricted(ProtectedExternally.class)
+    default void buildEnvVars(AbstractBuild<?, ?> build, EnvVars env) {
+        if (Util.isOverridden(EnvironmentContributingAction.class,
+                              getClass(), "buildEnvVars", Run.class, EnvVars.class, Node.class)) {
+            buildEnvVars(build, env, build.getBuiltOn());
+        }
+    }
 }

core/src/main/java/hudson/model/ParametersAction.java

@@ -138,10 +138,11 @@ public class ParametersAction implements RunAction2, Iterable<ParameterValue>, Q
         }
     }
 
-    public void buildEnvVars(AbstractBuild<?,?> build, EnvVars env) {
+    @Override
+    public void buildEnvVars(Run<?,?> run, EnvVars env, Node node) {
         for (ParameterValue p : getParameters()) {
             if (p == null) continue;
-            p.buildEnvironment(build, env); 
+            p.buildEnvironment(run, env);
         }
     }
 

core/src/main/java/hudson/model/Run.java

@@ -2301,6 +2301,9 @@ public abstract class Run <JobT extends Job<JobT,RunT>,RunT extends Run<JobT,Run
         for (EnvironmentContributor ec : EnvironmentContributor.all().reverseView())
             ec.buildEnvironmentFor(this,env,listener);
 
+        for (EnvironmentContributingAction a : getActions(EnvironmentContributingAction.class))
+            a.buildEnvVars(this,env,n);
+
         return env;
     }
 

core/src/test/java/hudson/model/EnvironmentContributingActionTest.java

+package hudson.model;
+
+import hudson.EnvVars;
+import org.junit.Test;
+
+import javax.annotation.CheckForNull;
+
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.assertFalse;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+
+public class EnvironmentContributingActionTest {
+    class OverrideRun extends InvisibleAction implements EnvironmentContributingAction {
+        private boolean wasCalled = false;
+
+        @Override
+        public void buildEnvVars(Run<?, ?> run, EnvVars env, @CheckForNull Node node) {
+            wasCalled = true;
+        }
+
+        boolean wasNewMethodCalled() {
+            return wasCalled;
+        }
+    }
+
+    class OverrideAbstractBuild extends InvisibleAction implements EnvironmentContributingAction {
+        private boolean wasCalled = false;
+
+        @Override
+        @SuppressWarnings("deprecation")
+        public void buildEnvVars(AbstractBuild<?, ?> abstractBuild, EnvVars envVars) {
+            wasCalled = true;
+        }
+
+        boolean wasDeprecatedMethodCalled() {
+            return wasCalled;
+        }
+    }
+
+    class OverrideBoth extends InvisibleAction implements EnvironmentContributingAction {
+        private boolean wasCalledAstractBuild = false;
+        private boolean wasCalledRun = false;
+
+        @SuppressWarnings("deprecation")
+        @Override
+        public void buildEnvVars(AbstractBuild<?, ?> abstractBuild, EnvVars envVars) {
+            wasCalledAstractBuild = true;
+        }
+
+        @Override
+        public void buildEnvVars(Run<?, ?> run, EnvVars env, @CheckForNull Node node) {
+            wasCalledRun = true;
+        }
+
+        boolean wasDeprecatedMethodCalled() {
+            return wasCalledAstractBuild;
+        }
+
+        boolean wasRunCalled() {
+            return wasCalledRun;
+        }
+    }
+
+    private final EnvVars envVars = mock(EnvVars.class);
+
+    @Test
+    public void testOverrideRunMethodAndCallNewMethod() throws Exception {
+        Run run = mock(Run.class);
+        Node node = mock(Node.class);
+
+        OverrideRun overrideRun = new OverrideRun();
+        overrideRun.buildEnvVars(run, envVars, node);
+
+        assertTrue(overrideRun.wasNewMethodCalled());
+    }
+
+    /**
+     * If only non-deprecated method was overridden it would be executed even if someone would call deprecated method.
+     * @throws Exception if happens.
+     */
+    @Test
+    @SuppressWarnings("deprecation")
+    public void testOverrideRunMethodAndCallDeprecatedMethod() throws Exception {
+        AbstractBuild abstractBuild = mock(AbstractBuild.class);
+        when(abstractBuild.getBuiltOn()).thenReturn(mock(Node.class));
+
+        OverrideRun overrideRun = new OverrideRun();
+        overrideRun.buildEnvVars(abstractBuild, envVars);
+
+        assertTrue(overrideRun.wasNewMethodCalled());
+    }
+
+    /**
+     * {@link AbstractBuild} should work as before.
+     * @throws Exception if happens.
+     */
+    @Test
+    public void testOverrideAbstractBuildAndCallNewMethodWithAbstractBuild() throws Exception {
+        AbstractBuild abstractBuild = mock(AbstractBuild.class);
+        Node node = mock(Node.class);
+
+        OverrideAbstractBuild action = new OverrideAbstractBuild();
+        action.buildEnvVars(abstractBuild, envVars, node);
+
+        assertTrue(action.wasDeprecatedMethodCalled());
+    }
+
+    /**
+     * {@link Run} should not execute method that was overridden for {@link AbstractBuild}.
+     * @throws Exception if happens.
+     */
+    @Test
+    public void testOverrideAbstractBuildAndCallNewMethodWithRun() throws Exception {
+        Run run = mock(Run.class);
+        Node node = mock(Node.class);
+
+        OverrideAbstractBuild action = new OverrideAbstractBuild();
+        action.buildEnvVars(run, envVars, node);
+
+        assertFalse(action.wasDeprecatedMethodCalled());
+    }
+
+    /**
+     * If someone wants to use overridden deprecated method, it would still work.
+     * @throws Exception if happens.
+     */
+    @Test
+    public void testOverrideAbstractBuildAndCallDeprecatedMethod() throws Exception {
+        AbstractBuild abstractBuild = mock(AbstractBuild.class);
+
+        OverrideAbstractBuild overrideRun = new OverrideAbstractBuild();
+        overrideRun.buildEnvVars(abstractBuild, envVars);
+
+        assertTrue(overrideRun.wasDeprecatedMethodCalled());
+    }
+
+    @Test
+    public void testOverrideBothAndCallNewMethod() throws Exception {
+        Run run = mock(Run.class);
+        Node node = mock(Node.class);
+
+        OverrideBoth overrideRun = new OverrideBoth();
+        overrideRun.buildEnvVars(run, envVars, node);
+
+        assertTrue(overrideRun.wasRunCalled());
+    }
+
+    @Test
+    public void testOverrideBothAndCallDeprecatedMethod() throws Exception {
+        AbstractBuild abstractBuild = mock(AbstractBuild.class);
+
+        OverrideBoth overrideRun = new OverrideBoth();
+        overrideRun.buildEnvVars(abstractBuild, envVars);
+
+        assertTrue(overrideRun.wasDeprecatedMethodCalled());
+    }
+}
\ No newline at end of file

core/src/test/java/hudson/model/ParametersActionTest.java

@@ -105,7 +105,7 @@ public class ParametersActionTest {
         
         // Interaction with build
         EnvVars vars = new EnvVars();
-        parametersAction.buildEnvVars(build, vars);
+        parametersAction.buildEnvVars(build, vars, build.getBuiltOn());
         assertEquals(2, vars.size());   
         parametersAction.createVariableResolver(build);