[FIXED JENKINS-33683] Clarify Action methods can return null

jenkinsci · Mar 21, 2016 · eefafb2 · eefafb2
1 parent e4d7da6
commit eefafb2
Show file tree

Hide file tree

Showing 7 changed files with 27 additions and 17 deletions.
diff --git a/core/src/main/java/hudson/Functions.java b/core/src/main/java/hudson/Functions.java
@@ -1540,7 +1540,7 @@ public static String joinPath(String... components) {
      */
     public static String getActionUrl(String itUrl,Action action) {
         String urlName = action.getUrlName();
-        if(urlName==null)   return null;    // to avoid NPE and fail to render the whole page
+        if(urlName==null)   return null;    // Should not be displayed
         try {
             if (new URI(urlName).isAbsolute()) {
                 return urlName;

diff --git a/core/src/main/java/hudson/model/Action.java b/core/src/main/java/hudson/model/Action.java
@@ -25,6 +25,8 @@
 
 import hudson.Functions;
 
+import javax.annotation.CheckForNull;
+
 /**
  * Object that contributes additional information, behaviors, and UIs to {@link ModelObject}
  * (more specifically {@link Actionable} objects, which most interesting {@link ModelObject}s are.)
@@ -90,15 +92,17 @@ public interface Action extends ModelObject {
      * @see Functions#isAnonymous()
      * @see Functions#getIconFilePath(Action)
      */
-    String getIconFileName();
+    @CheckForNull String getIconFileName();
 
     /**
      * Gets the string to be displayed.
      *
      * The convention is to capitalize the first letter of each word,
-     * such as "Test Result". 
+     * such as "Test Result".
+     *
+     * @return Can be null in case the action is hidden.
      */
-    String getDisplayName();
+    @CheckForNull String getDisplayName();
 
     /**
      * Gets the URL path name.
@@ -124,5 +128,5 @@ public interface Action extends ModelObject {
      *      (when you do that, be sure to also return null from {@link #getIconFileName()}.
      * @see Functions#getActionUrl(String, Action)
      */
-    String getUrlName();
+    @CheckForNull String getUrlName();
 }
diff --git a/core/src/main/java/hudson/model/ManagementLink.java b/core/src/main/java/hudson/model/ManagementLink.java
@@ -33,6 +33,9 @@
 import java.util.List;
 import org.kohsuke.stapler.interceptor.RequirePOST;
 
+import javax.annotation.CheckForNull;
+import javax.annotation.Nonnull;
+
 /**
  * Extension point to add icon to <tt>http://server/hudson/manage</tt> page.
  *
@@ -59,7 +62,7 @@ public abstract class ManagementLink implements ExtensionPoint, Action {
      *      This is useful for defining {@link ManagementLink} that only shows up under
      *      certain circumstances.
      */
-    public abstract String getIconFileName();
+    public abstract @CheckForNull String getIconFileName();
 
     /**
      * Returns a short description of what this link does. This text
@@ -80,7 +83,7 @@ public String getDescription() {
      * so relative paths are interpreted against the root {@link Jenkins} object.
      */
     @Override
-    public abstract String getUrlName();
+    public abstract @CheckForNull String getUrlName();
 
     /**
      * Allows implementations to request that this link show a confirmation dialog, and use POST if confirmed.
@@ -102,16 +105,16 @@ public boolean getRequiresConfirmation() {
     public static final List<ManagementLink> LIST = ExtensionListView.createList(ManagementLink.class);
 
     /**
-     * All regsitered instances.
+     * All registered instances.
      */
-    public static ExtensionList<ManagementLink> all() {
+    public static @Nonnull ExtensionList<ManagementLink> all() {
         return ExtensionList.lookup(ManagementLink.class);
     }
 
     /**
      * @return permission required for user to access this management link, in addition to {@link Jenkins#ADMINISTER}
      */
-    public Permission getRequiredPermission() {
+    public @CheckForNull Permission getRequiredPermission() {
         return null;
     }
 

diff --git a/core/src/main/java/hudson/model/User.java b/core/src/main/java/hudson/model/User.java
@@ -73,6 +73,7 @@
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
@@ -903,11 +904,11 @@ public Descriptor getDescriptorByName(String className) {
 
     public Object getDynamic(String token) {
         for(Action action: getTransientActions()){
-            if(action.getUrlName().equals(token))
+            if(Objects.equals(action.getUrlName(), token))
                 return action;
         }
         for(Action action: getPropertyActions()){
-            if(action.getUrlName().equals(token))
+            if(Objects.equals(action.getUrlName(), token))
                 return action;
         }
         return null;

diff --git a/core/src/main/java/hudson/model/View.java b/core/src/main/java/hudson/model/View.java
@@ -544,7 +544,7 @@ public Object getDynamic(String token) {
         for (Action a : getActions()) {
             String url = a.getUrlName();
             if (url==null)  continue;
-            if(a.getUrlName().equals(token))
+            if (url.equals(token))
                 return a;
         }
         return null;

diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java
@@ -191,6 +191,8 @@
 import hudson.views.MyViewsTabBar;
 import hudson.views.ViewsTabBar;
 import hudson.widgets.Widget;
+
+import java.util.Objects;
 import java.util.TimerTask;
 import java.util.concurrent.CountDownLatch;
 import jenkins.ExtensionComponentSet;
@@ -3274,7 +3276,7 @@ public Object getDynamic(String token) {
                 return a;
         }
         for (Action a : getManagementLinks())
-            if(a.getUrlName().equals(token))
+            if (Objects.equals(a.getUrlName(), token))
                 return a;
         return null;
     }
@@ -4292,7 +4294,9 @@ public Collection<String> getUnprotectedRootActions() {
         // TODO consider caching (expiring cache when actions changes)
         for (Action a : getActions()) {
             if (a instanceof UnprotectedRootAction) {
-                names.add(a.getUrlName());
+                String url = a.getUrlName();
+                if (url == null) continue;
+                names.add(url);
             }
         }
         return names;

diff --git a/test/src/test/java/hudson/model/ManagementLinkTest.java b/test/src/test/java/hudson/model/ManagementLinkTest.java
@@ -35,9 +35,7 @@
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.JenkinsRule.WebClient;
 import org.jvnet.hudson.test.TestExtension;
-import org.xml.sax.SAXException;
 
-import java.io.IOException;
 import java.util.List;
 
 /**