Fix same params detection (change and test)

src/main/java/hudson/plugins/throttleconcurrents/ThrottleJobProperty.java

@@ -52,7 +52,7 @@
 public class ThrottleJobProperty extends JobProperty<Job<?,?>> {
     // Replaced by categories, to support, well, multiple categories per job (starting from 1.3)
     @Deprecated transient String category;
-    
+
     private Integer maxConcurrentPerNode;
     private Integer maxConcurrentTotal;
     private List<String> categories;
@@ -65,12 +65,16 @@ public class ThrottleJobProperty extends JobProperty<Job<?,?>> {
     // The paramsToUseForLimit is assigned by end-user configuration and
     // is generally a string with names of build arguments to consider,
     // and is empty, or has one arg name, or a token-separated list of
-    // such names.
+    // such names (see PARAMS_LIMIT_SEPARATOR) below.
     // The paramsToCompare is an array of arg name strings, one per
     // list entry, processed from paramsToUseForLimit.
     private String paramsToUseForLimit;
     private transient List<String> paramsToCompare;
 
+    // Documentation only stated "," but its use was broken for so long,
+    // that probably people used de-facto working whitespace instead.
+    private final static String PARAMS_LIMIT_SEPARATOR = ",\\s*|\\s+";
+
     /**
      * Store a config version so we're able to migrate config on various
      * functionality upgrades.
@@ -97,17 +101,7 @@ public ThrottleJobProperty(Integer maxConcurrentPerNode,
         this.limitOneJobWithMatchingParams = limitOneJobWithMatchingParams;
         this.matrixOptions = matrixOptions;
         this.paramsToUseForLimit = paramsToUseForLimit;
-        if ((this.paramsToUseForLimit != null)) {
-            if ((this.paramsToUseForLimit.length() > 0)) {
-                this.paramsToCompare = Arrays.asList(ArrayUtils.nullToEmpty(StringUtils.split(this.paramsToUseForLimit)));
-            }
-            else {
-                this.paramsToCompare = new ArrayList<String>();
-            }
-        }
-        else {
-            this.paramsToCompare = new ArrayList<String>();
-        }
+        this.setParamsToCompare(this.paramsToUseForLimit);
     }
 
 
@@ -226,20 +220,54 @@ public boolean isThrottleMatrixConfigurations() {
                 : ThrottleMatrixProjectOptions.DEFAULT.isThrottleMatrixConfigurations();
     }
 
-    public List<String> getParamsToCompare() {
-        if (paramsToCompare == null) {
-            if ((paramsToUseForLimit != null)) {
-                if ((paramsToUseForLimit.length() > 0)) {
-                    paramsToCompare = Arrays.asList(paramsToUseForLimit.split(","));
-                }
-                else {
-                    paramsToCompare = new ArrayList<String>();
-                }
+    /**
+     * @param paramsToUseForLimit   Set the paramsToCompare list
+     * of Strings from the user-input token-separated String.
+     *
+     * There is no return, but the this.paramsToCompare will be a
+     * valid non-null populated or empty array after this call.
+     */
+    public void setParamsToCompare(String paramsToUseForLimit) {
+        // If there is any contents, tell GC that it is reapable
+        this.paramsToCompare = null;
+
+        if ((paramsToUseForLimit != null)) {
+            if ((paramsToUseForLimit.length() > 0)) {
+                List<String> list = new ArrayList<String>(Arrays.asList(ArrayUtils.nullToEmpty(paramsToUseForLimit.split(PARAMS_LIMIT_SEPARATOR))));
+                // Due to type casting, DO NOT MERGE with the line above
+                // in accordance with https://stackoverflow.com/a/5520808/4715872
+                // Logically, we should only remove all blanks "" and the
+                // split() should have taken care of other chars... but
+                // better safe than sorry. Also remove("") did not seem
+                // to cut it for more than one blank entry in the List.
+                list.removeAll(Arrays.asList("", " ", "\n", "\t", ","));
+                this.paramsToCompare = list;
             }
             else {
-                paramsToCompare = new ArrayList<String>();
+                this.paramsToCompare = new ArrayList<String>();
             }
         }
+        else {
+            this.paramsToCompare = new ArrayList<String>();
+        }
+    }
+
+    /**
+     * Returns a copy of this.paramsToCompare, and (unlike a proper getter)
+     * if it was not set yet, make sure first to populate this array from
+     * the user-provided this.paramsToUseForLimit string. Note that this
+     * call only re-synchronizes paramsToCompare from paramsToUseForLimit
+     * if paramsToCompare was null, so that normally should not happen. This
+     * logic was present here before refactoring it into setParamsToCompare()
+     * above which allows to actually update the cached array on demand if
+     * needed later (e.g. when/if a setParamsToUseForLimit() is introduced).
+     *
+     * @return A populated or empty array (not a null) after this call.
+     */
+    public List<String> getParamsToCompare() {
+        if (paramsToCompare == null) {
+            this.setParamsToCompare(this.paramsToUseForLimit);
+        }
         return paramsToCompare;
     }
 

src/main/java/hudson/plugins/throttleconcurrents/ThrottleQueueTaskDispatcher.java

@@ -336,12 +336,15 @@ private boolean isAnotherBuildWithSameParametersRunningOnNode(Node node, Queue.I
         }
         Computer computer = node.toComputer();
         List<String> paramsToCompare = tjp.getParamsToCompare();
+        Integer paramsToCompareSize = paramsToCompare.size();
         List<ParameterValue> itemParams = getParametersFromQueueItem(item);
 
-        if (paramsToCompare.size() > 0) {
+        if (paramsToCompareSize > 0) {
             itemParams = doFilterParams(paramsToCompare, itemParams);
         }
 
+        String itemTaskName = item.task.getName();
+
         // Look at all executors of specified node => computer,
         // and what work units they are busy with (if any) - and
         // whether one of these executing units is an instance
@@ -352,9 +355,11 @@ private boolean isAnotherBuildWithSameParametersRunningOnNode(Node node, Queue.I
                 final Queue.Executable currentExecutable = exec.getCurrentExecutable();
                 final SubTask parentTask = currentExecutable != null ? currentExecutable.getParent() : null;
                 if (currentExecutable != null &&
-                        parentTask.getOwnerTask().getName().equals(item.task.getName())) {
+                        parentTask.getOwnerTask().getName().equals(itemTaskName)) {
                     List<ParameterValue> executingUnitParams = getParametersFromWorkUnit(exec.getCurrentWorkUnit());
-                    executingUnitParams = doFilterParams(paramsToCompare, executingUnitParams);
+                    if (paramsToCompareSize > 0) {
+                        executingUnitParams = doFilterParams(paramsToCompare, executingUnitParams);
+                    }
 
                     // An already executing work unit (of the same name) can have more
                     // parameters than the queued item, e.g. due to env injection or by

src/test/java/hudson/plugins/throttleconcurrents/ThrottleJobPropertyTest.java

@@ -199,6 +199,159 @@ public void testThrottleJobConstructorShouldStoreArguments() {
         assertEquals(expectedThrottleOption, property.getThrottleOption());
     }
 
+    @Test
+    @WithoutJenkins
+    public void testThrottleJobConstructorShouldParseParamsToUseForLimit() {
+        // We just set these values but do not test much, see above for that
+        Integer expectedMaxConcurrentPerNode = anyInt();
+        Integer expectedMaxConcurrentTotal = anyInt();
+        List<String> expectedCategories = Collections.emptyList();
+        boolean expectedThrottleEnabled = anyBoolean();
+        String expectedThrottleOption = anyString();
+        boolean expectedLimitOneJobWithMatchingParams = true;
+
+        // Mix the comma and whitespace separators as commas were documented
+        // in original codebase but did not work so people must have used the
+        // whitespaces de-facto.
+        // Note that the data type of paramsToUseForLimit is a List (ordered)
+        // so in the test we expect it as such. Production code seems to use
+        // it as an unordered set as suffices for filtering, however.
+        // (0*) Null and empty string inputs should result in an empty list
+        // object; surrounding whitespace should be truncated so a non-empty
+        // input made of just separators is effectively empty for us too.
+        String assignedParamsToUseForLimit00 = null;
+        List<String> expectedParamsToUseForLimit00 = new ArrayList<String>();
+        String assignedParamsToUseForLimit0 = "";
+        List<String> expectedParamsToUseForLimit0 = new ArrayList<String>();
+        String assignedParamsToUseForLimit0a = " ";
+        List<String> expectedParamsToUseForLimit0a = new ArrayList<String>();
+        String assignedParamsToUseForLimit0b = " , ";
+        List<String> expectedParamsToUseForLimit0b = new ArrayList<String>();
+        String assignedParamsToUseForLimit0c = " ,,,  \n";
+        List<String> expectedParamsToUseForLimit0c = new ArrayList<String>();
+        // (1) One buildarg name listed in input becomes the only one string
+        // in the list; surrounding whitespace should be truncated
+        String assignedParamsToUseForLimit1 = "ONE_PARAM";
+        List<String> expectedParamsToUseForLimit1 = Arrays.asList("ONE_PARAM");
+        String assignedParamsToUseForLimit1a = " ONE_PARAM";
+        List<String> expectedParamsToUseForLimit1a = Arrays.asList("ONE_PARAM");
+        String assignedParamsToUseForLimit1b = " ONE_PARAM\n";
+        List<String> expectedParamsToUseForLimit1b = Arrays.asList("ONE_PARAM");
+        // (2) Two buildarg names listed in input become two strings in the list
+        String assignedParamsToUseForLimit2 = "TWO,PARAMS";
+        List<String> expectedParamsToUseForLimit2 = Arrays.asList("TWO", "PARAMS");
+        // (3) Different separators handled the same
+        String assignedParamsToUseForLimit3 = "THREE DIFFERENT,PARAMS";
+        List<String> expectedParamsToUseForLimit3 = Arrays.asList("THREE", "DIFFERENT", "PARAMS");
+        // (4) Several separating tokens together must go away as one - we want
+        // no empties in the resulting list
+        String assignedParamsToUseForLimit4 = "FOUR ,SOMEWHAT\t,DIFFERENT , PARAMS";
+        List<String> expectedParamsToUseForLimit4 = Arrays.asList("FOUR", "SOMEWHAT", "DIFFERENT", "PARAMS");
+        // (5) Java does not really have multilines, but still... note that
+        // if any whitespace should be there in the carry-over of string
+        // representation, it is the coder's responsibility to ensure some.
+        String assignedParamsToUseForLimit5 = "Multi\nline" +
+                "string,for	kicks\n" +
+                "EOL";
+        List<String> expectedParamsToUseForLimit5 = Arrays.asList("Multi", "linestring", "for", "kicks", "EOL");
+
+        ThrottleJobProperty property00 = new ThrottleJobProperty(expectedMaxConcurrentPerNode,
+                expectedMaxConcurrentTotal,
+                expectedCategories, expectedThrottleEnabled, expectedThrottleOption,
+                expectedLimitOneJobWithMatchingParams,
+                assignedParamsToUseForLimit00,
+                ThrottleMatrixProjectOptions.DEFAULT);
+        assertEquals(expectedParamsToUseForLimit00, property00.getParamsToCompare());
+
+        ThrottleJobProperty property0 = new ThrottleJobProperty(expectedMaxConcurrentPerNode,
+                expectedMaxConcurrentTotal,
+                expectedCategories, expectedThrottleEnabled, expectedThrottleOption,
+                expectedLimitOneJobWithMatchingParams,
+                assignedParamsToUseForLimit0,
+                ThrottleMatrixProjectOptions.DEFAULT);
+        assertEquals(expectedParamsToUseForLimit0, property0.getParamsToCompare());
+
+        ThrottleJobProperty property0a = new ThrottleJobProperty(expectedMaxConcurrentPerNode,
+                expectedMaxConcurrentTotal,
+                expectedCategories, expectedThrottleEnabled, expectedThrottleOption,
+                expectedLimitOneJobWithMatchingParams,
+                assignedParamsToUseForLimit0a,
+                ThrottleMatrixProjectOptions.DEFAULT);
+        assertEquals(expectedParamsToUseForLimit0a, property0a.getParamsToCompare());
+
+        ThrottleJobProperty property0b = new ThrottleJobProperty(expectedMaxConcurrentPerNode,
+                expectedMaxConcurrentTotal,
+                expectedCategories, expectedThrottleEnabled, expectedThrottleOption,
+                expectedLimitOneJobWithMatchingParams,
+                assignedParamsToUseForLimit0b,
+                ThrottleMatrixProjectOptions.DEFAULT);
+        assertEquals(expectedParamsToUseForLimit0b, property0b.getParamsToCompare());
+
+        ThrottleJobProperty property0c = new ThrottleJobProperty(expectedMaxConcurrentPerNode,
+                expectedMaxConcurrentTotal,
+                expectedCategories, expectedThrottleEnabled, expectedThrottleOption,
+                expectedLimitOneJobWithMatchingParams,
+                assignedParamsToUseForLimit0c,
+                ThrottleMatrixProjectOptions.DEFAULT);
+        assertEquals(expectedParamsToUseForLimit0c, property0c.getParamsToCompare());
+
+        ThrottleJobProperty property1 = new ThrottleJobProperty(expectedMaxConcurrentPerNode,
+                expectedMaxConcurrentTotal,
+                expectedCategories, expectedThrottleEnabled, expectedThrottleOption,
+                expectedLimitOneJobWithMatchingParams,
+                assignedParamsToUseForLimit1,
+                ThrottleMatrixProjectOptions.DEFAULT);
+        assertEquals(expectedParamsToUseForLimit1, property1.getParamsToCompare());
+
+        ThrottleJobProperty property1a = new ThrottleJobProperty(expectedMaxConcurrentPerNode,
+                expectedMaxConcurrentTotal,
+                expectedCategories, expectedThrottleEnabled, expectedThrottleOption,
+                expectedLimitOneJobWithMatchingParams,
+                assignedParamsToUseForLimit1a,
+                ThrottleMatrixProjectOptions.DEFAULT);
+        assertEquals(expectedParamsToUseForLimit1a, property1a.getParamsToCompare());
+
+        ThrottleJobProperty property1b = new ThrottleJobProperty(expectedMaxConcurrentPerNode,
+                expectedMaxConcurrentTotal,
+                expectedCategories, expectedThrottleEnabled, expectedThrottleOption,
+                expectedLimitOneJobWithMatchingParams,
+                assignedParamsToUseForLimit1b,
+                ThrottleMatrixProjectOptions.DEFAULT);
+        assertEquals(expectedParamsToUseForLimit1b, property1b.getParamsToCompare());
+
+        ThrottleJobProperty property2 = new ThrottleJobProperty(expectedMaxConcurrentPerNode,
+                expectedMaxConcurrentTotal,
+                expectedCategories, expectedThrottleEnabled, expectedThrottleOption,
+                expectedLimitOneJobWithMatchingParams,
+                assignedParamsToUseForLimit2,
+                ThrottleMatrixProjectOptions.DEFAULT);
+        assertEquals(expectedParamsToUseForLimit2, property2.getParamsToCompare());
+
+        ThrottleJobProperty property3 = new ThrottleJobProperty(expectedMaxConcurrentPerNode,
+                expectedMaxConcurrentTotal,
+                expectedCategories, expectedThrottleEnabled, expectedThrottleOption,
+                expectedLimitOneJobWithMatchingParams,
+                assignedParamsToUseForLimit3,
+                ThrottleMatrixProjectOptions.DEFAULT);
+        assertEquals(expectedParamsToUseForLimit3, property3.getParamsToCompare());
+
+        ThrottleJobProperty property4 = new ThrottleJobProperty(expectedMaxConcurrentPerNode,
+                expectedMaxConcurrentTotal,
+                expectedCategories, expectedThrottleEnabled, expectedThrottleOption,
+                expectedLimitOneJobWithMatchingParams,
+                assignedParamsToUseForLimit4,
+                ThrottleMatrixProjectOptions.DEFAULT);
+        assertEquals(expectedParamsToUseForLimit4, property4.getParamsToCompare());
+
+        ThrottleJobProperty property5 = new ThrottleJobProperty(expectedMaxConcurrentPerNode,
+                expectedMaxConcurrentTotal,
+                expectedCategories, expectedThrottleEnabled, expectedThrottleOption,
+                expectedLimitOneJobWithMatchingParams,
+                assignedParamsToUseForLimit5,
+                ThrottleMatrixProjectOptions.DEFAULT);
+        assertEquals(expectedParamsToUseForLimit5, property5.getParamsToCompare());
+    }
+
     @Test
     @WithoutJenkins
     public void testThrottleJobShouldCopyCategoriesToConcurrencySafeList() {