From e4d7da6053883be13d8b7398d2d2cbcb87aa01f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20Gond=C5=BEa?= Date: Mon, 21 Mar 2016 10:30:02 +0100 Subject: [PATCH 1/4] [JENKINS-33683] Reproduce --- .../java/hudson/model/ManagementLinkTest.java | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/test/src/test/java/hudson/model/ManagementLinkTest.java b/test/src/test/java/hudson/model/ManagementLinkTest.java index 5de5dd3d51..ad07987e95 100644 --- a/test/src/test/java/hudson/model/ManagementLinkTest.java +++ b/test/src/test/java/hudson/model/ManagementLinkTest.java @@ -23,6 +23,7 @@ */ package hudson.model; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; import com.gargoylesoftware.htmlunit.html.DomNodeUtil; @@ -30,9 +31,13 @@ import com.gargoylesoftware.htmlunit.html.HtmlAnchor; import com.gargoylesoftware.htmlunit.html.HtmlPage; import org.junit.Rule; import org.junit.Test; +import org.jvnet.hudson.test.Issue; 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; /** @@ -59,4 +64,28 @@ public class ManagementLinkTest { ((HtmlAnchor)anchors.get(i)).click(); } } + + @Test @Issue("JENKINS-33683") + public void invisibleLinks() throws Exception { + assertEquals(null, j.jenkins.getDynamic("and_fail_trying")); + } + + @TestExtension("invisibleLinks") + public static final class InvisibleManagementLink extends ManagementLink { + + @Override + public String getIconFileName() { + return null; + } + + @Override + public String getDisplayName() { + return null; + } + + @Override + public String getUrlName() { + return null; + } + } } From eefafb2b38a5ee5cf51c72a1e9c902ec6f38dc8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20Gond=C5=BEa?= Date: Mon, 21 Mar 2016 10:33:58 +0100 Subject: [PATCH 2/4] [FIXED JENKINS-33683] Clarify Action methods can return null --- core/src/main/java/hudson/Functions.java | 2 +- core/src/main/java/hudson/model/Action.java | 12 ++++++++---- core/src/main/java/hudson/model/ManagementLink.java | 13 ++++++++----- core/src/main/java/hudson/model/User.java | 5 +++-- core/src/main/java/hudson/model/View.java | 2 +- core/src/main/java/jenkins/model/Jenkins.java | 8 ++++++-- .../test/java/hudson/model/ManagementLinkTest.java | 2 -- 7 files changed, 27 insertions(+), 17 deletions(-) diff --git a/core/src/main/java/hudson/Functions.java b/core/src/main/java/hudson/Functions.java index 2208b9fb02..63894c008a 100644 --- a/core/src/main/java/hudson/Functions.java +++ b/core/src/main/java/hudson/Functions.java @@ -1540,7 +1540,7 @@ public class Functions { */ 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 index f73bd34032..2e8d2e641f 100644 --- a/core/src/main/java/hudson/model/Action.java +++ b/core/src/main/java/hudson/model/Action.java @@ -25,6 +25,8 @@ package hudson.model; 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 index dca459ca70..041fda35d0 100644 --- a/core/src/main/java/hudson/model/ManagementLink.java +++ b/core/src/main/java/hudson/model/ManagementLink.java @@ -33,6 +33,9 @@ import jenkins.model.Jenkins; import java.util.List; import org.kohsuke.stapler.interceptor.RequirePOST; +import javax.annotation.CheckForNull; +import javax.annotation.Nonnull; + /** * Extension point to add icon to http://server/hudson/manage 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 abstract class ManagementLink implements ExtensionPoint, Action { * 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 abstract class ManagementLink implements ExtensionPoint, Action { public static final List LIST = ExtensionListView.createList(ManagementLink.class); /** - * All regsitered instances. + * All registered instances. */ - public static ExtensionList all() { + public static @Nonnull ExtensionList 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 index 4a16c9f153..99f1a39a4e 100644 --- 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.HashSet; 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 class User extends AbstractModelObject implements AccessControlled, Descr 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 index 0cfa562922..02737b8f56 100644 --- a/core/src/main/java/hudson/model/View.java +++ b/core/src/main/java/hudson/model/View.java @@ -544,7 +544,7 @@ public abstract class View extends AbstractModelObject implements AccessControll 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 index 9e5bdf8afa..f1acf92b40 100644 --- 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.DefaultViewsTabBar; 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 class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve 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 class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve // 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 index ad07987e95..f6f1167eaa 100644 --- 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.Issue; 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; /** From 4c02f1db33820733d9500aaa4e5304b30dc0e2ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20Gond=C5=BEa?= Date: Mon, 21 Mar 2016 10:43:48 +0100 Subject: [PATCH 3/4] [JENKINS-33683] Do not restrict invisible action to single test --- test/src/test/java/hudson/model/ManagementLinkTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/src/test/java/hudson/model/ManagementLinkTest.java b/test/src/test/java/hudson/model/ManagementLinkTest.java index f6f1167eaa..6df8a85752 100644 --- a/test/src/test/java/hudson/model/ManagementLinkTest.java +++ b/test/src/test/java/hudson/model/ManagementLinkTest.java @@ -68,7 +68,7 @@ public class ManagementLinkTest { assertEquals(null, j.jenkins.getDynamic("and_fail_trying")); } - @TestExtension("invisibleLinks") + @TestExtension // Intentionally hooked in all tests public static final class InvisibleManagementLink extends ManagementLink { @Override From 8fdca297132b953b5572c97480b8017695f92d6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20Gond=C5=BEa?= Date: Tue, 22 Mar 2016 11:07:31 +0100 Subject: [PATCH 4/4] [JENKINS-33683] Annotate Function#getActionUrl() as well --- core/src/main/java/hudson/Functions.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/Functions.java b/core/src/main/java/hudson/Functions.java index 63894c008a..41d74cb7e1 100644 --- a/core/src/main/java/hudson/Functions.java +++ b/core/src/main/java/hudson/Functions.java @@ -1537,8 +1537,10 @@ public class Functions { /** * Computes the hyperlink to actions, to handle the situation when the {@link Action#getUrlName()} * returns absolute URL. + * + * @return null in case the action should not be presented to the user. */ - public static String getActionUrl(String itUrl,Action action) { + public static @CheckForNull String getActionUrl(String itUrl,Action action) { String urlName = action.getUrlName(); if(urlName==null) return null; // Should not be displayed try {