fix for incorrect handling of logical operator priorities

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) {

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<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;
+             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())));
              }
-                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;
+             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<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()) {
+        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);
-        } else {
-            RelationalOperator operator = readRelationalOperator();
-            ValueNode right = readValueNode();
-            return new RelationalExpressionNode(left, operator, right);
-        }
     }
 
     private LogicalOperator readLogicalOperator(){

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<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);
     }

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')]");
     }
 
 

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<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"