[FIXED JENKINS-44263] - Get rid of explict cached value access.

In EnvInject 3.1 I was doing refactoring for better Pipeline Support, and I have somehow removed cache calculation in the Build lazy loading chain. It caused regression (missing data) in EnvInjectPluginAction#getEnvInjectVarList(), which was always reading the cache value instead of invoking the Getter method.
jenkinsci · Jun 19, 2017 · 994a02b · 994a02b
1 parent 5066aee
commit 994a02b
Show file tree

Hide file tree

Showing 2 changed files with 67 additions and 4 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectPluginAction.java b/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectPluginAction.java
@@ -63,10 +63,11 @@ public Object getTarget() {
 
     @Nonnull
     private EnvInjectVarList getEnvInjectVarList() {
-        if (envMap == null) {
+        final Map<String, String> currentEnvMap = getEnvMap();
+        if (currentEnvMap == null) {
             return new EnvInjectVarList(Collections.<String,String>emptyMap());
         }
-        return new EnvInjectVarList(Maps.transformEntries(envMap,
+        return new EnvInjectVarList(Maps.transformEntries(currentEnvMap,
                 new Maps.EntryTransformer<String, String, String>() {
                     public String transformEntry(String key, String value) {
                         final Set<String> sensibleVars = getSensibleVariables();
@@ -77,8 +78,9 @@ public String transformEntry(String key, String value) {
 
     @Override
     public void buildEnvVars(@Nonnull AbstractBuild<?, ?> build, @Nonnull EnvVars env) {
-        if (envMap != null) {
-            env.putAll(envMap);
+        final Map<String, String> currentEnvMap = getEnvMap();
+        if (currentEnvMap != null) {
+            env.putAll(currentEnvMap);
         }
     }
 }
diff --git a/src/test/java/org/jenkinsci/plugins/envinject/EnvInjectVarListTest.java b/src/test/java/org/jenkinsci/plugins/envinject/EnvInjectVarListTest.java
@@ -0,0 +1,61 @@
+package org.jenkinsci.plugins.envinject;
+
+import hudson.model.FreeStyleBuild;
+import hudson.model.FreeStyleProject;
+import hudson.model.Run;
+import java.util.Map;
+import javax.annotation.Nonnull;
+import org.junit.Assert;
+import static org.junit.Assert.assertEquals;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.SingleFileSCM;
+
+/**
+ * Tests for {@link EnvInjectVarList}.
+ * @author Oleg Nenashev
+ */
+public class EnvInjectVarListTest {
+
+    @Rule 
+    public JenkinsRule j = new JenkinsRule();
+
+    @Test 
+    @Issue("JENKINS-44263")
+    public void envVarsShouldBeCachedProperlyAfterReload() throws Exception {
+        final FreeStyleProject p = j.jenkins.createProject(FreeStyleProject.class, "project");
+        p.getBuildersList().add(new EnvInjectBuilder(null, "TEXT_VAR=tvalue"));
+        final FreeStyleBuild build = j.buildAndAssertSuccess(p);
+
+        // Check vars before the reload
+        {
+            EnvInjectVarList varList = getVarListOrFail(build);
+            Map<String, String> envMap = varList.getEnvMap();
+            Assert.assertNotNull("EnvInject vars list is null for the run before reload", envMap);
+            Assert.assertTrue("TEXT_VAR is not present in the list", envMap.containsKey("TEXT_VAR"));
+        }
+
+        // Reload and check vars
+        // build.reload() does not assign parents for RunAction2, hence we apply a workaround
+        p.doReload();
+        final Run<?, ?> reloadedBuild = p.getBuildByNumber(build.getNumber());
+        {
+            EnvInjectVarList varList = getVarListOrFail(reloadedBuild);
+            Map<String, String> envMap = varList.getEnvMap();
+            Assert.assertNotNull("EnvInject vars list is null for the run after the reload", envMap);
+            Assert.assertTrue("TEXT_VAR is not present in the list", envMap.containsKey("TEXT_VAR"));
+            Assert.assertEquals("TEXT_VAR has wrong value", "tvalue", envMap.get("TEXT_VAR"));
+        }
+    }
+
+    @Nonnull
+    private EnvInjectVarList getVarListOrFail(@Nonnull Run<?, ?> run) throws AssertionError {
+        EnvInjectPluginAction action = run.getAction(EnvInjectPluginAction.class);
+        Assert.assertNotNull("EnvInject action is not set for the run " + run, action);
+        EnvInjectVarList list = (EnvInjectVarList)action.getTarget();
+        Assert.assertNotNull("Unexpected state. EnvInject Var List is nul for the run " + run, list);
+        return list;
+    }
+}