Merge pull request #129 from jglick/transient-JENKINS-49328

[JENKINS-49328] Prevent inappropriate serialization of MultiJobBuild.SubBuild.build
jenkinsci · Mar 1, 2018 · 8b65c64 · 8b65c64
2 parents e70e0b7 + 62ffb8f
commit 8b65c64
Show file tree

Hide file tree

Showing 6 changed files with 75 additions and 23 deletions.
diff --git a/Jenkinsfile b/Jenkinsfile
@@ -0,0 +1,2 @@
+buildPlugin jenkinsVersions: [null, '2.89.2'],
+  platforms: ['linux'] // TODO fix e.g. testConditionalPhase
diff --git a/pom.xml b/pom.xml
@@ -3,7 +3,8 @@
 	<parent>
 		<groupId>org.jenkins-ci.plugins</groupId>
 		<artifactId>plugin</artifactId>
-		<version>2.9</version>
+		<version>3.2</version>
+                <relativePath />
 	</parent>
 	<!-- parent> <groupId>org.jvnet.hudson.plugins</groupId> <artifactId>hudson-plugin-parent</artifactId> 
 		<version>2.1.0</version> <relativePath>../pom.xml</relativePath> </parent -->
@@ -46,14 +47,6 @@
 
 
 	<dependencies>
-		<dependency>
-			<groupId>org.jenkins-ci.main</groupId>
-			<artifactId>jenkins-war</artifactId>
-			<type>war</type>
-			<version>${project.parent.version}</version>
-			<scope>test</scope>
-		</dependency>
-
 		<dependency>
 			<groupId>org.jenkins-ci.plugins</groupId>
 			<artifactId>parameterized-trigger</artifactId>
@@ -91,6 +84,14 @@
 					<groupId>org.apache.httpcomponents</groupId>
 					<artifactId>httpcore</artifactId>
 				</exclusion>
+                                <exclusion> <!-- TODO pending maven-plugin 3.0 -->
+                                    <groupId>org.apache.ant</groupId>
+                                    <artifactId>ant</artifactId>
+                                </exclusion>
+                                <exclusion> <!-- TODO pending maven-plugin 3.0 -->
+                                    <groupId>com.google.inject</groupId>
+                                    <artifactId>guice</artifactId>
+                                </exclusion>
 			</exclusions>
 		</dependency>
 
@@ -219,6 +220,8 @@
 
 	<properties>
 		<findbugs.failOnError>false</findbugs.failOnError>
+                <jenkins.version>1.625.3</jenkins.version>
+                <java.level>7</java.level>
 	</properties>
 
 </project>
diff --git a/src/main/java/com/tikal/jenkins/plugins/multijob/MultiJobBuild.java b/src/main/java/com/tikal/jenkins/plugins/multijob/MultiJobBuild.java
@@ -27,6 +27,7 @@
 import hudson.model.StringParameterValue;
 import hudson.scm.ChangeLogSet;
 import hudson.scm.ChangeLogSet.Entry;
+import javax.annotation.CheckForNull;
 import jenkins.model.Jenkins;
 
 @ExportedBean(defaultVisibility = 999)
@@ -93,7 +94,7 @@ public String getBuildParams(SubBuild subBuild) {
                 } catch (Exception e) {
                     continue;
                 }
-                String value = stringParameter.value;
+                String value = (String) stringParameter.getValue();
                 String name = stringParameter.getName();
                 buffer.append("<input type='text' size='15' value='")
                         .append(name)
@@ -196,7 +197,7 @@ public static class SubBuild {
         private final String url;
         private final boolean retry;
         private final boolean aborted;
-        private final AbstractBuild<?, ?> build;
+        private String buildID;
 
         public SubBuild(String parentJobName, int parentBuildNumber,
                 String jobName, int buildNumber, String phaseName,
@@ -213,7 +214,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 +232,7 @@ public SubBuild(String parentJobName, int parentBuildNumber,
             this.url = url;
             this.retry = retry;
             this.aborted = aborted;
-			this.build = build;
+            buildID = build.getExternalizableId();
         }
 
         @Exported
@@ -298,8 +299,15 @@ public String toString() {
         }
 
 		@Exported
+        @CheckForNull
 		public AbstractBuild<?,?> getBuild() {
-			return build;
+            if (buildID != null) {
+                Run<?, ?> build = Run.fromExternalizableId(buildID);
+                if (build instanceof AbstractBuild) {
+                    return (AbstractBuild) build;
+                }
+            } // else null if loaded from historical data prior to JENKINS-49328
+			return null;
 		}
     }
 }
diff --git a/src/main/java/com/tikal/jenkins/plugins/multijob/MultiJobProject.java b/src/main/java/com/tikal/jenkins/plugins/multijob/MultiJobProject.java
@@ -158,4 +158,23 @@ protected void submit(StaplerRequest req, StaplerResponse rsp) throws IOExceptio
             setResumeEnvVars(resumeEnvVars);
         }
     }
+
+    // TODO as in https://github.com/jenkinsci/maven-plugin/pull/109:
+    @Override
+    public boolean scheduleBuild(Cause c) {
+        return super.scheduleBuild(c);
+    }
+    @Override
+    public boolean scheduleBuild(int quietPeriod, Cause c) {
+        return super.scheduleBuild(quietPeriod, c);
+    }
+    @Override
+    public boolean scheduleBuild(int quietPeriod) {
+        return super.scheduleBuild(quietPeriod);
+    }
+    @Override
+    public boolean scheduleBuild() {
+        return super.scheduleBuild();
+    }
+
 }
diff --git a/src/main/java/com/tikal/jenkins/plugins/multijob/PhaseJobsConfig.java b/src/main/java/com/tikal/jenkins/plugins/multijob/PhaseJobsConfig.java
@@ -252,12 +252,12 @@ private void savePhaseJobConfigParameters(String localJobName) {
 				String paramValue = null;
 				if (pdef instanceof StringParameterDefinition) {
 					StringParameterDefinition stringParameterDefinition = (StringParameterDefinition) pdef;
-					paramValue = stringParameterDefinition
-							.getDefaultParameterValue().value;
+					paramValue = (String) stringParameterDefinition
+							.getDefaultParameterValue().getValue();
 				} else if (pdef instanceof BooleanParameterDefinition) {
 					BooleanParameterDefinition booleanParameterDefinition = (BooleanParameterDefinition) pdef;
 					paramValue = String.valueOf(booleanParameterDefinition
-							.getDefaultParameterValue().value);
+							.getDefaultParameterValue().getValue());
 				}
 				sb.append(pdef.getName()).append("=").append(paramValue)
 						.append("\n");

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());
+        }
     }
 
 }