Merge pull request #29 from sjudd/master

Un-escape XML when reading attributes.
tomasbjerre · Dec 31, 2017 · 1c427e7 · 1c427e7
2 parents 637ab5f + 9b10f80
commit 1c427e7
Show file tree

Hide file tree

Showing 5 changed files with 128 additions and 9 deletions.
diff --git a/src/main/java/se/bjurr/violations/lib/parsers/ViolationParserUtils.java b/src/main/java/se/bjurr/violations/lib/parsers/ViolationParserUtils.java
@@ -9,7 +9,10 @@
 import java.io.StringWriter;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import javax.xml.stream.XMLStreamReader;
@@ -20,6 +23,46 @@
 import se.bjurr.violations.lib.util.Optional;
 
 public final class ViolationParserUtils {
+  private static final Map<String, Character> XML_ESCAPE_CHARACTER_MAP;
+  private static final String[] XML_ESCAPE_CHARACTERS;
+  private static final char XML_ESCAPE_START = '&';
+
+  static {
+    Map<String, Character> temp = new HashMap<>();
+    temp.put("&apos;", '\'');
+    temp.put("&quot;", '\"');
+    temp.put("&amp;", '&');
+    temp.put("&lt;", '<');
+    temp.put("&gt;", '>');
+    XML_ESCAPE_CHARACTER_MAP = Collections.unmodifiableMap(temp);
+    XML_ESCAPE_CHARACTERS = temp.keySet().toArray(new String[0]);
+  }
+
+  private static String unEscapeXml(String input) {
+    StringBuilder result = new StringBuilder(input.length());
+    for (int i = 0; i < input.length(); ) {
+      char current = input.charAt(i);
+
+      boolean isValidXmlEscape = false;
+      if (current == XML_ESCAPE_START) {
+        for (String escapeCharacter : XML_ESCAPE_CHARACTERS) {
+          if (input.startsWith(escapeCharacter, i)) {
+            result.append(XML_ESCAPE_CHARACTER_MAP.get(escapeCharacter));
+            i += escapeCharacter.length();
+            isValidXmlEscape = true;
+            break;
+          }
+        }
+      }
+
+      if (!isValidXmlEscape) {
+        result.append(current);
+        i++;
+      }
+    }
+    return result.toString();
+  }
+
   public static String asString(XMLStreamReader xmlr) throws Exception {
     final Transformer transformer = TransformerFactory.newInstance().newTransformer();
     final StringWriter stringWriter = new StringWriter();
@@ -31,12 +74,12 @@ public static Optional<String> findAttribute(String in, String attribute) {
     Pattern pattern = Pattern.compile(attribute + "='([^']+?)'");
     Matcher matcher = pattern.matcher(in);
     if (matcher.find()) {
-      return fromNullable(matcher.group(1));
+      return fromNullable(unEscapeXml(matcher.group(1)));
     }
     pattern = Pattern.compile(attribute + "=\"([^\"]+?)\"");
     matcher = pattern.matcher(in);
     if (matcher.find()) {
-      return fromNullable(matcher.group(1));
+      return fromNullable(unEscapeXml(matcher.group(1)));
     }
     return absent();
   }

diff --git a/src/test/java/se/bjurr/violations/lib/AndroidLintTest.java b/src/test/java/se/bjurr/violations/lib/AndroidLintTest.java
@@ -39,7 +39,7 @@ public void testThatViolationsCanBeParsed() {
                 .setRule("Correctness")
                 .setMessage(
                     "ScrollViewSize: ScrollView size validation\n"
-                        + "This LinearLayout should use `android:layout_height=&quot;wrap_content&quot;`\n"
+                        + "This LinearLayout should use `android:layout_height=\"wrap_content\"`\n"
                         + "ScrollView children must set their `layout_width` or `layout_height` attributes to `wrap_content` rather than `fill_parent` or `match_parent` in the scrolling dimension") //
                 .setSeverity(WARN) //
                 .build(), //

diff --git a/src/test/java/se/bjurr/violations/lib/CPPCheckTest.java b/src/test/java/se/bjurr/violations/lib/CPPCheckTest.java
@@ -15,9 +15,9 @@
 public class CPPCheckTest {
 
   private static final String MSG_1 =
-      "The scope of the variable 'i' can be reduced. The scope of the variable 'i' can be reduced. Warning: It can be unsafe to fix this message. Be careful. Especially when there are inner loops. Here is an example where cppcheck will write that the scope for 'i' can be reduced:&#xa;void f(int x)&#xa;{&#xa;    int i = 0;&#xa;    if (x) {&#xa;        // it's safe to move 'int i = 0' here&#xa;        for (int n = 0; n &lt; 10; ++n) {&#xa;            // it is possible but not safe to move 'int i = 0' here&#xa;            do_something(&amp;i);&#xa;        }&#xa;    }&#xa;}&#xa;When you see this message it is always safe to reduce the variable scope 1 level.";
+      "The scope of the variable 'i' can be reduced. The scope of the variable 'i' can be reduced. Warning: It can be unsafe to fix this message. Be careful. Especially when there are inner loops. Here is an example where cppcheck will write that the scope for 'i' can be reduced:&#xa;void f(int x)&#xa;{&#xa;    int i = 0;&#xa;    if (x) {&#xa;        // it's safe to move 'int i = 0' here&#xa;        for (int n = 0; n < 10; ++n) {&#xa;            // it is possible but not safe to move 'int i = 0' here&#xa;            do_something(&i);&#xa;        }&#xa;    }&#xa;}&#xa;When you see this message it is always safe to reduce the variable scope 1 level.";
   private static final String MSG_2 =
-      "The scope of the variable 'i' can be reduced. The scope of the variable 'i' can be reduced. Warning: It can be unsafe to fix this message. Be careful. Especially when there are inner loops. Here is an example where cppcheck will write that the scope for 'i' can be reduced:&#xa;void f(int x)&#xa;{&#xa;    int i = 0;&#xa;    if (x) {&#xa;        // it's safe to move 'int i = 0' here&#xa;        for (int n = 0; n &lt; 10; ++n) {&#xa;            // it is possible but not safe to move 'int i = 0' here&#xa;            do_something(&amp;i);&#xa;        }&#xa;    }&#xa;}&#xa;When you see this message it is always safe to reduce the variable scope 1 level.";
+      "The scope of the variable 'i' can be reduced. The scope of the variable 'i' can be reduced. Warning: It can be unsafe to fix this message. Be careful. Especially when there are inner loops. Here is an example where cppcheck will write that the scope for 'i' can be reduced:&#xa;void f(int x)&#xa;{&#xa;    int i = 0;&#xa;    if (x) {&#xa;        // it's safe to move 'int i = 0' here&#xa;        for (int n = 0; n < 10; ++n) {&#xa;            // it is possible but not safe to move 'int i = 0' here&#xa;            do_something(&i);&#xa;        }&#xa;    }&#xa;}&#xa;When you see this message it is always safe to reduce the variable scope 1 level.";
 
   @Test
   public void testThatViolationsCanBeParsed() {

diff --git a/src/test/java/se/bjurr/violations/lib/CheckstyleTest.java b/src/test/java/se/bjurr/violations/lib/CheckstyleTest.java
@@ -14,11 +14,79 @@
 import se.bjurr.violations.lib.model.Violation;
 
 public class CheckstyleTest {
+  private final String rootFolder = getRootFolder();
 
   @Test
-  public void testThatViolationsCanBeParsed() {
-    final String rootFolder = getRootFolder();
+  public void testThatViolationsWithXmlSpecialCharactersCanBeParsed() {
+    final List<Violation> actual =
+        violationsApi() //
+            .withPattern(".*/checkstyle/special_chars\\.xml$") //
+            .inFolder(rootFolder) //
+            .findAll(CHECKSTYLE) //
+            .violations();
 
+    assertThat(actual)
+        .containsExactly(
+            violationBuilder() //
+                .setParser(CHECKSTYLE) //
+                .setFile("/src/main/java/se/bjurr/violations/lib/example/MyClass.java") //
+                .setSource(null) //
+                .setStartLine(11) //
+                .setEndLine(11) //
+                .setColumn(10) //
+                .setMessage("\'Must have at least one statement.\'") //
+                .setRule("com.puppycrawl.tools.checkstyle.checks.blocks.EmptyBlockCheck") //
+                .setSeverity(INFO) //
+                .build(), //
+            violationBuilder() //
+                .setParser(CHECKSTYLE) //
+                .setFile("/src/main/java/se/bjurr/violations/lib/example/MyClass.java") //
+                .setSource(null) //
+                .setStartLine(12) //
+                .setEndLine(12) //
+                .setColumn(10) //
+                .setMessage("Must have at least one \"statement\".") //
+                .setRule("com.puppycrawl.tools.checkstyle.checks.blocks.EmptyBlockCheck") //
+                .setSeverity(INFO) //
+                .build(), //
+            violationBuilder() //
+                .setParser(CHECKSTYLE) //
+                .setFile("/src/main/java/se/bjurr/violations/lib/example/MyClass.java") //
+                .setSource(null) //
+                .setStartLine(13) //
+                .setEndLine(13) //
+                .setColumn(10) //
+                .setMessage("one is < two") //
+                .setRule("one.should.be.greater.than.two") //
+                .setSeverity(INFO) //
+                .build(), //
+            violationBuilder() //
+                .setParser(CHECKSTYLE) //
+                .setFile("/src/main/java/se/bjurr/violations/lib/example/MyClass.java") //
+                .setSource(null) //
+                .setStartLine(14) //
+                .setEndLine(14) //
+                .setColumn(10) //
+                .setMessage("two is > one") //
+                .setRule("two.should.be.less.than.one") //
+                .setSeverity(INFO) //
+                .build(), //
+            violationBuilder() //
+                .setParser(CHECKSTYLE) //
+                .setFile("/src/main/java/se/bjurr/violations/lib/example/MyClass.java") //
+                .setSource(null) //
+                .setStartLine(15) //
+                .setEndLine(15) //
+                .setColumn(10) //
+                .setMessage("one & one is two") //
+                .setRule("one.and.one.should.be.three") //
+                .setSeverity(INFO) //
+                .build() //
+            );
+  }
+
+  @Test
+  public void testThatViolationsCanBeParsed() {
     final List<Violation> actual =
         violationsApi() //
             .withPattern(".*/checkstyle/main\\.xml$") //
@@ -100,8 +168,6 @@ public void testThatPHPViolationsCanBeParsed() {
 
   @Test
   public void testThatPHPViolationsCanBeParsedIfNoSource() {
-    final String rootFolder = getRootFolder();
-
     final List<Violation> actual =
         violationsApi() //
             .withPattern(".*/checkstyle/checkstyle-no-source\\.xml$") //

diff --git a/src/test/resources/checkstyle/special_chars.xml b/src/test/resources/checkstyle/special_chars.xml
@@ -0,0 +1,10 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<checkstyle version="5.9">
+  <file name="/src/main/java/se/bjurr/violations/lib/example/MyClass.java">
+    <error line="11" column="10" severity="info" message="&apos;Must have at least one statement.&apos;" source="com.puppycrawl.tools.checkstyle.checks.blocks.EmptyBlockCheck"/>
+    <error line="12" column="10" severity="info" message="Must have at least one &quot;statement&quot;." source="com.puppycrawl.tools.checkstyle.checks.blocks.EmptyBlockCheck"/>
+    <error line="13" column="10" severity="info" message="one is &lt; two" source="one.should.be.greater.than.two" />
+    <error line="14" column="10" severity="info" message="two is &gt; one" source="two.should.be.less.than.one"/>
+    <error line="15" column="10" severity="info" message="one &amp; one is two" source="one.and.one.should.be.three"/>
+  </file>
+</checkstyle>