[core] RuleSetFactory: Filter by minimumPriority at the end

-   Resolves #394

pmd-core/src/main/java/net/sourceforge/pmd/RuleSet.java

@@ -122,15 +122,24 @@ public class RuleSet implements ChecksumAware {
          * Add a new rule to this ruleset. Note that this method does not check
          * for duplicates.
          *
-         * @param rule
+         * @param newRule
          *            the rule to be added
          * @return The same builder, for a fluid programming interface
          */
-        public RuleSetBuilder addRule(final Rule rule) {
+        public RuleSetBuilder addRule(final Rule newRule) {
-            if (rule == null) {
+            if (newRule == null) {
                 throw new IllegalArgumentException(MISSING_RULE);
             }
-            rules.add(rule);
+
+            // check for duplicates - adding more than one rule with the same name will
+            // be problematic - see #RuleSet.getRuleByName(String)
+            for (Rule rule : rules) {
+                if (rule.getName().equals(newRule.getName()) && rule.getLanguage() == newRule.getLanguage()) {
+                    LOG.warning("Duplicated rule: " + newRule.getName());
+                }
+            }
+
+            rules.add(newRule);
             return this;
         }
 
@@ -387,6 +396,17 @@ public class RuleSet implements ChecksumAware {
         public RuleSet build() {
             return new RuleSet(this);
         }
+
+        public void filterRulesByPriority(RulePriority minimumPriority) {
+            Iterator<Rule> iterator = rules.iterator();
+            while (iterator.hasNext()) {
+                Rule rule = iterator.next();
+                if (rule.getPriority().compareTo(minimumPriority) > 0) {
+                    LOG.fine("Removing rule " + rule.getName() + " due to priority: " + rule.getPriority() + " required: " + minimumPriority);
+                    iterator.remove();
+                }
+            }
+        }
     }
 
     /**

pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactory.java

@@ -380,6 +380,8 @@ public class RuleSetFactory {
                 ruleSetBuilder.withDescription("Missing description");
             }
 
+            ruleSetBuilder.filterRulesByPriority(minimumPriority);
+
             return ruleSetBuilder.build();
         } catch (ClassNotFoundException cnfe) {
             return classNotFoundProblem(cnfe);
@@ -496,7 +498,9 @@ public class RuleSetFactory {
         }
         final RuleSetReference ruleSetReference = new RuleSetReference(ref, true, excludedRulesCheck);
 
-        RuleSetFactory ruleSetFactory = new RuleSetFactory(this, warnDeprecated);
+        // load the ruleset with minimum priority low, so that we get all rules, to be able to exclude any rule
+        // minimum priority will be applied again, before constructing the final ruleset
+        RuleSetFactory ruleSetFactory = new RuleSetFactory(resourceLoader, RulePriority.LOW, warnDeprecated, this.compatibilityFilter != null);
         RuleSet otherRuleSet = ruleSetFactory.createRuleSet(RuleSetReferenceId.parse(ref).get(0));
         for (Rule rule : otherRuleSet.getRules()) {
             excludedRulesCheck.remove(rule.getName());
@@ -504,12 +508,12 @@ public class RuleSetFactory {
                 RuleReference ruleReference = new RuleReference();
                 ruleReference.setRuleSetReference(ruleSetReference);
                 ruleReference.setRule(rule);
-                ruleSetBuilder.addRuleIfNotExists(ruleReference);
-
                 // override the priority
                 if (priority != null) {
                     ruleReference.setPriority(RulePriority.valueOf(Integer.parseInt(priority)));
                 }
+
+                ruleSetBuilder.addRuleIfNotExists(ruleReference);
             }
         }
         if (!excludedRulesCheck.isEmpty()) {
@@ -639,12 +643,9 @@ public class RuleSetFactory {
                 throw new IllegalArgumentException(UNEXPECTED_ELEMENT + nodeName
                         + "> encountered as child of <rule> element for Rule " + rule.getName());
             }
+        }
 
-        }
+        ruleSetBuilder.addRule(rule);
-        if (StringUtils.isNotBlank(ruleSetReferenceId.getRuleName())
-                || rule.getPriority().compareTo(minimumPriority) <= 0) {
-            ruleSetBuilder.addRule(rule);
-        }
     }
 
     private static boolean hasAttributeSetTrue(Element element, String attributeId) {
@@ -679,7 +680,9 @@ public class RuleSetFactory {
             return;
         }
 
-        RuleSetFactory ruleSetFactory = new RuleSetFactory(this, warnDeprecated);
+        // load the ruleset with minimum priority low, so that we get all rules, to be able to exclude any rule
+        // minimum priority will be applied again, before constructing the final ruleset
+        RuleSetFactory ruleSetFactory = new RuleSetFactory(resourceLoader, RulePriority.LOW, warnDeprecated, this.compatibilityFilter != null);
 
         boolean isSameRuleSet = false;
         RuleSetReferenceId otherRuleSetReferenceId = RuleSetReferenceId.parse(ref).get(0);
@@ -756,11 +759,8 @@ public class RuleSetFactory {
             }
         }
 
-        if (StringUtils.isNotBlank(ruleSetReferenceId.getRuleName())
+        if (withDeprecatedRuleReferences || !isSameRuleSet || !ruleReference.isDeprecated()) {
-                || referencedRule.getPriority().compareTo(minimumPriority) <= 0) {
+            ruleSetBuilder.addRuleReplaceIfExists(ruleReference);
-            if (withDeprecatedRuleReferences || !isSameRuleSet || !ruleReference.isDeprecated()) {
-                ruleSetBuilder.addRuleReplaceIfExists(ruleReference);
-            }
         }
     }
 

pmd-core/src/test/java/net/sourceforge/pmd/RuleSetFactoryTest.java

@@ -363,6 +363,45 @@ public class RuleSetFactoryTest {
         assertNotNull(ruleSet.getRuleByName("ExternalRefRuleNameRefRef"));
     }
 
+    @Test
+    public void testOverridePriorityLoadWithMinimum() throws RuleSetNotFoundException {
+        RuleSetFactory rsf = new RuleSetFactory(new ResourceLoader(), RulePriority.MEDIUM_LOW, true, true);
+        RuleSet ruleset = rsf.createRuleSet("net/sourceforge/pmd/rulesets/ruleset-minimum-priority.xml");
+        // only one rule should remain, since we filter out the other rules by minimum priority
+        assertEquals("Number of Rules", 1, ruleset.getRules().size());
+
+        // Priority is overridden and applied, rule is missing
+        assertNull(ruleset.getRuleByName("DummyBasicMockRule"));
+
+        // that's the remaining rule
+        assertNotNull(ruleset.getRuleByName("SampleXPathRule"));
+
+        // now, load with default minimum priority
+        rsf = new RuleSetFactory();
+        ruleset = rsf.createRuleSet("net/sourceforge/pmd/rulesets/ruleset-minimum-priority.xml");
+        assertEquals("Number of Rules", 2, ruleset.getRules().size());
+        Rule dummyBasicMockRule = ruleset.getRuleByName("DummyBasicMockRule");
+        assertEquals("Wrong Priority", RulePriority.LOW, dummyBasicMockRule.getPriority());
+    }
+
+    @Test
+    public void testExcludeWithMinimumPriority() throws RuleSetNotFoundException {
+        RuleSetFactory rsf = new RuleSetFactory(new ResourceLoader(), RulePriority.HIGH, true, true);
+        RuleSet ruleset = rsf.createRuleSet("net/sourceforge/pmd/rulesets/ruleset-minimum-priority-exclusion.xml");
+        // no rules should be loaded
+        assertEquals("Number of Rules", 0, ruleset.getRules().size());
+
+        // now, load with default minimum priority
+        rsf = new RuleSetFactory();
+        ruleset = rsf.createRuleSet("net/sourceforge/pmd/rulesets/ruleset-minimum-priority-exclusion.xml");
+        // only one rule, we have excluded one...
+        assertEquals("Number of Rules", 1, ruleset.getRules().size());
+        // rule is excluded
+        assertNull(ruleset.getRuleByName("DummyBasicMockRule"));
+        // that's the remaining rule
+        assertNotNull(ruleset.getRuleByName("SampleXPathRule"));
+    }
+
     @Test
     public void testOverrideMessage() throws RuleSetNotFoundException {
         Rule r = loadFirstRule(REF_OVERRIDE_ORIGINAL_NAME);

pmd-core/src/test/resources/net/sourceforge/pmd/rulesets/ruleset-minimum-priority-exclusion.xml

@@ -0,0 +1,15 @@
+<?xml version="1.0"?>
+<ruleset name="Test Reference Ruleset" xmlns="http://pmd.sourceforge.net/ruleset/2.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xsi:schemaLocation="http://pmd.sourceforge.net/ruleset/2.0.0 http://pmd.sourceforge.net/ruleset_2_0_0.xsd">
+
+    <description>
+  Ruleset used by test RuleSetFactoryTest
+  </description>
+
+    <!-- Load in the complete basic ruleset -->
+    <rule ref="rulesets/dummy/basic.xml">
+        <!-- but exclude DummyBasicMockRule -->
+        <exclude name="DummyBasicMockRule"/>
+    </rule>
+
+</ruleset>

pmd-core/src/test/resources/net/sourceforge/pmd/rulesets/ruleset-minimum-priority.xml

@@ -0,0 +1,17 @@
+<?xml version="1.0"?>
+<ruleset name="Test Reference Ruleset" xmlns="http://pmd.sourceforge.net/ruleset/2.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xsi:schemaLocation="http://pmd.sourceforge.net/ruleset/2.0.0 http://pmd.sourceforge.net/ruleset_2_0_0.xsd">
+
+    <description>
+  Ruleset used by test RuleSetFactoryTest
+  </description>
+
+    <!-- Load in the complete basic ruleset -->
+    <rule ref="rulesets/dummy/basic.xml" />
+
+    <!-- Override the priority of a specific rule -->
+    <rule ref="rulesets/dummy/basic.xml/DummyBasicMockRule">
+        <priority>5</priority> <!-- original priority: 3 -->
+    </rule>
+
+</ruleset>

pmd-core/src/test/resources/rulesets/dummy/basic.xml

@@ -32,5 +32,5 @@ Just for test
                 ]]></value>
             </property>
         </properties>
-    </rule>"
+    </rule>
 </ruleset>