Skip to content

Commit

Permalink
Merge pull request #51 from allegro/if-else-validation
Browse files Browse the repository at this point in the history 
Fixed message on if with missing else branch, extended error messages
  • Loading branch information
@krzysiekbielicki
krzysiekbielicki authored Apr 4, 2024
2 parents bee44e3 + 7fbd4a7 commit b5c5687
Show file tree
Hide file tree
Showing 6 changed files with 98 additions and 59 deletions.
15 changes: 15 additions & 0 deletions src/main/java/pl/allegro/tech/opel/LabeledStringMatcher.java
View file
Original file line number Diff line number Diff line change
@@ -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);
}
}
28 changes: 17 additions & 11 deletions src/main/java/pl/allegro/tech/opel/OpelEngine.java
View file
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -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) {
Expand Down
11 changes: 7 additions & 4 deletions src/main/java/pl/allegro/tech/opel/OpelParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/pl/allegro/tech/opel/OpelParsingResult.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
19 changes: 9 additions & 10 deletions src/test/groovy/pl/allegro/tech/opel/OpelEngineIntegrationSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
82 changes: 49 additions & 33 deletions src/test/groovy/pl/allegro/tech/opel/OpelEngineMapIntegrationSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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()
}
}

0 comments on commit b5c5687

Please sign in to comment.