Skip to content

Commit 56c60ee

Browse files
agrgragrgr
authored
CLIENT-4526 Fix error listener's silent omitting (#67)
* Fix error handling, update tests * Move control structures from basicExpression to operand to enable composability with comparison operators * Propagate ANTLR error message from lexer and parser combined * Propagate char position parameter from ANTLR error message
1 parent e134da4 commit 56c60ee

File tree

13 files changed

+397
-50
lines changed

13 files changed

+397
-50
lines changed

‎src/main/antlr4/com/aerospike/dsl/Condition.g4‎

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,7 @@ logicalOrExpression
1515
;
1616

1717
logicalAndExpression
18-
: basicExpression ('and' basicExpression)* # AndExpression
19-
;
20-
21-
basicExpression
22-
: 'not' '(' expression ')' # NotExpression
23-
| 'exclusive' '(' expression (',' expression)+ ')' # ExclusiveExpression
24-
| 'let' '(' variableDefinition (',' variableDefinition)* ')' 'then' '(' expression ')' # LetExpression
25-
| 'when' '(' expressionMapping (',' expressionMapping)* ',' 'default' '=>' expression ')' # WhenExpression
26-
| comparisonExpression # ComparisonExpressionWrapper
18+
: comparisonExpression ('and' comparisonExpression)* # AndExpression
2719
;
2820

2921
comparisonExpression
@@ -78,7 +70,7 @@ unaryExpression
7870
;
7971

8072
variableDefinition
81-
: stringOperand '=' expression
73+
: NAME_IDENTIFIER '=' expression
8274
;
8375

8476
expressionMapping
@@ -97,8 +89,20 @@ operand
9789
| placeholder
9890
| '$.' pathOrMetadata
9991
| '(' expression ')'
92+
| notExpression
93+
| exclusiveExpression
94+
| letExpression
95+
| whenExpression
10096
;
10197

98+
notExpression: 'not' '(' expression ')';
99+
100+
exclusiveExpression: 'exclusive' '(' expression (',' expression)+ ')';
101+
102+
letExpression: 'let' '(' variableDefinition (',' variableDefinition)* ')' 'then' '(' expression ')';
103+
104+
whenExpression: 'when' '(' expressionMapping (',' expressionMapping)* ',' 'default' '=>' expression ')';
105+
102106
functionCall
103107
: NAME_IDENTIFIER '(' expression (',' expression)* ')'
104108
;

‎src/main/java/com/aerospike/dsl/impl/DSLParserErrorListener.java‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@
1010

1111
class DSLParserErrorListener extends BaseErrorListener {
1212

13+
private String firstLexerError;
14+
private String firstParserError;
15+
1316
@Override
1417
public void syntaxError(Recognizer<?, ?> recognizer, Object offendingSymbol,
1518
int line, int charPositionInLine,
@@ -18,6 +21,7 @@ public void syntaxError(Recognizer<?, ?> recognizer, Object offendingSymbol,
1821
// LEADING_DOT_FLOAT_HEX_OR_BINARY (.0xff/.0b101) is never accepted by the grammar;
1922
// LEADING_DOT_FLOAT is the offending token in cases like ".3.7" where ".7" is lexed
2023
// as a valid-looking token but appears where an operator was expected.
24+
// These throw immediately to provide a specific error message.
2125
if (token.getType() == ConditionLexer.LEADING_DOT_FLOAT_HEX_OR_BINARY
2226
|| token.getType() == ConditionLexer.LEADING_DOT_FLOAT) {
2327
throw new DslParseException("Invalid float literal: " + token.getText());
@@ -31,5 +35,23 @@ public void syntaxError(Recognizer<?, ?> recognizer, Object offendingSymbol,
3135
throw new DslParseException("Invalid float literal: " + token.getText() + nextText);
3236
}
3337
}
38+
if (recognizer instanceof Parser) {
39+
if (firstParserError == null) {
40+
firstParserError = msg + " at character " + charPositionInLine;
41+
}
42+
} else {
43+
if (firstLexerError == null) {
44+
firstLexerError = msg + " at character " + charPositionInLine;
45+
}
46+
}
47+
}
48+
49+
String getErrorMessage() {
50+
if (firstLexerError != null && firstParserError != null) {
51+
return "[Lexer] " + firstLexerError + "; [Parser] " + firstParserError;
52+
}
53+
if (firstLexerError != null) return "[Lexer] " + firstLexerError;
54+
if (firstParserError != null) return "[Parser] " + firstParserError;
55+
return null;
3456
}
3557
}

‎src/main/java/com/aerospike/dsl/impl/DSLParserImpl.java‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -49,20 +49,35 @@ public CTX[] parseCTX(String pathToCtx) {
4949
throw new DslParseException("Path must not be null or empty");
5050
}
5151

52-
ParseTree parseTree = getParseTree(pathToCtx);
5352
try {
53+
ParseTree parseTree = getParseTree(pathToCtx);
5454
return buildCtx(new ExpressionConditionVisitor().visit(parseTree));
5555
} catch (Exception e) {
5656
throw new DslParseException("Could not parse the given DSL path input", e);
5757
}
5858
}
5959

60-
private ParseTree getParseTree(String input) {
60+
private ConditionParser createParser(String input, DSLParserErrorListener errorListener) {
6161
ConditionLexer lexer = new ConditionLexer(CharStreams.fromString(input));
62+
lexer.removeErrorListeners();
63+
lexer.addErrorListener(errorListener);
6264
CommonTokenStream tokenStream = new CommonTokenStream(lexer);
6365
ConditionParser parser = new ConditionParser(tokenStream);
64-
parser.addErrorListener(new DSLParserErrorListener());
65-
return parser.parse();
66+
parser.removeErrorListeners();
67+
parser.addErrorListener(errorListener);
68+
return parser;
69+
}
70+
71+
private ParseTree getParseTree(String input) {
72+
DSLParserErrorListener errorListener = new DSLParserErrorListener();
73+
ConditionParser parser = createParser(input, errorListener);
74+
ParseTree tree = parser.parse();
75+
76+
String errorMessage = errorListener.getErrorMessage();
77+
if (errorMessage != null) {
78+
throw new DslParseException("Could not parse given DSL expression input: " + errorMessage);
79+
}
80+
return tree;
6681
}
6782

6883
private ParsedExpression getParsedExpression(ParseTree parseTree, PlaceholderValues placeholderValues,

‎src/main/java/com/aerospike/dsl/visitor/ExpressionConditionVisitor.java‎

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ public AbstractPart visitLetExpression(ConditionParser.LetExpressionContext ctx)
5454
// iterate through each definition
5555
for (ConditionParser.VariableDefinitionContext vdc : ctx.variableDefinition()) {
5656
AbstractPart part = visit(vdc.expression());
57-
LetOperand letOperand = new LetOperand(part, vdc.stringOperand().getText());
57+
LetOperand letOperand = new LetOperand(part, vdc.NAME_IDENTIFIER().getText());
5858
expressions.add(letOperand);
5959
}
6060
// last expression is the action (described after "then")
@@ -86,14 +86,12 @@ public AbstractPart visitWhenExpression(ConditionParser.WhenExpressionContext ct
8686

8787
@Override
8888
public AbstractPart visitAndExpression(ConditionParser.AndExpressionContext ctx) {
89-
// If there's only one basicExpression and no 'and' operators, just pass through
90-
if (ctx.basicExpression().size() == 1) {
91-
return visit(ctx.basicExpression(0));
89+
if (ctx.comparisonExpression().size() == 1) {
90+
return visit(ctx.comparisonExpression(0));
9291
}
9392

9493
List<ExpressionContainer> expressions = new ArrayList<>();
95-
// iterate through each sub-expression
96-
for (ConditionParser.BasicExpressionContext ec : ctx.basicExpression()) {
94+
for (ConditionParser.ComparisonExpressionContext ec : ctx.comparisonExpression()) {
9795
ExpressionContainer expr = (ExpressionContainer) visit(ec);
9896
if (expr == null) return null;
9997

@@ -146,12 +144,6 @@ public AbstractPart visitExclusiveExpression(ConditionParser.ExclusiveExpression
146144
ExpressionContainer.ExprPartsOperation.EXCLUSIVE_STRUCTURE);
147145
}
148146

149-
@Override
150-
public AbstractPart visitComparisonExpressionWrapper(ConditionParser.ComparisonExpressionWrapperContext ctx) {
151-
// Pass through the wrapper
152-
return visit(ctx.comparisonExpression());
153-
}
154-
155147
@Override
156148
public AbstractPart visitBitwiseExpressionWrapper(ConditionParser.BitwiseExpressionWrapperContext ctx) {
157149
// Pass through the wrapper

‎src/test/java/com/aerospike/dsl/expression/BinExpressionsTests.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,9 @@ void binNotEquals() {
6666
void negativeStringBinEquals() {
6767
assertThatThrownBy(() -> parseFilterExp(ExpressionContext.of("$.strBin == yes")))
6868
.isInstanceOf(DslParseException.class)
69-
.hasMessageContaining("Unexpected identifier");
69+
.hasMessageContaining("Could not parse given DSL expression input")
70+
.hasMessageContaining("[Parser] mismatched input '<EOF>'")
71+
.hasMessageContaining("at character 15");
7072
}
7173

7274
// TODO: Will be handled in FMWK-486

‎src/test/java/com/aerospike/dsl/expression/ControlStructuresTests.java‎

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,48 @@ void whenWithMultipleDeclarations() {
6767
expected);
6868
}
6969

70+
@Test
71+
void whenWithArithmeticBranchesComparedToValue() {
72+
Exp expected = Exp.eq(
73+
Exp.cond(
74+
Exp.eq(Exp.intBin("A"), Exp.val(0)), Exp.add(Exp.intBin("D"), Exp.intBin("E")),
75+
Exp.eq(Exp.intBin("A"), Exp.val(1)), Exp.sub(Exp.intBin("D"), Exp.intBin("E")),
76+
Exp.eq(Exp.intBin("A"), Exp.val(2)), Exp.mul(Exp.intBin("D"), Exp.intBin("E")),
77+
Exp.val(-1)
78+
),
79+
Exp.val(2)
80+
);
81+
82+
TestUtils.parseFilterExpressionAndCompare(
83+
ExpressionContext.of("when($.A == 0 => $.D + $.E, " +
84+
"$.A == 1 => $.D - $.E, " +
85+
"$.A == 2 => $.D * $.E, " +
86+
"default => -1) == 2"),
87+
expected);
88+
}
89+
90+
@Test
91+
void notWhenWithArithmeticBranches() {
92+
Exp expected = Exp.not(
93+
Exp.eq(
94+
Exp.cond(
95+
Exp.eq(Exp.intBin("A"), Exp.val(0)), Exp.add(Exp.intBin("D"), Exp.intBin("E")),
96+
Exp.eq(Exp.intBin("A"), Exp.val(1)), Exp.sub(Exp.intBin("D"), Exp.intBin("E")),
97+
Exp.eq(Exp.intBin("A"), Exp.val(2)), Exp.mul(Exp.intBin("D"), Exp.intBin("E")),
98+
Exp.val(-1)
99+
),
100+
Exp.val(2)
101+
)
102+
);
103+
104+
TestUtils.parseFilterExpressionAndCompare(
105+
ExpressionContext.of("not(when($.A == 0 => $.D + $.E, " +
106+
"$.A == 1 => $.D - $.E, " +
107+
"$.A == 2 => $.D * $.E, " +
108+
"default => -1) == 2)"),
109+
expected);
110+
}
111+
70112
@Test
71113
void withMultipleVariablesDefinitionAndUsage() {
72114
Exp expected = Exp.let(
@@ -81,7 +123,7 @@ void withMultipleVariablesDefinitionAndUsage() {
81123

82124
TestUtils.parseFilterExpressionAndCompare(ExpressionContext.of("let (x = 1, y = ${x} + 1) then (${x} + ${y})"),
83125
expected);
84-
// different spacing style
126+
// Different spacing style
85127
TestUtils.parseFilterExpressionAndCompare(ExpressionContext.of("let(x = 1, y = ${x} + 1) then(${x} + ${y})"),
86128
expected);
87129
}

‎src/test/java/com/aerospike/dsl/expression/ExplicitTypesTests.java‎

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,9 @@ void stringComparisonNegativeTest() {
4848
TestUtils.parseFilterExpressionAndCompare(ExpressionContext.of("$.stringBin1.get(type: STRING) == yes"),
4949
Exp.eq(Exp.stringBin("stringBin1"), Exp.val("yes"))))
5050
.isInstanceOf(DslParseException.class)
51-
.hasMessageContaining("Unexpected identifier");
51+
.hasMessageContaining("Could not parse given DSL expression input")
52+
.hasMessageContaining("[Parser] mismatched input '<EOF>'")
53+
.hasMessageContaining("at character 37");
5254
}
5355

5456
@Test
@@ -130,7 +132,9 @@ void listComparison_constantOnRightSide_NegativeTest() {
130132
Exp.eq(Exp.listBin("listBin1"), Exp.val(List.of("yes", "of course"))))
131133
)
132134
.isInstanceOf(DslParseException.class)
133-
.hasMessageContaining("Unexpected identifier");
135+
.hasMessageContaining("Could not parse given DSL expression input")
136+
.hasMessageContaining("[Parser] mismatched input ','")
137+
.hasMessageContaining("at character 34");
134138
}
135139

136140
@Test
@@ -173,7 +177,9 @@ void listComparison_constantOnLeftSide_NegativeTest() {
173177
Exp.eq(Exp.val(List.of("yes", "of course")), Exp.listBin("listBin1")))
174178
)
175179
.isInstanceOf(DslParseException.class)
176-
.hasMessage("Could not parse given DSL expression input");
180+
.hasMessageContaining("Could not parse given DSL expression input")
181+
.hasMessageContaining("[Parser] no viable alternative at input")
182+
.hasMessageContaining("at character 4");
177183
}
178184

179185
@SuppressWarnings("unchecked")
@@ -243,7 +249,9 @@ void mapComparison_constantOnRightSide_NegativeTest() {
243249
Exp.eq(Exp.mapBin("mapBin1"), Exp.val(treeMapOf("yes", "of course"))))
244250
)
245251
.isInstanceOf(DslParseException.class)
246-
.hasMessage("Unable to parse map operand");
252+
.hasMessageContaining("Could not parse given DSL expression input")
253+
.hasMessageContaining("[Parser] extraneous input 'yes'")
254+
.hasMessageContaining("at character 29");
247255

248256
assertThatThrownBy(() ->
249257
TestUtils.parseFilterExpressionAndCompare(ExpressionContext.of("$.mapBin1.get(type: MAP) == ['yes', 'of course']"),
@@ -258,7 +266,9 @@ void mapComparison_constantOnRightSide_NegativeTest() {
258266
Exp.eq(Exp.mapBin("mapBin1"), Exp.val(List.of("yes", "of course"))))
259267
)
260268
.isInstanceOf(DslParseException.class)
261-
.hasMessage("Unable to parse map operand");
269+
.hasMessageContaining("Could not parse given DSL expression input")
270+
.hasMessageContaining("[Parser] extraneous input '['")
271+
.hasMessageContaining("at character 29");
262272
}
263273

264274
@Test
@@ -312,7 +322,9 @@ void mapComparison_constantOnLeftSide_NegativeTest() {
312322
Exp.eq(Exp.mapBin("mapBin1"), Exp.val(treeMapOf("of course", "yes"))))
313323
)
314324
.isInstanceOf(DslParseException.class)
315-
.hasMessage("Could not parse given DSL expression input");
325+
.hasMessageContaining("Could not parse given DSL expression input")
326+
.hasMessageContaining("[Parser] no viable alternative at input")
327+
.hasMessageContaining("at character 1");
316328

317329
assertThatThrownBy(() ->
318330
TestUtils.parseFilterExpressionAndCompare(ExpressionContext.of("['yes', 'of course'] == $.mapBin1.get(type: MAP)"), // incorrect: must be {}
@@ -327,7 +339,9 @@ void mapComparison_constantOnLeftSide_NegativeTest() {
327339
Exp.eq(Exp.val(List.of("yes", "of course")), Exp.mapBin("mapBin1")))
328340
)
329341
.isInstanceOf(DslParseException.class)
330-
.hasMessage("Could not parse given DSL expression input");
342+
.hasMessageContaining("Could not parse given DSL expression input")
343+
.hasMessageContaining("[Parser] no viable alternative at input")
344+
.hasMessageContaining("at character 1");
331345
}
332346

333347
@Test

‎src/test/java/com/aerospike/dsl/expression/InNegativeTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -348,7 +348,7 @@ void negVarBoundToCountPath() {
348348
void negVarDefLetInScalarVar() {
349349
assertThatThrownBy(() -> parseFilterExp(
350350
ExpressionContext.of(
351-
"let(x = 5, y = ($.bin.get(type: INT) in ${x})) then (y == true)")))
351+
"let(x = 5, y = ($.bin.get(type: INT) in ${x})) then (${y} == true)")))
352352
.isInstanceOf(DslParseException.class)
353353
.hasMessageContaining("IN operation requires a List as the right operand")
354354
.hasMessageContaining("variable 'x'");

‎src/test/java/com/aerospike/dsl/expression/ListExpressionsTests.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -306,7 +306,9 @@ void listBinElementCount() {
306306
void negativeSyntaxList() {
307307
assertThatThrownBy(() -> parseFilterExp(ExpressionContext.of("$.listBin1.[stringValue] == 100")))
308308
.isInstanceOf(DslParseException.class)
309-
.hasMessageContaining("Could not parse given DSL expression input");
309+
.hasMessageContaining("Could not parse given DSL expression input")
310+
.hasMessageContaining("[Parser] no viable alternative at input")
311+
.hasMessageContaining("at character 12");
310312
}
311313

312314
//@Test

‎src/test/java/com/aerospike/dsl/expression/LogicalExpressionsTests.java‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -94,19 +94,27 @@ void flatHierarchyAnd() {
9494
void negativeSyntaxLogicalOperators() {
9595
assertThatThrownBy(() -> parseFilterExp(ExpressionContext.of("($.intBin1 > 100 and ($.intBin2 > 100) or")))
9696
.isInstanceOf(DslParseException.class)
97-
.hasMessageContaining("Could not parse given DSL expression input");
97+
.hasMessageContaining("Could not parse given DSL expression input")
98+
.hasMessageContaining("[Parser] no viable alternative at input")
99+
.hasMessageContaining("at character 41");
98100

99101
assertThatThrownBy(() -> parseFilterExp(ExpressionContext.of("and ($.intBin1 > 100 and ($.intBin2 > 100)")))
100102
.isInstanceOf(DslParseException.class)
101-
.hasMessageContaining("Could not parse given DSL expression input");
103+
.hasMessageContaining("Could not parse given DSL expression input")
104+
.hasMessageContaining("[Parser] extraneous input 'and'")
105+
.hasMessageContaining("at character 0");
102106

103107
assertThatThrownBy(() -> parseFilterExp(ExpressionContext.of("($.intBin1 > 100 and ($.intBin2 > 100) not")))
104108
.isInstanceOf(DslParseException.class)
105-
.hasMessageContaining("Could not parse given DSL expression input");
109+
.hasMessageContaining("Could not parse given DSL expression input")
110+
.hasMessageContaining("[Parser] no viable alternative at input")
111+
.hasMessageContaining("at character 39");
106112

107113
assertThatThrownBy(() -> parseFilterExp(ExpressionContext.of("($.intBin1 > 100 and ($.intBin2 > 100) exclusive")))
108114
.isInstanceOf(DslParseException.class)
109-
.hasMessageContaining("Could not parse given DSL expression input");
115+
.hasMessageContaining("Could not parse given DSL expression input")
116+
.hasMessageContaining("[Parser] no viable alternative at input")
117+
.hasMessageContaining("at character 39");
110118
}
111119

112120
@Test
@@ -115,6 +123,8 @@ void negativeBinLogicalExclusiveWithOneParam() {
115123
Exp.exclusive(
116124
Exp.eq(Exp.stringBin("hand"), Exp.val("hook")))))
117125
.isInstanceOf(DslParseException.class)
118-
.hasMessage("Exclusive logical operator requires 2 or more expressions");
126+
.hasMessageContaining("Could not parse given DSL expression input")
127+
.hasMessageContaining("[Parser] no viable alternative at input")
128+
.hasMessageContaining("at character 26");
119129
}
120130
}

0 commit comments

Comments
 (0)