From 67b3ce788818da55859c19105533786bff02debc Mon Sep 17 00:00:00 2001 From: Alexey Makeyev Date: Wed, 23 Dec 2015 14:43:11 +0300 Subject: [PATCH 1/4] fix for incorrect handling of logical operator priorities --- .../jsonpath/internal/CharacterIndex.java | 19 +++ .../internal/filter/FilterCompiler.java | 151 ++++++++++-------- .../filter/LogicalExpressionNode.java | 14 ++ .../jayway/jsonpath/FilterCompilerTest.java | 10 +- .../com/jayway/jsonpath/PathCompilerTest.java | 14 ++ 5 files changed, 141 insertions(+), 67 deletions(-) diff --git a/json-path/src/main/java/com/jayway/jsonpath/internal/CharacterIndex.java b/json-path/src/main/java/com/jayway/jsonpath/internal/CharacterIndex.java index 503c1dde..b93811c3 100644 --- a/json-path/src/main/java/com/jayway/jsonpath/internal/CharacterIndex.java +++ b/json-path/src/main/java/com/jayway/jsonpath/internal/CharacterIndex.java @@ -198,6 +198,25 @@ public class CharacterIndex { } } + public void readSignificantChar(char c) { + if (skipBlanks().currentChar() != c) { + throw new InvalidPathException(String.format("Expected character: %c", c)); + } + incrementPosition(1); + } + + public void readSignificantSubSequence(CharSequence s) { + skipBlanks(); + if (! inBounds(position + s.length() - 1)) { + throw new InvalidPathException(String.format("End of string reached while expecting: %s", s)); + } + if (! subSequence(position, position + s.length()).equals(s)) { + throw new InvalidPathException(String.format("Expected: %s", s)); + } + + incrementPosition(s.length()); + } + public int indexOfPreviousSignificantChar(int startPosition){ int readPosition = startPosition - 1; while (!isOutOfBounds(readPosition) && charAt(readPosition) == SPACE) { diff --git a/json-path/src/main/java/com/jayway/jsonpath/internal/filter/FilterCompiler.java b/json-path/src/main/java/com/jayway/jsonpath/internal/filter/FilterCompiler.java index 1f0b11ed..7126b278 100644 --- a/json-path/src/main/java/com/jayway/jsonpath/internal/filter/FilterCompiler.java +++ b/json-path/src/main/java/com/jayway/jsonpath/internal/filter/FilterCompiler.java @@ -7,7 +7,8 @@ import com.jayway.jsonpath.internal.CharacterIndex; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.util.Stack; +import java.util.ArrayList; +import java.util.List; public class FilterCompiler { private static final Logger logger = LoggerFactory.getLogger(FilterCompiler.class); @@ -71,64 +72,14 @@ public class FilterCompiler { public Predicate compile() { try { - Stack opsStack = new Stack(); - Stack expStack = new Stack(); - - 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 NOT: - filter.incrementPosition(1); - break; - case OR: - case AND: - LogicalOperator operatorNode = readLogicalOperator(); - opsStack.push(operatorNode); - break; - default: - if(expStack.size() > 0 && opsStack.isEmpty()){ - throw new InvalidPathException("Expected logical operator (&&, ||) to follow expression " + expStack.peek().toString()); - } - RelationalExpressionNode relationalExpressionNode = readExpression(); - expStack.push(relationalExpressionNode); - break; - } - 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()); - } - - Predicate predicate = expStack.pop(); - - if (logger.isTraceEnabled()) logger.trace(predicate.toString()); - - return predicate; + final ExpressionNode result = readLogicalOR(); + filter.skipBlanks(); + if (filter.inBounds()) { + throw new InvalidPathException(String.format("Expected end of filter expression instead of: %s", + filter.subSequence(filter.position(), filter.length()))); + } + + return result; } catch (InvalidPathException e){ throw e; } catch (Exception e) { @@ -140,6 +91,13 @@ public class FilterCompiler { switch (filter.skipBlanks().currentChar()) { case DOC_CONTEXT : return readPath(); case EVAL_CONTEXT : return readPath(); + case NOT: + filter.incrementPosition(1); + switch (filter.skipBlanks().currentChar()) { + case DOC_CONTEXT : return readPath(); + case EVAL_CONTEXT : return readPath(); + default: throw new InvalidPathException(String.format("Unexpected character: %c", NOT)); + } default : return readLiteral(); } } @@ -159,19 +117,80 @@ public class FilterCompiler { } } + /* + * LogicalOR = LogicalAND { '||' LogicalAND } + * LogicalAND = LogicalANDOperand { '&&' LogicalANDOperand } + * LogicalANDOperand = RelationalExpression | '(' LogicalOR ')' + * RelationalExpression = Value [ RelationalOperator Value ] + */ + + private ExpressionNode readLogicalOR() { + final List ops = new ArrayList(); + ops.add(readLogicalAND()); + + while (true) { + int savepoint = filter.position(); + try { + filter.readSignificantSubSequence(LogicalOperator.OR.getOperatorString()); + ops.add(readLogicalAND()); + } + catch (InvalidPathException exc) { + filter.setPosition(savepoint); + break; + } + } + + return 1 == ops.size() ? ops.get(0) : LogicalExpressionNode.createLogicalOr(ops); + } + + private ExpressionNode readLogicalAND() { + /// @fixme copy-pasted + final List ops = new ArrayList(); + ops.add(readLogicalANDOperand()); + + while (true) { + int savepoint = filter.position(); + try { + filter.readSignificantSubSequence(LogicalOperator.AND.getOperatorString()); + ops.add(readLogicalANDOperand()); + } + catch (InvalidPathException exc) { + filter.setPosition(savepoint); + break; + } + } + + return 1 == ops.size() ? ops.get(0) : LogicalExpressionNode.createLogicalAnd(ops); + } + + private ExpressionNode readLogicalANDOperand() { + if (filter.skipBlanks().currentCharIs(OPEN_PARENTHESIS)) { + filter.readSignificantChar(OPEN_PARENTHESIS); + final ExpressionNode op = readLogicalOR(); + filter.readSignificantChar(CLOSE_PARENTHESIS); + return op; + } + + return readExpression(); + } + private RelationalExpressionNode readExpression() { ValueNode left = readValueNode(); - if(expressionIsTerminated()) { - ValueNode.PathNode pathNode = left.asPathNode(); - left = pathNode.asExistsCheck(pathNode.shouldExists()); - RelationalOperator operator = RelationalOperator.EXISTS; - ValueNode right = left.asPathNode().shouldExists() ? ValueNode.TRUE : ValueNode.FALSE; - return new RelationalExpressionNode(left, operator, right); - } else { + int savepoint = filter.position(); + try { RelationalOperator operator = readRelationalOperator(); ValueNode right = readValueNode(); return new RelationalExpressionNode(left, operator, right); } + catch (InvalidPathException exc) { + filter.setPosition(savepoint); + } + + ValueNode.PathNode pathNode = left.asPathNode(); + left = pathNode.asExistsCheck(pathNode.shouldExists()); + RelationalOperator operator = RelationalOperator.EXISTS; + ValueNode right = left.asPathNode().shouldExists() ? ValueNode.TRUE : ValueNode.FALSE; + return new RelationalExpressionNode(left, operator, right); } private LogicalOperator readLogicalOperator(){ diff --git a/json-path/src/main/java/com/jayway/jsonpath/internal/filter/LogicalExpressionNode.java b/json-path/src/main/java/com/jayway/jsonpath/internal/filter/LogicalExpressionNode.java index a16ac842..3a5d9605 100644 --- a/json-path/src/main/java/com/jayway/jsonpath/internal/filter/LogicalExpressionNode.java +++ b/json-path/src/main/java/com/jayway/jsonpath/internal/filter/LogicalExpressionNode.java @@ -3,6 +3,7 @@ package com.jayway.jsonpath.internal.filter; import com.jayway.jsonpath.internal.Utils; import java.util.ArrayList; +import java.util.Collection; import java.util.List; public class LogicalExpressionNode extends ExpressionNode { @@ -13,16 +14,29 @@ public class LogicalExpressionNode extends ExpressionNode { return new LogicalExpressionNode(left, LogicalOperator.OR, right); } + public static LogicalExpressionNode createLogicalOr(Collection operands){ + return new LogicalExpressionNode(LogicalOperator.OR, operands); + } + public static LogicalExpressionNode createLogicalAnd(ExpressionNode left,ExpressionNode right){ return new LogicalExpressionNode(left, LogicalOperator.AND, right); } + public static LogicalExpressionNode createLogicalAnd(Collection operands){ + return new LogicalExpressionNode(LogicalOperator.AND, operands); + } + private LogicalExpressionNode(ExpressionNode left, LogicalOperator operator, ExpressionNode right) { chain.add(left); chain.add(right); this.operator = operator; } + private LogicalExpressionNode(LogicalOperator operator, Collection operands) { + chain.addAll(operands); + this.operator = operator; + } + public LogicalExpressionNode and(LogicalExpressionNode other){ return createLogicalAnd(this, other); } diff --git a/json-path/src/test/java/com/jayway/jsonpath/FilterCompilerTest.java b/json-path/src/test/java/com/jayway/jsonpath/FilterCompilerTest.java index ce744e38..eee4565b 100644 --- a/json-path/src/test/java/com/jayway/jsonpath/FilterCompilerTest.java +++ b/json-path/src/test/java/com/jayway/jsonpath/FilterCompilerTest.java @@ -29,7 +29,7 @@ public class FilterCompilerTest { assertThat(compile("[?((@.firstname || @.lastname) && @.and)]").toString()).isEqualTo("[?((@['firstname'] || @['lastname']) && @['and'])]"); assertThat(compile("[?((@.a || @.b || @.c) && @.x)]").toString()).isEqualTo("[?((@['a'] || @['b'] || @['c']) && @['x'])]"); assertThat(compile("[?((@.a && @.b && @.c) || @.x)]").toString()).isEqualTo("[?((@['a'] && @['b'] && @['c']) || @['x'])]"); - assertThat(compile("[?((@.a && @.b || @.c) || @.x)]").toString()).isEqualTo("[?((@['a'] && (@['b'] || @['c'])) || @['x'])]"); + assertThat(compile("[?((@.a && @.b || @.c) || @.x)]").toString()).isEqualTo("[?(((@['a'] && @['b']) || @['c']) || @['x'])]"); assertThat(compile("[?((@.a && @.b) || (@.c && @.d))]").toString()).isEqualTo("[?((@['a'] && @['b']) || (@['c'] && @['d']))]"); assertThat(compile("[?(@.a IN [1,2,3])]").toString()).isEqualTo("[?(@['a'] IN [1,2,3])]"); assertThat(compile("[?(@.a IN {'foo':'bar'})]").toString()).isEqualTo("[?(@['a'] IN {'foo':'bar'})]"); @@ -64,6 +64,12 @@ public class FilterCompilerTest { compile("[?(@.foo == x)]"); } + @Test + public void or_has_lower_priority_than_and() { + assertThat(compile("[?(@.category == 'fiction' && @.author == 'Evelyn Waugh' || @.price > 15)]").toString()) + .isEqualTo("[?((@['category'] == 'fiction' && @['author'] == 'Evelyn Waugh') || @['price'] > 15)]"); + } + @Test public void invalid_filters_does_not_compile() { assertInvalidPathException("[?(@))]"); @@ -73,6 +79,8 @@ public class FilterCompilerTest { assertInvalidPathException("[?(@ == 1' )]"); assertInvalidPathException("[?(@.foo bar == 1)]"); assertInvalidPathException("[?(@.i == 5 @.i == 8)]"); + assertInvalidPathException("[?(!5)]"); + assertInvalidPathException("[?(!'foo')]"); } diff --git a/json-path/src/test/java/com/jayway/jsonpath/PathCompilerTest.java b/json-path/src/test/java/com/jayway/jsonpath/PathCompilerTest.java index feef9ebe..7708bc07 100644 --- a/json-path/src/test/java/com/jayway/jsonpath/PathCompilerTest.java +++ b/json-path/src/test/java/com/jayway/jsonpath/PathCompilerTest.java @@ -222,6 +222,20 @@ public class PathCompilerTest { assertThat(result).isEmpty(); } + @Test + public void issue_predicate_or_has_lower_priority_than_and() { + String json = "{\n" + + " \"logs\": [\n" + + " {\n" + + " \"id\": 2\n" + + " }\n" + + " ]\n" + + "}"; + + List result = JsonPath.read(json, "$.logs[?(@.x && @.y || @.id)]"); + assertThat(result).hasSize(1); + } + @Test public void issue_predicate_can_have_square_bracket_in_prop() { String json = "{\n" From 604b42f865c73464bf1f1abebc3b4cae41afb086 Mon Sep 17 00:00:00 2001 From: Alexey Makeyev Date: Wed, 23 Dec 2015 14:53:58 +0300 Subject: [PATCH 2/4] one more test for parentheses parsing --- .../src/test/java/com/jayway/jsonpath/FilterCompilerTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/json-path/src/test/java/com/jayway/jsonpath/FilterCompilerTest.java b/json-path/src/test/java/com/jayway/jsonpath/FilterCompilerTest.java index eee4565b..c568b63c 100644 --- a/json-path/src/test/java/com/jayway/jsonpath/FilterCompilerTest.java +++ b/json-path/src/test/java/com/jayway/jsonpath/FilterCompilerTest.java @@ -44,6 +44,7 @@ public class FilterCompilerTest { assertThat(compile("[?($[\"firstname\"][\"lastname\"])]").toString()).isEqualTo("[?($[\"firstname\"][\"lastname\"])]"); assertThat(compile("[?($[\"firstname\"].lastname)]").toString()).isEqualTo("[?($[\"firstname\"]['lastname'])]"); assertThat(compile("[?($[\"firstname\", \"lastname\"])]").toString()).isEqualTo("[?($[\"firstname\",\"lastname\"])]"); + assertThat(compile("[?(((@.a && @.b || @.c)) || @.x)]").toString()).isEqualTo("[?(((@['a'] && @['b']) || @['c']) || @['x'])]"); } From 58c454b52d40dbf6086a6e9cf41b38770f2dbec4 Mon Sep 17 00:00:00 2001 From: Alexey Makeyev Date: Wed, 23 Dec 2015 15:26:03 +0300 Subject: [PATCH 3/4] Fix issue #175 - Size evaluation could break indefinite path evaluation --- .../jayway/jsonpath/internal/filter/EvaluatorFactory.java | 3 +++ .../src/test/java/com/jayway/jsonpath/InlineFilterTest.java | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/json-path/src/main/java/com/jayway/jsonpath/internal/filter/EvaluatorFactory.java b/json-path/src/main/java/com/jayway/jsonpath/internal/filter/EvaluatorFactory.java index c103ab28..43b705a4 100644 --- a/json-path/src/main/java/com/jayway/jsonpath/internal/filter/EvaluatorFactory.java +++ b/json-path/src/main/java/com/jayway/jsonpath/internal/filter/EvaluatorFactory.java @@ -138,6 +138,9 @@ public class EvaluatorFactory { private static class SizeEvaluator implements Evaluator { @Override public boolean evaluate(ValueNode left, ValueNode right, Predicate.PredicateContext ctx) { + if (! right.isNumberNode()) { + return false; + } int expectedSize = right.asNumberNode().getNumber().intValue(); if(left.isStringNode()){ diff --git a/json-path/src/test/java/com/jayway/jsonpath/InlineFilterTest.java b/json-path/src/test/java/com/jayway/jsonpath/InlineFilterTest.java index 5f19a8df..1a130344 100644 --- a/json-path/src/test/java/com/jayway/jsonpath/InlineFilterTest.java +++ b/json-path/src/test/java/com/jayway/jsonpath/InlineFilterTest.java @@ -209,4 +209,9 @@ public class InlineFilterTest extends BaseTest { public void escape_pattern() { assertHasOneResult("[\"x\"]", "$[?(@ =~ /\\/|x/)]", conf); } + + @Test + public void filter_evaluation_does_not_break_path_evaluation() { + assertHasOneResult("[{\"s\": \"fo\", \"expected_size\": \"m\"}, {\"s\": \"lo\", \"expected_size\": 2}]", "$[?(@.s size @.expected_size)]", conf); + } } From 0d235b9ca62de90bd26de1f6bf431888a1452d24 Mon Sep 17 00:00:00 2001 From: Alexey Makeyev Date: Wed, 23 Dec 2015 15:35:48 +0300 Subject: [PATCH 4/4] more generic fix for issue #175 --- .../jayway/jsonpath/internal/path/PredicatePathToken.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/json-path/src/main/java/com/jayway/jsonpath/internal/path/PredicatePathToken.java b/json-path/src/main/java/com/jayway/jsonpath/internal/path/PredicatePathToken.java index b50b848c..af668634 100644 --- a/json-path/src/main/java/com/jayway/jsonpath/internal/path/PredicatePathToken.java +++ b/json-path/src/main/java/com/jayway/jsonpath/internal/path/PredicatePathToken.java @@ -72,7 +72,11 @@ public class PredicatePathToken extends PathToken { Predicate.PredicateContext ctx = new PredicateContextImpl(obj, root, configuration, evaluationContext.documentEvalCache()); for (Predicate predicate : predicates) { - if (!predicate.apply (ctx)) { + try { + if (!predicate.apply(ctx)) { + return false; + } + } catch (InvalidPathException e) { return false; } }