From 8c124beb469bfc465b5e0225df7a77a8765dab6c Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 4 Oct 2024 13:21:31 -0400 Subject: [PATCH 1/9] `FlowExecution.getOwner` left null after `WorkflowRun.reload` --- .../jenkinsci/plugins/workflow/job/WorkflowRun.java | 4 ++++ .../plugins/workflow/job/WorkflowRunTest.java | 10 ++++++++++ 2 files changed, 14 insertions(+) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java b/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java index 0e77ac5d..9080e76b 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java @@ -553,6 +553,10 @@ private String key() { // super.reload() forces result to be FAILURE, so working around that new XmlFile(XSTREAM,new File(getRootDir(),"build.xml")).unmarshal(this); + var _execution = execution; + if (_execution != null) { + _execution.onLoad(new Owner(this)); + } } @Override protected void onLoad() { diff --git a/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java b/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java index 68d6a872..ed7fb671 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java @@ -29,6 +29,7 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; import static org.junit.Assert.assertEquals; @@ -600,6 +601,15 @@ private void assertCulprits(WorkflowRun b, String... expectedIds) throws IOExcep assertPollingBaselines(b3.checkouts(listener), nullValue(), nullValue()); } + @Test public void reloadOwner() throws Exception { + WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p"); + p.setDefinition(new CpsFlowDefinition("", true)); + WorkflowRun b = r.buildAndAssertSuccess(p); + assertThat("right owner before reload", b.getExecution().getOwner(), is(b.asFlowExecutionOwner())); + b.reload(); + assertThat("right owner after reload", b.getExecution().getOwner(), is(b.asFlowExecutionOwner())); + } + // This test is to ensure that the shortDescription on the CancelCause is escaped properly on summary.jelly @Issue("SECURITY-3042") @Test public void escapedDisplayNameAfterAbort() throws Exception { From c33437fa9715d371b5a12c699c25dc6794e813f4 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 7 Nov 2024 15:35:21 -0500 Subject: [PATCH 2/9] Various test failures; this change is not intended for running builds --- .../org/jenkinsci/plugins/workflow/job/WorkflowRun.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java b/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java index e91748ff..7ead89dd 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java @@ -553,9 +553,11 @@ private String key() { // super.reload() forces result to be FAILURE, so working around that new XmlFile(XSTREAM,new File(getRootDir(),"build.xml")).unmarshal(this); - var _execution = execution; - if (_execution != null) { - _execution.onLoad(new Owner(this)); + if (Boolean.TRUE.equals(this.completed)) { + var _execution = execution; + if (_execution != null) { + _execution.onLoad(new Owner(this)); + } } } From 67a8dbcf7d05ac6630e49fa6b79fc86b44afeb94 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 7 Nov 2024 15:39:27 -0500 Subject: [PATCH 3/9] Adding synchronization, though the locking model in this class is pretty confusing --- .../jenkinsci/plugins/workflow/job/WorkflowRun.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java b/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java index 7ead89dd..2a23c4cf 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java @@ -553,10 +553,12 @@ private String key() { // super.reload() forces result to be FAILURE, so working around that new XmlFile(XSTREAM,new File(getRootDir(),"build.xml")).unmarshal(this); - if (Boolean.TRUE.equals(this.completed)) { - var _execution = execution; - if (_execution != null) { - _execution.onLoad(new Owner(this)); + synchronized (getMetadataGuard()) { + if (Boolean.TRUE.equals(this.completed)) { + var _execution = execution; + if (_execution != null) { + _execution.onLoad(new Owner(this)); + } } } } From 2bf2a14798a07a89c0bffdb0ec2c8b780c494b1a Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 7 Nov 2024 15:42:17 -0500 Subject: [PATCH 4/9] Simplifying field access --- .../java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java b/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java index 2a23c4cf..43ff31f9 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java @@ -555,9 +555,8 @@ private String key() { new XmlFile(XSTREAM,new File(getRootDir(),"build.xml")).unmarshal(this); synchronized (getMetadataGuard()) { if (Boolean.TRUE.equals(this.completed)) { - var _execution = execution; - if (_execution != null) { - _execution.onLoad(new Owner(this)); + if (execution != null) { + execution.onLoad(new Owner(this)); } } } From e7b219c68ea83c0001fc9aed42bf72c407a7c77b Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 7 Nov 2024 15:54:08 -0500 Subject: [PATCH 5/9] Ah, only made sense to do this when `executionLoaded` --- .../java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java b/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java index 43ff31f9..a72ae7d9 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java @@ -555,7 +555,7 @@ private String key() { new XmlFile(XSTREAM,new File(getRootDir(),"build.xml")).unmarshal(this); synchronized (getMetadataGuard()) { if (Boolean.TRUE.equals(this.completed)) { - if (execution != null) { + if (execution != null && executionLoaded) { execution.onLoad(new Owner(this)); } } From 05115ad96bdf3f6090f24ed2a98103c80a4a8217 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 7 Nov 2024 16:16:25 -0500 Subject: [PATCH 6/9] Despite using `JenkinsRule` as a `Rule` not `ClassRule`, reusing the same project name for `reloadOwner` breaks the `culprits` test https://github.com/jenkinsci/workflow-job-plugin/pull/472#issuecomment-2463214340 --- .../org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java b/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java index bf55edaf..ed84651e 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunTest.java @@ -620,7 +620,7 @@ private static String checkoutString(GitSampleRepoRule repo, boolean changelog, } @Test public void reloadOwner() throws Exception { - WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p"); + WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "reloadOwner"); p.setDefinition(new CpsFlowDefinition("", true)); WorkflowRun b = r.buildAndAssertSuccess(p); assertThat("right owner before reload", b.getExecution().getOwner(), is(b.asFlowExecutionOwner())); From 30581507237b6c9e36d7056f01633651a9342b63 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 7 Nov 2024 16:22:12 -0500 Subject: [PATCH 7/9] Single `if` statement --- .../org/jenkinsci/plugins/workflow/job/WorkflowRun.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java b/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java index a72ae7d9..a2722d18 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java @@ -554,10 +554,8 @@ private String key() { // super.reload() forces result to be FAILURE, so working around that new XmlFile(XSTREAM,new File(getRootDir(),"build.xml")).unmarshal(this); synchronized (getMetadataGuard()) { - if (Boolean.TRUE.equals(this.completed)) { - if (execution != null && executionLoaded) { - execution.onLoad(new Owner(this)); - } + if (Boolean.TRUE.equals(this.completed) && execution != null && executionLoaded) { + execution.onLoad(new Owner(this)); } } } From 83e8f00cc86a28ea4dd4b2eb9e64f2264b37668b Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 7 Nov 2024 16:22:34 -0500 Subject: [PATCH 8/9] Simplified syntax --- .../java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java b/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java index a2722d18..5d5b07ee 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java @@ -554,7 +554,7 @@ private String key() { // super.reload() forces result to be FAILURE, so working around that new XmlFile(XSTREAM,new File(getRootDir(),"build.xml")).unmarshal(this); synchronized (getMetadataGuard()) { - if (Boolean.TRUE.equals(this.completed) && execution != null && executionLoaded) { + if (Boolean.TRUE.equals(completed) && execution != null && executionLoaded) { execution.onLoad(new Owner(this)); } } From 8e33232b5dcee7ecf8a4935514da6b09fd1ba03f Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 7 Nov 2024 16:44:01 -0500 Subject: [PATCH 9/9] SpotBugs possibly being too cautious --- .../org/jenkinsci/plugins/workflow/job/WorkflowRun.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java b/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java index 5d5b07ee..dcce8d4c 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java @@ -554,8 +554,11 @@ private String key() { // super.reload() forces result to be FAILURE, so working around that new XmlFile(XSTREAM,new File(getRootDir(),"build.xml")).unmarshal(this); synchronized (getMetadataGuard()) { - if (Boolean.TRUE.equals(completed) && execution != null && executionLoaded) { - execution.onLoad(new Owner(this)); + if (Boolean.TRUE.equals(completed) && executionLoaded) { + var _execution = execution; + if (_execution != null) { + _execution.onLoad(new Owner(this)); + } } } }