pmd: fix #1089 When changing priority in a custom ruleset, violations reported twice

pmd/etc/changelog.txt

@@ -9,6 +9,7 @@ Fixed bug 1082: CPD performance issue on larger projects
 Fixed bug 1085: NullPointerException by at net.sourceforge.pmd.lang.java.rule.design.GodClassRule.visit(GodClassRule.java:313)
 Fixed bug 1086: Unsupported Element and Attribute in Ant Task Example
 Fixed bug 1087: PreserveStackTrace (still) ignores initCause()
+Fixed bug 1089: When changing priority in a custom ruleset, violations reported twice
 
 
 April 5, 2013 - 5.0.3:

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

@@ -73,7 +73,7 @@ public class RuleSet {
 	}
 
 	/**
-	 * Add a new rule to this ruleset
+	 * Add a new rule to this ruleset. Note that this method does not check for duplicates.
 	 *
 	 * @param rule the rule to be added
 	 */
@@ -84,6 +84,53 @@ public class RuleSet {
 		rules.add(rule);
 	}
 
+	/**
+	 * Adds a rule. If a rule with the same name and language already existed before in the ruleset,
+	 * then the new rule will replace it. This makes sure that the rule configured is overridden.
+	 * @param rule
+	 * @return <code>true</code> if the new rule replaced an existing one, otherwise <code>false</code>.
+	 */
+	public boolean addRuleReplaceIfExists(Rule rule) {
+        if (rule == null) {
+            throw new IllegalArgumentException("Missing rule");
+        }
+
+        boolean replaced = false;
+        for (Iterator<Rule> it = rules.iterator(); it.hasNext(); ) {
+            Rule r = it.next();
+            if (r.getName().equals(rule.getName()) && r.getLanguage() == rule.getLanguage()) {
+                it.remove();
+                replaced = true;
+            }
+        }
+        addRule(rule);
+        return replaced;
+	}
+
+	/**
+	 * Only adds a rule to the ruleset if no rule with the same name for the same language was added
+	 * before, so that the existent rule configuration won't be overridden.
+	 * @param rule
+	 * @return <code>true</code> if the rule was added, <code>false</code> otherwise
+	 */
+	public boolean addRuleIfNotExists(Rule rule) {
+        if (rule == null) {
+            throw new IllegalArgumentException("Missing rule");
+        }
+
+        boolean exists = false;
+        for (Rule r : rules) {
+            if (r.getName().equals(rule.getName()) && r.getLanguage() == rule.getLanguage()) {
+                exists = true;
+                break;
+            }
+        }
+        if (!exists) {
+            addRule(rule);
+        }
+        return !exists;
+	}
+
 	/**
 	 * Add a new rule by reference to this ruleset.
 	 *
@@ -146,8 +193,7 @@ public class RuleSet {
 	 */
 	public Rule getRuleByName(String ruleName) {
 		
-		for (Iterator<Rule> i = rules.iterator(); i.hasNext();) {
-			Rule r = i.next();
+		for (Rule r : rules) {
 			if (r.getName().equals(ruleName)) {
 				return r;
 			}

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

@@ -301,7 +301,7 @@ public class RuleSetFactory {
 				RuleReference ruleReference = new RuleReference();
 				ruleReference.setRuleSetReference(ruleSetReference);
 				ruleReference.setRule(rule);
-				ruleSet.addRule(ruleReference);
+				ruleSet.addRuleIfNotExists(ruleReference);
 
 				// override the priority
 				if (priority != null) {
@@ -517,7 +517,7 @@ public class RuleSetFactory {
 
 		if (StringUtil.isNotEmpty(ruleSetReferenceId.getRuleName())
 				|| referencedRule.getPriority().compareTo(minimumPriority) <= 0) {
-			ruleSet.addRule(ruleReference);
+			ruleSet.addRuleReplaceIfExists(ruleReference);
 		}
 	}
 

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

@@ -113,7 +113,7 @@ public class RuleSetFactoryTest {
 	    Assert.assertNotNull("Test ruleset not found - can't continue with test!", in);
 
 	    RuleSetFactory rsf = new RuleSetFactory();
-	    RuleSet rs = rsf.createRuleSet("net/sourceforge/pmd/rulesets/reference-ruleset.xml");
+	    RuleSets rs = rsf.createRuleSets("net/sourceforge/pmd/rulesets/reference-ruleset.xml");
 	    // added by referencing a complete ruleset (java-basic)
 	    assertNotNull(rs.getRuleByName("JumbledIncrementer"));
 	    assertNotNull(rs.getRuleByName("ForLoopShouldBeWhileLoop"));
@@ -127,8 +127,12 @@ public class RuleSetFactoryTest {
 
 	    Rule emptyCatchBlock = rs.getRuleByName("EmptyCatchBlock");
 	    assertNotNull(emptyCatchBlock);
-	    // default priority in java-empty is 3, but overridden to 2
-	    assertEquals(2, emptyCatchBlock.getPriority().getPriority());
+
+	    Rule collapsibleIfStatements = rs.getRuleByName("CollapsibleIfStatements");
+	    assertEquals("Just combine them!", collapsibleIfStatements.getMessage());
+	    // assert that CollapsibleIfStatements is only once added to the ruleset, so that it really
+	    // overwrites the configuration inherited from java/basic.xml
+	    assertEquals(1, countRule(rs, "CollapsibleIfStatements"));
 
 	    Rule cyclomaticComplexity = rs.getRuleByName("CyclomaticComplexity");
 	    assertNotNull(cyclomaticComplexity);
@@ -145,6 +149,7 @@ public class RuleSetFactoryTest {
 	    Rule simplifyBooleanExpressions = rs.getRuleByName("SimplifyBooleanExpressions");
 	    assertNotNull(simplifyBooleanExpressions);
 	    assertEquals(5, simplifyBooleanExpressions.getPriority().getPriority());
+	    assertEquals(1, countRule(rs, "SimplifyBooleanExpressions"));
 	    // priority overridden for whole design group
 	    Rule useUtilityClass = rs.getRuleByName("UseSingleton");
 	    assertNotNull(useUtilityClass);
@@ -154,6 +159,16 @@ public class RuleSetFactoryTest {
 	    assertEquals(2, simplifyBooleanReturns.getPriority().getPriority());
 	}
 
+    private int countRule(RuleSets rs, String ruleName) {
+        int count = 0;
+	    for (Rule r : rs.getAllRules()) {
+	        if (ruleName.equals(r.getName())) {
+	            count++;
+	        }
+	    }
+        return count;
+    }
+
 	@Test(expected = RuleSetNotFoundException.class)
 	public void testRuleSetNotFound() throws RuleSetNotFoundException {
 		RuleSetFactory rsf = new RuleSetFactory();

pmd/src/test/resources/net/sourceforge/pmd/rulesets/reference-ruleset.xml

@@ -16,12 +16,16 @@
   <rule ref="rulesets/java/imports.xml/DuplicateImports"/>
 
   <!-- We want to customize this rule a bit, change the message and raise the priority  -->
+  <!-- Note: This rule is deprecated in basic.xml and was moved to empty.xml -->
   <rule
    ref="rulesets/java/basic.xml/EmptyCatchBlock"
    message="Must handle exceptions">
     <priority>2</priority>
   </rule>
 
+  <!-- Customize the message for collapsible if statements -->
+  <rule ref="rulesets/java/basic.xml/CollapsibleIfStatements" message="Just combine them!"/>
+
   <!-- Now we'll customize a rule's property value -->
   <rule ref="rulesets/java/codesize.xml/CyclomaticComplexity">
     <properties>