Skip to content

Commit

Permalink
Improved error handling and error messages in FilterCompiler.
Browse files Browse the repository at this point in the history
  • Loading branch information
@kallestenflo
kallestenflo committed Nov 26, 2015
1 parent 0fdc030 commit d1475e1
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 65 deletions.
2 changes: 2 additions & 0 deletions json-path/src/main/java/com/jayway/jsonpath/internal/CharacterIndex.java
View file
Expand Up @@ -67,6 +67,7 @@ public int indexOfClosingSquareBracket(int startPosition) {
}
return -1;
}

public int indexOfMatchingCloseChar(int startPosition, char openChar, char closeChar, boolean skipStrings, boolean skipRegex) {
if(charAt(startPosition) != openChar){
throw new InvalidPathException("Expected " + openChar + " but found " + charAt(startPosition));
Expand Down Expand Up @@ -136,6 +137,7 @@ public int indexOfMatchingCloseChar(int startPosition, char openChar, char close
}
return -1;
}

public int indexOfClosingBracket(int startPosition, boolean skipStrings, boolean skipRegex) {
return indexOfMatchingCloseChar(startPosition, OPEN_PARENTHESIS, CLOSE_PARENTHESIS, skipStrings, skipRegex);
}
Expand Down
129 changes: 66 additions & 63 deletions json-path/src/main/java/com/jayway/jsonpath/internal/filter/FilterCompiler.java
View file
Expand Up @@ -41,18 +41,15 @@ public class FilterCompiler {
private static final char TRUE = 't';
private static final char FALSE = 'f';
private static final char NULL = 'n';
private static final char BANG = '!';
private static final char NOT = '!';
private static final char PATTERN = '/';
private static final char IGNORE_CASE = 'i';

private CharacterIndex filter;

public static Filter compile(String filterString) {
try {
FilterCompiler compiler = new FilterCompiler(filterString);
return new CompiledFilter(compiler.compile());
} catch (Exception e){
throw new InvalidPathException("Could not compile inline filter : " + filterString, e);
}
FilterCompiler compiler = new FilterCompiler(filterString);
return new CompiledFilter(compiler.compile());
}

private FilterCompiler(String filterString) {
Expand All @@ -73,61 +70,67 @@ private FilterCompiler(String filterString) {
}

public Predicate compile() {

Stack<LogicalOperator> opsStack = new Stack<LogicalOperator>();
Stack<ExpressionNode> expStack = new Stack<ExpressionNode>();

int unbalancedBrackets = 0;

while (filter.skipBlanks().inBounds()) {
int pos = filter.position();

switch (filter.currentChar()) {
case OPEN_PARENTHESIS:
unbalancedBrackets++;
filter.incrementPosition(1);
break;
case CLOSE_PARENTHESIS:
unbalancedBrackets--;
filter.incrementPosition(1);
ExpressionNode expressionNode = expStack.pop();
if(!opsStack.isEmpty()){
if(expStack.isEmpty()){
throw new InvalidPathException("Expected expression on right hand side of operator");
}
ExpressionNode right = expStack.pop();
expressionNode = ExpressionNode.createExpressionNode(expressionNode, opsStack.pop(), right);
while(!opsStack.isEmpty()){
expressionNode = ExpressionNode.createExpressionNode(expressionNode, opsStack.pop(), expStack.pop());
try {
Stack<LogicalOperator> opsStack = new Stack<LogicalOperator>();
Stack<ExpressionNode> expStack = new Stack<ExpressionNode>();

int unbalancedParenthesis = 0;

while (filter.skipBlanks().inBounds()) {
int pos = filter.position();

switch (filter.currentChar()) {
case OPEN_PARENTHESIS:
unbalancedParenthesis++;
filter.incrementPosition(1);
break;
case CLOSE_PARENTHESIS:
unbalancedParenthesis--;
filter.incrementPosition(1);
ExpressionNode expressionNode = expStack.pop();
if (!opsStack.isEmpty()) {
if (expStack.isEmpty()) {
throw new InvalidPathException("Expected expression on right hand side of operator " + opsStack.peek().getOperatorString() + " in filter " + filter);
}
ExpressionNode right = expStack.pop();
expressionNode = ExpressionNode.createExpressionNode(expressionNode, opsStack.pop(), right);
while (!opsStack.isEmpty()) {
expressionNode = ExpressionNode.createExpressionNode(expressionNode, opsStack.pop(), expStack.pop());
}
}
}
expStack.push(expressionNode);
break;
case BANG:
filter.incrementPosition(1);
break;
case OR:
case AND:
LogicalOperator operatorNode = readLogicalOperator();
opsStack.push(operatorNode);
break;
default:
RelationalExpressionNode relationalExpressionNode = readExpression();
expStack.push(relationalExpressionNode);
break;
expStack.push(expressionNode);
break;
case NOT:
filter.incrementPosition(1);
break;
case OR:
case AND:
LogicalOperator operatorNode = readLogicalOperator();
opsStack.push(operatorNode);
break;
default:
RelationalExpressionNode relationalExpressionNode = readExpression();
expStack.push(relationalExpressionNode);
break;
}
if (pos >= filter.position()) {
throw new InvalidPathException("Failed to parse filter " + filter.toString());
}
}
if(pos >= filter.position()){
throw new InvalidPathException("Failed to parse filter " + filter.toString());
if (unbalancedParenthesis != 0) {
throw new InvalidPathException("Failed to parse filter. Parenthesis are not balanced. " + filter.toString());
}
}
if(unbalancedBrackets != 0){
throw new InvalidPathException("Failed to parse filter. Brackets are not balanced. " + filter.toString());
}

Predicate predicate = expStack.pop();
logger.trace(predicate.toString());
Predicate predicate = expStack.pop();

return predicate;
if (logger.isTraceEnabled()) logger.trace(predicate.toString());

return predicate;
} catch (InvalidPathException e){
throw e;
} catch (Exception e) {
throw new InvalidPathException("Failed to parse filter: " + filter + ", error on position: " + filter.position() + ", char: " + filter.currentChar());
}
}

private ValueNode readValueNode() {
Expand Down Expand Up @@ -205,7 +208,7 @@ private RelationalOperator readRelationalOperator() {

private ValueNode.NullNode readNullLiteral() {
int begin = filter.position();
if(filter.currentChar() == 'n' && filter.inBounds(filter.position() + 3)){
if(filter.currentChar() == NULL && filter.inBounds(filter.position() + 3)){
CharSequence nullValue = filter.subSequence(filter.position(), filter.position() + 4);
if("null".equals(nullValue.toString())){
logger.trace("NullLiteral from {} to {} -> [{}]", begin, filter.position()+3, nullValue);
Expand Down Expand Up @@ -243,7 +246,7 @@ private ValueNode.PatternNode readPattern() {
if (closingIndex == -1) {
throw new InvalidPathException("Pattern not closed. Expected " + PATTERN + " in " + filter);
} else {
if(filter.inBounds(closingIndex+1) && filter.charAt(closingIndex+1) == 'i'){
if(filter.inBounds(closingIndex+1) && filter.charAt(closingIndex+1) == IGNORE_CASE){
closingIndex++;
}
filter.setPosition(closingIndex + 1);
Expand All @@ -258,7 +261,7 @@ private ValueNode.StringNode readStringLiteral(char endChar) {

int closingSingleQuoteIndex = filter.nextIndexOfUnescaped(endChar);
if (closingSingleQuoteIndex == -1) {
throw new InvalidPathException("String not closed. Expected " + endChar + " in " + filter);
throw new InvalidPathException("String literal does not have matching quotes. Expected " + endChar + " in " + filter);
} else {
filter.setPosition(closingSingleQuoteIndex + 1);
}
Expand All @@ -280,7 +283,7 @@ private ValueNode.NumberNode readNumberLiteral() {

private ValueNode.BooleanNode readBooleanLiteral() {
int begin = filter.position();
int end = filter.currentChar() == 't' ? filter.position() + 3 : filter.position() + 4;
int end = filter.currentChar() == TRUE ? filter.position() + 3 : filter.position() + 4;

if(!filter.inBounds(end)){
throw new InvalidPathException("Expected boolean literal");
Expand Down Expand Up @@ -319,7 +322,7 @@ private ValueNode.PathNode readPath() {
}
}

boolean shouldExists = !(previousSignificantChar == BANG);
boolean shouldExists = !(previousSignificantChar == NOT);
CharSequence path = filter.subSequence(begin, filter.position());
return ValueNode.createPathNode(path, false, shouldExists);
}
Expand Down Expand Up @@ -359,7 +362,7 @@ private boolean isLogicalOperatorChar(char c) {
}

private boolean isRelationalOperatorChar(char c) {
return c == LT || c == GT || c == EQ || c == TILDE || c == BANG;
return c == LT || c == GT || c == EQ || c == TILDE || c == NOT;
}

private static final class CompiledFilter extends Filter {
Expand Down
2 changes: 1 addition & 1 deletion json-path/src/main/java/com/jayway/jsonpath/internal/filter/RelationalOperator.java
View file
Expand Up @@ -33,7 +33,7 @@ public static RelationalOperator fromString(String operatorString){
return operator;
}
}
throw new InvalidPathException("Operator " + operatorString + " not supported ");
throw new InvalidPathException("Filter operator " + operatorString + " is not supported!");
}

@Override
Expand Down
5 changes: 4 additions & 1 deletion json-path/src/test/java/com/jayway/jsonpath/FilterCompilerTest.java
View file
Expand Up @@ -7,7 +7,6 @@

public class FilterCompilerTest {


@Test
public void valid_filters_compile() {
assertThat(compile("[?(@)]").toString()).isEqualTo("[?(@)]");
Expand Down Expand Up @@ -60,6 +59,10 @@ public void string_can_contain_path_chars() {
assertThat(compile("[?(@[\")]@$)]\"] == \")]@$)]\")]").toString()).isEqualTo("[?(@[\")]@$)]\"] == \")]@$)]\")]");
}

@Test(expected = InvalidPathException.class)
public void invalid_path_when_string_literal_is_unquoted() {
compile("[?(@.foo == x)]");
}

This comment has been minimized.

Sign in to view

Copy link
@zline

zline Dec 4, 2015

Contributor

I should note that this change (first introduced in 1a72fc0 I think) breaks backward compatibility. Would not it be more user-friendly to accept simple [a-z][a-z0-9_]* strings as string literals?

This comment has been minimized.

Sign in to view

Copy link
@kallestenflo

kallestenflo Dec 6, 2015

Author Contributor

I know, but I consider it a bug that it worked in the first place.
It only makes it harder to parse and I think it's normal to quote your string literals. Does it really add value to support unquoted strings?

I have a 2.1.1 in the pipe that will accept double or single quotes.

This comment has been minimized.

Sign in to view

Copy link
@zline

zline Dec 7, 2015

Contributor

It only makes it harder to parse and I think it's normal to quote your string literals. Does it really add value to support unquoted strings?

It's an interesting question and it's similar to issue #145 (support of non-strict comparison). I think answer depends on who the users of the library are. If only programmers are considered than it's ok to limit such flexibility - they are used to write code in languages with strict syntax and strict type systems. But if we consider not programmers but users with some technical skills like analysts or managers than it's helpful to allow less strict but self-explanatory path expressions. We are using some jsonpath-based functions in our query language which runs on hadoop and our users love to write something quick and powerful like json.find(b_log, '$[?(@.type==snippet && @.db==twitter)].p').

As of parsing of literals, my way of handling this is to define non-contradictory syntax for each subtype of literal and just try to read them in according (to syntax) order. e.g. quoted string, number, boolean, null, object, array, pattern, unquoted string [a-z][a-z0-9_]* or parsing failed. This way is simple and gets rid of excess checks.

@Test
public void invalid_filters_does_not_compile() {
Expand Down

0 comments on commit d1475e1

Please sign in to comment.