From 863c2735ddaa8d521c0a663c90840b2f50655cf5 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Tue, 17 Sep 2024 14:13:23 +0200 Subject: [PATCH 1/3] Introduce SaveableListener#onDeleted Usually `Saveable` objects are written, but it can happen on occasion that they get deleted, and it wasn't generating an event for every case. This provides a more fine-grained event that can be handled by implemented listeners. In my case, I have a use case in CloudBees CI where I need to clear a cache entry when a Saveable gets deleted from disk. --- .../main/java/hudson/logging/LogRecorder.java | 2 +- .../main/java/hudson/model/AbstractItem.java | 1 + core/src/main/java/hudson/model/Run.java | 2 ++ .../model/listeners/SaveableListener.java | 30 +++++++++++++------ core/src/main/java/jenkins/model/Nodes.java | 1 + .../logging/LogRecorderManagerTest.java | 2 +- .../java/hudson/model/AbstractItemTest.java | 26 +++++++++++----- test/src/test/java/hudson/model/RunTest.java | 15 ++++++++++ .../test/java/jenkins/model/NodesTest.java | 17 +++++++++++ 9 files changed, 78 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/hudson/logging/LogRecorder.java b/core/src/main/java/hudson/logging/LogRecorder.java index ad4c40028547..c11c1bbfa57e 100644 --- a/core/src/main/java/hudson/logging/LogRecorder.java +++ b/core/src/main/java/hudson/logging/LogRecorder.java @@ -556,7 +556,7 @@ public void delete() throws IOException { loggers.forEach(Target::disable); getParent().getRecorders().forEach(logRecorder -> logRecorder.getLoggers().forEach(Target::enable)); - SaveableListener.fireOnChange(this, getConfigFile()); + SaveableListener.fireOnDeleted(this, getConfigFile()); } /** diff --git a/core/src/main/java/hudson/model/AbstractItem.java b/core/src/main/java/hudson/model/AbstractItem.java index 0e712d8a2e88..84bbb7156752 100644 --- a/core/src/main/java/hudson/model/AbstractItem.java +++ b/core/src/main/java/hudson/model/AbstractItem.java @@ -815,6 +815,7 @@ public void delete() throws IOException, InterruptedException { ItemDeletion.deregister(this); } } + SaveableListener.fireOnDeleted(this, getConfigFile()); getParent().onDeleted(AbstractItem.this); Jenkins.get().rebuildDependencyGraphAsync(); } diff --git a/core/src/main/java/hudson/model/Run.java b/core/src/main/java/hudson/model/Run.java index 37b09fd47d25..450be07b7f1a 100644 --- a/core/src/main/java/hudson/model/Run.java +++ b/core/src/main/java/hudson/model/Run.java @@ -1570,6 +1570,7 @@ public void delete() throws IOException { )); //Still firing the delete listeners; just no need to clean up rootDir RunListener.fireDeleted(this); + SaveableListener.fireOnDeleted(this, getDataFile()); synchronized (this) { // avoid holding a lock while calling plugin impls of onDeleted removeRunFromParent(); } @@ -1578,6 +1579,7 @@ public void delete() throws IOException { //The root dir exists and is a directory that needs to be purged RunListener.fireDeleted(this); + SaveableListener.fireOnDeleted(this, getDataFile()); if (artifactManager != null) { deleteArtifacts(); diff --git a/core/src/main/java/hudson/model/listeners/SaveableListener.java b/core/src/main/java/hudson/model/listeners/SaveableListener.java index 46bbc6ab60be..02747877e76f 100644 --- a/core/src/main/java/hudson/model/listeners/SaveableListener.java +++ b/core/src/main/java/hudson/model/listeners/SaveableListener.java @@ -30,8 +30,7 @@ import hudson.ExtensionPoint; import hudson.XmlFile; import hudson.model.Saveable; -import java.util.logging.Level; -import java.util.logging.Logger; +import jenkins.util.Listeners; /** * Receives notifications about save actions on {@link Saveable} objects in Hudson. @@ -54,6 +53,17 @@ public abstract class SaveableListener implements ExtensionPoint { */ public void onChange(Saveable o, XmlFile file) {} + /** + * Called when a {@link Saveable} object gets deleted. + * + * @param o + * The saveable object. + * @param file + * The {@link XmlFile} for this saveable object. + * @since TODO + */ + public void onDeleted(Saveable o, XmlFile file) {} + /** * Registers this object as an active listener so that it can start getting * callbacks invoked. @@ -77,13 +87,15 @@ public void unregister() { * Fires the {@link #onChange} event. */ public static void fireOnChange(Saveable o, XmlFile file) { - for (SaveableListener l : all()) { - try { - l.onChange(o, file); - } catch (Throwable t) { - Logger.getLogger(SaveableListener.class.getName()).log(Level.WARNING, null, t); - } - } + Listeners.notify(SaveableListener.class, false, l -> l.onChange(o, file)); + } + + /** + * Fires the {@link #onDeleted} event. + * @since TODO + */ + public static void fireOnDeleted(Saveable o, XmlFile file) { + Listeners.notify(SaveableListener.class, false, l -> l.onDeleted(o, file)); } /** diff --git a/core/src/main/java/jenkins/model/Nodes.java b/core/src/main/java/jenkins/model/Nodes.java index 8f4c0e5eafc6..e15d391c4975 100644 --- a/core/src/main/java/jenkins/model/Nodes.java +++ b/core/src/main/java/jenkins/model/Nodes.java @@ -295,6 +295,7 @@ public void run() { jenkins.trimLabels(node); } NodeListener.fireOnDeleted(node); + SaveableListener.fireOnDeleted(node, getConfigFile(node)); } } diff --git a/test/src/test/java/hudson/logging/LogRecorderManagerTest.java b/test/src/test/java/hudson/logging/LogRecorderManagerTest.java index 608e7eb0b464..101729038a33 100644 --- a/test/src/test/java/hudson/logging/LogRecorderManagerTest.java +++ b/test/src/test/java/hudson/logging/LogRecorderManagerTest.java @@ -229,7 +229,7 @@ public static class DeletingLogRecorderListener extends SaveableListener { private static boolean recordDeletion; @Override - public void onChange(Saveable o, XmlFile file) { + public void onDeleted(Saveable o, XmlFile file) { if (o instanceof LogRecorder && "dummy".equals(((LogRecorder) o).getName())) { if (!file.exists()) { recordDeletion = true; diff --git a/test/src/test/java/hudson/model/AbstractItemTest.java b/test/src/test/java/hudson/model/AbstractItemTest.java index 165c39a96e6e..d211d802fe63 100644 --- a/test/src/test/java/hudson/model/AbstractItemTest.java +++ b/test/src/test/java/hudson/model/AbstractItemTest.java @@ -6,6 +6,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; import hudson.ExtensionList; import hudson.XmlFile; @@ -61,7 +62,7 @@ public void reload() throws Exception { // reload away p.doReload(); - assertFalse(SaveableListener.class.getSimpleName() + " should not have been called", testSaveableListener.wasCalled()); + assertFalse(SaveableListener.class.getSimpleName() + " should not have been called", testSaveableListener.isChangeCalled()); assertEquals("Good Evening", p.getDescription()); @@ -70,6 +71,9 @@ public void reload() throws Exception { assertNotEquals(b, b2); // should be different object assertEquals(b.getDescription(), b2.getDescription()); // but should have the same properties + + p.delete(); + assertTrue(SaveableListener.class.getSimpleName() + " should have been called", testSaveableListener.isDeleteCalled()); } @Test @@ -158,21 +162,29 @@ private String getPath(URL u) { public static class TestSaveableListener extends SaveableListener { private Saveable saveable; - private boolean called; + private boolean changeCalled; + private boolean deleteCalled; private void setSaveable(Saveable saveable) { this.saveable = saveable; } - public boolean wasCalled() { - return called; + public boolean isChangeCalled() { + return changeCalled; + } + + public boolean isDeleteCalled() { + return deleteCalled; } @Override public void onChange(Saveable o, XmlFile file) { - if (o == saveable) { - this.called = true; - } + this.changeCalled |= o == saveable; + } + + @Override + public void onDeleted(Saveable o, XmlFile file) { + this.deleteCalled |= o == saveable; } } } diff --git a/test/src/test/java/hudson/model/RunTest.java b/test/src/test/java/hudson/model/RunTest.java index b6a145857020..3aba76f24e05 100644 --- a/test/src/test/java/hudson/model/RunTest.java +++ b/test/src/test/java/hudson/model/RunTest.java @@ -32,8 +32,12 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; +import hudson.ExtensionList; import hudson.FilePath; import hudson.Launcher; +import hudson.XmlFile; +import hudson.model.listeners.ItemListener; +import hudson.model.listeners.SaveableListener; import hudson.tasks.ArtifactArchiver; import hudson.tasks.BuildTrigger; import hudson.tasks.Builder; @@ -112,6 +116,17 @@ public class RunTest { FreeStyleBuild b = j.buildAndAssertSuccess(p); b.delete(); assertTrue(Mgr.deleted.get()); + assertTrue(ExtensionList.lookupSingleton(SaveableListenerImpl.class).deleted); + } + + @TestExtension("deleteArtifactsCustom") + public static class SaveableListenerImpl extends SaveableListener { + boolean deleted; + + @Override + public void onDeleted(Saveable o, XmlFile file) { + deleted = true; + } } @Issue("SECURITY-1902") diff --git a/test/src/test/java/jenkins/model/NodesTest.java b/test/src/test/java/jenkins/model/NodesTest.java index 275dd9a3e5fb..c7207b9977c5 100644 --- a/test/src/test/java/jenkins/model/NodesTest.java +++ b/test/src/test/java/jenkins/model/NodesTest.java @@ -37,10 +37,13 @@ import edu.umd.cs.findbugs.annotations.NonNull; import hudson.ExtensionList; +import hudson.XmlFile; import hudson.model.Descriptor; import hudson.model.Failure; import hudson.model.Node; +import hudson.model.Saveable; import hudson.model.Slave; +import hudson.model.listeners.SaveableListener; import hudson.slaves.ComputerLauncher; import hudson.slaves.DumbSlave; import java.io.IOException; @@ -99,6 +102,10 @@ public void addNodeShouldReplaceExistingNode() throws Exception { assertEquals(0, l.deleted); assertEquals(1, l.updated); assertEquals(1, l.created); + var saveableListener = ExtensionList.lookupSingleton(SaveableListenerImpl.class); + assertEquals(0, saveableListener.deleted); + r.jenkins.removeNode(newNode); + assertEquals(1, saveableListener.deleted); } @TestExtension("addNodeShouldReplaceExistingNode") @@ -122,6 +129,16 @@ protected void onCreated(Node node) { } } + @TestExtension("addNodeShouldReplaceExistingNode") + public static final class SaveableListenerImpl extends SaveableListener { + int deleted; + + @Override + public void onDeleted(Saveable o, XmlFile file) { + deleted++; + } + } + @Test @Issue("JENKINS-56403") public void replaceNodeShouldRemoveOldNode() throws Exception { From 3ed5370f03f7a7c15f6d62a4f0e058eb7dddbe21 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Tue, 17 Sep 2024 17:34:14 +0200 Subject: [PATCH 2/3] Spotless --- test/src/test/java/hudson/model/RunTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/test/src/test/java/hudson/model/RunTest.java b/test/src/test/java/hudson/model/RunTest.java index 3aba76f24e05..34be1f14c2ef 100644 --- a/test/src/test/java/hudson/model/RunTest.java +++ b/test/src/test/java/hudson/model/RunTest.java @@ -36,7 +36,6 @@ import hudson.FilePath; import hudson.Launcher; import hudson.XmlFile; -import hudson.model.listeners.ItemListener; import hudson.model.listeners.SaveableListener; import hudson.tasks.ArtifactArchiver; import hudson.tasks.BuildTrigger; From 4cf76b9c2115b8578d701f20c9b53ee1eb4447f5 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Wed, 25 Sep 2024 11:00:19 +0200 Subject: [PATCH 3/3] Explicitly test that the user that has performed the change can be obtained from the Saveable listener. --- .../java/hudson/model/AbstractItemTest.java | 52 +++++++++++++++---- 1 file changed, 43 insertions(+), 9 deletions(-) diff --git a/test/src/test/java/hudson/model/AbstractItemTest.java b/test/src/test/java/hudson/model/AbstractItemTest.java index d211d802fe63..8e2a5036b14a 100644 --- a/test/src/test/java/hudson/model/AbstractItemTest.java +++ b/test/src/test/java/hudson/model/AbstractItemTest.java @@ -34,6 +34,7 @@ import org.jvnet.hudson.test.MockAuthorizationStrategy; import org.jvnet.hudson.test.SleepBuilder; import org.jvnet.hudson.test.TestExtension; +import org.springframework.security.core.Authentication; public class AbstractItemTest { @@ -46,12 +47,21 @@ public class AbstractItemTest { @Test public void reload() throws Exception { Jenkins jenkins = j.jenkins; - FreeStyleProject p = jenkins.createProject(FreeStyleProject.class, "foo"); - p.setDescription("Hello World"); + jenkins.setSecurityRealm(j.createDummySecurityRealm()); + MockAuthorizationStrategy mas = new MockAuthorizationStrategy(); + mas.grant(Item.CONFIGURE).everywhere().to("alice", "bob"); + mas.grant(Item.READ).everywhere().to("alice"); - FreeStyleBuild b = j.buildAndAssertSuccess(p); - b.setDescription("This is my build"); + FreeStyleProject p; + FreeStyleBuild b; + var alice = User.getById("alice", true); + try (ACLContext ignored = ACL.as(alice)) { + p = jenkins.createProject(FreeStyleProject.class, "foo"); + p.setDescription("Hello World"); + b = j.buildAndAssertSuccess(p); + b.setDescription("This is my build"); + } // update on disk representation Path path = p.getConfigFile().getFile().toPath(); Files.writeString(path, Files.readString(path, StandardCharsets.UTF_8).replaceAll("Hello World", "Good Evening"), StandardCharsets.UTF_8); @@ -63,8 +73,6 @@ public void reload() throws Exception { p.doReload(); assertFalse(SaveableListener.class.getSimpleName() + " should not have been called", testSaveableListener.isChangeCalled()); - - assertEquals("Good Evening", p.getDescription()); FreeStyleBuild b2 = p.getBuildByNumber(1); @@ -72,8 +80,17 @@ public void reload() throws Exception { assertNotEquals(b, b2); // should be different object assertEquals(b.getDescription(), b2.getDescription()); // but should have the same properties - p.delete(); + try (var ignored = ACL.as(alice)) { + p.setDescription("This is Alice's project"); + } + assertTrue(SaveableListener.class.getSimpleName() + " should have been called", testSaveableListener.isChangeCalled()); + assertThat(testSaveableListener.getChangeUser(), equalTo(alice.impersonate2())); + + try (var ignored = ACL.as(alice)) { + p.delete(); + } assertTrue(SaveableListener.class.getSimpleName() + " should have been called", testSaveableListener.isDeleteCalled()); + assertThat(testSaveableListener.getDeleteUser(), equalTo(alice.impersonate2())); } @Test @@ -163,7 +180,10 @@ public static class TestSaveableListener extends SaveableListener { private Saveable saveable; private boolean changeCalled; + private Authentication changeUser; + private boolean deleteCalled; + private Authentication deleteUser; private void setSaveable(Saveable saveable) { this.saveable = saveable; @@ -173,18 +193,32 @@ public boolean isChangeCalled() { return changeCalled; } + public Authentication getChangeUser() { + return changeUser; + } + public boolean isDeleteCalled() { return deleteCalled; } + public Authentication getDeleteUser() { + return deleteUser; + } + @Override public void onChange(Saveable o, XmlFile file) { - this.changeCalled |= o == saveable; + if (o == saveable) { + changeCalled = true; + changeUser = Jenkins.getAuthentication2(); + } } @Override public void onDeleted(Saveable o, XmlFile file) { - this.deleteCalled |= o == saveable; + if (o == saveable) { + deleteCalled = true; + deleteUser = Jenkins.getAuthentication2(); + } } } }