Merge pull request #177 from zline/master

kallestenflo · kallestenflo · commit 3c5f57f4f747 · 2015-12-29T12:40:35.000+01:00
Fix for logical operator priorities and issue #175
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
@@ -198,6 +198,25 @@ public char nextSignificantChar(int startPosition) {
         }
     }
 
+    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/EvaluatorFactory.java b/json-path/src/main/java/com/jayway/jsonpath/internal/filter/EvaluatorFactory.java
@@ -138,6 +138,9 @@ public boolean evaluate(ValueNode left, ValueNode right, Predicate.PredicateCont
     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/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 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 @@ private FilterCompiler(String filterString) {
 
     public Predicate compile() {
         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 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 @@ private ValueNode readValueNode() {
         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 @@ private ValueNode readLiteral(){
         }
     }
 
+    /*
+     *  LogicalOR               = LogicalAND { '||' LogicalAND }
+     *  LogicalAND              = LogicalANDOperand { '&&' LogicalANDOperand }
+     *  LogicalANDOperand       = RelationalExpression | '(' LogicalOR ')'
+     *  RelationalExpression    = Value [ RelationalOperator Value ]
+     */
+
+    private ExpressionNode readLogicalOR() {
+        final List<ExpressionNode> ops = new ArrayList<ExpressionNode>();
+        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<ExpressionNode> ops = new ArrayList<ExpressionNode>();
+        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
@@ -3,6 +3,7 @@
 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 static LogicalExpressionNode createLogicalOr(ExpressionNode left,Expressi
         return new LogicalExpressionNode(left, LogicalOperator.OR, right);
     }
 
+    public static LogicalExpressionNode createLogicalOr(Collection<ExpressionNode> 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<ExpressionNode> 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<ExpressionNode> operands) {
+        chain.addAll(operands);
+        this.operator = operator;
+    }
+
     public LogicalExpressionNode and(LogicalExpressionNode other){
         return createLogicalAnd(this, other);
     }
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
@@ -72,7 +72,11 @@ public boolean accept(final Object obj, final Object root, final Configuration c
         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;
             }
         }
diff --git 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 void valid_filters_compile() {
         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'})]");
@@ -44,6 +44,7 @@ public void valid_filters_compile() {
         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'])]");
 
     }
 
@@ -64,6 +65,12 @@ public void invalid_path_when_string_literal_is_unquoted() {
         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 +80,8 @@ public void invalid_filters_does_not_compile() {
         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/InlineFilterTest.java b/json-path/src/test/java/com/jayway/jsonpath/InlineFilterTest.java
@@ -209,4 +209,9 @@ public void escaped_literals2() {
     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);
+    }
 }
diff --git 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 void issue_predicate_brackets_must_change_priorities() {
         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<String> 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"

Original file line number	Diff line number	Diff line change
`@@ -72,7 +72,11 @@ public boolean accept(final Object obj, final Object root, final Configuration c`
`72`	`72`	`Predicate.PredicateContext ctx = new PredicateContextImpl(obj, root, configuration, evaluationContext.documentEvalCache());`
`73`	`73`
`74`	`74`	`for (Predicate predicate : predicates) {`
`75`		`- if (!predicate.apply (ctx)) {`
	`75`	`+ try {`
	`76`	`+ if (!predicate.apply(ctx)) {`
	`77`	`+ return false;`
	`78`	`+ }`
	`79`	`+ } catch (InvalidPathException e) {`
`76`	`80`	`return false;`
`77`	`81`	`}`
`78`	`82`	`}`
Original file line number	Diff line number	Diff line change
`@@ -209,4 +209,9 @@ public void escaped_literals2() {`
`209`	`209`	`public void escape_pattern() {`
`210`	`210`	`assertHasOneResult("[\"x\"]", "$[?(@ =~ /\\/\|x/)]", conf);`
`211`	`211`	`}`
	`212`	`+`
	`213`	`+ @Test`
	`214`	`+ public void filter_evaluation_does_not_break_path_evaluation() {`
	`215`	`+ assertHasOneResult("[{\"s\": \"fo\", \"expected_size\": \"m\"}, {\"s\": \"lo\", \"expected_size\": 2}]", "$[?(@.s size @.expected_size)]", conf);`
	`216`	`+ }`
`212`	`217`	`}`