Merge pull request #10 from jglick/slow-getCategoryProjects-JENKINS-1…

…9623 [FIXED JENKINS-19623] getCategoryProjects without permission checks
jenkinsci · Dec 7, 2013 · 18c7b4f · 18c7b4f
2 parents 1bc16cd + d5ead9d
commit 18c7b4f
Show file tree

Hide file tree

Showing 3 changed files with 129 additions and 25 deletions.
diff --git a/src/main/java/hudson/plugins/throttleconcurrents/ThrottleJobProperty.java b/src/main/java/hudson/plugins/throttleconcurrents/ThrottleJobProperty.java
@@ -4,6 +4,8 @@
 import hudson.model.AbstractDescribableImpl;
 import hudson.model.AbstractProject;
 import hudson.model.Descriptor;
+import hudson.model.Item;
+import hudson.model.ItemGroup;
 import hudson.model.Job;
 import hudson.model.JobProperty;
 import hudson.model.JobPropertyDescriptor;
@@ -12,7 +14,13 @@
 import hudson.Util;
 
 import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
+import java.util.WeakHashMap;
+import jenkins.model.Jenkins;
 
 import net.sf.json.JSONObject;
 
@@ -79,7 +87,24 @@ public Object readResolve() {
 
         return this;
     }
-
+
+    @Override protected void setOwner(AbstractProject<?,?> owner) {
+        super.setOwner(owner);
+        if (throttleEnabled && categories != null) {
+            Map<String,Map<ThrottleJobProperty,Void>> propertiesByCategory = ((DescriptorImpl) getDescriptor()).propertiesByCategory;
+            synchronized (propertiesByCategory) {
+                for (String c : categories) {
+                    Map<ThrottleJobProperty,Void> properties = propertiesByCategory.get(c);
+                    if (properties == null) {
+                        properties = new WeakHashMap<ThrottleJobProperty,Void>();
+                        propertiesByCategory.put(c, properties);
+                    }
+                    properties.put(this, null);
+                }
+            }
+        }
+    }
+
     public boolean getThrottleEnabled() {
         return throttleEnabled;
     }
@@ -106,10 +131,42 @@ public Integer getMaxConcurrentTotal() {
         return maxConcurrentTotal;
     }
 
+    static List<AbstractProject<?,?>> getCategoryProjects(String category) {
+        assert category != null && !category.equals("");
+        List<AbstractProject<?,?>> categoryProjects = new ArrayList<AbstractProject<?, ?>>();
+        Collection<ThrottleJobProperty> properties;
+        Map<String,Map<ThrottleJobProperty,Void>> propertiesByCategory = Jenkins.getInstance().getDescriptorByType(DescriptorImpl.class).propertiesByCategory;
+        synchronized (propertiesByCategory) {
+            Map<ThrottleJobProperty,Void> _properties = propertiesByCategory.get(category);
+            properties = _properties != null ? new ArrayList<ThrottleJobProperty>(_properties.keySet()) : Collections.<ThrottleJobProperty>emptySet();
+        }
+        for (ThrottleJobProperty t : properties) {
+            if (t.getThrottleEnabled()) {
+                if (t.getCategories() != null && t.getCategories().contains(category)) {
+                    AbstractProject<?,?> p = t.owner;
+                    if (/* not deleted */getItem(p.getParent(), p.getName()) == p && /* has not since been reconfigured */ p.getProperty(ThrottleJobProperty.class) == t) {
+                        categoryProjects.add(p);
+                    }
+                }
+            }
+        }
+        return categoryProjects;
+    }
+    private static Item getItem(ItemGroup group, String name) {
+        if (group instanceof Jenkins) {
+            return ((Jenkins) group).getItemMap().get(name);
+        } else {
+            return group.getItem(name);
+        }
+    }
+
     @Extension
     public static final class DescriptorImpl extends JobPropertyDescriptor {
         private List<ThrottleCategory> categories;
 
+        /** Map from category names, to properties including that category. */
+        private Map<String,Map<ThrottleJobProperty,Void>> propertiesByCategory = new HashMap<String,Map<ThrottleJobProperty,Void>>();
+
         public DescriptorImpl() {
             super(ThrottleJobProperty.class);
             load();

diff --git a/src/main/java/hudson/plugins/throttleconcurrents/ThrottleQueueTaskDispatcher.java b/src/main/java/hudson/plugins/throttleconcurrents/ThrottleQueueTaskDispatcher.java
@@ -14,9 +14,9 @@
 import hudson.model.queue.CauseOfBlockage;
 import hudson.model.queue.QueueTaskDispatcher;
 
-import java.util.ArrayList;
 import java.util.List;
 import java.util.Set;
+import java.util.logging.Level;
 import java.util.logging.Logger;
 
 @Extension
@@ -50,7 +50,7 @@ else if (tjp.getThrottleOption().equals("category")) {
                     for (String catNm : tjp.getCategories()) {
                         // Quick check that catNm itself is a real string.
                         if (catNm != null && !catNm.equals("")) {
-                            List<AbstractProject<?,?>> categoryProjects = getCategoryProjects(catNm);
+                            List<AbstractProject<?,?>> categoryProjects = ThrottleJobProperty.getCategoryProjects(catNm);
 
                             ThrottleJobProperty.ThrottleCategory category =
                                 ((ThrottleJobProperty.DescriptorImpl)tjp.getDescriptor()).getCategoryByName(catNm);
@@ -115,7 +115,7 @@ else if (tjp.getThrottleOption().equals("category")) {
                 for (String catNm : tjp.getCategories()) {
                     // Quick check that catNm itself is a real string.
                     if (catNm != null && !catNm.equals("")) {
-                        List<AbstractProject<?,?>> categoryProjects = getCategoryProjects(catNm);
+                        List<AbstractProject<?,?>> categoryProjects = ThrottleJobProperty.getCategoryProjects(catNm);
 
                         ThrottleJobProperty.ThrottleCategory category =
                             ((ThrottleJobProperty.DescriptorImpl)tjp.getDescriptor()).getCategoryByName(catNm);
@@ -161,7 +161,7 @@ private ThrottleJobProperty getThrottleJobProperty(Task task) {
 
     private int buildsOfProjectOnNode(Node node, Task task) {
         int runCount = 0;
-        LOGGER.fine("Checking for builds of " + task.getName() + " on node " + node.getDisplayName());
+        LOGGER.log(Level.FINE, "Checking for builds of {0} on node {1}", new Object[] {task.getName(), node.getDisplayName()});
 
         // I think this'll be more reliable than job.getBuilds(), which seemed to not always get
         // a build right after it was launched, for some reason.
@@ -199,24 +199,6 @@ private int buildsOnExecutor(Task task, Executor exec) {
         return runCount;
     }
 
-    private List<AbstractProject<?,?>> getCategoryProjects(String category) {
-        List<AbstractProject<?,?>> categoryProjects = new ArrayList<AbstractProject<?,?>>();
-
-        if (category != null && !category.equals("")) {
-            for (AbstractProject<?,?> p : Hudson.getInstance().getAllItems(AbstractProject.class)) {
-                ThrottleJobProperty t = p.getProperty(ThrottleJobProperty.class);
-
-                if (t!=null && t.getThrottleEnabled()) {
-                    if (t.getCategories()!=null && t.getCategories().contains(category)) {
-                        categoryProjects.add(p);
-                    }
-                }
-            }
-        }
-
-        return categoryProjects;
-    }
-
     /**
      * @param node to compare labels with.
      * @param category to compare labels with.
@@ -239,8 +221,7 @@ private int getMaxConcurrentPerNodeBasedOnMatchingLabels(
                     String nodeLabel = aNodeLabel.getDisplayName();
                     if(nodeLabel.equals(throttledNodeLabel)) {
                         maxConcurrentPerNodeLabeledIfMatch = nodeLabeledPair.getMaxConcurrentPerNodeLabeled().intValue();
-                        LOGGER.fine("node labels match");
-                        LOGGER.fine("=> maxConcurrentPerNode' = "+maxConcurrentPerNodeLabeledIfMatch);
+                        LOGGER.log(Level.FINE, "node labels match; => maxConcurrentPerNode'' = {0}", maxConcurrentPerNodeLabeledIfMatch);
                         nodeLabelsMatch = true;
                         break;
                     }

diff --git a/src/test/java/hudson/plugins/throttleconcurrents/ThrottleJobPropertyTest.java b/src/test/java/hudson/plugins/throttleconcurrents/ThrottleJobPropertyTest.java
@@ -0,0 +1,66 @@
+package hudson.plugins.throttleconcurrents;
+
+import hudson.model.AbstractProject;
+import hudson.model.FreeStyleProject;
+import hudson.model.Job;
+import hudson.security.ACL;
+import hudson.security.AuthorizationStrategy;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashSet;
+import org.jvnet.hudson.test.Bug;
+import org.jvnet.hudson.test.HudsonTestCase;
+
+public class ThrottleJobPropertyTest extends HudsonTestCase {
+
+    private static final String THROTTLE_OPTION_CATEGORY = "category"; // TODO move this into ThrottleJobProperty and use consistently; same for "project"
+
+    @Bug(19623)
+    public void testGetCategoryProjects() throws Exception {
+        String alpha = "alpha", beta = "beta", gamma = "gamma"; // category names
+        FreeStyleProject p1 = createFreeStyleProject("p1");
+        FreeStyleProject p2 = createFreeStyleProject("p2");
+        p2.addProperty(new ThrottleJobProperty(1, 1, Arrays.asList(alpha), false, THROTTLE_OPTION_CATEGORY));
+        FreeStyleProject p3 = createFreeStyleProject("p3");
+        p3.addProperty(new ThrottleJobProperty(1, 1, Arrays.asList(alpha, beta), true, THROTTLE_OPTION_CATEGORY));
+        FreeStyleProject p4 = createFreeStyleProject("p4");
+        p4.addProperty(new ThrottleJobProperty(1, 1, Arrays.asList(beta, gamma), true, THROTTLE_OPTION_CATEGORY));
+        // TODO when core dep ≥1.480.3, add cloudbees-folder as a test dependency so we can check jobs inside folders
+        assertProjects(alpha, p3);
+        assertProjects(beta, p3, p4);
+        assertProjects(gamma, p4);
+        assertProjects("delta");
+        p4.renameTo("p-4");
+        assertProjects(gamma, p4);
+        p4.delete();
+        assertProjects(gamma);
+        AbstractProject<?,?> p3b = jenkins.<AbstractProject<?,?>>copy(p3, "p3b");
+        assertProjects(beta, p3, p3b);
+        p3.removeProperty(ThrottleJobProperty.class);
+        assertProjects(beta, p3b);
+    }
+    private void assertProjects(String category, AbstractProject<?,?>... projects) {
+        jenkins.setAuthorizationStrategy(new RejectAllAuthorizationStrategy());
+        try {
+            assertEquals(new HashSet<AbstractProject<?,?>>(Arrays.asList(projects)), new HashSet<AbstractProject<?,?>>(ThrottleJobProperty.getCategoryProjects(category)));
+        } finally {
+            jenkins.setAuthorizationStrategy(AuthorizationStrategy.UNSECURED); // do not check during e.g. rebuildDependencyGraph from delete
+        }
+    }
+    private static class RejectAllAuthorizationStrategy extends AuthorizationStrategy {
+        RejectAllAuthorizationStrategy() {}
+        @Override public ACL getRootACL() {
+            return new AuthorizationStrategy.Unsecured().getRootACL();
+        }
+        @Override public Collection<String> getGroups() {
+            return Collections.emptySet();
+        }
+        @Override public ACL getACL(Job<?,?> project) {
+            fail("not even supposed to be looking at " + project);
+            return super.getACL(project);
+        }
+    }
+
+
+}