[FIXED JENKINS-18364] Functions.getRelativeLinkTo is called many time…

…s during rendering, so skip checking View.getItems which is expensive.
jenkinsci · Apr 1, 2014 · bcc8614 · bcc8614
1 parent 7037c23
commit bcc8614
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 16 deletions.
diff --git a/changelog.html b/changelog.html
@@ -55,7 +55,9 @@
 <!-- Record your changes in the trunk here. -->
 <div id="trunk" style="display:none"><!--=TRUNK-BEGIN=-->
 <ul class=image>
-  <li class=>
+  <li class=bug>
+    Faster rendering of views containing many items.
+    (<a href="https://issues.jenkins-ci.org/browse/JENKINS-18364">issue 18364</a>)
 </ul>
 </div><!--=TRUNK-END=-->
 

diff --git a/core/src/main/java/hudson/Functions.java b/core/src/main/java/hudson/Functions.java
@@ -1014,20 +1014,15 @@ public static String getRelativeLinkTo(Item p) {
 
         Item i=p;
         String url = "";
-        Collection<TopLevelItem> viewItems;
-        if (view != null) {
-            viewItems = view.getItems();
-        } else {
-            viewItems = Collections.emptyList();
-        }
         while(true) {
             ItemGroup ig = i.getParent();
             url = i.getShortUrl()+url;
 
             if(ig== Jenkins.getInstance() || (view != null && ig == view.getOwnerItemGroup())) {
                 assert i instanceof TopLevelItem;
-                if(viewItems.contains((TopLevelItem)i)) {
-                    // if p and the current page belongs to the same view, then return a relative path
+                if (view != null) {
+                    // assume p and the current page belong to the same view, so return a relative path
+                    // (even if they did not, View.getItem does not by default verify ownership)
                     return normalizeURI(ancestors.get(view)+'/'+url);
                 } else {
                     // otherwise return a path from the root Hudson

diff --git a/core/src/test/java/hudson/FunctionsTest.java b/core/src/test/java/hudson/FunctionsTest.java
@@ -23,31 +23,30 @@
  */
 package hudson;
 
-import static org.junit.Assert.assertEquals;
-import static org.powermock.api.mockito.PowerMockito.mock;
-import static org.powermock.api.mockito.PowerMockito.mockStatic;
-import static org.powermock.api.mockito.PowerMockito.when;
 import hudson.model.Action;
+import hudson.model.Computer;
 import hudson.model.Item;
 import hudson.model.ItemGroup;
 import hudson.model.TopLevelItem;
 import hudson.model.View;
-
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.Locale;
 import java.util.logging.Level;
 import java.util.logging.LogRecord;
-
 import jenkins.model.Jenkins;
-
+import static org.junit.Assert.assertEquals;
+import org.junit.Ignore;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.jvnet.hudson.test.Bug;
 import org.kohsuke.stapler.Ancestor;
 import org.kohsuke.stapler.Stapler;
 import org.kohsuke.stapler.StaplerRequest;
+import static org.powermock.api.mockito.PowerMockito.mock;
+import static org.powermock.api.mockito.PowerMockito.mockStatic;
+import static org.powermock.api.mockito.PowerMockito.when;
 import org.powermock.core.classloader.annotations.PrepareForTest;
 import org.powermock.modules.junit4.PowerMockRunner;
 
@@ -135,6 +134,23 @@ public void testGetRelativeLinkTo_JobContainedInView() throws Exception{
         assertEquals("job/i/", result);
     }
 
+    @Test
+    @PrepareForTest({Stapler.class, Jenkins.class})
+    public void testGetRelativeLinkTo_JobFromComputer() throws Exception{
+        Jenkins j = createMockJenkins();
+        ItemGroup parent = j;
+        String contextPath = "/jenkins";
+        StaplerRequest req = createMockRequest(contextPath);
+        mockStatic(Stapler.class);
+        when(Stapler.getCurrentRequest()).thenReturn(req);
+        Computer computer = mock(Computer.class);
+        createMockAncestors(req, createAncestor(computer, "."), createAncestor(j, "../.."));
+        TopLevelItem i = createMockItem(parent, "job/i/");
+        String result = Functions.getRelativeLinkTo(i);
+        assertEquals("/jenkins/job/i/", result);
+    }
+
+    @Ignore("too expensive to make it correct")
     @Test
     @PrepareForTest({Stapler.class, Jenkins.class})
     public void testGetRelativeLinkTo_JobNotContainedInView() throws Exception{