diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 91f4f78ac6..5687105f4a 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -313,6 +313,7 @@ a warning will now be produced suggesting users to adopt it for better performan ### Fixed Issues * all + * [#394](https://github.com/pmd/pmd/issues/394): \[core] PMD exclude rules are failing with IllegalArgumentException with non-default minimumPriority * [#532](https://github.com/pmd/pmd/issues/532): \[core] security concerns on URL-based rulesets * [#538](https://github.com/pmd/pmd/issues/538): \[core] Provide an XML Schema for XML reports * [#600](https://github.com/pmd/pmd/issues/600): \[core] Nullpointer while creating cache File @@ -423,6 +424,9 @@ a warning will now be produced suggesting users to adopt it for better performan * The class `net.sourceforge.pmd.lang.java.typeresolution.typedefinition.JavaTypeDefinition` is now abstract, and has been enhanced to provide several new methods. +* The constructor of `net.sourceforge.pmd.RuleSetFactory`, which took a `ClassLoader` is deprecated. + Please use the alternative constructor with the `net.sourceforge.pmd.util.ResourceLoader` instead. + ### External Contributions * [#287](https://github.com/pmd/pmd/pull/287): \[apex] Make Rule suppression work - [Robert Sösemann](https://github.com/up2go-rsoesemann) diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/RuleSet.java b/pmd-core/src/main/java/net/sourceforge/pmd/RuleSet.java index 870f9d0bb1..bf3572e437 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/RuleSet.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/RuleSet.java @@ -122,15 +122,26 @@ 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) { - if (rule == null) { + public RuleSetBuilder addRule(final Rule newRule) { + 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("The rule with name " + newRule.getName() + " is duplicated. " + + "Future versions of PMD will reject to load such rulesets."); + break; + } + } + + rules.add(newRule); return this; } @@ -163,15 +174,22 @@ public class RuleSet implements ChecksumAware { * same language was added before, so that the existent rule * configuration won't be overridden. * - * @param rule + * @param ruleOrRef * the new rule to add * @return The same builder, for a fluid programming interface */ - public RuleSetBuilder addRuleIfNotExists(final Rule rule) { - if (rule == null) { + public RuleSetBuilder addRuleIfNotExists(final Rule ruleOrRef) { + if (ruleOrRef == null) { throw new IllegalArgumentException(MISSING_RULE); } + // resolve the underlying rule, to avoid adding duplicated rules + // if the rule has been renamed/merged and moved at the same time + Rule rule = ruleOrRef; + while (rule instanceof RuleReference) { + rule = ((RuleReference) rule).getRule(); + } + boolean exists = false; for (final Rule r : rules) { if (r.getName().equals(rule.getName()) && r.getLanguage() == rule.getLanguage()) { @@ -180,7 +198,7 @@ public class RuleSet implements ChecksumAware { } } if (!exists) { - addRule(rule); + addRule(ruleOrRef); } return this; } @@ -387,6 +405,17 @@ public class RuleSet implements ChecksumAware { public RuleSet build() { return new RuleSet(this); } + + public void filterRulesByPriority(RulePriority minimumPriority) { + Iterator 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(); + } + } + } } /** diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactory.java b/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactory.java index 466c1068c9..4edf435e21 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactory.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactory.java @@ -62,6 +62,16 @@ public class RuleSetFactory { this(new ResourceLoader(), RulePriority.LOW, false, true); } + /** + * @deprecated Use {@link #RuleSetFactory(ResourceLoader, RulePriority, boolean, boolean)} with + * {@link ResourceLoader} instead of a {@link ClassLoader}. + */ + @Deprecated // to be removed with PMD 7.0.0. + public RuleSetFactory(final ClassLoader classLoader, final RulePriority minimumPriority, + final boolean warnDeprecated, final boolean enableCompatibility) { + this(new ResourceLoader(classLoader), minimumPriority, warnDeprecated, enableCompatibility); + } + public RuleSetFactory(final ResourceLoader resourceLoader, final RulePriority minimumPriority, final boolean warnDeprecated, final boolean enableCompatibility) { this.resourceLoader = resourceLoader; @@ -367,6 +377,8 @@ public class RuleSetFactory { ruleSetBuilder.withDescription("Missing description"); } + ruleSetBuilder.filterRulesByPriority(minimumPriority); + return ruleSetBuilder.build(); } catch (ClassNotFoundException cnfe) { return classNotFoundProblem(cnfe); @@ -483,22 +495,46 @@ 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)); + List potentialRules = new ArrayList<>(); + int countDeprecated = 0; for (Rule rule : otherRuleSet.getRules()) { excludedRulesCheck.remove(rule.getName()); - if (!ruleSetReference.getExcludes().contains(rule.getName()) && !rule.isDeprecated()) { + if (!ruleSetReference.getExcludes().contains(rule.getName())) { 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))); } + + if (rule.isDeprecated()) { + countDeprecated++; + } + potentialRules.add(ruleReference); } } + + boolean rulesetDeprecated = false; + if (potentialRules.size() == countDeprecated) { + // all rules in the ruleset have been deprecated - the ruleset itself is considered to be deprecated + rulesetDeprecated = true; + LOG.warning("The RuleSet " + ref + " has been deprecated."); + } + + for (RuleReference r : potentialRules) { + if (rulesetDeprecated || !r.getRule().isDeprecated()) { + // add the rule, if either the ruleset itself is deprecated (then we add all rules) + // or if the rule is not deprecated (in that case, the ruleset might contain deprecated as well + // as valid references to rules) + ruleSetBuilder.addRuleIfNotExists(r); + } + } + if (!excludedRulesCheck.isEmpty()) { throw new IllegalArgumentException( "Unable to exclude rules " + excludedRulesCheck + "; perhaps the rule name is mispelled?"); @@ -529,10 +565,7 @@ public class RuleSetFactory { Rule rule = new RuleFactory().buildRule(ruleElement); rule.setRuleSetName(ruleSetBuilder.getName()); - if (StringUtils.isNotBlank(ruleSetReferenceId.getRuleName()) - || rule.getPriority().compareTo(minimumPriority) <= 0) { - ruleSetBuilder.addRule(rule); - } + ruleSetBuilder.addRule(rule); } @@ -564,7 +597,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); @@ -611,11 +646,8 @@ public class RuleSetFactory { ruleReference.setRuleSetReference(ruleSetReference); - if (StringUtils.isNotBlank(ruleSetReferenceId.getRuleName()) - || referencedRule.getPriority().compareTo(minimumPriority) <= 0) { - if (withDeprecatedRuleReferences || !isSameRuleSet || !ruleReference.isDeprecated()) { - ruleSetBuilder.addRuleReplaceIfExists(ruleReference); - } + if (withDeprecatedRuleReferences || !isSameRuleSet || !ruleReference.isDeprecated()) { + ruleSetBuilder.addRuleReplaceIfExists(ruleReference); } } diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/RuleSetFactoryTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/RuleSetFactoryTest.java index bd7b78a0f7..99e152a4e3 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/RuleSetFactoryTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/RuleSetFactoryTest.java @@ -372,6 +372,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); @@ -461,7 +500,7 @@ public class RuleSetFactoryTest { assertNotNull("RuleSet", ruleSet); assertFalse("RuleSet empty", ruleSet.getRules().isEmpty()); // No deprecated Rules should be loaded when loading an entire RuleSet - // by reference. + // by reference - unless it contains only deprecated rules - then all rules would be added Rule r = ruleSet.getRuleByName(DEPRECATED_RULE_NAME); assertNull("Deprecated Rule Reference", r); for (Rule rule : ruleSet.getRules()) { @@ -469,6 +508,13 @@ public class RuleSetFactoryTest { } } + @Test + public void testDeprecatedRuleSetReference() throws RuleSetNotFoundException { + RuleSetFactory ruleSetFactory = new RuleSetFactory(); + RuleSet ruleSet = ruleSetFactory.createRuleSet("net/sourceforge/pmd/rulesets/ruleset-deprecated.xml"); + assertEquals(2, ruleSet.getRules().size()); + } + @Test public void testExternalReferences() throws RuleSetNotFoundException { RuleSet rs = loadRuleSet(EXTERNAL_REFERENCE_RULE_SET); diff --git a/pmd-core/src/test/resources/net/sourceforge/pmd/rulesets/ruleset-deprecated.xml b/pmd-core/src/test/resources/net/sourceforge/pmd/rulesets/ruleset-deprecated.xml new file mode 100644 index 0000000000..fa912a0144 --- /dev/null +++ b/pmd-core/src/test/resources/net/sourceforge/pmd/rulesets/ruleset-deprecated.xml @@ -0,0 +1,11 @@ + + + + + Ruleset used by test RuleSetFactoryTest + + + + + \ No newline at end of file diff --git a/pmd-core/src/test/resources/net/sourceforge/pmd/rulesets/ruleset-minimum-priority-exclusion.xml b/pmd-core/src/test/resources/net/sourceforge/pmd/rulesets/ruleset-minimum-priority-exclusion.xml new file mode 100644 index 0000000000..564335d810 --- /dev/null +++ b/pmd-core/src/test/resources/net/sourceforge/pmd/rulesets/ruleset-minimum-priority-exclusion.xml @@ -0,0 +1,15 @@ + + + + + Ruleset used by test RuleSetFactoryTest + + + + + + + + + \ No newline at end of file diff --git a/pmd-core/src/test/resources/net/sourceforge/pmd/rulesets/ruleset-minimum-priority.xml b/pmd-core/src/test/resources/net/sourceforge/pmd/rulesets/ruleset-minimum-priority.xml new file mode 100644 index 0000000000..9693540999 --- /dev/null +++ b/pmd-core/src/test/resources/net/sourceforge/pmd/rulesets/ruleset-minimum-priority.xml @@ -0,0 +1,17 @@ + + + + + Ruleset used by test RuleSetFactoryTest + + + + + + + + 5 + + + \ No newline at end of file diff --git a/pmd-core/src/test/resources/rulesets/dummy/basic.xml b/pmd-core/src/test/resources/rulesets/dummy/basic.xml index 0a141832a2..30ffe0cc47 100644 --- a/pmd-core/src/test/resources/rulesets/dummy/basic.xml +++ b/pmd-core/src/test/resources/rulesets/dummy/basic.xml @@ -32,5 +32,5 @@ Just for test ]]> - " + \ No newline at end of file diff --git a/pmd-core/src/test/resources/rulesets/dummy/deprecated.xml b/pmd-core/src/test/resources/rulesets/dummy/deprecated.xml new file mode 100644 index 0000000000..d254b17b8f --- /dev/null +++ b/pmd-core/src/test/resources/rulesets/dummy/deprecated.xml @@ -0,0 +1,15 @@ + + + + + Ruleset used by test RuleSetReferenceIdTest + + + + + + + + + \ No newline at end of file diff --git a/pmd-java/src/main/resources/rulesets/java/controversial.xml b/pmd-java/src/main/resources/rulesets/java/controversial.xml index 7d59e822b2..802e50c84c 100644 --- a/pmd-java/src/main/resources/rulesets/java/controversial.xml +++ b/pmd-java/src/main/resources/rulesets/java/controversial.xml @@ -10,9 +10,6 @@ The Controversial ruleset contains rules that, for whatever reason, are consider They are held here to allow people to include them as they see fit within their custom rulesets. - - - @@ -41,4 +38,7 @@ They are held here to allow people to include them as they see fit within their + + + diff --git a/pmd-java/src/main/resources/rulesets/java/unnecessary.xml b/pmd-java/src/main/resources/rulesets/java/unnecessary.xml index 7dc73555c8..7d151cf0b4 100644 --- a/pmd-java/src/main/resources/rulesets/java/unnecessary.xml +++ b/pmd-java/src/main/resources/rulesets/java/unnecessary.xml @@ -13,7 +13,6 @@ The Unnecessary Ruleset contains a collection of rules for unnecessary code. - @@ -21,4 +20,7 @@ The Unnecessary Ruleset contains a collection of rules for unnecessary code. + + +