Merge pull request #3958 from oowekyala:issue2352-ruleset-deprecations

[core] Deprecate some syntax for ruleset references #3958
This commit is contained in:
Andreas Dangel committed 2022-05-28 09:04:23 +02:00
commit 16df537813
8 files changed
+193 -70

No files matched your search

+13
View File
@@ -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 \<lang\>-\<ruleset\> 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:
- `<lang-name>-<ruleset-name>`, 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.
@@ -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;
@@ -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<String> rulesetReferences)
throws RuleSetNotFoundException {
boolean withDeprecatedRuleReferences, Set<String> 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<String> rulesetReferences)
throws RuleSetNotFoundException {
private void parseRuleSetReferenceNode(RuleSetBuilder ruleSetBuilder, Element ruleElement,
String ref,
RuleSetReferenceId ruleSetReferenceId, Set<String> rulesetReferences)
throws RuleSetNotFoundException {
String priority = null;
NodeList childNodes = ruleElement.getChildNodes();
Set<String> 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<RuleReference> 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<RuleSetReferenceId> 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();
@@ -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));
@@ -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;
* <td>all</td>
* </tr>
* <tr>
* <td>java-basic</td>
* <td>rulesets/java/basic.xml</td>
* <td>all</td>
* </tr>
* <tr>
* <td>50</td>
* <td>rulesets/releases/50.xml</td>
* <td>all</td>
* </tr>
* <tr>
* <td>rulesets/java/basic.xml/EmptyCatchBlock</td>
* <td>rulesets/java/basic.xml</td>
* <td>EmptyCatchBlock</td>
@@ -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<RuleSetReferenceId> 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<RuleSetReferenceId> parse(String referenceString, boolean warnDeprecated) {
List<RuleSetReferenceId> 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 {
* <i>ruleSetFileName</i> for all Rule external references,
* <i>ruleSetFileName/ruleName</i>, for a single Rule external
* references, or <i>ruleName</i> 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) {
@@ -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(
"<?xml version=\"1.0\"?>\n" + "<ruleset \n"
+ " name='foo'"
+ " xmlns=\"http://pmd.sourceforge.net/ruleset/2.0.0\"\n"
+ " xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\"\n"
+ " xsi:schemaLocation=\"http://pmd.sourceforge.net/ruleset/2.0.0 https://pmd.sourceforge.io/ruleset_2_0_0.xsd\">\n"
+ " <description>Custom ruleset for tests</description>\n"
+ " <rule ref=\"dummy-basic\"/>\n"
+ " </ruleset>\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 {
+ "<priority>3</priority>\n"
+ "</rule></ruleset>";
private static final String SINGLE_RULE_EMPTY_REF = "<?xml version=\"1.0\"?>\n"
+ "<ruleset name=\"test\">\n"
+ "<description>testdesc</description>\n"
+ "<rule \n"
+ "language=\"dummy\" \n"
+ "ref=\"\" \n"
+ "name=\"MockRuleName\" \n"
+ "message=\"avoid the mock rule\" \n"
+ "class=\"net.sourceforge.pmd.lang.rule.MockRule\">\n"
+ "<priority>3</priority>\n"
+ "</rule></ruleset>";
private static final String MULTIPLE_RULES = "<?xml version=\"1.0\"?>\n"
+ "<ruleset name=\"test\">\n"
+ "\n"
@@ -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);
}
@@ -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'"));
}
/**