Introduce SaveableListener#onDeleted (#9743)

* 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. * Spotless * Explicitly test that the user that has performed the change can be obtained from the Saveable listener.
jenkinsci · Oct 2, 2024 · a6f723c · a6f723c
1 parent 68ea077
commit a6f723c
Show file tree

Hide file tree

Showing 9 changed files with 115 additions and 22 deletions.
diff --git 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
@@ -814,6 +814,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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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;
@@ -33,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 {
 
@@ -45,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);
@@ -61,15 +72,25 @@ 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());
 
         FreeStyleBuild b2 = p.getBuildByNumber(1);
 
         assertNotEquals(b, b2); // should be different object
         assertEquals(b.getDescription(), b2.getDescription()); // but should have the same properties
+
+        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
@@ -158,20 +179,45 @@ private String getPath(URL u) {
     public static class TestSaveableListener extends SaveableListener {
         private Saveable saveable;
 
-        private boolean called;
+        private boolean changeCalled;
+        private Authentication changeUser;
+
+        private boolean deleteCalled;
+        private Authentication deleteUser;
 
         private void setSaveable(Saveable saveable) {
             this.saveable = saveable;
         }
 
-        public boolean wasCalled() {
-            return called;
+        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) {
             if (o == saveable) {
-                this.called = true;
+                changeCalled = true;
+                changeUser = Jenkins.getAuthentication2();
+            }
+        }
+
+        @Override
+        public void onDeleted(Saveable o, XmlFile file) {
+            if (o == saveable) {
+                deleteCalled = true;
+                deleteUser = Jenkins.getAuthentication2();
             }
         }
     }

diff --git a/test/src/test/java/hudson/model/RunTest.java b/test/src/test/java/hudson/model/RunTest.java
@@ -32,8 +32,11 @@
 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.SaveableListener;
 import hudson.tasks.ArtifactArchiver;
 import hudson.tasks.BuildTrigger;
 import hudson.tasks.Builder;
@@ -112,6 +115,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
@@ -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 {