From 3004e76257d3be73cd272753fff0738593b9aa88 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Fri, 3 Jul 2020 21:11:57 +0200 Subject: [PATCH 01/19] [core] CPD: Add correct XML 1.0 escaping for code snippets --- pmd-core/pom.xml | 4 ++++ .../net/sourceforge/pmd/cpd/XMLRenderer.java | 3 ++- .../sourceforge/pmd/cpd/XMLRendererTest.java | 22 +++++++++++++++---- pmd-doc/pom.xml | 1 - pom.xml | 5 +++++ 5 files changed, 29 insertions(+), 6 deletions(-) diff --git a/pmd-core/pom.xml b/pmd-core/pom.xml index 0089efa13a..18669acb64 100644 --- a/pmd-core/pom.xml +++ b/pmd-core/pom.xml @@ -134,6 +134,10 @@ org.apache.commons commons-lang3 + + org.apache.commons + commons-text + org.ow2.asm asm diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/cpd/XMLRenderer.java b/pmd-core/src/main/java/net/sourceforge/pmd/cpd/XMLRenderer.java index 33230152d9..5390e9410f 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/cpd/XMLRenderer.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/cpd/XMLRenderer.java @@ -18,6 +18,7 @@ import javax.xml.transform.TransformerFactory; import javax.xml.transform.dom.DOMSource; import javax.xml.transform.stream.StreamResult; +import org.apache.commons.text.StringEscapeUtils; import org.w3c.dom.Document; import org.w3c.dom.Element; @@ -140,7 +141,7 @@ public final class XMLRenderer implements Renderer, CPDRenderer { // the code snippet has normalized line endings String platformSpecific = codeSnippet.replace("\n", System.lineSeparator()); Element codefragment = doc.createElement("codefragment"); - codefragment.appendChild(doc.createCDATASection(platformSpecific)); + codefragment.appendChild(doc.createCDATASection(StringEscapeUtils.escapeXml10(platformSpecific))); duplication.appendChild(codefragment); } return duplication; diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/cpd/XMLRendererTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/cpd/XMLRendererTest.java index 22eca255a7..a7ddbb5474 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/cpd/XMLRendererTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/cpd/XMLRendererTest.java @@ -5,6 +5,7 @@ package net.sourceforge.pmd.cpd; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -196,6 +197,23 @@ public class XMLRendererTest { assertTrue(report.contains(espaceChar)); } + @Test + public void testRendererXMLEscaping() throws IOException { + String formfeed = "\u000C"; + String codefragment = "code fragment" + formfeed + "\nline2\nline3"; + CPDRenderer renderer = new XMLRenderer(); + List list = new ArrayList<>(); + Mark mark1 = createMark("public", "file1", 1, 2, codefragment); + Mark mark2 = createMark("public", "file2", 5, 2, codefragment); + Match match1 = new Match(75, mark1, mark2); + list.add(match1); + + StringWriter sw = new StringWriter(); + renderer.render(list.iterator(), sw); + String report = sw.toString(); + assertFalse(report.contains(formfeed)); + } + private Mark createMark(String image, String tokenSrcID, int beginLine, int lineCount, String code) { Mark result = new Mark(new TokenEntry(image, tokenSrcID, beginLine)); @@ -214,8 +232,4 @@ public class XMLRendererTest { result.setSourceCode(new SourceCode(new SourceCode.StringCodeLoader(code))); return result; } - - public static junit.framework.Test suite() { - return new junit.framework.JUnit4TestAdapter(XMLRendererTest.class); - } } diff --git a/pmd-doc/pom.xml b/pmd-doc/pom.xml index 22f43c9c0b..e57272388c 100644 --- a/pmd-doc/pom.xml +++ b/pmd-doc/pom.xml @@ -95,7 +95,6 @@ org.apache.commons commons-text - 1.6 org.yaml diff --git a/pom.xml b/pom.xml index 277c3f1a07..69ee3c3c4d 100644 --- a/pom.xml +++ b/pom.xml @@ -669,6 +669,11 @@ commons-lang3 3.8.1 + + org.apache.commons + commons-text + 1.6 + org.slf4j slf4j-api From 3bb091e26d71c10179c2b40f122dd862c843e9a5 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sun, 5 Jul 2020 11:19:54 +0200 Subject: [PATCH 02/19] [core] CPD: Also escape filename, explicitly set XML 1.0 --- .../java/net/sourceforge/pmd/cpd/XMLRenderer.java | 4 +++- .../net/sourceforge/pmd/cpd/XMLRendererTest.java | 15 ++++++++++----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/cpd/XMLRenderer.java b/pmd-core/src/main/java/net/sourceforge/pmd/cpd/XMLRenderer.java index 5390e9410f..3cb2ead41b 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/cpd/XMLRenderer.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/cpd/XMLRenderer.java @@ -77,6 +77,7 @@ public final class XMLRenderer implements Renderer, CPDRenderer { try { TransformerFactory tf = TransformerFactory.newInstance(); Transformer transformer = tf.newTransformer(); + transformer.setOutputProperty(OutputKeys.VERSION, "1.0"); transformer.setOutputProperty(OutputKeys.METHOD, "xml"); transformer.setOutputProperty(OutputKeys.ENCODING, encoding); transformer.setOutputProperty(OutputKeys.INDENT, "yes"); @@ -120,7 +121,8 @@ public final class XMLRenderer implements Renderer, CPDRenderer { mark = iterator.next(); final Element file = doc.createElement("file"); file.setAttribute("line", String.valueOf(mark.getBeginLine())); - file.setAttribute("path", mark.getFilename()); + String filenameXml10 = StringEscapeUtils.unescapeXml(StringEscapeUtils.escapeXml10(mark.getFilename())); + file.setAttribute("path", filenameXml10); file.setAttribute("endline", String.valueOf(mark.getEndLine())); final int beginCol = mark.getBeginColumn(); final int endCol = mark.getEndColumn(); diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/cpd/XMLRendererTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/cpd/XMLRendererTest.java index a7ddbb5474..d5340d2c67 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/cpd/XMLRendererTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/cpd/XMLRendererTest.java @@ -31,6 +31,9 @@ import net.sourceforge.pmd.cpd.renderer.CPDRenderer; public class XMLRendererTest { private static final String ENCODING = (String) System.getProperties().get("file.encoding"); + private static final String FORM_FEED = "\u000C"; // this character is invalid in XML 1.0 documents + private static final String FORM_FEED_ENTITY = " "; // this is also not allowed in XML 1.0 documents + @Test public void testWithNoDuplication() throws IOException { @@ -186,8 +189,8 @@ public class XMLRendererTest { CPDRenderer renderer = new XMLRenderer(); List list = new ArrayList<>(); final String espaceChar = "<"; - Mark mark1 = createMark("public", "/var/F" + '<' + "oo.java", 48, 6, "code fragment"); - Mark mark2 = createMark("void", "/var/F list = new ArrayList<>(); Mark mark1 = createMark("public", "file1", 1, 2, codefragment); @@ -211,7 +215,8 @@ public class XMLRendererTest { StringWriter sw = new StringWriter(); renderer.render(list.iterator(), sw); String report = sw.toString(); - assertFalse(report.contains(formfeed)); + assertFalse(report.contains(FORM_FEED)); + assertFalse(report.contains(FORM_FEED_ENTITY)); } private Mark createMark(String image, String tokenSrcID, int beginLine, int lineCount, String code) { From d9279732443f899d05dbec533bbf0ab3aab4fddb Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sun, 5 Jul 2020 14:21:46 +0200 Subject: [PATCH 03/19] [core] PMD XMLRenderer - correctly support encoding The XMLRenderer uses by default UTF-8 encoding, but the writer uses the system default encoding, which doesn't work well together. Provide new experimental API Renderer::setReportFile, so that renderer implementations can create their own writers. The default implementation in AbstractRenderer is backwards compatible. --- .../main/java/net/sourceforge/pmd/PMD.java | 3 +- .../net/sourceforge/pmd/PMDConfiguration.java | 3 +- .../pmd/renderers/AbstractRenderer.java | 14 ++++ .../sourceforge/pmd/renderers/Renderer.java | 14 ++++ .../pmd/renderers/XMLRenderer.java | 74 ++++++++++++++----- .../java/net/sourceforge/pmd/util/IOUtil.java | 35 ++++++++- .../java/net/sourceforge/pmd/ReportTest.java | 24 +++++- .../pmd/renderers/XMLRendererTest.java | 41 ++++++++-- .../net/sourceforge/pmd/ant/PMDTaskTest.java | 21 +++++- .../pmd/cpd/CPDCommandLineInterfaceTest.java | 2 +- 10 files changed, 196 insertions(+), 35 deletions(-) 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 e93c667ac1..066d2fc935 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/PMD.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/PMD.java @@ -217,8 +217,7 @@ public class PMD { try (TimedOperation to = TimeTracker.startOperation(TimedOperationCategory.REPORTING)) { renderer = configuration.createRenderer(); renderers = Collections.singletonList(renderer); - - renderer.setWriter(IOUtil.createWriter(configuration.getReportFile())); + renderer.setReportFile(configuration.getReportFile()); renderer.start(); } diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/PMDConfiguration.java b/pmd-core/src/main/java/net/sourceforge/pmd/PMDConfiguration.java index 532d1b6575..694519e344 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/PMDConfiguration.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/PMDConfiguration.java @@ -19,7 +19,6 @@ import net.sourceforge.pmd.lang.LanguageVersionDiscoverer; import net.sourceforge.pmd.renderers.Renderer; import net.sourceforge.pmd.renderers.RendererFactory; import net.sourceforge.pmd.util.ClasspathClassLoader; -import net.sourceforge.pmd.util.IOUtil; /** * This class contains the details for the runtime configuration of PMD. There @@ -406,7 +405,7 @@ public class PMDConfiguration extends AbstractConfiguration { renderer.setUseShortNames(Arrays.asList(inputPaths.split(","))); } if (withReportWriter) { - renderer.setWriter(IOUtil.createWriter(reportFile)); + renderer.setReportFile(reportFile); } return renderer; } diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/renderers/AbstractRenderer.java b/pmd-core/src/main/java/net/sourceforge/pmd/renderers/AbstractRenderer.java index 6279575266..88b69d2202 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/renderers/AbstractRenderer.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/renderers/AbstractRenderer.java @@ -12,9 +12,11 @@ import java.util.List; import org.apache.commons.io.IOUtils; import net.sourceforge.pmd.PMDConfiguration; +import net.sourceforge.pmd.annotation.Experimental; import net.sourceforge.pmd.cli.PMDParameters; import net.sourceforge.pmd.internal.util.ShortFilenameUtil; import net.sourceforge.pmd.properties.AbstractPropertySource; +import net.sourceforge.pmd.util.IOUtil; /** * Abstract base class for {@link Renderer} implementations. @@ -109,4 +111,16 @@ public abstract class AbstractRenderer extends AbstractPropertySource implements IOUtils.closeQuietly(writer); } } + + /** + * {@inheritDoc} + * + *

This default implementation always uses the system default charset for the writer. + * Overwrite in specific renderers to support other charsets. + */ + @Experimental + @Override + public void setReportFile(String reportFilename) { + this.setWriter(IOUtil.createWriter(reportFilename)); + } } diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/renderers/Renderer.java b/pmd-core/src/main/java/net/sourceforge/pmd/renderers/Renderer.java index 6f42f25b78..3a2d8f8601 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/renderers/Renderer.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/renderers/Renderer.java @@ -9,6 +9,7 @@ import java.io.Writer; import java.util.List; import net.sourceforge.pmd.Report; +import net.sourceforge.pmd.annotation.Experimental; import net.sourceforge.pmd.properties.PropertyDescriptor; import net.sourceforge.pmd.properties.PropertySource; import net.sourceforge.pmd.util.datasource.DataSource; @@ -163,4 +164,17 @@ public interface Renderer extends PropertySource { void end() throws IOException; void flush() throws IOException; + + /** + * Sets the filename where the report should be written to. If no filename is provided, + * the renderer should write to stdout. + * + *

Implementations must initialize the writer of the renderer. + * + *

See {@link AbstractRenderer#setReportFile(String)} for the default impl. + * + * @param reportFilename the filename (optional). + */ + @Experimental + void setReportFile(String reportFilename); } diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XMLRenderer.java b/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XMLRenderer.java index eb71e43749..3e49814b43 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XMLRenderer.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XMLRenderer.java @@ -4,17 +4,24 @@ package net.sourceforge.pmd.renderers; +import java.io.File; import java.io.IOException; +import java.io.OutputStreamWriter; +import java.nio.charset.Charset; +import java.nio.charset.UnsupportedCharsetException; +import java.nio.file.Files; import java.text.SimpleDateFormat; import java.util.Date; import java.util.Iterator; +import org.apache.commons.lang3.StringUtils; +import org.apache.commons.text.StringEscapeUtils; + import net.sourceforge.pmd.PMD; import net.sourceforge.pmd.PMDVersion; import net.sourceforge.pmd.Report; import net.sourceforge.pmd.RuleViolation; import net.sourceforge.pmd.properties.StringProperty; -import net.sourceforge.pmd.util.StringUtil; /** * Renderer to XML format. @@ -26,7 +33,6 @@ public class XMLRenderer extends AbstractIncrementingRenderer { // TODO 7.0.0 use PropertyDescriptor or something more specialized public static final StringProperty ENCODING = new StringProperty("encoding", "XML encoding format, defaults to UTF-8.", "UTF-8", 0); - private boolean useUTF8 = false; public XMLRenderer() { super(NAME, "XML format."); @@ -46,9 +52,6 @@ public class XMLRenderer extends AbstractIncrementingRenderer { @Override public void start() throws IOException { String encoding = getProperty(ENCODING); - if ("utf-8".equalsIgnoreCase(encoding)) { - useUTF8 = true; - } StringBuilder buf = new StringBuilder(500); buf.append("").append(PMD.EOL); @@ -78,7 +81,7 @@ public class XMLRenderer extends AbstractIncrementingRenderer { } filename = nextFilename; buf.append("").append(PMD.EOL); } @@ -87,9 +90,9 @@ public class XMLRenderer extends AbstractIncrementingRenderer { buf.append("\" begincolumn=\"").append(rv.getBeginColumn()); buf.append("\" endcolumn=\"").append(rv.getEndColumn()); buf.append("\" rule=\""); - StringUtil.appendXmlEscaped(buf, rv.getRule().getName(), useUTF8); + buf.append(escape(rv.getRule().getName())); buf.append("\" ruleset=\""); - StringUtil.appendXmlEscaped(buf, rv.getRule().getRuleSetName(), useUTF8); + buf.append(escape(rv.getRule().getRuleSetName())); buf.append('"'); maybeAdd("package", rv.getPackageName(), buf); maybeAdd("class", rv.getClassName(), buf); @@ -99,7 +102,7 @@ public class XMLRenderer extends AbstractIncrementingRenderer { buf.append(" priority=\""); buf.append(rv.getRule().getPriority().getPriority()); buf.append("\">").append(PMD.EOL); - StringUtil.appendXmlEscaped(buf, rv.getDescription(), useUTF8); + buf.append(escape(rv.getDescription())); buf.append(PMD.EOL); buf.append(""); @@ -119,9 +122,9 @@ public class XMLRenderer extends AbstractIncrementingRenderer { for (Report.ProcessingError pe : errors) { buf.setLength(0); buf.append("").append(PMD.EOL); buf.append("").append(PMD.EOL); buf.append("").append(PMD.EOL); @@ -133,13 +136,13 @@ public class XMLRenderer extends AbstractIncrementingRenderer { for (Report.SuppressedViolation s : suppressed) { buf.setLength(0); buf.append("").append(PMD.EOL); writer.write(buf.toString()); } @@ -149,9 +152,9 @@ public class XMLRenderer extends AbstractIncrementingRenderer { for (final Report.ConfigurationError ce : configErrors) { buf.setLength(0); buf.append("").append(PMD.EOL); writer.write(buf.toString()); } @@ -162,7 +165,7 @@ public class XMLRenderer extends AbstractIncrementingRenderer { private void maybeAdd(String attr, String value, StringBuilder buf) { if (value != null && value.length() > 0) { buf.append(' ').append(attr).append("=\""); - StringUtil.appendXmlEscaped(buf, value, useUTF8); + buf.append(escape(value)); buf.append('"'); } } @@ -179,6 +182,41 @@ public class XMLRenderer extends AbstractIncrementingRenderer { .append('"'); } + @Override + public void setReportFile(String reportFilename) { + String encoding = getProperty(ENCODING); + + try { + Charset charset = Charset.forName(encoding); + this.writer = StringUtils.isBlank(reportFilename) ? new OutputStreamWriter(System.out, charset) + : Files.newBufferedWriter(new File(reportFilename).toPath(), charset); + } catch (IOException | UnsupportedCharsetException e) { + throw new IllegalArgumentException(e); + } + } + + /** + * Escape unicode characters for non UTF-8 encodings. + */ + private String escape(String text) { + String result = StringEscapeUtils.escapeXml10(text); + String encoding = getProperty(ENCODING); + if (!"UTF-8".equalsIgnoreCase(encoding)) { + StringBuilder sb = new StringBuilder(result); + for (int i = 0; i < sb.length(); i++) { + char c = sb.charAt(i); + // surrogate characters are not allowed in XML + if (Character.isHighSurrogate(c)) { + char low = sb.charAt(i + 1); + int codepoint = Character.toCodePoint(c, low); + sb.replace(i, i + 2, "&#x" + Integer.toHexString(codepoint) + ";"); + } + } + result = sb.toString(); + } + return result; + } + // FIXME: elapsed time not available until the end of the processing /* * private String createTimeElapsedAttr(Report rpt) { diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/util/IOUtil.java b/pmd-core/src/main/java/net/sourceforge/pmd/util/IOUtil.java index 9691b152f2..1ef21f6135 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/util/IOUtil.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/util/IOUtil.java @@ -12,7 +12,11 @@ import java.io.OutputStreamWriter; import java.io.Reader; import java.io.Writer; import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; +import java.nio.charset.UnsupportedCharsetException; import java.nio.file.Files; +import java.security.AccessController; +import java.security.PrivilegedAction; import org.apache.commons.io.IOUtils; import org.apache.commons.lang3.StringUtils; @@ -35,10 +39,39 @@ public final class IOUtil { return new OutputStreamWriter(System.out); } + /** + * Gets the current default charset. + * + *

In contrast to {@link Charset#defaultCharset()}, the result is not cached, + * so that in unit tests, the charset can be changed. + * @return + */ + private static Charset getDefaultCharset() { + String csn = AccessController.doPrivileged(new PrivilegedAction() { + @Override + public String run() { + return System.getProperty("file.encoding"); + } + }); + try { + return Charset.forName(csn); + } catch (UnsupportedCharsetException e) { + return StandardCharsets.UTF_8; + } + } + + /** + * Creates a writer that writes to the given file or to stdout. + * + *

Warning: This writer always uses the system default charset. + * + * @param reportFile the file name (optional) + * @return the writer, never null + */ public static Writer createWriter(String reportFile) { try { return StringUtils.isBlank(reportFile) ? createWriter() - : Files.newBufferedWriter(new File(reportFile).toPath(), Charset.defaultCharset()); + : Files.newBufferedWriter(new File(reportFile).toPath(), getDefaultCharset()); } catch (IOException e) { throw new IllegalArgumentException(e); } diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/ReportTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/ReportTest.java index 2e5f2c5e16..224c2b4064 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/ReportTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/ReportTest.java @@ -1,4 +1,4 @@ -/** +/* * BSD-style license; for more info see http://pmd.sourceforge.net/license.html */ @@ -9,11 +9,16 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import java.io.File; +import java.io.FileInputStream; import java.io.IOException; import java.io.StringWriter; +import java.nio.charset.Charset; +import java.nio.file.Files; +import java.nio.file.Path; import java.util.Iterator; import java.util.Map; +import org.apache.commons.io.IOUtils; import org.junit.Test; import net.sourceforge.pmd.lang.ast.DummyNode; @@ -196,4 +201,21 @@ public class ReportTest implements ThreadSafeReportListener { renderer.end(); return writer.toString(); } + + public static String renderTempFile(Renderer renderer, Report report, Charset expectedCharset) throws IOException { + Path tempFile = Files.createTempFile("pmd-report-test", null); + String absolutePath = tempFile.toAbsolutePath().toString(); + + renderer.setReportFile(absolutePath); + renderer.start(); + renderer.renderFileReport(report); + renderer.end(); + renderer.flush(); + + try (FileInputStream input = new FileInputStream(absolutePath)) { + return IOUtils.toString(input, expectedCharset); + } finally { + Files.delete(tempFile); + } + } } diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/renderers/XMLRendererTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/renderers/XMLRendererTest.java index 675574e4ac..299795d5b5 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/renderers/XMLRendererTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/renderers/XMLRendererTest.java @@ -6,10 +6,14 @@ package net.sourceforge.pmd.renderers; import java.io.File; import java.io.StringReader; +import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; import javax.xml.parsers.DocumentBuilderFactory; import org.junit.Assert; +import org.junit.Rule; import org.junit.Test; +import org.junit.contrib.java.lang.system.RestoreSystemProperties; import org.w3c.dom.Document; import org.w3c.dom.NodeList; import org.xml.sax.InputSource; @@ -28,6 +32,8 @@ import net.sourceforge.pmd.lang.ast.Node; import net.sourceforge.pmd.lang.rule.ParametricRuleViolation; public class XMLRendererTest extends AbstractRendererTest { + @Rule // Restores system properties after test + public final RestoreSystemProperties restoreSystemProperties = new RestoreSystemProperties(); @Override public Renderer getRenderer() { @@ -90,12 +96,13 @@ public class XMLRendererTest extends AbstractRendererTest { return new ParametricRuleViolation(new FooRule(), ctx, node, description); } - private void verifyXmlEscaping(Renderer renderer, String shouldContain) throws Exception { + private void verifyXmlEscaping(Renderer renderer, String shouldContain, Charset charset) throws Exception { + renderer.setProperty(XMLRenderer.ENCODING, charset.name()); Report report = new Report(); String surrogatePair = "\ud801\udc1c"; - String msg = "The String literal \"Tokenizer " + surrogatePair + "\" appears..."; + String msg = "The String literal \"Tokénizer " + surrogatePair + "\" appears..."; report.addRuleViolation(createRuleViolation(msg)); - String actual = ReportTest.render(renderer, report); + String actual = ReportTest.renderTempFile(renderer, report, charset); Assert.assertTrue(actual.contains(shouldContain)); Document doc = DocumentBuilderFactory.newInstance().newDocumentBuilder() .parse(new InputSource(new StringReader(actual))); @@ -107,15 +114,13 @@ public class XMLRendererTest extends AbstractRendererTest { @Test public void testXMLEscapingWithUTF8() throws Exception { Renderer renderer = getRenderer(); - renderer.setProperty(XMLRenderer.ENCODING, "UTF-8"); - verifyXmlEscaping(renderer, "\ud801\udc1c"); + verifyXmlEscaping(renderer, "\ud801\udc1c", StandardCharsets.UTF_8); } @Test public void testXMLEscapingWithoutUTF8() throws Exception { Renderer renderer = getRenderer(); - renderer.setProperty(XMLRenderer.ENCODING, "ISO-8859-1"); - verifyXmlEscaping(renderer, "𐐜"); + verifyXmlEscaping(renderer, "𐐜", StandardCharsets.ISO_8859_1); } public String getHeader() { @@ -125,4 +130,26 @@ public class XMLRendererTest extends AbstractRendererTest { + " xsi:schemaLocation=\"http://pmd.sourceforge.net/report/2.0.0 http://pmd.sourceforge.net/report_2_0_0.xsd\"" + PMD.EOL + " version=\"" + PMDVersion.VERSION + "\" timestamp=\"2014-10-06T19:30:51.262\">" + PMD.EOL; } + + @Test + public void testCorrectCharset() throws Exception { + System.setProperty("file.encoding", StandardCharsets.ISO_8859_1.name()); + + Renderer renderer = getRenderer(); + + Report report = new Report(); + String formFeed = "\u000C"; + String specialChar = "é"; + String originalChars = formFeed + specialChar; // u000C should be removed, é should be encoded correctly as UTF-8 + String msg = "The String literal \"" + originalChars + "\" appears..."; + report.addRuleViolation(createRuleViolation(msg)); + String actual = ReportTest.renderTempFile(renderer, report, StandardCharsets.UTF_8); + Assert.assertTrue(actual.contains(specialChar)); + Assert.assertFalse(actual.contains(formFeed)); + Document doc = DocumentBuilderFactory.newInstance().newDocumentBuilder() + .parse(new InputSource(new StringReader(actual))); + NodeList violations = doc.getElementsByTagName("violation"); + Assert.assertEquals(1, violations.getLength()); + Assert.assertEquals(msg.replaceAll(formFeed, ""), violations.item(0).getTextContent().trim()); + } } diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/ant/PMDTaskTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/ant/PMDTaskTest.java index 2857e65958..3250e895eb 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/ant/PMDTaskTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/ant/PMDTaskTest.java @@ -7,6 +7,7 @@ package net.sourceforge.pmd.ant; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import java.io.UnsupportedEncodingException; import java.lang.reflect.Field; import java.nio.charset.Charset; import java.util.Locale; @@ -149,14 +150,28 @@ public class PMDTaskTest extends AbstractAntTestHelper { assertTrue(report.contains("unusedVariableWithÜmlaut")); } + private static String convert(String report) { + // reinterpret output as cp1252 - ant BuildFileRule can only unicode + StringBuilder sb = new StringBuilder(report.length()); + for (int i = 0; i < report.length(); i++) { + char c = report.charAt(i); + if (c > 0x7f) { + sb.append((char) (c & 0xff)); + } else { + sb.append(c); + } + } + return sb.toString(); + } + @Test - public void testFormatterEncodingWithXMLConsole() { + public void testFormatterEncodingWithXMLConsole() throws UnsupportedEncodingException { setDefaultCharset("cp1252"); executeTarget("testFormatterEncodingWithXMLConsole"); - String report = buildRule.getOutput(); + String report = convert(buildRule.getOutput()); assertTrue(report.startsWith("")); - assertTrue(report.contains("unusedVariableWithÜmlaut")); + assertTrue(report.contains("unusedVariableWithÜmlaut")); } @Test diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/cpd/CPDCommandLineInterfaceTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/cpd/CPDCommandLineInterfaceTest.java index 6150151d61..dad635c5d5 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/cpd/CPDCommandLineInterfaceTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/cpd/CPDCommandLineInterfaceTest.java @@ -77,7 +77,7 @@ public class CPDCommandLineInterfaceTest extends BaseCPDCLITest { String out = getOutput(); Assert.assertTrue(out.startsWith("")); - Assert.assertTrue(Pattern.compile("System\\.out\\.println\\([ij] \\+ \"ä\"\\);").matcher(out).find()); + Assert.assertTrue(Pattern.compile("System\\.out\\.println\\([ij] \\+ "ä"\\);").matcher(out).find()); Assert.assertEquals(4, Integer.parseInt(System.getProperty(CPDCommandLineInterface.STATUS_CODE_PROPERTY))); } From 60f905b0ef01843dde9d43e6ee5cb80b5b6b6a3c Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sun, 5 Jul 2020 14:46:04 +0200 Subject: [PATCH 04/19] [doc] Update release notes, fixes #2615 --- docs/pages/release_notes.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index c1806b1dab..5ca75e6812 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -18,6 +18,8 @@ This is a {{ site.pmd.release_type }} release. * apex-bestpractices * [#2626](https://github.com/pmd/pmd/issues/2626): \[apex] UnusedLocalVariable - false positive on case insensitivity allowed in Apex +* core + * [#2615](https://github.com/pmd/pmd/issues/2615): \[core] PMD/CPD produces invalid XML (insufficient escaping/wrong encoding) ### API Changes From 0bd9b1db6b5bf2b11859ab2114e8398e2a7d507a Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Sun, 5 Jul 2020 22:10:14 +0200 Subject: [PATCH 05/19] [core] XMLRenderer - avoid unmappable characters for non UTF-8 encodings --- .../main/java/net/sourceforge/pmd/cpd/XMLRenderer.java | 7 +++++++ .../net/sourceforge/pmd/renderers/XMLRenderer.java | 2 ++ .../net/sourceforge/pmd/renderers/XMLRendererTest.java | 10 ++++++---- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/cpd/XMLRenderer.java b/pmd-core/src/main/java/net/sourceforge/pmd/cpd/XMLRenderer.java index 3cb2ead41b..1417d281ca 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/cpd/XMLRenderer.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/cpd/XMLRenderer.java @@ -121,6 +121,13 @@ public final class XMLRenderer implements Renderer, CPDRenderer { mark = iterator.next(); final Element file = doc.createElement("file"); file.setAttribute("line", String.valueOf(mark.getBeginLine())); + /* + * StringEscapeUtils escape basic chars like "<" and remove invalid chars like U+000C. + * But the DOM impl already escapes "<" by itself in attribute values, but doesn't remove + * invalid chars. In order to avoid double escaping, this escapes it once with StringEscapeUtils + * and unescapes it again - result is, that only invalid chars are removed (no escaping). + * Escaping is left to the DOM impl. + */ String filenameXml10 = StringEscapeUtils.unescapeXml(StringEscapeUtils.escapeXml10(mark.getFilename())); file.setAttribute("path", filenameXml10); file.setAttribute("endline", String.valueOf(mark.getEndLine())); diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XMLRenderer.java b/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XMLRenderer.java index 3e49814b43..2f7c74d7da 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XMLRenderer.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XMLRenderer.java @@ -210,6 +210,8 @@ public class XMLRenderer extends AbstractIncrementingRenderer { char low = sb.charAt(i + 1); int codepoint = Character.toCodePoint(c, low); sb.replace(i, i + 2, "&#x" + Integer.toHexString(codepoint) + ";"); + } else if (c > 0xff) { + sb.replace(i, i + 1, "&#x" + Integer.toHexString((int) c) + ";"); } } result = sb.toString(); diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/renderers/XMLRendererTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/renderers/XMLRendererTest.java index 299795d5b5..e3f38e8b8c 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/renderers/XMLRendererTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/renderers/XMLRendererTest.java @@ -100,7 +100,7 @@ public class XMLRendererTest extends AbstractRendererTest { renderer.setProperty(XMLRenderer.ENCODING, charset.name()); Report report = new Report(); String surrogatePair = "\ud801\udc1c"; - String msg = "The String literal \"Tokénizer " + surrogatePair + "\" appears..."; + String msg = "The String 'literal' \"TokénizĀr " + surrogatePair + "\" appears..."; report.addRuleViolation(createRuleViolation(msg)); String actual = ReportTest.renderTempFile(renderer, report, charset); Assert.assertTrue(actual.contains(shouldContain)); @@ -139,12 +139,14 @@ public class XMLRendererTest extends AbstractRendererTest { Report report = new Report(); String formFeed = "\u000C"; - String specialChar = "é"; - String originalChars = formFeed + specialChar; // u000C should be removed, é should be encoded correctly as UTF-8 + // é = U+00E9 : can be represented in ISO-8859-1 as is + // Ā = U+0100 : cannot be represented in ISO-8859-1 -> would be a unmappable character, needs to be escaped + String specialChars = "éĀ"; + String originalChars = formFeed + specialChars; // u000C should be removed, é should be encoded correctly as UTF-8 String msg = "The String literal \"" + originalChars + "\" appears..."; report.addRuleViolation(createRuleViolation(msg)); String actual = ReportTest.renderTempFile(renderer, report, StandardCharsets.UTF_8); - Assert.assertTrue(actual.contains(specialChar)); + Assert.assertTrue(actual.contains(specialChars)); Assert.assertFalse(actual.contains(formFeed)); Document doc = DocumentBuilderFactory.newInstance().newDocumentBuilder() .parse(new InputSource(new StringReader(actual))); From 53c581b69a0ff78959f97ae29e5a8afb15f4ef9b Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Mon, 6 Jul 2020 21:44:47 +0200 Subject: [PATCH 06/19] Update pmd-regression-tester --- Gemfile | 4 ++-- Gemfile.lock | 25 +++++++++++++++---------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/Gemfile b/Gemfile index ce6c6e2b08..8d17050419 100644 --- a/Gemfile +++ b/Gemfile @@ -1,9 +1,9 @@ source 'https://rubygems.org/' # bleeding edge from git -#gem 'pmdtester', :git => 'https://github.com/pmd/pmd-regression-tester.git' +gem 'pmdtester', :git => 'https://github.com/pmd/pmd-regression-tester.git' -gem 'pmdtester', '~> 1.0' +#gem 'pmdtester', '~> 1.0' gem 'danger', '~> 5.6', '>= 5.6' # This group is only needed for rendering release notes diff --git a/Gemfile.lock b/Gemfile.lock index a95b9119a8..8a3f6d0e2e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,3 +1,13 @@ +GIT + remote: https://github.com/pmd/pmd-regression-tester.git + revision: fa39704981b07ab732ce54ebc6a4578e1865399b + specs: + pmdtester (1.1.0.pre.SNAPSHOT) + differ (~> 0.1) + nokogiri (~> 1.8) + rufus-scheduler (~> 3.5) + slop (~> 4.6) + GEM remote: https://rubygems.org/ specs: @@ -31,9 +41,9 @@ GEM multipart-post (>= 1.2, < 3) faraday-http-cache (1.3.1) faraday (~> 0.8) - fugit (1.3.5) + fugit (1.3.6) et-orbi (~> 1.1, >= 1.1.8) - raabro (~> 1.1) + raabro (~> 1.3) git (1.7.0) rchardet (~> 1.8) kramdown (1.17.0) @@ -42,21 +52,16 @@ GEM multipart-post (2.1.1) nap (1.1.0) no_proxy_fix (0.1.2) - nokogiri (1.10.9) + nokogiri (1.10.10) mini_portile2 (~> 2.4.0) octokit (4.18.0) faraday (>= 0.9) sawyer (~> 0.8.0, >= 0.5.3) open4 (1.3.4) - pmdtester (1.0.0) - differ (~> 0.1) - nokogiri (~> 1.8) - rufus-scheduler (~> 3.5) - slop (~> 4.6) public_suffix (4.0.5) raabro (1.3.1) rchardet (1.8.0) - rouge (3.19.0) + rouge (3.20.0) rufus-scheduler (3.6.0) fugit (~> 1.1, >= 1.1.6) safe_yaml (1.0.5) @@ -76,7 +81,7 @@ PLATFORMS DEPENDENCIES danger (~> 5.6, >= 5.6) liquid (>= 4.0.0) - pmdtester (~> 1.0) + pmdtester! rouge (>= 1.7, < 4) safe_yaml (>= 1.0) From bb97b693f226c5ddc13c5b8acb274d497f3bbfdd Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Wed, 8 Jul 2020 20:29:26 +0200 Subject: [PATCH 07/19] Update pmd-regression-tester 1.0.1 --- Gemfile | 4 ++-- Gemfile.lock | 17 ++++++----------- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/Gemfile b/Gemfile index 8d17050419..ce6c6e2b08 100644 --- a/Gemfile +++ b/Gemfile @@ -1,9 +1,9 @@ source 'https://rubygems.org/' # bleeding edge from git -gem 'pmdtester', :git => 'https://github.com/pmd/pmd-regression-tester.git' +#gem 'pmdtester', :git => 'https://github.com/pmd/pmd-regression-tester.git' -#gem 'pmdtester', '~> 1.0' +gem 'pmdtester', '~> 1.0' gem 'danger', '~> 5.6', '>= 5.6' # This group is only needed for rendering release notes diff --git a/Gemfile.lock b/Gemfile.lock index 8a3f6d0e2e..28d16589fd 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,13 +1,3 @@ -GIT - remote: https://github.com/pmd/pmd-regression-tester.git - revision: fa39704981b07ab732ce54ebc6a4578e1865399b - specs: - pmdtester (1.1.0.pre.SNAPSHOT) - differ (~> 0.1) - nokogiri (~> 1.8) - rufus-scheduler (~> 3.5) - slop (~> 4.6) - GEM remote: https://rubygems.org/ specs: @@ -58,6 +48,11 @@ GEM faraday (>= 0.9) sawyer (~> 0.8.0, >= 0.5.3) open4 (1.3.4) + pmdtester (1.0.1) + differ (~> 0.1) + nokogiri (~> 1.8) + rufus-scheduler (~> 3.5) + slop (~> 4.6) public_suffix (4.0.5) raabro (1.3.1) rchardet (1.8.0) @@ -81,7 +76,7 @@ PLATFORMS DEPENDENCIES danger (~> 5.6, >= 5.6) liquid (>= 4.0.0) - pmdtester! + pmdtester (~> 1.0) rouge (>= 1.7, < 4) safe_yaml (>= 1.0) From 2e006697e0aac3594de2d0df618d0db0e2c5534e Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 9 Jul 2020 10:56:44 +0200 Subject: [PATCH 08/19] [core] Refactor XMLRenderer to use XMLStreamWriter In order to properly support different encodings, a OutputStream is needed. Then Java will take care of unmappaple characters and encode them as entities for XML. For backwards compatibility, a writer is still created and exposed. --- .../pmd/renderers/XMLRenderer.java | 312 ++++++++++-------- .../pmd/renderers/XSLTRenderer.java | 40 +-- .../pmd/renderers/XMLRendererTest.java | 8 +- .../net/sourceforge/pmd/ant/PMDTaskTest.java | 13 +- 4 files changed, 200 insertions(+), 173 deletions(-) diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XMLRenderer.java b/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XMLRenderer.java index 2f7c74d7da..23ba8fd334 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XMLRenderer.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XMLRenderer.java @@ -6,16 +6,23 @@ package net.sourceforge.pmd.renderers; import java.io.File; import java.io.IOException; +import java.io.OutputStream; import java.io.OutputStreamWriter; -import java.nio.charset.Charset; -import java.nio.charset.UnsupportedCharsetException; +import java.io.UnsupportedEncodingException; +import java.io.Writer; import java.nio.file.Files; import java.text.SimpleDateFormat; import java.util.Date; import java.util.Iterator; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import javax.xml.XMLConstants; +import javax.xml.stream.XMLOutputFactory; +import javax.xml.stream.XMLStreamException; +import javax.xml.stream.XMLStreamWriter; +import org.apache.commons.io.output.WriterOutputStream; import org.apache.commons.lang3.StringUtils; -import org.apache.commons.text.StringEscapeUtils; import net.sourceforge.pmd.PMD; import net.sourceforge.pmd.PMDVersion; @@ -34,6 +41,13 @@ public class XMLRenderer extends AbstractIncrementingRenderer { public static final StringProperty ENCODING = new StringProperty("encoding", "XML encoding format, defaults to UTF-8.", "UTF-8", 0); + private static final String PMD_REPORT_NS_URI = "http://pmd.sourceforge.net/report/2.0.0"; + private static final String PMD_REPORT_NS_LOCATION = "http://pmd.sourceforge.net/report_2_0_0.xsd"; + private static final String XSI_NS_PREFIX = "xsi"; + + private XMLStreamWriter xmlWriter; + private OutputStream stream; + public XMLRenderer() { super(NAME, "XML format."); definePropertyDescriptor(ENCODING); @@ -53,170 +67,204 @@ public class XMLRenderer extends AbstractIncrementingRenderer { public void start() throws IOException { String encoding = getProperty(ENCODING); - StringBuilder buf = new StringBuilder(500); - buf.append("").append(PMD.EOL); - createVersionAttr(buf); - createTimestampAttr(buf); - // FIXME: elapsed time not available until the end of the processing - // buf.append(createTimeElapsedAttr(report)); - buf.append('>').append(PMD.EOL); - writer.write(buf.toString()); + try { + xmlWriter.writeStartDocument(encoding, "1.0"); + xmlWriter.writeCharacters(PMD.EOL); + xmlWriter.setDefaultNamespace(PMD_REPORT_NS_URI); + xmlWriter.writeStartElement(PMD_REPORT_NS_URI, "pmd"); + xmlWriter.writeDefaultNamespace(PMD_REPORT_NS_URI); + xmlWriter.writeNamespace(XSI_NS_PREFIX, XMLConstants.W3C_XML_SCHEMA_INSTANCE_NS_URI); + xmlWriter.writeAttribute(XSI_NS_PREFIX, XMLConstants.W3C_XML_SCHEMA_INSTANCE_NS_URI, "schemaLocation", + PMD_REPORT_NS_URI + " " + PMD_REPORT_NS_LOCATION); + xmlWriter.writeAttribute("version", PMDVersion.VERSION); + xmlWriter.writeAttribute("timestamp", new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSS").format(new Date())); + // FIXME: elapsed time not available until the end of the processing + // xmlWriter.writeAttribute("time_elapsed", ...); + } catch (XMLStreamException e) { + throw new IOException(e); + } } @Override public void renderFileViolations(Iterator violations) throws IOException { - StringBuilder buf = new StringBuilder(500); String filename = null; - // rule violations - while (violations.hasNext()) { - buf.setLength(0); - RuleViolation rv = violations.next(); - String nextFilename = determineFileName(rv.getFilename()); - if (!nextFilename.equals(filename)) { - // New File - if (filename != null) { - // Not first file ? - buf.append("").append(PMD.EOL); + try { + // rule violations + while (violations.hasNext()) { + RuleViolation rv = violations.next(); + String nextFilename = determineFileName(rv.getFilename()); + if (!nextFilename.equals(filename)) { + // New File + if (filename != null) { + // Not first file ? + xmlWriter.writeEndElement(); + } + filename = nextFilename; + xmlWriter.writeCharacters(PMD.EOL); + xmlWriter.writeStartElement("file"); + xmlWriter.writeAttribute("name", filename); + xmlWriter.writeCharacters(PMD.EOL); } - filename = nextFilename; - buf.append("").append(PMD.EOL); + + xmlWriter.writeStartElement("violation"); + xmlWriter.writeAttribute("beginline", String.valueOf(rv.getBeginLine())); + xmlWriter.writeAttribute("endline", String.valueOf(rv.getEndLine())); + xmlWriter.writeAttribute("begincolumn", String.valueOf(rv.getBeginColumn())); + xmlWriter.writeAttribute("endcolumn", String.valueOf(rv.getEndColumn())); + xmlWriter.writeAttribute("rule", rv.getRule().getName()); + xmlWriter.writeAttribute("ruleset", rv.getRule().getRuleSetName()); + maybeAdd("package", rv.getPackageName()); + maybeAdd("class", rv.getClassName()); + maybeAdd("method", rv.getMethodName()); + maybeAdd("variable", rv.getVariableName()); + maybeAdd("externalInfoUrl", rv.getRule().getExternalInfoUrl()); + xmlWriter.writeAttribute("priority", String.valueOf(rv.getRule().getPriority().getPriority())); + xmlWriter.writeCharacters(PMD.EOL); + xmlWriter.writeCharacters(removeInvalidCharacters(rv.getDescription())); + xmlWriter.writeCharacters(PMD.EOL); + xmlWriter.writeEndElement(); + xmlWriter.writeCharacters(PMD.EOL); } - - buf.append("").append(PMD.EOL); - buf.append(escape(rv.getDescription())); - - buf.append(PMD.EOL); - buf.append(""); - buf.append(PMD.EOL); - writer.write(buf.toString()); - } - if (filename != null) { // Not first file ? - writer.write(""); - writer.write(PMD.EOL); + if (filename != null) { // Not first file ? + xmlWriter.writeEndElement(); + } + } catch (XMLStreamException e) { + throw new IOException(e); } } @Override public void end() throws IOException { - StringBuilder buf = new StringBuilder(500); - // errors - for (Report.ProcessingError pe : errors) { - buf.setLength(0); - buf.append("").append(PMD.EOL); - buf.append("").append(PMD.EOL); - buf.append("").append(PMD.EOL); - writer.write(buf.toString()); - } - - // suppressed violations - if (showSuppressedViolations) { - for (Report.SuppressedViolation s : suppressed) { - buf.setLength(0); - buf.append("").append(PMD.EOL); - writer.write(buf.toString()); + try { + // errors + for (Report.ProcessingError pe : errors) { + xmlWriter.writeCharacters(PMD.EOL); + xmlWriter.writeStartElement("error"); + xmlWriter.writeAttribute("filename", determineFileName(pe.getFile())); + xmlWriter.writeAttribute("msg", pe.getMsg()); + xmlWriter.writeCharacters(PMD.EOL); + xmlWriter.writeCData(pe.getDetail()); + xmlWriter.writeCharacters(PMD.EOL); + xmlWriter.writeEndElement(); } - } - // config errors - for (final Report.ConfigurationError ce : configErrors) { - buf.setLength(0); - buf.append("").append(PMD.EOL); - writer.write(buf.toString()); - } + // suppressed violations + if (showSuppressedViolations) { + for (Report.SuppressedViolation s : suppressed) { + xmlWriter.writeCharacters(PMD.EOL); + xmlWriter.writeStartElement("suppressedviolation"); + xmlWriter.writeAttribute("filename", determineFileName(s.getRuleViolation().getFilename())); + xmlWriter.writeAttribute("suppressiontype", s.suppressedByNOPMD() ? "nopmd" : "annotation"); + xmlWriter.writeAttribute("msg", s.getRuleViolation().getDescription()); + xmlWriter.writeAttribute("usermsg", s.getUserMessage() == null ? "" : s.getUserMessage()); + xmlWriter.writeEndElement(); + } + } - writer.write("" + PMD.EOL); + // config errors + for (final Report.ConfigurationError ce : configErrors) { + xmlWriter.writeCharacters(PMD.EOL); + xmlWriter.writeEmptyElement("configerror"); + xmlWriter.writeAttribute("rule", ce.rule().getName()); + xmlWriter.writeAttribute("msg", ce.issue()); + } + xmlWriter.writeCharacters(PMD.EOL); + xmlWriter.writeEndElement(); // + xmlWriter.writeCharacters(PMD.EOL); + xmlWriter.flush(); + } catch (XMLStreamException e) { + throw new IOException(e); + } } - private void maybeAdd(String attr, String value, StringBuilder buf) { + private void maybeAdd(String attr, String value) throws XMLStreamException { if (value != null && value.length() > 0) { - buf.append(' ').append(attr).append("=\""); - buf.append(escape(value)); - buf.append('"'); + xmlWriter.writeAttribute(attr, value); } } - private void createVersionAttr(StringBuilder buffer) { - buffer.append("Allowed characters are: + *

+ * Char ::= #x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD] | [#x10000-#x10FFFF] + * // any Unicode character, excluding the surrogate blocks, FFFE, and FFFF. + *
+ * (see Extensible Markup Language (XML) 1.0 (Fifth Edition)). */ - private String escape(String text) { - String result = StringEscapeUtils.escapeXml10(text); + private String removeInvalidCharacters(String text) { + Pattern pattern = Pattern.compile( + "\\x00|\\x01|\\x02|\\x03|\\x04|\\x05|\\x06|\\x07|\\x08|" + + "\\x0b|\\x0c|\\x0e|\\x0f|" + + "\\x10|\\x11|\\x12|\\x13|\\x14|\\x15|\\x16|\\x17|\\x18|" + + "\\x19|\\x1a|\\x1b|\\x1c|\\x1d|\\x1e|\\x1f"); + Matcher matcher = pattern.matcher(text); + return matcher.replaceAll(""); + } + + @Override + public void setWriter(final Writer writer) { String encoding = getProperty(ENCODING); - if (!"UTF-8".equalsIgnoreCase(encoding)) { - StringBuilder sb = new StringBuilder(result); - for (int i = 0; i < sb.length(); i++) { - char c = sb.charAt(i); - // surrogate characters are not allowed in XML - if (Character.isHighSurrogate(c)) { - char low = sb.charAt(i + 1); - int codepoint = Character.toCodePoint(c, low); - sb.replace(i, i + 2, "&#x" + Integer.toHexString(codepoint) + ";"); - } else if (c > 0xff) { - sb.replace(i, i + 1, "&#x" + Integer.toHexString((int) c) + ";"); - } - } - result = sb.toString(); + // for backwards compatibility, create a OutputStream that writes to the writer. + this.stream = new WriterOutputStream(writer, encoding); + + XMLOutputFactory outputFactory = XMLOutputFactory.newFactory(); + try { + this.xmlWriter = outputFactory.createXMLStreamWriter(this.stream, encoding); + // for backwards compatibility, also provide a writer. + // Note: both XMLStreamWriter and this writer will write to this.stream + this.writer = new WrappedOutputStreamWriter(xmlWriter, stream, encoding); + } catch (XMLStreamException | UnsupportedEncodingException e) { + throw new RuntimeException(e); + } + } + + private static class WrappedOutputStreamWriter extends OutputStreamWriter { + private final XMLStreamWriter xmlWriter; + + WrappedOutputStreamWriter(XMLStreamWriter xmlWriter, OutputStream out, String charset) throws UnsupportedEncodingException { + super(out, charset); + this.xmlWriter = xmlWriter; + } + + @Override + public void flush() throws IOException { + try { + xmlWriter.flush(); + } catch (XMLStreamException e) { + throw new IOException(e); + } + super.flush(); + } + + @Override + public void close() throws IOException { + try { + xmlWriter.close(); + } catch (XMLStreamException e) { + throw new IOException(e); + } + super.close(); } - return result; } // FIXME: elapsed time not available until the end of the processing diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XSLTRenderer.java b/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XSLTRenderer.java index 28a3bd2132..b9f6ce6791 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XSLTRenderer.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XSLTRenderer.java @@ -45,6 +45,7 @@ public class XSLTRenderer extends XMLRenderer { private Transformer transformer; private String xsltFilename = "/pmd-nicerhtml.xsl"; private Writer outputWriter; + private StringWriter stringWriter; public XSLTRenderer() { super(); @@ -71,8 +72,8 @@ public class XSLTRenderer extends XMLRenderer { // We keep the inital writer to put the final html output this.outputWriter = getWriter(); // We use a new one to store the XML... - Writer w = new StringWriter(); - setWriter(w); + this.stringWriter = new StringWriter(); + setWriter(stringWriter); // If don't find the xsl no need to bother doing the all report, // so we check this here... InputStream xslt = null; @@ -100,16 +101,14 @@ public class XSLTRenderer extends XMLRenderer { * The stylesheet provided as an InputStream */ private void prepareTransformer(InputStream xslt) { - if (xslt != null) { - try { - // Get a TransformerFactory object - TransformerFactory factory = TransformerFactory.newInstance(); - StreamSource src = new StreamSource(xslt); - // Get an XSL Transformer object - this.transformer = factory.newTransformer(src); - } catch (TransformerConfigurationException e) { - e.printStackTrace(); - } + try { + // Get a TransformerFactory object + TransformerFactory factory = TransformerFactory.newInstance(); + StreamSource src = new StreamSource(xslt); + // Get an XSL Transformer object + this.transformer = factory.newTransformer(src); + } catch (TransformerConfigurationException e) { + throw new RuntimeException(e); } } @@ -118,25 +117,17 @@ public class XSLTRenderer extends XMLRenderer { // First we finish the XML report super.end(); // Now we transform it using XSLT - if (writer instanceof StringWriter) { - StringWriter w = (StringWriter) writer; - Document doc = this.getDocument(w.toString()); - this.transform(doc); - } else { - // Should not happen ! - throw new RuntimeException("Wrong writer"); - } - + Document doc = this.getDocument(stringWriter.toString()); + this.transform(doc); } private void transform(Document doc) { DOMSource source = new DOMSource(doc); - this.setWriter(new StringWriter()); StreamResult result = new StreamResult(this.outputWriter); try { transformer.transform(source, result); } catch (TransformerException e) { - e.printStackTrace(); + throw new RuntimeException(e); } } @@ -145,8 +136,7 @@ public class XSLTRenderer extends XMLRenderer { DocumentBuilder parser = DocumentBuilderFactory.newInstance().newDocumentBuilder(); return parser.parse(new InputSource(new StringReader(xml))); } catch (ParserConfigurationException | SAXException | IOException e) { - e.printStackTrace(); + throw new RuntimeException(e); } - return null; } } diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/renderers/XMLRendererTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/renderers/XMLRendererTest.java index e3f38e8b8c..390d3a812e 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/renderers/XMLRendererTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/renderers/XMLRendererTest.java @@ -125,10 +125,10 @@ public class XMLRendererTest extends AbstractRendererTest { public String getHeader() { return "" + PMD.EOL - + "" + PMD.EOL; + + "" + PMD.EOL; } @Test diff --git a/pmd-java/src/test/java/net/sourceforge/pmd/ant/PMDTaskTest.java b/pmd-java/src/test/java/net/sourceforge/pmd/ant/PMDTaskTest.java index 3250e895eb..82802e409c 100644 --- a/pmd-java/src/test/java/net/sourceforge/pmd/ant/PMDTaskTest.java +++ b/pmd-java/src/test/java/net/sourceforge/pmd/ant/PMDTaskTest.java @@ -8,10 +8,8 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import java.io.UnsupportedEncodingException; -import java.lang.reflect.Field; import java.nio.charset.Charset; import java.util.Locale; -import java.util.Objects; import org.apache.commons.io.FileUtils; import org.junit.Rule; @@ -112,17 +110,8 @@ public class PMDTaskTest extends AbstractAntTestHelper { } }; - // See http://stackoverflow.com/questions/361975/setting-the-default-java-character-encoding and http://stackoverflow.com/a/14987992/1169968 private static void setDefaultCharset(String charsetName) { - try { - System.setProperty("file.encoding", charsetName); - Field charset = Charset.class.getDeclaredField("defaultCharset"); - charset.setAccessible(true); - charset.set(null, null); - Objects.requireNonNull(Charset.defaultCharset()); - } catch (Exception e) { - throw new RuntimeException(e); - } + System.setProperty("file.encoding", charsetName); } @Rule From e6600ec8560d85df738d5622416563f8e3f2fe7d Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 9 Jul 2020 11:07:06 +0200 Subject: [PATCH 09/19] [core] Provide StringUtil.removeInvalidXml10Characters And use it in both CPD/PMD XMLRenderers. --- .../net/sourceforge/pmd/cpd/XMLRenderer.java | 11 +++----- .../pmd/renderers/XMLRenderer.java | 25 ++---------------- .../net/sourceforge/pmd/util/StringUtil.java | 26 ++++++++++++++++++- .../sourceforge/pmd/util/StringUtilTest.java | 12 --------- 4 files changed, 30 insertions(+), 44 deletions(-) diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/cpd/XMLRenderer.java b/pmd-core/src/main/java/net/sourceforge/pmd/cpd/XMLRenderer.java index 1417d281ca..5d8ee511d7 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/cpd/XMLRenderer.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/cpd/XMLRenderer.java @@ -23,6 +23,7 @@ import org.w3c.dom.Document; import org.w3c.dom.Element; import net.sourceforge.pmd.cpd.renderer.CPDRenderer; +import net.sourceforge.pmd.util.StringUtil; /** * @author Philippe T'Seyen - original implementation @@ -121,14 +122,8 @@ public final class XMLRenderer implements Renderer, CPDRenderer { mark = iterator.next(); final Element file = doc.createElement("file"); file.setAttribute("line", String.valueOf(mark.getBeginLine())); - /* - * StringEscapeUtils escape basic chars like "<" and remove invalid chars like U+000C. - * But the DOM impl already escapes "<" by itself in attribute values, but doesn't remove - * invalid chars. In order to avoid double escaping, this escapes it once with StringEscapeUtils - * and unescapes it again - result is, that only invalid chars are removed (no escaping). - * Escaping is left to the DOM impl. - */ - String filenameXml10 = StringEscapeUtils.unescapeXml(StringEscapeUtils.escapeXml10(mark.getFilename())); + // only remove invalid characters, escaping is done by the DOM impl. + String filenameXml10 = StringUtil.removedInvalidXml10Characters(mark.getFilename()); file.setAttribute("path", filenameXml10); file.setAttribute("endline", String.valueOf(mark.getEndLine())); final int beginCol = mark.getBeginColumn(); diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XMLRenderer.java b/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XMLRenderer.java index 23ba8fd334..aba0dd8265 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XMLRenderer.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/renderers/XMLRenderer.java @@ -14,8 +14,6 @@ import java.nio.file.Files; import java.text.SimpleDateFormat; import java.util.Date; import java.util.Iterator; -import java.util.regex.Matcher; -import java.util.regex.Pattern; import javax.xml.XMLConstants; import javax.xml.stream.XMLOutputFactory; import javax.xml.stream.XMLStreamException; @@ -29,6 +27,7 @@ import net.sourceforge.pmd.PMDVersion; import net.sourceforge.pmd.Report; import net.sourceforge.pmd.RuleViolation; import net.sourceforge.pmd.properties.StringProperty; +import net.sourceforge.pmd.util.StringUtil; /** * Renderer to XML format. @@ -121,7 +120,7 @@ public class XMLRenderer extends AbstractIncrementingRenderer { maybeAdd("externalInfoUrl", rv.getRule().getExternalInfoUrl()); xmlWriter.writeAttribute("priority", String.valueOf(rv.getRule().getPriority().getPriority())); xmlWriter.writeCharacters(PMD.EOL); - xmlWriter.writeCharacters(removeInvalidCharacters(rv.getDescription())); + xmlWriter.writeCharacters(StringUtil.removedInvalidXml10Characters(rv.getDescription())); xmlWriter.writeCharacters(PMD.EOL); xmlWriter.writeEndElement(); xmlWriter.writeCharacters(PMD.EOL); @@ -201,26 +200,6 @@ public class XMLRenderer extends AbstractIncrementingRenderer { } } - /** - * Remove characters, that are not allowed in XML 1.0 documents. - * - *

Allowed characters are: - *

- * Char ::= #x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD] | [#x10000-#x10FFFF] - * // any Unicode character, excluding the surrogate blocks, FFFE, and FFFF. - *
- * (see Extensible Markup Language (XML) 1.0 (Fifth Edition)). - */ - private String removeInvalidCharacters(String text) { - Pattern pattern = Pattern.compile( - "\\x00|\\x01|\\x02|\\x03|\\x04|\\x05|\\x06|\\x07|\\x08|" - + "\\x0b|\\x0c|\\x0e|\\x0f|" - + "\\x10|\\x11|\\x12|\\x13|\\x14|\\x15|\\x16|\\x17|\\x18|" - + "\\x19|\\x1a|\\x1b|\\x1c|\\x1d|\\x1e|\\x1f"); - Matcher matcher = pattern.matcher(text); - return matcher.replaceAll(""); - } - @Override public void setWriter(final Writer writer) { String encoding = getProperty(ENCODING); diff --git a/pmd-core/src/main/java/net/sourceforge/pmd/util/StringUtil.java b/pmd-core/src/main/java/net/sourceforge/pmd/util/StringUtil.java index fcb8aaaab8..c4f5fa1422 100644 --- a/pmd-core/src/main/java/net/sourceforge/pmd/util/StringUtil.java +++ b/pmd-core/src/main/java/net/sourceforge/pmd/util/StringUtil.java @@ -9,8 +9,11 @@ import java.util.Arrays; import java.util.Iterator; import java.util.List; import java.util.Locale; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import org.apache.commons.lang3.StringUtils; +import org.apache.commons.text.StringEscapeUtils; import net.sourceforge.pmd.annotation.InternalApi; @@ -27,6 +30,12 @@ public final class StringUtil { private static final String[] EMPTY_STRINGS = new String[0]; + private static final Pattern XML_10_INVALID_CHARS = Pattern.compile( + "\\x00|\\x01|\\x02|\\x03|\\x04|\\x05|\\x06|\\x07|\\x08|" + + "\\x0b|\\x0c|\\x0e|\\x0f|" + + "\\x10|\\x11|\\x12|\\x13|\\x14|\\x15|\\x16|\\x17|\\x18|" + + "\\x19|\\x1a|\\x1b|\\x1c|\\x1d|\\x1e|\\x1f"); + private StringUtil() { } @@ -211,8 +220,9 @@ public final class StringUtil { * @param src * @param supportUTF8 override the default setting, whether special characters should be replaced with entities ( * false) or should be included as is ( true). - * + * @deprecated for removal. Use {@link StringEscapeUtils#escapeXml10(String)} instead. */ + @Deprecated public static void appendXmlEscaped(StringBuilder buf, String src, boolean supportUTF8) { char c; int i = 0; @@ -245,6 +255,20 @@ public final class StringUtil { } } + /** + * Remove characters, that are not allowed in XML 1.0 documents. + * + *

Allowed characters are: + *

+ * Char ::= #x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD] | [#x10000-#x10FFFF] + * // any Unicode character, excluding the surrogate blocks, FFFE, and FFFF. + *
+ * (see Extensible Markup Language (XML) 1.0 (Fifth Edition)). + */ + public static String removedInvalidXml10Characters(String text) { + Matcher matcher = XML_10_INVALID_CHARS.matcher(text); + return matcher.replaceAll(""); + } /** * Replace some whitespace characters so they are visually apparent. diff --git a/pmd-core/src/test/java/net/sourceforge/pmd/util/StringUtilTest.java b/pmd-core/src/test/java/net/sourceforge/pmd/util/StringUtilTest.java index 21df9d406e..387928c89e 100644 --- a/pmd-core/src/test/java/net/sourceforge/pmd/util/StringUtilTest.java +++ b/pmd-core/src/test/java/net/sourceforge/pmd/util/StringUtilTest.java @@ -36,14 +36,6 @@ public class StringUtilTest { assertEquals("replaceString didn't work with a char", "f", StringUtil.replaceString("foo", 'o', null)); } - /** - * Usually you would set the system property - * "net.sourceforge.pmd.supportUTF8" to either "no" or "yes", to switch UTF8 - * support. - * - * e.g. - * System.setProperty("net.sourceforge.pmd.supportUTF8","yes"); - */ @Test public void testUTF8NotSupported() { StringBuilder sb = new StringBuilder(); @@ -68,8 +60,4 @@ public class StringUtilTest { StringUtil.appendXmlEscaped(sb, test, true); assertEquals("é", sb.toString()); } - - public static junit.framework.Test suite() { - return new junit.framework.JUnit4TestAdapter(StringUtilTest.class); - } } From a3670e41358253d64e677958952e188324782678 Mon Sep 17 00:00:00 2001 From: Mykhailo Palahuta Date: Thu, 16 Jul 2020 10:29:14 +0300 Subject: [PATCH 10/19] [java] False negative: LiteralsFirstInComparisons for methods returning Strings (2569) --- .../LiteralsFirstInComparisonsRule.java | 189 +++++++++++------- .../xml/LiteralsFirstInComparisons.xml | 13 ++ 2 files changed, 131 insertions(+), 71 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LiteralsFirstInComparisonsRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LiteralsFirstInComparisonsRule.java index 667240f5d8..5074e84937 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LiteralsFirstInComparisonsRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LiteralsFirstInComparisonsRule.java @@ -4,7 +4,8 @@ package net.sourceforge.pmd.lang.java.rule.bestpractices; -import net.sourceforge.pmd.lang.ast.Node; +import java.util.List; + import net.sourceforge.pmd.lang.java.ast.ASTArgumentList; import net.sourceforge.pmd.lang.java.ast.ASTArguments; import net.sourceforge.pmd.lang.java.ast.ASTConditionalAndExpression; @@ -29,63 +30,71 @@ public class LiteralsFirstInComparisonsRule extends AbstractJavaRule { } @Override - public Object visit(ASTPrimaryExpression node, Object data) { - ASTPrimaryPrefix primaryPrefix = node.getFirstChildOfType(ASTPrimaryPrefix.class); - ASTPrimarySuffix primarySuffix = node.getFirstChildOfType(ASTPrimarySuffix.class); - if (primaryPrefix != null && primarySuffix != null) { - ASTName name = primaryPrefix.getFirstChildOfType(ASTName.class); - if (name == null || isIrrelevantImage(name.getImage())) { - return data; - } - if (!isSingleStringLiteralArgument(primarySuffix)) { - return data; - } - if (isWithinNullComparison(node)) { - return data; - } - addViolation(data, node); + public Object visit(ASTPrimaryExpression expression, Object data) { + String opName = getOperationName(expression); + ASTPrimarySuffix primarySuffix = getSuffixOfArguments(expression); + if (opName == null || primarySuffix == null) { + return data; } - return node; + if (isStringLiteralComparison(opName, primarySuffix) && !isWithinNullComparison(expression)) { + addViolation(data, expression); + } + return data; } - private boolean isIrrelevantImage(String image) { + private String getOperationName(ASTPrimaryExpression primaryExpression) { + return isMethodsChain(primaryExpression) ? getOperationNameBySuffix(primaryExpression) + : getOperationNameByPrefix(primaryExpression); + } + + private boolean isMethodsChain(ASTPrimaryExpression primaryExpression) { + return primaryExpression.getNumChildren() > 2; + } + + private String getOperationNameBySuffix(ASTPrimaryExpression primaryExpression) { + ASTPrimarySuffix opAsSuffix = getPrimarySuffixAtIndexFromEnd(primaryExpression, 1); + if (opAsSuffix != null) { + String opName = opAsSuffix.getImage(); // name of pattern "operation" + return "." + opName; + } + return null; + } + + private String getOperationNameByPrefix(ASTPrimaryExpression primaryExpression) { + ASTPrimaryPrefix opAsPrefix = primaryExpression.getFirstChildOfType(ASTPrimaryPrefix.class); + if (opAsPrefix != null) { + ASTName opName = opAsPrefix.getFirstChildOfType(ASTName.class); // name of pattern "*.operation" + return opName != null ? opName.getImage() : null; + } + return null; + } + + private ASTPrimarySuffix getSuffixOfArguments(ASTPrimaryExpression primaryExpression) { + return getPrimarySuffixAtIndexFromEnd(primaryExpression, 0); + } + + private ASTPrimarySuffix getPrimarySuffixAtIndexFromEnd(ASTPrimaryExpression primaryExpression, int indexFromEnd) { + List primarySuffixes = primaryExpression.findChildrenOfType(ASTPrimarySuffix.class); + if (!primarySuffixes.isEmpty()) { + int suffixIndex = primarySuffixes.size() - 1 - indexFromEnd; + return primarySuffixes.get(suffixIndex); + } + return null; + } + + private boolean isStringLiteralComparison(String opName, ASTPrimarySuffix argsSuffix) { + return isComparisonOperation(opName) && isSingleStringLiteralArgument(argsSuffix); + } + + private boolean isComparisonOperation(String op) { for (String comparisonOp : COMPARISON_OPS) { - if (image.endsWith(comparisonOp)) { - return false; - } - } - return true; - } - - private boolean isWithinNullComparison(ASTPrimaryExpression node) { - for (ASTExpression parentExpr : node.getParentsOfType(ASTExpression.class)) { - if (isComparisonWithNull(parentExpr, "==", ASTConditionalOrExpression.class) - || isComparisonWithNull(parentExpr, "!=", ASTConditionalAndExpression.class)) { + if (op.endsWith(comparisonOp)) { return true; } } return false; } - /* - * Expression/ConditionalAndExpression//EqualityExpression(@Image='!=']//NullLiteral - * Expression/ConditionalOrExpression//EqualityExpression(@Image='==']//NullLiteral - */ - private boolean isComparisonWithNull(ASTExpression parentExpr, String equalOperator, Class condition) { - Node condExpr = null; - ASTEqualityExpression eqExpr = null; - if (parentExpr != null) { - condExpr = parentExpr.getFirstChildOfType(condition); - } - if (condExpr != null) { - eqExpr = condExpr.getFirstDescendantOfType(ASTEqualityExpression.class); - } - if (eqExpr != null) { - return eqExpr.hasImageEqualTo(equalOperator) && eqExpr.hasDescendantOfType(ASTNullLiteral.class); - } - return false; - } - /* * This corresponds to the following XPath expression: * (../PrimarySuffix/Arguments/ArgumentList/Expression/PrimaryExpression/PrimaryPrefix/Literal[@StringLiteral= true()]) @@ -93,35 +102,73 @@ public class LiteralsFirstInComparisonsRule extends AbstractJavaRule { * ( count(../PrimarySuffix/Arguments/ArgumentList/Expression) = 1 ) */ private boolean isSingleStringLiteralArgument(ASTPrimarySuffix primarySuffix) { - if (!primarySuffix.isArguments() || primarySuffix.getArgumentCount() != 1) { + return isSingleArgumentSuffix(primarySuffix) && isStringLiteralFirstArgumentOfSuffix(primarySuffix); + } + + private boolean isSingleArgumentSuffix(ASTPrimarySuffix primarySuffix) { + return primarySuffix.getArgumentCount() == 1; + } + + private boolean isStringLiteralFirstArgumentOfSuffix(ASTPrimarySuffix primarySuffix) { + try { + JavaNode firstArg = getFirstArgument(primarySuffix); + return isStringLiteral(firstArg); + } catch (NullPointerException e) { return false; } - Node node = primarySuffix; - node = node.getFirstChildOfType(ASTArguments.class); - if (node != null) { - node = node.getFirstChildOfType(ASTArgumentList.class); - if (node.getNumChildren() != 1) { - return false; - } - } - if (node != null) { - node = node.getFirstChildOfType(ASTExpression.class); - } - if (node != null) { - node = node.getFirstChildOfType(ASTPrimaryExpression.class); - } - if (node != null) { - node = node.getFirstChildOfType(ASTPrimaryPrefix.class); - } - if (node != null) { - node = node.getFirstChildOfType(ASTLiteral.class); - } - if (node != null) { + } + + private JavaNode getFirstArgument(ASTPrimarySuffix primarySuffix) { + ASTArguments arguments = primarySuffix.getFirstChildOfType(ASTArguments.class); + ASTArgumentList argumentList = arguments.getFirstChildOfType(ASTArgumentList.class); + ASTExpression expression = argumentList.getFirstChildOfType(ASTExpression.class); + ASTPrimaryExpression primaryExpression = expression.getFirstChildOfType(ASTPrimaryExpression.class); + ASTPrimaryPrefix primaryPrefix = primaryExpression.getFirstChildOfType(ASTPrimaryPrefix.class); + return primaryPrefix.getFirstChildOfType(ASTLiteral.class); + } + + private boolean isStringLiteral(JavaNode node) { + if (node instanceof ASTLiteral) { ASTLiteral literal = (ASTLiteral) node; - if (literal.isStringLiteral()) { + return literal.isStringLiteral(); + } + return false; + } + + /* + * Expression/ConditionalAndExpression//EqualityExpression(@Image='!=']//NullLiteral + * Expression/ConditionalOrExpression//EqualityExpression(@Image='==']//NullLiteral + */ + private boolean isWithinNullComparison(ASTPrimaryExpression node) { + for (ASTExpression parentExpr : node.getParentsOfType(ASTExpression.class)) { + if (isNullComparison(parentExpr)) { return true; } } return false; } + + private boolean isNullComparison(ASTExpression expression) { + return isAndNotNullComparison(expression) || isOrNullComparison(expression); + } + + private boolean isAndNotNullComparison(ASTExpression expression) { + ASTConditionalAndExpression andExpression = expression + .getFirstChildOfType(ASTConditionalAndExpression.class); + return andExpression != null && hasEqualityExpressionWithNullLiteral(andExpression, "!="); + } + + private boolean isOrNullComparison(ASTExpression expression) { + ASTConditionalOrExpression orExpression = expression + .getFirstChildOfType(ASTConditionalOrExpression.class); + return orExpression != null && hasEqualityExpressionWithNullLiteral(orExpression, "=="); + } + + private boolean hasEqualityExpressionWithNullLiteral(JavaNode node, String equalityOp) { + ASTEqualityExpression equalityExpression = node.getFirstDescendantOfType(ASTEqualityExpression.class); + if (equalityExpression != null && equalityExpression.hasImageEqualTo(equalityOp)) { + return equalityExpression.hasDescendantOfType(ASTNullLiteral.class); + } + return false; + } } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml index 024ff927ab..d60b29ed40 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml @@ -272,6 +272,19 @@ public class Foo { boolean bar(String x) { return contentEquals("2"); } +} + ]]> + + + + bad, testing false negative at the end of a chain + 1 + From 74fb7ba1fbed81b1bf9897d0856e6ff16eba3ca7 Mon Sep 17 00:00:00 2001 From: Mykhailo Palahuta Date: Fri, 17 Jul 2020 16:30:37 +0300 Subject: [PATCH 11/19] [java] UseCollectionIsEmpty can not detect the case this.foo.size() (2543) --- .../UseCollectionIsEmptyRule.java | 93 +++++++++++++------ .../xml/UseCollectionIsEmpty.xml | 20 ++++ 2 files changed, 84 insertions(+), 29 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UseCollectionIsEmptyRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UseCollectionIsEmptyRule.java index ea55ce94c4..552d1116a7 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UseCollectionIsEmptyRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/UseCollectionIsEmptyRule.java @@ -4,18 +4,23 @@ package net.sourceforge.pmd.lang.java.rule.bestpractices; -import java.util.Arrays; +import static java.util.Arrays.asList; +import static java.util.Collections.singletonList; + import java.util.HashMap; import java.util.List; import java.util.Map; import net.sourceforge.pmd.lang.ast.Node; +import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceBody; import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceType; import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTName; +import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression; import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix; import net.sourceforge.pmd.lang.java.ast.ASTPrimarySuffix; import net.sourceforge.pmd.lang.java.ast.ASTResultType; +import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclarator; import net.sourceforge.pmd.lang.java.rule.AbstractInefficientZeroCheck; import net.sourceforge.pmd.lang.java.symboltable.ClassScope; import net.sourceforge.pmd.lang.java.symboltable.JavaNameOccurrence; @@ -45,43 +50,78 @@ public class UseCollectionIsEmptyRule extends AbstractInefficientZeroCheck { */ @Override public boolean isTargetMethod(JavaNameOccurrence occ) { - if (occ.getNameForWhichThisIsAQualifier() != null) { - if (occ.getLocation().getImage().endsWith(".size")) { - return true; - } - } - return false; + return occ.getNameForWhichThisIsAQualifier() != null + && occ.getLocation().getImage().endsWith(".size"); } @Override public Map> getComparisonTargets() { + List zeroAndOne = asList("0", "1"); + List zero = singletonList("0"); Map> rules = new HashMap<>(); - rules.put("<", Arrays.asList("0", "1")); - rules.put(">", Arrays.asList("0")); - rules.put("==", Arrays.asList("0")); - rules.put("!=", Arrays.asList("0")); - rules.put(">=", Arrays.asList("0", "1")); - rules.put("<=", Arrays.asList("0")); + rules.put("<", zeroAndOne); + rules.put(">", zero); + rules.put("==", zero); + rules.put("!=", zero); + rules.put(">=", zeroAndOne); + rules.put("<=", zero); return rules; } @Override public Object visit(ASTPrimarySuffix node, Object data) { - if (node.getImage() != null && node.getImage().endsWith("size")) { - - ASTClassOrInterfaceType type = getTypeOfPrimaryPrefix(node); - if (type == null) { - type = getTypeOfMethodCall(node); - } - - if (type != null && CollectionUtil.isCollectionType(type.getType(), true)) { - Node expr = node.getParent().getParent(); - checkNodeAndReport(data, node, expr); - } + if (isSizeMethodCall(node) && isCalledOnCollection(node)) { + Node expr = node.getParent().getParent(); + checkNodeAndReport(data, node, expr); } return data; } + private boolean isSizeMethodCall(ASTPrimarySuffix primarySuffix) { + String calledMethodName = primarySuffix.getImage(); + return calledMethodName != null && calledMethodName.endsWith("size"); + } + + private boolean isCalledOnCollection(ASTPrimarySuffix primarySuffix) { + ASTClassOrInterfaceType calledOnType = getTypeOfVariable(primarySuffix); + if (calledOnType == null) { + calledOnType = getTypeOfMethodCall(primarySuffix); + } + return calledOnType != null + && CollectionUtil.isCollectionType(calledOnType.getType(), true); + } + + private ASTClassOrInterfaceType getTypeOfVariable(ASTPrimarySuffix primarySuffix) { + ASTPrimaryExpression primaryExpression = primarySuffix.getFirstParentOfType(ASTPrimaryExpression.class); + ASTPrimaryPrefix varPrefix = primaryExpression.getFirstChildOfType(ASTPrimaryPrefix.class); + if (prefixWithNoModifiers(varPrefix)) { + return varPrefix.getFirstDescendantOfType(ASTClassOrInterfaceType.class); + } + String varName = getVariableNameBySuffix(primaryExpression); + return varName != null ? getTypeOfVariableByName(varName, primaryExpression) : null; + } + + private boolean prefixWithNoModifiers(ASTPrimaryPrefix primaryPrefix) { + return !primaryPrefix.usesSuperModifier() && !primaryPrefix.usesThisModifier(); + } + + private String getVariableNameBySuffix(ASTPrimaryExpression primaryExpression) { + ASTPrimarySuffix varSuffix = primaryExpression + .getFirstChildOfType(ASTPrimarySuffix.class); + return varSuffix.getImage(); + } + + private ASTClassOrInterfaceType getTypeOfVariableByName(String varName, ASTPrimaryExpression expr) { + ASTClassOrInterfaceBody classBody = expr.getFirstParentOfType(ASTClassOrInterfaceBody.class); + List varDeclarators = classBody.findDescendantsOfType(ASTVariableDeclarator.class); + for (ASTVariableDeclarator varDeclarator : varDeclarators) { + if (varDeclarator.getName().equals(varName)) { + return varDeclarator.getFirstDescendantOfType(ASTClassOrInterfaceType.class); + } + } + return null; + } + private ASTClassOrInterfaceType getTypeOfMethodCall(ASTPrimarySuffix node) { ASTClassOrInterfaceType type = null; ASTName methodName = node.getParent().getFirstChildOfType(ASTPrimaryPrefix.class) @@ -100,9 +140,4 @@ public class UseCollectionIsEmptyRule extends AbstractInefficientZeroCheck { } return type; } - - private ASTClassOrInterfaceType getTypeOfPrimaryPrefix(ASTPrimarySuffix node) { - return node.getParent().getFirstChildOfType(ASTPrimaryPrefix.class) - .getFirstDescendantOfType(ASTClassOrInterfaceType.class); - } } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UseCollectionIsEmpty.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UseCollectionIsEmpty.xml index 664fdaa1c9..010fd20ee9 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UseCollectionIsEmpty.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UseCollectionIsEmpty.xml @@ -311,6 +311,26 @@ public class PmdBugBait { throw new IllegalArgumentException(); } } +} + ]]> + + + + #2543 false negative on this.collection.size + 1 + list = new ArrayList<>(); + + public void bar() { + if (this.list.size() == 0) { + throw new RuntimeException("Empty list"); + } + } } ]]> From c351314f3017acaafccce02334095f67944d4e29 Mon Sep 17 00:00:00 2001 From: Mykhailo Palahuta Date: Wed, 22 Jul 2020 13:46:08 +0300 Subject: [PATCH 12/19] LiteralsFirstInComparisonsRule: ignore two string literals comparison --- .../LiteralsFirstInComparisonsRule.java | 28 +++++++++++++++---- .../xml/LiteralsFirstInComparisons.xml | 13 +++++++++ 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LiteralsFirstInComparisonsRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LiteralsFirstInComparisonsRule.java index 5074e84937..3419dd8d65 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LiteralsFirstInComparisonsRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/bestpractices/LiteralsFirstInComparisonsRule.java @@ -31,17 +31,29 @@ public class LiteralsFirstInComparisonsRule extends AbstractJavaRule { @Override public Object visit(ASTPrimaryExpression expression, Object data) { - String opName = getOperationName(expression); - ASTPrimarySuffix primarySuffix = getSuffixOfArguments(expression); - if (opName == null || primarySuffix == null) { - return data; - } - if (isStringLiteralComparison(opName, primarySuffix) && !isWithinNullComparison(expression)) { + if (violatesLiteralsFirstInComparisonsRule(expression)) { addViolation(data, expression); } return data; } + private boolean violatesLiteralsFirstInComparisonsRule(ASTPrimaryExpression expression) { + return !hasStringLiteralFirst(expression) && isNullableComparisonWithStringLiteral(expression); + } + + private boolean hasStringLiteralFirst(ASTPrimaryExpression expression) { + ASTPrimaryPrefix primaryPrefix = expression.getFirstChildOfType(ASTPrimaryPrefix.class); + ASTLiteral firstLiteral = primaryPrefix.getFirstDescendantOfType(ASTLiteral.class); + return firstLiteral != null && firstLiteral.isStringLiteral(); + } + + private boolean isNullableComparisonWithStringLiteral(ASTPrimaryExpression expression) { + String opName = getOperationName(expression); + ASTPrimarySuffix argsSuffix = getSuffixOfArguments(expression); + return opName != null && argsSuffix != null && isStringLiteralComparison(opName, argsSuffix) + && isNotWithinNullComparison(expression); + } + private String getOperationName(ASTPrimaryExpression primaryExpression) { return isMethodsChain(primaryExpression) ? getOperationNameBySuffix(primaryExpression) : getOperationNameByPrefix(primaryExpression); @@ -135,6 +147,10 @@ public class LiteralsFirstInComparisonsRule extends AbstractJavaRule { return false; } + private boolean isNotWithinNullComparison(ASTPrimaryExpression node) { + return !isWithinNullComparison(node); + } + /* * Expression/ConditionalAndExpression//EqualityExpression(@Image='!=']//NullLiteral * Expression/ConditionalOrExpression//EqualityExpression(@Image='==']//NullLiteral diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml index d60b29ed40..90ca6ac7a7 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml @@ -285,6 +285,19 @@ public class Foo { File f; return f.getFileType().equals("testStr"); } +} + ]]> + + + + ok, should be ignored in case both operands are string literals + 0 + From 66d243efa11a71fe51363c22790dd0764b9bb80e Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 23 Jul 2020 10:10:26 +0200 Subject: [PATCH 13/19] [java] LiteralsFirstInComparison: additional test case --- .../xml/LiteralsFirstInComparisons.xml | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml index 90ca6ac7a7..f76ba2658f 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/LiteralsFirstInComparisons.xml @@ -298,6 +298,23 @@ public class Foo { public void bar() { this.isFoo = "Hello".equals("World"); } +} + ]]> + + + + Equals on method result with String argument + 1 + 6 + From 7b1ccf4837afe66af546a9af4c918b3b2cfd8403 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 23 Jul 2020 10:13:40 +0200 Subject: [PATCH 14/19] [doc] Update release notes, fixes #2569, refs #2651 --- docs/pages/release_notes.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index ef6e847db0..467f6859c6 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -18,11 +18,14 @@ This is a {{ site.pmd.release_type }} release. * apex-bestpractices * [#2626](https://github.com/pmd/pmd/issues/2626): \[apex] UnusedLocalVariable - false positive on case insensitivity allowed in Apex +* java-bestpractices + * [#2569](https://github.com/pmd/pmd/issues/2569): \[java] LiteralsFirstInComparisons: False negative for methods returning Strings ### API Changes ### External Contributions * [#2590](https://github.com/pmd/pmd/pull/2590): Update libraries snyk is referring to as `unsafe` - [Artem Krosheninnikov](https://github.com/KroArtem) +* [#2651](https://github.com/pmd/pmd/pull/2651): \[java] False negative: LiteralsFirstInComparisons for methods... (2569) - [Mykhailo Palahuta](https://github.com/Drofff) {% endtocmaker %} From 6b44e326ce034b43f2870474f55e424d886a2183 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 23 Jul 2020 10:44:22 +0200 Subject: [PATCH 15/19] [java] UseCollectionIsEmpty: improve test cases --- .../bestpractices/xml/UseCollectionIsEmpty.xml | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UseCollectionIsEmpty.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UseCollectionIsEmpty.xml index 010fd20ee9..d4f085f0df 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UseCollectionIsEmpty.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/bestpractices/xml/UseCollectionIsEmpty.xml @@ -7,6 +7,7 @@ fail, == 0 1 + 3 fail, != 0 1 + 3 fail, != 0 1 + 3 fail, 0 == 1 + 3 fail, > 0 1 + 3 #1214 UseCollectionIsEmpty misses some usage 5 + 25,28,31,34,37 - + #2543 false negative on this.collection.size 1 + 10 list = new ArrayList<>(); + private Bar notACollection = new Bar(); public void bar() { if (this.list.size() == 0) { throw new RuntimeException("Empty list"); } + if (notACollection.size() == 0) { } + if (this.notACollection.size() == 0) { } + } + + public int size() { + return 0; } } ]]> From f1fa3753413b5085d30cc3f1d263cb091db5fdba Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 23 Jul 2020 10:45:56 +0200 Subject: [PATCH 16/19] [doc] Update release nots, fixes #2543, refs #2652 --- docs/pages/release_notes.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index ef6e847db0..a096eb3d0e 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -18,11 +18,14 @@ This is a {{ site.pmd.release_type }} release. * apex-bestpractices * [#2626](https://github.com/pmd/pmd/issues/2626): \[apex] UnusedLocalVariable - false positive on case insensitivity allowed in Apex +* java-bestpractices + * [#2543](https://github.com/pmd/pmd/issues/2543): \[java] UseCollectionIsEmpty can not detect the case this.foo.size() ### API Changes ### External Contributions * [#2590](https://github.com/pmd/pmd/pull/2590): Update libraries snyk is referring to as `unsafe` - [Artem Krosheninnikov](https://github.com/KroArtem) +* [#2652](https://github.com/pmd/pmd/pull/2652): \[java] UseCollectionIsEmpty can not detect the case this.foo.size() - [Mykhailo Palahuta](https://github.com/Drofff) {% endtocmaker %} From 6ee17d44f7b855c80c611c5e79eea960ab1015fa Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 23 Jul 2020 11:44:39 +0200 Subject: [PATCH 17/19] [doc] Fix javadoc in release notes for constructor --- docs/_plugins/javadoc_tag.rb | 3 +++ docs/pages/release_notes.md | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/docs/_plugins/javadoc_tag.rb b/docs/_plugins/javadoc_tag.rb index 56b6e78561..6ca08afdf0 100644 --- a/docs/_plugins/javadoc_tag.rb +++ b/docs/_plugins/javadoc_tag.rb @@ -180,6 +180,9 @@ class JavadocTag < Liquid::Tag if member_suffix && Regexp.new('(\w+|)(' + ARGUMENTS_REGEX.source + ")?") =~ member_suffix suffix = $1 # method or field name + if suffix == '' + suffix = '<init>' + end if opts.show_args? && $2 && !$2.empty? # is method diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index c9f2a2ff4d..77a52a90d3 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -52,8 +52,8 @@ This is a {{ site.pmd.release_type }} release. * {% jdoc core::lang.rule.RuleChainVisitor %} and all implementations in language modules * {% jdoc core::lang.rule.AbstractRuleChainVisitor %} -* {% jdoc core::lang.Language#getRuleChainVisitorClass() %} -* {% jdoc core::lang.BaseLanguageModule#(java.lang.String,java.lang.String,java.lang.String,java.lang.Class,java.lang.String...) %} +* {% jdoc !!core::lang.Language#getRuleChainVisitorClass() %} +* {% jdoc !!core::lang.BaseLanguageModule#(java.lang.String,java.lang.String,java.lang.String,java.lang.Class,java.lang.String...) %} ### External Contributions From 8c06dbcd4c797726254e6c8e1e074a4379d5536d Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 23 Jul 2020 12:39:19 +0200 Subject: [PATCH 18/19] [doc] Improve performance of rule doc generator Now we walk PMD source tree only once to resolve the file names to rulesets and rules instead walking the whole tree again for each ruleset/rule. --- .../pmd/docs/GenerateRuleDocsCmd.java | 5 + .../pmd/docs/RuleDocGenerator.java | 111 ++++++++++-------- 2 files changed, 66 insertions(+), 50 deletions(-) diff --git a/pmd-doc/src/main/java/net/sourceforge/pmd/docs/GenerateRuleDocsCmd.java b/pmd-doc/src/main/java/net/sourceforge/pmd/docs/GenerateRuleDocsCmd.java index fbb78a6898..75c56f7f49 100644 --- a/pmd-doc/src/main/java/net/sourceforge/pmd/docs/GenerateRuleDocsCmd.java +++ b/pmd-doc/src/main/java/net/sourceforge/pmd/docs/GenerateRuleDocsCmd.java @@ -30,6 +30,11 @@ public final class GenerateRuleDocsCmd { } public static void main(String[] args) throws RuleSetNotFoundException { + if (args.length != 1) { + System.err.println("One argument is required: The base directory of the module pmd-doc."); + System.exit(1); + } + long start = System.currentTimeMillis(); Path output = FileSystems.getDefault().getPath(args[0]).resolve("..").toAbsolutePath().normalize(); System.out.println("Generating docs into " + output); diff --git a/pmd-doc/src/main/java/net/sourceforge/pmd/docs/RuleDocGenerator.java b/pmd-doc/src/main/java/net/sourceforge/pmd/docs/RuleDocGenerator.java index 818ad3e7c8..97852a5c75 100644 --- a/pmd-doc/src/main/java/net/sourceforge/pmd/docs/RuleDocGenerator.java +++ b/pmd-doc/src/main/java/net/sourceforge/pmd/docs/RuleDocGenerator.java @@ -75,6 +75,12 @@ public class RuleDocGenerator { private final Path root; private final FileWriter writer; + /** Caches rule class name to java source file mapping. */ + private final Map allRules = new HashMap<>(); + /** Caches ruleset to ruleset xml file mapping. */ + private final Map allRulesets = new HashMap<>(); + + public RuleDocGenerator(FileWriter writer, Path root) { this.writer = Objects.requireNonNull(writer, "A file writer must be provided"); this.root = Objects.requireNonNull(root, "Root directory must be provided"); @@ -91,6 +97,7 @@ public class RuleDocGenerator { try { sortedRulesets = sortRulesets(registeredRulesets); sortedAdditionalRulesets = sortRulesets(resolveAdditionalRulesets(additionalRulesets)); + determineRuleClassSourceFiles(sortedRulesets); generateLanguageIndex(sortedRulesets, sortedAdditionalRulesets); generateRuleSetIndex(sortedRulesets); @@ -345,7 +352,7 @@ public class RuleDocGenerator { String permalink = RULESET_INDEX_PERMALINK_PATTERN .replace("${language.tersename}", languageTersename) .replace("${ruleset.name}", rulesetFilename); - String ruleSetSourceFilepath = "../" + getRuleSetSourceFilepath(ruleset); + String ruleSetSourceFilepath = "../" + allRulesets.get(ruleset.getFileName()); List lines = new LinkedList<>(); lines.add("---"); @@ -415,7 +422,7 @@ public class RuleDocGenerator { } else { lines.add("**This rule is defined by the following Java class:** " + "[" + rule.getRuleClass() + "](" - + GITHUB_SOURCE_LINK + getRuleClassSourceFilepath(rule.getRuleClass()) + + GITHUB_SOURCE_LINK + allRules.get(rule.getRuleClass()) + ")"); } lines.add(""); @@ -607,63 +614,67 @@ public class RuleDocGenerator { } /** - * Searches for the source file of the given ruleset. This provides the information - * for the "editme" link. + * Walks through the root directory once to get all rule source file path names and ruleset names. + * This provides the information for the "editme" links. * - * @param ruleset the ruleset to search for. - * @return - * @throws IOException + * @param sortedRulesets all the rulesets and rules */ - private String getRuleSetSourceFilepath(RuleSet ruleset) throws IOException { - final String rulesetFilename = FilenameUtils.normalize(StringUtils.chomp(ruleset.getFileName())); - final List foundPathResult = new LinkedList<>(); - - Files.walkFileTree(root, new SimpleFileVisitor() { - @Override - public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { - String path = file.toString(); - if (path.contains("src") && path.endsWith(rulesetFilename)) { - foundPathResult.add(file); - return FileVisitResult.TERMINATE; + private void determineRuleClassSourceFiles(Map> sortedRulesets) { + // first collect all the classes, we need to resolve and the rulesets + for (List rulesets : sortedRulesets.values()) { + for (RuleSet ruleset : rulesets) { + String rulesetFilename = FilenameUtils.normalize(StringUtils.chomp(ruleset.getFileName())); + allRulesets.put(ruleset.getFileName(), rulesetFilename); + for (Rule rule : ruleset.getRules()) { + String ruleClass = rule.getRuleClass(); + String relativeSourceFilename = ruleClass.replaceAll("\\.", Matcher.quoteReplacement(File.separator)) + + ".java"; + allRules.put(ruleClass, relativeSourceFilename); } - return super.visitFile(file, attrs); } - }); - - if (!foundPathResult.isEmpty()) { - Path foundPath = foundPathResult.get(0); - foundPath = root.relativize(foundPath); - // Note: the path is normalized to unix path separators, so that the editme link - // uses forward slashes - return FilenameUtils.normalize(foundPath.toString(), true); } - return StringUtils.chomp(ruleset.getFileName()); - } + // then go and search the actual files + try { + Files.walkFileTree(root, new SimpleFileVisitor() { + @Override + public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) { + String path = file.toString(); - private String getRuleClassSourceFilepath(String ruleClass) throws IOException { - final String relativeSourceFilename = ruleClass.replaceAll("\\.", Matcher.quoteReplacement(File.separator)) - + ".java"; - final List foundPathResult = new LinkedList<>(); + if (path.contains("src")) { + String foundRuleClass = null; + for (Map.Entry entry : allRules.entrySet()) { + if (path.endsWith(entry.getValue())) { + foundRuleClass = entry.getKey(); + break; + } + } + if (foundRuleClass != null) { + Path foundPath = root.relativize(file); + // Note: the path is normalized to unix path separators, so that the editme link + // uses forward slashes + allRules.put(foundRuleClass, FilenameUtils.normalize(foundPath.toString(), true)); + } - Files.walkFileTree(root, new SimpleFileVisitor() { - @Override - public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { - String path = file.toString(); - if (path.contains("src") && path.endsWith(relativeSourceFilename)) { - foundPathResult.add(file); - return FileVisitResult.TERMINATE; + String foundRuleset = null; + for (Map.Entry entry : allRulesets.entrySet()) { + if (path.endsWith(entry.getValue())) { + foundRuleset = entry.getKey(); + break; + } + } + if (foundRuleset != null) { + Path foundPath = root.relativize(file); + // Note: the path is normalized to unix path separators, so that the editme link + // uses forward slashes + allRulesets.put(foundRuleset, FilenameUtils.normalize(foundPath.toString(), true)); + } + } + return FileVisitResult.CONTINUE; } - return super.visitFile(file, attrs); - } - }); - - if (!foundPathResult.isEmpty()) { - Path foundPath = foundPathResult.get(0); - foundPath = root.relativize(foundPath); - return FilenameUtils.normalize(foundPath.toString(), true); + }); + } catch (IOException e) { + throw new RuntimeException(e); } - - return FilenameUtils.normalize(relativeSourceFilename, true); } } From 55a6b5bef5c1ea9304d4fa117c9d2ad143381b3b Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 23 Jul 2020 13:53:52 +0200 Subject: [PATCH 19/19] [doc] Fix unit tests on Windows --- .../net/sourceforge/pmd/docs/RuleDocGenerator.java | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/pmd-doc/src/main/java/net/sourceforge/pmd/docs/RuleDocGenerator.java b/pmd-doc/src/main/java/net/sourceforge/pmd/docs/RuleDocGenerator.java index 97852a5c75..f62400b1a9 100644 --- a/pmd-doc/src/main/java/net/sourceforge/pmd/docs/RuleDocGenerator.java +++ b/pmd-doc/src/main/java/net/sourceforge/pmd/docs/RuleDocGenerator.java @@ -621,15 +621,22 @@ public class RuleDocGenerator { */ private void determineRuleClassSourceFiles(Map> sortedRulesets) { // first collect all the classes, we need to resolve and the rulesets + // this also provides a default fallback path, which is used in unit tests. + // if the actual file is found during walkFileTree, then the default fallback path + // is replaced by a correct path. for (List rulesets : sortedRulesets.values()) { for (RuleSet ruleset : rulesets) { - String rulesetFilename = FilenameUtils.normalize(StringUtils.chomp(ruleset.getFileName())); + // Note: the path is normalized to unix path separators, so that the editme link + // uses forward slashes + String rulesetFilename = FilenameUtils.normalize(StringUtils.chomp(ruleset.getFileName()), true); allRulesets.put(ruleset.getFileName(), rulesetFilename); for (Rule rule : ruleset.getRules()) { String ruleClass = rule.getRuleClass(); String relativeSourceFilename = ruleClass.replaceAll("\\.", Matcher.quoteReplacement(File.separator)) + ".java"; - allRules.put(ruleClass, relativeSourceFilename); + // Note: the path is normalized to unix path separators, so that the editme link + // uses forward slashes + allRules.put(ruleClass, FilenameUtils.normalize(relativeSourceFilename, true)); } } }