Repository: decorators-squad/eo-yaml (java)

Issue:
Value on the next line in a sequence doesn't escape correctly.
This YAML:
```sns:
  -
    "arn:aws:sns:{{ region }}:{{ account }}:Topic"
```

Returns the string as:
```'aws:sns:{{ region }}:{{ account }}:Topic"'```

This is due to the regex in escapedSequenceScalar not treating the dash as optional.

Pull request description:
Not escaping scalar in sequences correctly.
Closes #406 

Patch:
diff --git a/src/main/java/com/amihaiemil/eoyaml/AllYamlLines.java b/src/main/java/com/amihaiemil/eoyaml/AllYamlLines.java
index 5953f77..d46f5d1 100644
--- a/src/main/java/com/amihaiemil/eoyaml/AllYamlLines.java
+++ b/src/main/java/com/amihaiemil/eoyaml/AllYamlLines.java
@@ -30,6 +30,8 @@ package com.amihaiemil.eoyaml;
 import com.amihaiemil.eoyaml.exceptions.YamlReadingException;
 import java.util.Collection;
 import java.util.Iterator;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 
 /**
  * YamlLines default implementation. "All" refers to the fact that
@@ -37,14 +39,43 @@ import java.util.Iterator;
  * are cases where we need to iterate only over the lines which are
  * at the same indentation level and for that we use the decorator
  * {@link SameIndentationLevel}.
- * @checkstyle ExecutableStatementCount (400 lines)
- * @checkstyle CyclomaticComplexity (400 lines)
  * @author Mihai Andronache (amihaiemil@gmail.com)
  * @version $Id$
  * @since 1.0.0
  */
 final class AllYamlLines implements YamlLines {
 
+    /**
+     * There are 3 types of nodes: scalar, sequence and mapping.  This matches
+     * either a sequence or map - no match indicates it's a scalar.
+     *
+     * Does not handle '?' mapping key or flow mapping
+     * (https://yaml.org/spec/1.2/spec.html#id2790832).
+     *
+     * A sequence (group 2) is:
+     *  - [ ]* : 0 or more spaces
+     *    - [\-](|[ ]+.*) : a dash (-) optionally followed by 1 or more
+     *      spaces and any other characters.
+     *
+     * A map (group 4) is:
+     *  - [ ]* : 0 or more spaces followed by:
+     *    - a key:
+     *      - ('(?:[^'\\]|\\.)*') : a single (') quoted string or
+     *      - ("(?:[^"\\]|\\.)*") : double (") quoted string
+     *      - ([^"']*) : a non quoted string (characters other than ' or ").
+     *        - followed by:
+     *          - :(|[ ].*) : a colon (:) optionally followed by a space
+     *            and any other characters.
+     */
+    private static final Pattern SEQUENCE_OR_MAP = Pattern.compile("^("
+            + "([ ]*[\\-](|[ ]+.*))|"
+            + "([ ]*"
+                + "('(?:[^'\\\\]|\\\\.)*')|"
+                  + "(\"(?:[^\"\\\\]|\\\\.)*\")|"
+                  + "([^\"']*)"
+                + ":(|[ ].*))"
+            + ")$");
+
     /**
      * Yaml lines.
      */
@@ -116,7 +147,7 @@ final class AllYamlLines implements YamlLines {
         final YamlLine prev,
         final boolean guessIndentation
     ) {
-        final YamlNode node;
+        YamlNode node = null;
         final YamlLine first = new Skip(
             this,
             line -> line.number() <= prev.number(),
@@ -126,14 +157,18 @@ final class AllYamlLines implements YamlLines {
             line -> line.trimmed().startsWith("%"),
             line -> line.trimmed().startsWith("!!")
         ).iterator().next();
-        if (first.trimmed().startsWith("-")){
-            node = new ReadYamlSequence(prev, this, guessIndentation);
-        } else if(first.trimmed().contains(":")) {
-            node = new ReadYamlMapping(prev.number(), prev, this,
-                    guessIndentation);
-        } else if(this.original().size() == 1) {
+        Matcher matcher = SEQUENCE_OR_MAP.matcher(first.trimmed());
+        if (matcher.matches()) {
+            if (matcher.group(2) != null) {
+                node = new ReadYamlSequence(prev, this, guessIndentation);
+            } else if (matcher.group(4) != null) {
+                node = new ReadYamlMapping(prev.number(),
+                        prev, this, guessIndentation);
+            }
+        } else if (this.original().size() == 1) {
             node = new ReadPlainScalar(this, first);
-        } else {
+        }
+        if (node == null) {
             throw new YamlReadingException(
                 "Could not parse YAML starting at line " + (first.number() + 1)
                 + " . It should be a sequence (line should start with '-'), "
@@ -141,8 +176,8 @@ final class AllYamlLines implements YamlLines {
                 + "scalar, but it has " + this.lines.size() + " lines, "
                 + "while a plain scalar should be only 1 line!"
             );
+        } else {
+            return node;
         }
-        return node;
     }
-
 }
diff --git a/src/main/java/com/amihaiemil/eoyaml/ReadPlainScalar.java b/src/main/java/com/amihaiemil/eoyaml/ReadPlainScalar.java
index 4030403..aae1d9a 100644
--- a/src/main/java/com/amihaiemil/eoyaml/ReadPlainScalar.java
+++ b/src/main/java/com/amihaiemil/eoyaml/ReadPlainScalar.java
@@ -27,6 +27,9 @@
  */
 package com.amihaiemil.eoyaml;
 
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
 /**
  * A plain scalar value read from somewhere.
  * @author Mihai Andronace (amihaiemil@gmail.com)
@@ -35,6 +38,46 @@ package com.amihaiemil.eoyaml;
  */
 final class ReadPlainScalar extends BaseScalar {
 
+    /**
+     * Pattern to match scalars in mappings or sequences.
+     *
+     * Ignore zero or more spaces and a hyphen (-) followed by one
+     * or more spaces.
+     *
+     * A quoted scalar literal is inside:
+     *  - ('(?:[^'\\]|\\.)*') : a single (') quoted string or
+     *  - ("(?:[^"\\]|\\.)*") : double (") quoted string
+     *
+     * A scalar for a mapping are characters after:
+     *  - .*:[ ]+(.*) : Any characters before a colon followed by
+     *    one or more spaces.
+     *
+     * The sequence scalar is:
+     *   - -[ ]+(.*) : Any characters after a hyphen (-) and one more spaces.
+     */
+    private static final Pattern QUOTED_LITERAL_MAP_SEQ = Pattern.compile("^("
+            + "[ ]*(-[ ]+)"
+                + "(('(?:[^'\\\\]|\\\\.)*')|"
+                + "(\"(?:[^\"\\\\]|\\\\.)*\"))|"
+            + "(.*:[ ]+(.*))|"
+            + "(-[ ]+(.*))"
+            + ")$");
+
+    /**
+     * Regex group index that matches quoted literals.
+     */
+    private static final int QUOTED_LITERAL_GROUP = 3;
+
+    /**
+     * Regex group index that matches scalar values of mappings.
+     */
+    private static final int MAPPING_GROUP = 7;
+
+    /**
+     * Regex group index that matches scalar value of non-quotes sequences.
+     */
+    private static final int SEQUENCE_GROUP = 9;
+
     /**
      * All YAML Lines of the document.
      */
@@ -68,17 +111,15 @@ final class ReadPlainScalar extends BaseScalar {
      */
     @Override
     public String value() {
-        final String value;
-        final String trimmed = this.scalar.trimmed();
-        if(this.escapedSequenceScalar(this.scalar)) {
-            value = trimmed.substring(trimmed.indexOf('-')+1).trim();
-        } else {
-            if (trimmed.contains(":") && !trimmed.endsWith(":")) {
-                value = trimmed.substring(trimmed.indexOf(":") + 1).trim();
-            } else if (trimmed.startsWith("-") && trimmed.length() > 1) {
-                value = trimmed.substring(trimmed.indexOf('-') + 1).trim();
-            } else {
-                value = trimmed;
+        String value = this.scalar.trimmed();
+        Matcher matcher = this.escapedSequenceScalar(this.scalar);
+        if(matcher.matches()) {
+            if (matcher.group(QUOTED_LITERAL_GROUP) != null) {
+                value = matcher.group(QUOTED_LITERAL_GROUP);
+            } else if (matcher.group(MAPPING_GROUP) != null) {
+                value = matcher.group(MAPPING_GROUP).trim();
+            } else if (matcher.group(SEQUENCE_GROUP) != null) {
+                value = matcher.group(SEQUENCE_GROUP).trim();
             }
         }
         if("null".equals(value)) {
@@ -133,9 +174,8 @@ final class ReadPlainScalar extends BaseScalar {
      * @param dashLine Line.
      * @return True of false.
      */
-    private boolean escapedSequenceScalar(final YamlLine dashLine) {
+    private Matcher escapedSequenceScalar(final YamlLine dashLine) {
         final String trimmed = dashLine.trimmed();
-        return trimmed.matches("^[ ]*\\-[ ]*\".*\"$")
-            || trimmed.matches("^[ ]*\\-[ ]*\'.*\'$");
+        return QUOTED_LITERAL_MAP_SEQ.matcher(trimmed);
     }
 }


Test patch:
diff --git a/src/test/java/com/amihaiemil/eoyaml/AllYamlLinesTest.java b/src/test/java/com/amihaiemil/eoyaml/AllYamlLinesTest.java
index e5868de..08def79 100644
--- a/src/test/java/com/amihaiemil/eoyaml/AllYamlLinesTest.java
+++ b/src/test/java/com/amihaiemil/eoyaml/AllYamlLinesTest.java
@@ -180,6 +180,46 @@ public final class AllYamlLinesTest {
         );
     }
 
+    /**
+     * Unit test for AllYamlLines.toYamlNode(). The lines should turn
+     * into a literal with colons but no spaces after the colon into scalar.
+     */
+    @Test
+    public void turnsIntoScalarNotMapping() {
+        final List<YamlLine> lines = new ArrayList<>();
+        lines.add(new RtYamlLine("arn:something:something", 1));
+        final YamlLines yamlLines = new AllYamlLines(lines);
+        YamlNode actual = yamlLines.toYamlNode(new RtYamlLine("---", 0), false);
+        MatcherAssert.assertThat(
+                actual,
+                Matchers.instanceOf(Scalar.class)
+        );
+        MatcherAssert.assertThat(
+                actual.asScalar().value(),
+                Matchers.equalTo("arn:something:something")
+        );
+    }
+
+    /**
+     * Unit test for AllYamlLines.toYamlNode(). The lines should turn
+     * into a literal with a dash but no spaces after the dash into scalar.
+     */
+    @Test
+    public void turnsIntoScalarNotSequence() {
+        final List<YamlLine> lines = new ArrayList<>();
+        lines.add(new RtYamlLine("-foo", 1));
+        final YamlLines yamlLines = new AllYamlLines(lines);
+        YamlNode actual = yamlLines.toYamlNode(new RtYamlLine("---", 0), false);
+        MatcherAssert.assertThat(
+                actual,
+                Matchers.instanceOf(Scalar.class)
+        );
+        MatcherAssert.assertThat(
+                actual.asScalar().value(),
+                Matchers.equalTo("-foo")
+        );
+    }
+
     /**
      * Unit test for AllYamlLines.toYamlNode(). The lines should turn
      * into a (detected) mapping.
@@ -190,9 +230,10 @@ public final class AllYamlLinesTest {
         lines.add(new RtYamlLine("some: mapping", 1));
         lines.add(new RtYamlLine("for: test", 2));
         final YamlLines yamlLines = new AllYamlLines(lines);
+        YamlNode actual = yamlLines.toYamlNode(new RtYamlLine("---", 0), false);
         MatcherAssert.assertThat(
-            yamlLines.toYamlNode(new RtYamlLine("---", 0), false),
-            Matchers.instanceOf(ReadYamlMapping.class)
+                actual,
+                Matchers.instanceOf(ReadYamlMapping.class)
         );
     }
 
diff --git a/src/test/java/com/amihaiemil/eoyaml/PlainStringScalarTest.java b/src/test/java/com/amihaiemil/eoyaml/PlainStringScalarTest.java
index 248667c..5d673d3 100644
--- a/src/test/java/com/amihaiemil/eoyaml/PlainStringScalarTest.java
+++ b/src/test/java/com/amihaiemil/eoyaml/PlainStringScalarTest.java
@@ -103,6 +103,7 @@ public final class PlainStringScalarTest {
         PlainStringScalar second = new PlainStringScalar("java");
         PlainStringScalar digits = new PlainStringScalar("123");
         PlainStringScalar otherDigits = new PlainStringScalar("124");
+        PlainStringScalar nullScalar = new PlainStringScalar(null);
         MatcherAssert.assertThat(first.compareTo(first), Matchers.equalTo(0));
         MatcherAssert.assertThat(first.compareTo(second), Matchers.equalTo(0));
         MatcherAssert.assertThat(
@@ -114,6 +115,12 @@ public final class PlainStringScalarTest {
         MatcherAssert.assertThat(
             digits.compareTo(otherDigits), Matchers.lessThan(0)
         );
+        MatcherAssert.assertThat(
+            first.compareTo(nullScalar), Matchers.greaterThan(0)
+        );
+        MatcherAssert.assertThat(
+            nullScalar.compareTo(first), Matchers.lessThan(0)
+        );
     }
 
     /**
diff --git a/src/test/java/com/amihaiemil/eoyaml/ReadPlainScalarTest.java b/src/test/java/com/amihaiemil/eoyaml/ReadPlainScalarTest.java
index 6d212bc..1f47810 100644
--- a/src/test/java/com/amihaiemil/eoyaml/ReadPlainScalarTest.java
+++ b/src/test/java/com/amihaiemil/eoyaml/ReadPlainScalarTest.java
@@ -54,6 +54,31 @@ public final class ReadPlainScalarTest {
         MatcherAssert.assertThat(scalar.value(), Matchers.equalTo("value"));
     }
 
+    /**
+     * ReadPlainScalar can return the scalar's value from a mapping line.
+     */
+    @Test
+    public void returnsValueFromScalarLineWithColon() {
+        final Scalar scalar = new ReadPlainScalar(
+            new AllYamlLines(new ArrayList<>()),
+            new RtYamlLine("key:value", 0)
+        );
+        MatcherAssert.assertThat(scalar.value(), Matchers.equalTo("key:value"));
+    }
+
+    /**
+     * ReadPlainScalar can return the scalar's value from an
+     * unbalanced/incorrect string escaping.
+     */
+    @Test
+    public void returnsValueFromUnbalancedString() {
+        final Scalar scalar = new ReadPlainScalar(
+            new AllYamlLines(new ArrayList<>()),
+            new RtYamlLine("key: \"value'", 0)
+        );
+        MatcherAssert.assertThat(scalar.value(), Matchers.equalTo("\"value'"));
+    }
+
     /**
      * ReadPlainScalar can return the scalar's unescaped value
      * from a mapping line.
@@ -95,6 +120,19 @@ public final class ReadPlainScalarTest {
         MatcherAssert.assertThat(scalar.value(), Matchers.equalTo("null"));
     }
 
+    /**
+     * ReadPlainScalar can return the scalar's value from a scalar line
+     * (that looks like a sequence line).
+     */
+    @Test
+    public void returnsValueFromScalarLine() {
+        final Scalar scalar = new ReadPlainScalar(
+            new AllYamlLines(new ArrayList<>()),
+            new RtYamlLine("-value", 0)
+        );
+        MatcherAssert.assertThat(scalar.value(), Matchers.equalTo("-value"));
+    }
+
     /**
      * ReadPlainScalar can return the scalar's value from a sequence line.
      */
@@ -107,6 +145,37 @@ public final class ReadPlainScalarTest {
         MatcherAssert.assertThat(scalar.value(), Matchers.equalTo("value"));
     }
 
+    /**
+     * ReadPlainScalar can return the scalar's value from a sequence
+     * line with multiple spaces.
+     */
+    @Test
+    public void returnsValueFromSequenceLineMultipleSpaces() {
+        final Scalar scalar = new ReadPlainScalar(
+            new AllYamlLines(new ArrayList<>()),
+            new RtYamlLine("-    \"value\"", 0)
+        );
+        MatcherAssert.assertThat(scalar.value(), Matchers.equalTo("value"));
+    }
+
+    /**
+     * ReadPlainScalar can return the scalar's value from a sequence line where
+     * there's an extra line.
+     */
+    @Test
+    public void returnsValueOnNextLineFromSequence() {
+        final List<YamlLine> lines = new ArrayList<>();
+        lines.add(new RtYamlLine("sns: ", 0));
+        lines.add(new RtYamlLine("  -", 1));
+        lines.add(new RtYamlLine("    \"arn:aws:sns:{{ r }}:{{ a }}:T\"", 2));
+        final Scalar scalar = new ReadPlainScalar(
+                new AllYamlLines(lines), lines.get(2)
+        );
+        MatcherAssert.assertThat(scalar.value(),
+                Matchers.equalTo(
+                        "arn:aws:sns:{{ r }}:{{ a }}:T"));
+    }
+
     /**
      * ReadPlainScalar can return the scalar's null value from a sequence line.
      */