From 1c0badd42997dd5883246c43289cd704433c5755 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Wed, 1 May 2013 11:32:39 +0200 Subject: [PATCH] pmd: fix #1089 When changing priority in a custom ruleset, violations reported twice --- pmd/etc/changelog.txt | 1 + .../java/net/sourceforge/pmd/RuleSet.java | 52 +++++++++++++++++-- .../net/sourceforge/pmd/RuleSetFactory.java | 4 +- .../sourceforge/pmd/RuleSetFactoryTest.java | 21 ++++++-- .../pmd/rulesets/reference-ruleset.xml | 4 ++ 5 files changed, 74 insertions(+), 8 deletions(-) diff --git a/pmd/etc/changelog.txt b/pmd/etc/changelog.txt index 0dd827731e..b54c1431c8 100644 --- a/pmd/etc/changelog.txt +++ b/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: diff --git a/pmd/src/main/java/net/sourceforge/pmd/RuleSet.java b/pmd/src/main/java/net/sourceforge/pmd/RuleSet.java index 059327aa2a..0004ddca3e 100644 --- a/pmd/src/main/java/net/sourceforge/pmd/RuleSet.java +++ b/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 true if the new rule replaced an existing one, otherwise false. + */ + public boolean addRuleReplaceIfExists(Rule rule) { + if (rule == null) { + throw new IllegalArgumentException("Missing rule"); + } + + boolean replaced = false; + for (Iterator 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 true if the rule was added, false 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 i = rules.iterator(); i.hasNext();) { - Rule r = i.next(); + for (Rule r : rules) { if (r.getName().equals(ruleName)) { return r; } diff --git a/pmd/src/main/java/net/sourceforge/pmd/RuleSetFactory.java b/pmd/src/main/java/net/sourceforge/pmd/RuleSetFactory.java index c0b3d44b57..c83aa37f1e 100644 --- a/pmd/src/main/java/net/sourceforge/pmd/RuleSetFactory.java +++ b/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); } } diff --git a/pmd/src/test/java/net/sourceforge/pmd/RuleSetFactoryTest.java b/pmd/src/test/java/net/sourceforge/pmd/RuleSetFactoryTest.java index a062bdbacc..d18f7d3dc8 100644 --- a/pmd/src/test/java/net/sourceforge/pmd/RuleSetFactoryTest.java +++ b/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(); diff --git a/pmd/src/test/resources/net/sourceforge/pmd/rulesets/reference-ruleset.xml b/pmd/src/test/resources/net/sourceforge/pmd/rulesets/reference-ruleset.xml index ccf566657a..328f5d7253 100644 --- a/pmd/src/test/resources/net/sourceforge/pmd/rulesets/reference-ruleset.xml +++ b/pmd/src/test/resources/net/sourceforge/pmd/rulesets/reference-ruleset.xml @@ -16,12 +16,16 @@ + 2 + + +