Fixed message on if with missing else branch, extended error messages

allegro · Apr 4, 2024 · 7fbd4a7 · 7fbd4a7
1 parent bee44e3
commit 7fbd4a7
Show file tree

Hide file tree

Showing 6 changed files with 98 additions and 59 deletions.
diff --git a/src/main/java/pl/allegro/tech/opel/LabeledStringMatcher.java b/src/main/java/pl/allegro/tech/opel/LabeledStringMatcher.java
@@ -0,0 +1,15 @@
+package pl.allegro.tech.opel;
+
+import org.parboiled.Rule;
+import org.parboiled.matchers.StringMatcher;
+
+class LabeledStringMatcher extends StringMatcher {
+    public LabeledStringMatcher(Rule[] matchers, char[] characters) {
+        super(matchers, characters);
+    }
+
+    @Override
+    public String getLabel() {
+        return String.valueOf(characters);
+    }
+}
diff --git a/src/main/java/pl/allegro/tech/opel/OpelEngine.java b/src/main/java/pl/allegro/tech/opel/OpelEngine.java
@@ -1,11 +1,15 @@
 package pl.allegro.tech.opel;
 
 import org.parboiled.Parboiled;
+import org.parboiled.errors.ErrorUtils;
 import org.parboiled.errors.ParseError;
 import org.parboiled.parserunners.ReportingParseRunner;
 import org.parboiled.support.ParsingResult;
 
+import java.util.Collections;
+import java.util.StringJoiner;
 import java.util.concurrent.CompletableFuture;
+import java.util.stream.Collector;
 import java.util.stream.Collectors;
 
 public class OpelEngine {
@@ -41,17 +45,19 @@ public CompletableFuture<?> eval(String expression) {
     }
 
     private String additionalErrorMsg(ParsingResult<OpelNode> parsingResult) {
-        boolean hasAdditionalMsg = parsingResult.parseErrors.stream()
-                .map(ParseError::getErrorMessage)
-                .anyMatch(it -> it != null);
-        if (hasAdditionalMsg) {
-            return parsingResult.parseErrors.stream()
-                    .map(ParseError::getErrorMessage)
-                    .filter(it -> it != null)
-                    .collect(Collectors.joining(";", " because of ", ""));
-        } else {
-            return "";
-        }
+        var stringJoiner = new StringJoiner(";", " because of ", "");
+        stringJoiner.setEmptyValue("");
+        Collector<CharSequence, ?, String> joinerCollector = Collector.of(
+            () -> stringJoiner,
+            StringJoiner::add,
+            StringJoiner::merge,
+            StringJoiner::toString
+        );
+        return parsingResult.parseErrors.stream()
+            .map(ErrorUtils::printParseError)
+            .filter(it -> it != null)
+            .collect(joinerCollector);
+
     }
 
     public CompletableFuture<?> eval(String expression, EvalContext evalContext) {

diff --git a/src/main/java/pl/allegro/tech/opel/OpelParser.java b/src/main/java/pl/allegro/tech/opel/OpelParser.java
@@ -3,9 +3,9 @@
 import org.parboiled.BaseParser;
 import org.parboiled.Rule;
 import org.parboiled.annotations.BuildParseTree;
+import org.parboiled.annotations.DontLabel;
 import org.parboiled.annotations.SuppressSubnodes;
 
-import java.lang.invoke.MethodHandles;
 import java.math.BigDecimal;
 
 @BuildParseTree
@@ -334,10 +334,13 @@ Rule WhiteSpace() {
 
     // Handy method for handling whitespaces, see: https://github.com/sirthias/parboiled/wiki/Handling-Whitespace
     @Override
+    @DontLabel
     protected Rule fromStringLiteral(String string) {
-        return string.endsWith(" ") ?
-                Sequence(String(string.substring(0, string.length() - 1)), WhiteSpace()) :
-                String(string);
+        if (string.endsWith(" ")) {
+            var substring = string.substring(0, string.length() - 1);
+            return Sequence(String(substring), WhiteSpace()).label("'" + substring + "'");
+        }
+        return String(string).label("'" + string + "'");
     }
 
     protected String escapeString(String string) {

diff --git a/src/main/java/pl/allegro/tech/opel/OpelParsingResult.java b/src/main/java/pl/allegro/tech/opel/OpelParsingResult.java
@@ -30,7 +30,7 @@ public CompletableFuture<?> eval() {
     private CompletableFuture<?> evalWithFinalContext(EvalContext context) {
         return getParsedExpression()
                 .map(node -> node.getValue(context))
-                .orElseThrow(() -> new OpelException("Expression '" + expression + "' contain's syntax error"));
+                .orElseThrow(() -> new OpelException("Expression '" + expression + "' contain's syntax error " + ErrorUtils.printParseErrors(parsingResult)));
     }
 
     private Optional<OpelNode> getParsedExpression() {

diff --git a/src/test/groovy/pl/allegro/tech/opel/OpelEngineIntegrationSpec.groovy b/src/test/groovy/pl/allegro/tech/opel/OpelEngineIntegrationSpec.groovy
@@ -220,21 +220,20 @@ xyz'"""
 
         then:
         validationResult.errorMessage ==
-                '''Invalid input ',', expected ' ', '\\t', '\\n', fromStringLiteral or EOI (line 1, pos 3):
+                '''Invalid input ',', expected ' ', '\\t', '\\n', '*', '/', '+', '-', '>', '>=', '<', '<=', '==', '!=', '&&', '||', ';' or EOI (line 1, pos 3):
                   |1 ,= 5
                   |  ^\n'''.stripMargin()
     }
 
-    public static class RichString {
-        private final String delegate;
-
-        RichString(String delegate) {
-            this.delegate = delegate
-        }
+    def 'validation should return proper message for if expression with missing else'() {
+        when:
+        ExpressionValidationResult validationResult = create().build().validate("if (2 == 2) 'elo'")
 
-        String rev() {
-            return delegate.reverse()
-        }
+        then:
+        validationResult.errorMessage ==
+                '''Unexpected end of input, expected WhiteSpace, Train, '*', '/', '+', '-', '>', '>=', '<', '<=', '==', '!=', '&&', '||' or 'else' (line 1, pos 18):
+                  |if (2 == 2) 'elo'
+                  |                 ^\n'''.stripMargin()
     }
 
     @Unroll

diff --git a/src/test/groovy/pl/allegro/tech/opel/OpelEngineMapIntegrationSpec.groovy b/src/test/groovy/pl/allegro/tech/opel/OpelEngineMapIntegrationSpec.groovy
@@ -35,10 +35,10 @@ class OpelEngineMapIntegrationSpec extends Specification {
         engine.eval(input, EvalContextBuilder.create().withValues(values).build()).get() == new HashMap(expResult)
 
         where:
-        input                           || values                                          || expResult
-        "{x: (6+7), 'y':3 }"            || [x: CompletableFuture.completedFuture(2.0)]     || [(2.0): 13, y: 3]
-        "{x: 6+7, 'y':3 }"              || [x: CompletableFuture.completedFuture(2.0)]     || [(2.0): 13, y: 3]
-        "{x: 6+7, 'y':3 }"              || [x: CompletableFuture.completedFuture(null)]    || [(null): 13, y: 3]
+        input                || values                                       || expResult
+        "{x: (6+7), 'y':3 }" || [x: CompletableFuture.completedFuture(2.0)]  || [(2.0): 13, y: 3]
+        "{x: 6+7, 'y':3 }"   || [x: CompletableFuture.completedFuture(2.0)]  || [(2.0): 13, y: 3]
+        "{x: 6+7, 'y':3 }"   || [x: CompletableFuture.completedFuture(null)] || [(null): 13, y: 3]
     }
 
     @Unroll
@@ -50,12 +50,12 @@ class OpelEngineMapIntegrationSpec extends Specification {
         engine.eval(input).get() == expResult
 
         where:
-        input                              || expResult
-        "{(1+1.5): 2, 'y':3 }"             || [2.5: 2, y: 3]
-        "{('x' + 'x'):2}"                  || [xx: 2]
-        "{[].size():2}"                    || [0: 2]
-        "{([].size()):2}"                  || [0: 2]
-        "{(['1'].size()):['1'].size()}"    || [1: 1]
+        input                           || expResult
+        "{(1+1.5): 2, 'y':3 }"          || [2.5: 2, y: 3]
+        "{('x' + 'x'):2}"               || [xx: 2]
+        "{[].size():2}"                 || [0: 2]
+        "{([].size()):2}"               || [0: 2]
+        "{(['1'].size()):['1'].size()}" || [1: 1]
     }
 
     @Unroll
@@ -67,64 +67,64 @@ class OpelEngineMapIntegrationSpec extends Specification {
         engine.eval(input).get() == expResult
 
         where:
-        input                                    || expResult
-        "val x = {('x' + 'x'):2}; x.xx"          || 2
-        "val x = {('x' + 'x'):2}; x.get('xx')"   || 2
-        "val x = {('x' + 'x'):2}; x.xx"          || 2
-        "val x = {('x' + 'x'):2}; x['xx']"       || 2
+        input                                  || expResult
+        "val x = {('x' + 'x'):2}; x.xx"        || 2
+        "val x = {('x' + 'x'):2}; x.get('xx')" || 2
+        "val x = {('x' + 'x'):2}; x.xx"        || 2
+        "val x = {('x' + 'x'):2}; x['xx']"     || 2
     }
 
     @Unroll
     def 'should access map with [] notation'() {
         given:
         def engine = create()
-                .withCompletedValue('aMap', ['a' : 'x', 'b' : 'y', 'c' : 'z', (false) : 'xyz'])
+                .withCompletedValue('aMap', ['a': 'x', 'b': 'y', 'c': 'z', (false): 'xyz'])
                 .build()
 
         expect:
         engine.eval(input).get() == expResult
 
         where:
-        input                   || expResult
-        "{'x':2, 'y':3}['x']"   || 2
-        "aMap['b']"             || 'y'
-        "aMap[true == false]"   || 'xyz'
+        input                 || expResult
+        "{'x':2, 'y':3}['x']" || 2
+        "aMap['b']"           || 'y'
+        "aMap[true == false]" || 'xyz'
     }
 
     @Unroll
     def 'should access map with dot (.) notation'() {
         given:
         def engine = create()
-                .withCompletedValue('aMap', ['a' : 'x', 'b' : 'y', 'c' : 'z'])
+                .withCompletedValue('aMap', ['a': 'x', 'b': 'y', 'c': 'z'])
                 .build()
 
         expect:
         engine.eval(input).get() == expResult
 
         where:
-        input                          || expResult
-        "{'x':2, 'y':5}.x"             || 2
-        "aMap.b"                       || 'y'
+        input              || expResult
+        "{'x':2, 'y':5}.x" || 2
+        "aMap.b"           || 'y'
     }
 
-    static def fun = { it -> it.get(0).thenApply{x -> x+x} } as OpelAsyncFunction
+    static def fun = { it -> it.get(0).thenApply { x -> x + x } } as OpelAsyncFunction
 
     @Unroll
     def 'should prefer method call over field access with function as a value (#input)'() {
         given:
         def engine = create()
-                .withCompletedValue('aMap', ['get' : fun])
+                .withCompletedValue('aMap', ['get': fun])
                 .build()
 
         expect:
         engine.eval(input).get() == expResult
 
         where:
-        input                              || expResult
-        "aMap.get('get')"                  || fun
-        "(aMap.get)('get')"                || 'getget'
-        "({'get': x->x+x}.get('get'))('g')"|| 'gg'
-        "({'get': x->x+x}.get)('get')"     || 'getget'
+        input                               || expResult
+        "aMap.get('get')"                   || fun
+        "(aMap.get)('get')"                 || 'getget'
+        "({'get': x->x+x}.get('get'))('g')" || 'gg'
+        "({'get': x->x+x}.get)('get')"      || 'getget'
     }
 
     @Unroll
@@ -137,9 +137,25 @@ class OpelEngineMapIntegrationSpec extends Specification {
 
         then:
         OpelException ex = thrown()
-        ex.getMessage() == "Error parsing expression: '${input}'"
+        ex.getMessage() == "Error parsing expression: '$input' because of Invalid input $reason"
 
         where:
-        input << ["{}", "{ }", "{, }", "{  , }"]
+        input    || reason
+        '{}'     || ''''}', expected WhiteSpace or Pairs (line 1, pos 2):
+                       |{}
+                       | ^
+                       |'''.stripMargin()
+        '{ }'    || ''''}', expected ' ', '\\t', '\\n' or Pairs (line 1, pos 3):
+                       |{ }
+                       |  ^
+                       |'''.stripMargin()
+        '{, }'   || '''',', expected WhiteSpace or Pairs (line 1, pos 2):
+                       |{, }
+                       | ^
+                       |'''.stripMargin()
+        '{  , }' || '''',', expected ' ', '\\t', '\\n' or Pairs (line 1, pos 4):
+                       |{  , }
+                       |   ^
+                       |'''.stripMargin()
     }
 }