[JENKINS-49328] Prevent inappropriate serialization of MultiJobBuild.…

…SubBuild.build.
jenkinsci · Feb 2, 2018 · a1b8be0 · a1b8be0
1 parent 5dc7ed5
commit a1b8be0
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 10 deletions.
diff --git a/src/main/java/com/tikal/jenkins/plugins/multijob/MultiJobBuild.java b/src/main/java/com/tikal/jenkins/plugins/multijob/MultiJobBuild.java
@@ -196,7 +196,9 @@ public static class SubBuild {
         private final String url;
         private final boolean retry;
         private final boolean aborted;
-        private final AbstractBuild<?, ?> build;
+        @Deprecated
+        private transient AbstractBuild<?, ?> build;
+        private String buildID;
 
         public SubBuild(String parentJobName, int parentBuildNumber,
                 String jobName, int buildNumber, String phaseName,
@@ -213,7 +215,7 @@ public SubBuild(String parentJobName, int parentBuildNumber,
             this.url = url;
             this.retry = false;
             this.aborted = false;
-            this.build = build;
+            buildID = build.getExternalizableId();
         }
 
         public SubBuild(String parentJobName, int parentBuildNumber,
@@ -231,7 +233,14 @@ public SubBuild(String parentJobName, int parentBuildNumber,
             this.url = url;
             this.retry = retry;
             this.aborted = aborted;
-			this.build = build;
+            buildID = build.getExternalizableId();
+        }
+
+        private Object readResolve() {
+            if (build != null) {
+                buildID = build.getExternalizableId();
+            }
+            return this;
         }
 
         @Exported
@@ -299,7 +308,7 @@ public String toString() {
 
 		@Exported
 		public AbstractBuild<?,?> getBuild() {
-			return build;
+			return (AbstractBuild) Run.fromExternalizableId(buildID);
 		}
     }
 }
diff --git a/src/test/java/com/tikal/jenkins/plugins/multijob/test/ConditionalPhaseTest.java b/src/test/java/com/tikal/jenkins/plugins/multijob/test/ConditionalPhaseTest.java
@@ -1,5 +1,6 @@
 package com.tikal.jenkins.plugins.multijob.test;
 
+import com.tikal.jenkins.plugins.multijob.MultiJobBuild;
 import hudson.model.Result;
 import hudson.model.TopLevelItem;
 import hudson.model.Cause.UserCause;
@@ -14,7 +15,6 @@
 import org.jenkins_ci.plugins.run_condition.core.AlwaysRun;
 import org.jenkinsci.plugins.conditionalbuildstep.ConditionalBuilder;
 import org.junit.Assert;
-import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.JenkinsRule;
@@ -26,18 +26,20 @@
 import com.tikal.jenkins.plugins.multijob.PhaseJobsConfig.KillPhaseOnJobResultCondition;
 import com.tikal.jenkins.plugins.multijob.views.PhaseWrapper;
 import com.tikal.jenkins.plugins.multijob.views.ProjectWrapper;
+import org.jvnet.hudson.test.RestartableJenkinsRule;
 
 /**
  * @author Bartholdi Dominik (imod)
  */
 public class ConditionalPhaseTest {
     @Rule
-    public JenkinsRule j = new JenkinsRule();
+    public RestartableJenkinsRule rr = new RestartableJenkinsRule();
 
     @Test
     public void testConditionalPhase() throws Exception {
-        j.jenkins.getInjector().injectMembers(this);
-
+        rr.then(new RestartableJenkinsRule.Step() {
+            @Override
+            public void run(JenkinsRule j) throws Throwable {
         // MultiTop
         //  |_ FirstPhase
         //      |_ free
@@ -72,7 +74,7 @@ public void testConditionalPhase() throws Exception {
         blist.add(secondPhaseBuilder);
         multi.getBuildersList().add(new ConditionalBuilder(new AlwaysRun(), new BuildStepRunner.Run(), blist));
 
-        j.assertBuildStatus(Result.SUCCESS, multi.scheduleBuild2(0, new UserCause()).get());
+        MultiJobBuild b = j.assertBuildStatus(Result.SUCCESS, multi.scheduleBuild2(0, new UserCause()).get());
         Assert.assertTrue("shell task writes 'hello' to log", free.getLastBuild().getLog(10).contains("hello"));
         Assert.assertTrue("shell task writes 'dude' to log", multi.getLastBuild().getLog(10).contains("dude"));
         // check for correct number of items to be displayed
@@ -94,7 +96,25 @@ public void testConditionalPhase() throws Exception {
         Assert.assertEquals("there should be two phases", 2, numberOfPhases);
         Assert.assertEquals("there should be three projects", 3, numberOfProjects);
         Assert.assertEquals("there should be 1 conditional phase", 1, numberOfConditionalPhases);
-
+        assertSubBuilds(b, "Free2#1", "Free#1");
+            }
+        });
+        rr.then(new RestartableJenkinsRule.Step() {
+            @Override
+            public void run(JenkinsRule j) throws Throwable {
+                MultiJobProject multi = j.jenkins.getItemByFullName("MultiTop", MultiJobProject.class);
+                MultiJobBuild b = multi.getBuildByNumber(1);
+                // JENKINS-49328: ensure that SubBuild.getBuild() works after a restart:
+                assertSubBuilds(b, "Free2#1", "Free#1");
+            }
+        });
+    }
+
+    private void assertSubBuilds(MultiJobBuild b, String... externalIDs) {
+        List<String> ids = new ArrayList<>();
+        for (MultiJobBuild.SubBuild sub : b.getSubBuilds()) {
+            ids.add(sub.getBuild().getExternalizableId());
+        }
     }
 
 }