diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 3aa0ba7210..f8d2770b20 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -84,6 +84,7 @@ ruleset. Use the new rule {% rule java/codestyle/UnnecessarySemicolon %} instead * cli * [#1445](https://github.com/pmd/pmd/issues/1445): \[core] Allow CLI to take globs as parameters * core + * [#2352](https://github.com/pmd/pmd/issues/2352): \[core] Deprecate \-\ hyphen notation for ruleset references * [#3942](https://github.com/pmd/pmd/issues/3942): \[core] common-io path traversal vulnerability (CVE-2021-29425) * cs (c#) * [#3974](https://github.com/pmd/pmd/pull/3974): \[cs] Add option to ignore C# attributes (annotations) @@ -110,8 +111,20 @@ ruleset. Use the new rule {% rule java/codestyle/UnnecessarySemicolon %} instead ### API Changes +#### Deprecated ruleset references + +Ruleset references with the following formats are now deprecated and will produce a warning +when used on the CLI or in a ruleset XML file: +- `-`, eg `java-basic`, which resolves to `rulesets/java/basic.xml` +- the internal release number, eg `600`, which resolves to `rulesets/releases/600.xml` + +Use the explicit forms of these references to be compatible with PMD 7. + #### Deprecated API +- {% jdoc core::RuleSetReferenceId#toString() %} is now deprecated. The format of this + method will remain the same until PMD 7. The deprecation is intended to steer users + away from relying on this format, as it may be changed in PMD 7. - {% jdoc core::PMDConfiguration#getInputPaths() %} and {% jdoc core::PMDConfiguration#setInputPaths(java.lang.String) %} are now deprecated. A new set of methods have been added, which use lists and do not rely on comma splitting. diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/PMD.java b/pmd-core/src/main/java/net/sourceforge/pmd/PMD.java index 714bfb0e64..70dcecaacf 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/PMD.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/PMD.java @@ -242,6 +242,7 @@ public class PMD { @Deprecated @InternalApi public static int doPMD(PMDConfiguration configuration) { + LOG.fine("Current classpath:\n" + System.getProperty("java.class.path")); try (PmdAnalysis pmd = PmdAnalysis.create(configuration)) { if (pmd.getRulesets().isEmpty()) { return pmd.getReporter().numErrors() > 0 ? -1 : 0; 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 70af9df68b..196eaa8c2e 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactory.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetFactory.java @@ -492,17 +492,22 @@ public class RuleSetFactory { * @param rulesetReferences keeps track of already processed complete ruleset references in order to log a warning */ private void parseRuleNode(RuleSetReferenceId ruleSetReferenceId, RuleSetBuilder ruleSetBuilder, Node ruleNode, - boolean withDeprecatedRuleReferences, Set rulesetReferences) - throws RuleSetNotFoundException { + boolean withDeprecatedRuleReferences, Set rulesetReferences) + throws RuleSetNotFoundException { Element ruleElement = (Element) ruleNode; - String ref = ruleElement.getAttribute("ref"); - if (ref.endsWith("xml")) { - parseRuleSetReferenceNode(ruleSetBuilder, ruleElement, ref, rulesetReferences); - } else if (StringUtils.isBlank(ref)) { - parseSingleRuleNode(ruleSetReferenceId, ruleSetBuilder, ruleNode); - } else { - parseRuleReferenceNode(ruleSetReferenceId, ruleSetBuilder, ruleNode, ref, withDeprecatedRuleReferences); + if (ruleElement.hasAttribute("ref")) { + String ref = ruleElement.getAttribute("ref"); + RuleSetReferenceId refId = parseReferenceAndWarn(ruleSetBuilder, ref); + if (refId != null) { + if (refId.isAllRules()) { + parseRuleSetReferenceNode(ruleSetBuilder, ruleElement, ref, refId, rulesetReferences); + } else { + parseRuleReferenceNode(ruleSetReferenceId, ruleSetBuilder, ruleNode, ref, refId, withDeprecatedRuleReferences); + } + return; + } } + parseSingleRuleNode(ruleSetReferenceId, ruleSetBuilder, ruleNode); } /** @@ -519,8 +524,10 @@ public class RuleSetFactory { * The RuleSet reference. * @param rulesetReferences keeps track of already processed complete ruleset references in order to log a warning */ - private void parseRuleSetReferenceNode(RuleSetBuilder ruleSetBuilder, Element ruleElement, String ref, Set rulesetReferences) - throws RuleSetNotFoundException { + private void parseRuleSetReferenceNode(RuleSetBuilder ruleSetBuilder, Element ruleElement, + String ref, + RuleSetReferenceId ruleSetReferenceId, Set rulesetReferences) + throws RuleSetNotFoundException { String priority = null; NodeList childNodes = ruleElement.getChildNodes(); Set excludedRulesCheck = new HashSet<>(); @@ -539,7 +546,7 @@ public class RuleSetFactory { // 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 = toLoader().filterAbovePriority(RulePriority.LOW).warnDeprecated(false).toFactory(); - RuleSet otherRuleSet = ruleSetFactory.createRuleSet(RuleSetReferenceId.parse(ref).get(0)); + RuleSet otherRuleSet = ruleSetFactory.createRuleSet(ruleSetReferenceId); List potentialRules = new ArrayList<>(); int countDeprecated = 0; for (Rule rule : otherRuleSet.getRules()) { @@ -584,11 +591,23 @@ public class RuleSetFactory { if (rulesetReferences.contains(ref)) { LOG.warning("The ruleset " + ref + " is referenced multiple times in \"" - + ruleSetBuilder.getName() + "\"."); + + ruleSetBuilder.getName() + "\"."); } rulesetReferences.add(ref); } + private RuleSetReferenceId parseReferenceAndWarn(RuleSetBuilder ruleSetBuilder, String ref) { + List references = RuleSetReferenceId.parse(ref, warnDeprecated); + if (references.size() > 1 && warnDeprecated) { + LOG.warning("Using a comma separated list as a ref attribute is deprecated. " + + "All references but the first are ignored. Reference: '" + ref + "'"); + } else if (references.isEmpty()) { + LOG.warning("Empty ref attribute in ruleset '" + ruleSetBuilder.getName() + "'"); + return null; + } + return references.get(0); + } + /** * Parse a rule node as a single Rule. The Rule has been fully defined * within the context of the current RuleSet. @@ -641,7 +660,9 @@ public class RuleSetFactory { * or not */ private void parseRuleReferenceNode(RuleSetReferenceId ruleSetReferenceId, RuleSetBuilder ruleSetBuilder, - Node ruleNode, String ref, boolean withDeprecatedRuleReferences) throws RuleSetNotFoundException { + Node ruleNode, String ref, + RuleSetReferenceId otherRuleSetReferenceId, + boolean withDeprecatedRuleReferences) throws RuleSetNotFoundException { Element ruleElement = (Element) ruleNode; // Stop if we're looking for a particular Rule, and this element is not @@ -656,7 +677,6 @@ public class RuleSetFactory { RuleSetFactory ruleSetFactory = toLoader().filterAbovePriority(RulePriority.LOW).warnDeprecated(false).toFactory(); boolean isSameRuleSet = false; - RuleSetReferenceId otherRuleSetReferenceId = RuleSetReferenceId.parse(ref).get(0); if (!otherRuleSetReferenceId.isExternal() && containsRule(ruleSetReferenceId, otherRuleSetReferenceId.getRuleName())) { otherRuleSetReferenceId = new RuleSetReferenceId(ref, ruleSetReferenceId); @@ -743,6 +763,7 @@ public class RuleSetFactory { * @return {@code true} if the ruleName exists */ private boolean containsRule(RuleSetReferenceId ruleSetReferenceId, String ruleName) { + // TODO: avoid reloading the ruleset once again boolean found = false; try (InputStream ruleSet = ruleSetReferenceId.getInputStream(resourceLoader)) { DocumentBuilder builder = createDocumentBuilder(); diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetLoader.java b/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetLoader.java index efe727ac60..d071d585de 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetLoader.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetLoader.java @@ -150,7 +150,7 @@ public final class RuleSetLoader { * @throws RuleSetLoadException If any error occurs (eg, invalid syntax, or resource not found) */ public RuleSet loadFromResource(String rulesetPath) { - return loadFromResource(new RuleSetReferenceId(rulesetPath)); + return loadFromResource(new RuleSetReferenceId(rulesetPath, null, warnDeprecated)); } /** @@ -162,7 +162,7 @@ public final class RuleSetLoader { * @throws RuleSetLoadException If any error occurs (eg, invalid syntax) */ public RuleSet loadFromString(String filename, final String rulesetXmlContent) { - return loadFromResource(new RuleSetReferenceId(filename) { + return loadFromResource(new RuleSetReferenceId(filename, null, warnDeprecated) { @Override public InputStream getInputStream(ResourceLoader rl) { return new ByteArrayInputStream(rulesetXmlContent.getBytes(StandardCharsets.UTF_8)); diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetReferenceId.java b/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetReferenceId.java index 66dd5e47c9..82028f9993 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetReferenceId.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/RuleSetReferenceId.java @@ -11,6 +11,7 @@ import java.net.HttpURLConnection; import java.net.URL; import java.util.ArrayList; import java.util.List; +import java.util.logging.Logger; import org.apache.commons.lang3.StringUtils; @@ -58,16 +59,6 @@ import net.sourceforge.pmd.util.ResourceLoader; * all * * - * java-basic - * rulesets/java/basic.xml - * all - * - * - * 50 - * rulesets/releases/50.xml - * all - * - * * rulesets/java/basic.xml/EmptyCatchBlock * rulesets/java/basic.xml * EmptyCatchBlock @@ -85,12 +76,21 @@ import net.sourceforge.pmd.util.ResourceLoader; @Deprecated @InternalApi public class RuleSetReferenceId { + + // todo this class has issues... What is even an "external" ruleset? + // terminology and API should be clarified. + + // use the logger of RuleSetFactory, because the warnings conceptually come from there. + private static final Logger LOG = Logger.getLogger(RuleSetFactory.class.getName()); + private final boolean external; private final String ruleSetFileName; private final boolean allRules; private final String ruleName; private final RuleSetReferenceId externalRuleSetReferenceId; + private final String originalRef; + /** * Construct a RuleSetReferenceId for the given single ID string. * @@ -110,19 +110,34 @@ public class RuleSetReferenceId { * Rule. The external RuleSetReferenceId will be responsible for producing * the InputStream containing the Rule. * - * @param id - * The id string. - * @param externalRuleSetReferenceId - * A RuleSetReferenceId to associate with this new instance. - * @throws IllegalArgumentException - * If the ID contains a comma character. - * @throws IllegalArgumentException - * If external RuleSetReferenceId is not external. - * @throws IllegalArgumentException - * If the ID is not Rule reference when there is an external - * RuleSetReferenceId. + * @param id The id string. + * @param externalRuleSetReferenceId A RuleSetReferenceId to associate with this new instance. + * + * @throws IllegalArgumentException If the ID contains a comma character. + * @throws IllegalArgumentException If external RuleSetReferenceId is not external. + * @throws IllegalArgumentException If the ID is not Rule reference when there is an external + * RuleSetReferenceId. */ public RuleSetReferenceId(final String id, final RuleSetReferenceId externalRuleSetReferenceId) { + this(id, externalRuleSetReferenceId, false); + } + + /** + * Construct a RuleSetReferenceId for the given single ID string. If an + * external RuleSetReferenceId is given, the ID must refer to a non-external + * Rule. The external RuleSetReferenceId will be responsible for producing + * the InputStream containing the Rule. + * + * @param id The id string. + * @param externalRuleSetReferenceId A RuleSetReferenceId to associate with this new instance. + * + * @throws IllegalArgumentException If the ID contains a comma character. + * @throws IllegalArgumentException If external RuleSetReferenceId is not external. + * @throws IllegalArgumentException If the ID is not Rule reference when there is an external + * RuleSetReferenceId. + */ + RuleSetReferenceId(final String id, final RuleSetReferenceId externalRuleSetReferenceId, boolean warnDeprecated) { + this.originalRef = id; if (externalRuleSetReferenceId != null && !externalRuleSetReferenceId.isExternal()) { throw new IllegalArgumentException("Cannot pair with non-external <" + externalRuleSetReferenceId + ">."); @@ -177,8 +192,16 @@ public class RuleSetReferenceId { allRules = tempRuleName == null; } else { // resolve the ruleset name - it's maybe a built in ruleset - String builtinRuleSet = resolveBuiltInRuleset(tempRuleSetFileName); + String expandedRuleset = resolveDeprecatedBuiltInRulesetShorthand(tempRuleSetFileName); + String builtinRuleSet = expandedRuleset == null ? tempRuleSetFileName : expandedRuleset; if (checkRulesetExists(builtinRuleSet)) { + if (expandedRuleset != null && warnDeprecated) { + LOG.warning( + "Ruleset reference '" + tempRuleSetFileName + "' uses a deprecated form, use '" + + builtinRuleSet + "' instead" + ); + } + external = true; ruleSetFileName = builtinRuleSet; ruleName = tempRuleName; @@ -249,25 +272,22 @@ public class RuleSetReferenceId { * the ruleset name * @return the full classpath to the ruleset */ - private String resolveBuiltInRuleset(final String name) { - String result = null; - if (name != null) { - // Likely a simple RuleSet name - int index = name.indexOf('-'); - if (index >= 0) { - // Standard short name - result = "rulesets/" + name.substring(0, index) + '/' + name.substring(index + 1) + ".xml"; - } else { - // A release RuleSet? - if (name.matches("[0-9]+.*")) { - result = "rulesets/releases/" + name + ".xml"; - } else { - // Appears to be a non-standard RuleSet name - result = name; - } - } + private String resolveDeprecatedBuiltInRulesetShorthand(final String name) { + if (name == null) { + return null; } - return result; + // Likely a simple RuleSet name + int index = name.indexOf('-'); + if (index > 0) { + // Standard short name + return "rulesets/" + name.substring(0, index) + '/' + name.substring(index + 1) + ".xml"; + } + // A release RuleSet? + if (name.matches("[0-9]+.*")) { + return "rulesets/releases/" + name + ".xml"; + } + // Appears to be a non-standard RuleSet name + return null; } /** @@ -330,19 +350,31 @@ public class RuleSetReferenceId { * Parse a String comma separated list of RuleSet reference IDs into a List * of RuleReferenceId instances. * - * @param referenceString - * A comma separated list of RuleSet reference IDs. + * @param referenceString A comma separated list of RuleSet reference IDs. + * * @return The corresponding List of RuleSetReferenceId instances. */ public static List parse(String referenceString) { + return parse(referenceString, false); + } + + /** + * Parse a String comma separated list of RuleSet reference IDs into a List + * of RuleReferenceId instances. + * + * @param referenceString A comma separated list of RuleSet reference IDs. + * + * @return The corresponding List of RuleSetReferenceId instances. + */ + public static List parse(String referenceString, boolean warnDeprecated) { List references = new ArrayList<>(); if (referenceString != null && referenceString.trim().length() > 0) { if (referenceString.indexOf(',') == -1) { - references.add(new RuleSetReferenceId(referenceString)); + references.add(new RuleSetReferenceId(referenceString, null, warnDeprecated)); } else { for (String name : referenceString.split(",")) { - references.add(new RuleSetReferenceId(name.trim())); + references.add(new RuleSetReferenceId(name.trim(), null, warnDeprecated)); } } } @@ -405,9 +437,9 @@ public class RuleSetReferenceId { InputStream in = StringUtils.isBlank(ruleSetFileName) ? null : rl.loadResourceAsStream(ruleSetFileName); if (in == null) { - throw new RuleSetNotFoundException("Can't find resource '" + ruleSetFileName + "' for rule '" + ruleName + throw new RuleSetNotFoundException("Cannot resolve rule/ruleset reference '" + originalRef + "'" + ". Make sure the resource is a valid file or URL and is on the CLASSPATH. " - + "Here's the current classpath: " + System.getProperty("java.class.path")); + + "Use --debug (or a fine log level) to see the current classpath."); } return in; } else { @@ -422,8 +454,11 @@ public class RuleSetReferenceId { * ruleSetFileName for all Rule external references, * ruleSetFileName/ruleName, for a single Rule external * references, or ruleName otherwise. + * + * @deprecated Do not rely on the format of this method, it may be changed in PMD 7. */ @Override + @Deprecated public String toString() { if (ruleSetFileName != null) { if (allRules) { 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 bfbc7ce547..365754b9c6 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/RuleSetFactoryTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/RuleSetFactoryTest.java @@ -4,6 +4,7 @@ package net.sourceforge.pmd; +import static org.hamcrest.Matchers.containsString; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; @@ -20,6 +21,7 @@ import java.util.List; import java.util.Set; import org.apache.commons.lang3.StringUtils; +import org.hamcrest.MatcherAssert; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -148,6 +150,19 @@ public class RuleSetFactoryTest { assertEquals("avoid the mock rule", r.getMessage()); } + @Test + public void testSingleRuleEmptyRef() throws RuleSetNotFoundException { + RuleSet rs = loadRuleSet(SINGLE_RULE_EMPTY_REF); + assertEquals(1, rs.size()); + + MatcherAssert.assertThat(logging.getLog(), containsString("Empty ref attribute in ruleset 'test'")); + + Rule r = rs.getRules().iterator().next(); + assertEquals("MockRuleName", r.getName()); + assertEquals("net.sourceforge.pmd.lang.rule.MockRule", r.getRuleClass()); + assertEquals("avoid the mock rule", r.getMessage()); + } + @Test public void testMultipleRules() throws RuleSetNotFoundException { RuleSet rs = loadRuleSet(MULTIPLE_RULES); @@ -926,6 +941,23 @@ public class RuleSetFactoryTest { assertTrue(logging.getLog().contains("RuleSet name is missing.")); } + @Test + public void testDeprecatedRulesetReferenceProducesWarning() throws Exception { + RuleSetReferenceId ref = createRuleSetReferenceId( + "\n" + "\n" + + " Custom ruleset for tests\n" + + " \n" + + " \n"); + RuleSetLoader ruleSetFactory = new RuleSetLoader().warnDeprecated(true); + ruleSetFactory.loadFromResource(ref); + + MatcherAssert.assertThat(logging.getLog(), containsString("Ruleset reference 'dummy-basic' uses a deprecated form, use 'rulesets/dummy/basic.xml' instead")); + } + @Test public void testMissingRuleSetDescriptionIsWarning() throws Exception { RuleSetReferenceId ref = createRuleSetReferenceId( @@ -1066,6 +1098,18 @@ public class RuleSetFactoryTest { + "3\n" + ""; + private static final String SINGLE_RULE_EMPTY_REF = "\n" + + "\n" + + "testdesc\n" + + "\n" + + "3\n" + + ""; + private static final String MULTIPLE_RULES = "\n" + "\n" + "\n" diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/cli/CoreCliTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/cli/CoreCliTest.java index f1913018ce..5b595e0f5b 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/cli/CoreCliTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/cli/CoreCliTest.java @@ -5,6 +5,7 @@ package net.sourceforge.pmd.cli; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.containsStringIgnoringCase; import static org.hamcrest.Matchers.not; import static org.junit.Assert.assertEquals; @@ -21,6 +22,7 @@ import java.nio.file.Path; import java.util.logging.Logger; import org.hamcrest.Matcher; +import org.hamcrest.MatcherAssert; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -188,6 +190,13 @@ public class CoreCliTest { } } + @Test + public void testDeprecatedRulesetSyntaxOnCommandLine() { + startCapturingErrAndOut(); + runPmd(StatusCode.VIOLATIONS_FOUND, "--no-cache", "--dir", srcDir, "--rulesets", "dummy-basic"); + MatcherAssert.assertThat(errStreamCaptor.getLog(), containsString("Ruleset reference 'dummy-basic' uses a deprecated form, use 'rulesets/dummy/basic.xml' instead")); + } + @Test public void testWrongCliOptionsDoNotPrintUsage() { @@ -223,7 +232,7 @@ public class CoreCliTest { private static void runPmdSuccessfully(Object... args) { - runPmd(0, args); + runPmd(StatusCode.OK, args); } private static String[] argsToString(Object... args) { @@ -248,9 +257,9 @@ public class CoreCliTest { return StandardCharsets.UTF_8.decode(buf).toString(); } - private static void runPmd(int expectedExitCode, Object[] args) { + private static void runPmd(StatusCode expectedExitCode, Object... args) { StatusCode actualExitCode = PMD.runPmd(argsToString(args)); - assertEquals("Exit code", expectedExitCode, actualExitCode.toInt()); + assertEquals("Exit code", expectedExitCode, actualExitCode); } diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/cli/CLITest.java b/pmd-java/src/test/java/net/sourceforge/pmd/cli/CLITest.java index 22bcfca35e..defff23883 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/cli/CLITest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/cli/CLITest.java @@ -80,8 +80,8 @@ public class CLITest extends BaseCLITest { public void testWrongRuleset() { String[] args = { "-d", SOURCE_FOLDER, "-f", "text", "-R", "category/java/designn.xml", }; String log = runTest(StatusCode.ERROR, args); - assertThat(log, containsString("Can't find resource 'category/java/designn.xml' for rule 'null'." - + " Make sure the resource is a valid file")); + assertThat(log, containsString("Cannot resolve rule/ruleset reference " + + "'category/java/designn.xml'")); } /** @@ -91,8 +91,8 @@ public class CLITest extends BaseCLITest { public void testWrongRulesetWithRulename() { String[] args = { "-d", SOURCE_FOLDER, "-f", "text", "-R", "category/java/designn.xml/UseCollectionIsEmpty", }; String log = runTest(StatusCode.ERROR, args); - assertThat(log, containsString("Can't find resource 'category/java/designn.xml' for rule " - + "'UseCollectionIsEmpty'.")); + assertThat(log, containsString("Cannot resolve rule/ruleset reference" + + " 'category/java/designn.xml/UseCollectionIsEmpty'")); } /**