[FIXED JENKINS-17935] NPE from Run.getDynamic when a transient build …

…action is invisible. Amends lvotypko’s 33e0df7 to be as careful as bdff425 + f65da7f + 10e9b00 for persistent actions.
jenkinsci · May 13, 2013 · 8c53e2f · 8c53e2f
1 parent ea4b6b5
commit 8c53e2f
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 1 deletion.
diff --git a/changelog.html b/changelog.html
@@ -55,6 +55,9 @@
 <!-- Record your changes in the trunk here. -->
 <div id="trunk" style="display:none"><!--=TRUNK-BEGIN=-->
 <ul class=image>
+  <li class=bug>
+    NPE from <code>Run.getDynamic</code>.
+    (<a href="https://issues.jenkins-ci.org/browse/JENKINS-17935">issue 17935</a>)
   <li class=bug>
     Errors in <code>init.groovy</code> halted startup; changed to just log a warning.
     (<a href="https://issues.jenkins-ci.org/browse/JENKINS-17933">issue 17933</a>)

diff --git a/core/src/main/java/hudson/model/Run.java b/core/src/main/java/hudson/model/Run.java
@@ -328,6 +328,7 @@ public List<Action> getTransientActions() {
         List<Action> actions = new ArrayList<Action>();
         for (TransientBuildActionFactory factory: TransientBuildActionFactory.all()) {
             actions.addAll(factory.createFor(this));
+            assert !actions.contains(null) : "null action added by " + factory;
         }
         return Collections.unmodifiableList(actions);
     }
@@ -2210,8 +2211,13 @@ public Object getDynamic(String token, StaplerRequest req, StaplerResponse rsp)
         if (result == null){
             //check transient actions too
             for(Action action: getTransientActions()){
-                if(action.getUrlName().equals(token))
+                String urlName = action.getUrlName();
+                if (urlName == null) {
+                    continue;
+                }
+                if (urlName.equals(token)) {
                     return action;
+                }
             }
             // Next/Previous Build links on an action page (like /job/Abc/123/testReport)
             // will also point to same action (/job/Abc/124/testReport), but other builds

diff --git a/test/src/test/java/hudson/model/RunTest.java b/test/src/test/java/hudson/model/RunTest.java
@@ -23,11 +23,15 @@
  */
 package hudson.model;
 
+import java.net.HttpURLConnection;
+import java.util.Collection;
+import java.util.Collections;
 import static org.junit.Assert.*;
 
 import java.util.List;
 import org.junit.Rule;
 import org.junit.Test;
+import org.jvnet.hudson.test.Bug;
 import org.jvnet.hudson.test.JenkinsRule;
 
 /**
@@ -67,4 +71,26 @@ private List<? extends Run<?,?>.Artifact> createArtifactList(String... paths) th
         assertEquals(a.get(0).getDisplayPath(),"a.xml");
         assertEquals(a.get(1).getDisplayPath(),"a/a.xml");
     }
+
+    @Bug(17935)
+    @Test public void getDynamicInvisibleTransientAction() throws Exception {
+        TransientBuildActionFactory.all().add(0, new TransientBuildActionFactory() {
+            @Override public Collection<? extends Action> createFor(Run target) {
+                return Collections.singleton(new Action() {
+                    @Override public String getDisplayName() {
+                        return "Test";
+                    }
+                    @Override public String getIconFileName() {
+                        return null;
+                    }
+                    @Override public String getUrlName() {
+                        return null;
+                    }
+                });
+            }
+        });
+        j.assertBuildStatusSuccess(j.createFreeStyleProject("stuff").scheduleBuild2(0));
+        j.createWebClient().assertFails("job/stuff/1/nonexistent", HttpURLConnection.HTTP_NOT_FOUND);
+    }
+
 }