Fix or(expr) / and(expr) methods which were not correctly grouping lo…

…gical expressions In Geode always use paranthesis (`()`) when traversing `AND / OR` expression(s) for correct priority.
akhomchenko · Oct 17, 2019 · 50987bf · 50987bf
1 parent d5f42ff
commit 50987bf
Show file tree

Hide file tree

Showing 14 changed files with 339 additions and 40 deletions.
diff --git a/criteria/common/src/org/immutables/criteria/expression/SimpleCall.java b/criteria/common/src/org/immutables/criteria/expression/SimpleCall.java
@@ -65,10 +65,9 @@ public String toString() {
     final PrintWriter printer = new PrintWriter(writer);
     printer.append(SimpleCall.class.getSimpleName());
     printer.append("{");
-    printer.println(operator().name());
+    printer.print(operator().name());
     DebugExpressionVisitor<Void> debug = new DebugExpressionVisitor<>(printer);
     arguments().forEach(a -> {
-      printer.println();
       a.accept(debug);
     });
     printer.append("}");

diff --git a/criteria/common/src/org/immutables/criteria/matcher/AndMatcher.java b/criteria/common/src/org/immutables/criteria/matcher/AndMatcher.java
@@ -17,10 +17,7 @@
 package org.immutables.criteria.matcher;
 
 import org.immutables.criteria.Criterion;
-import org.immutables.criteria.expression.Expression;
-import org.immutables.criteria.expression.Expressions;
-
-import java.util.function.UnaryOperator;
+import org.immutables.criteria.expression.Operators;
 
 /**
  * Combines matchers using logical {@code AND}
@@ -30,15 +27,20 @@ public interface AndMatcher<R extends Criterion<?>> extends Matcher {
 
   /**
    * Combine {@code this} and {@code other} expression (criteria / matcher) using logical {@code AND}
-   * operator. Equivalent to {@code this AND other}.
+   * operator. Equivalent to {@code (this) AND (other)}.
+   *
+   * <p>Not exactly same as DNF expression
+   * <pre>
+   *   DNF : A.or().B.C is equivalent to A or B and C
+   *   this: A.or().B.and(C) is equivalent to (A or B) and C
+   * </pre>
    *
    * @param other other matcher
    * @return new root criteria with updated expression
    */
+  @SuppressWarnings("unchecked")
   default R and(R other) {
-    final CriteriaContext context = Matchers.extract(this);
-    final UnaryOperator<Expression> expr = e -> Expressions.and(Matchers.concat(e, other));
-    return context.applyAndCreateRoot(expr);
+    return Matchers.combine((R) this, other, Operators.AND);
   }
 
 }
diff --git a/criteria/common/src/org/immutables/criteria/matcher/CriteriaContext.java b/criteria/common/src/org/immutables/criteria/matcher/CriteriaContext.java
@@ -17,6 +17,7 @@
 package org.immutables.criteria.matcher;
 
 import org.immutables.criteria.expression.Expression;
+import org.immutables.criteria.expression.Expressions;
 import org.immutables.criteria.expression.Path;
 import org.immutables.criteria.expression.Query;
 import org.immutables.criteria.expression.Queryable;
@@ -35,7 +36,7 @@ public final class CriteriaContext implements Queryable {
 
   private final Expression expression;
   private final Path path;
-  private final Class<?> entityClass;
+  final Class<?> entityClass;
   private final CriteriaContext root;
   private final CriteriaContext parent;
   private final CriteriaCreator<?> creator;
@@ -44,7 +45,7 @@ public CriteriaContext(Class<?> entityClass, CriteriaCreator<?> creator) {
     this(entityClass, new DnfExpression(), null, creator, null);
   }
 
-  private CriteriaContext(Class<?> entityClass, Expression expression, Path path, CriteriaCreator<?> creator, CriteriaContext parent) {
+  CriteriaContext(Class<?> entityClass, Expression expression, Path path, CriteriaCreator<?> creator, CriteriaContext parent) {
     this.expression = expression;
     this.path = path;
     this.creator = Objects.requireNonNull(creator, "creator");
@@ -58,6 +59,14 @@ public Path path() {
   }
 
   public Expression expression() {
+    if (expression instanceof DnfExpression) {
+      return dnfExpression().isEmpty() ? path() : dnfExpression().simplify();
+    }
+
+    if (expression == null) {
+      return path();
+    }
+
     return expression;
   }
 
@@ -115,7 +124,15 @@ private DnfExpression dnfExpression() {
   @Override
   public Query query() {
     final Query query = Query.of(entityClass);
-    return !dnfExpression().isEmpty() ? query.withFilter(dnfExpression().simplify()) : query;
+    Expression expression = expression();
+    if (expression instanceof DnfExpression) {
+      DnfExpression dnfExpression = dnfExpression();
+      return dnfExpression.isEmpty() ? query : query.withFilter(dnfExpression.simplify());
+    } else if (expression != null) {
+      return query.withFilter(expression);
+    }
+
+    return query;
   }
 
   <R> R createWith(CriteriaCreator<R> creator) {
@@ -133,7 +150,14 @@ <R> R applyAndCreateRoot(UnaryOperator<Expression> fn) {
   CriteriaContext apply(UnaryOperator<Expression> fn) {
     Objects.requireNonNull(fn, "fn");
     final Expression apply = fn.apply(path);
-    final DnfExpression newExpression = dnfExpression().and(apply);
+    final Expression newExpression;
+    if (expression instanceof DnfExpression) {
+      newExpression = dnfExpression().and(apply);
+    } else if (expression != null) {
+      newExpression = Expressions.and(expression, apply);
+    } else {
+      newExpression = apply;
+    }
     final CriteriaContext parentOrSelf = parent != null ? parent : this;
     return new CriteriaContext(entityClass, newExpression, parentOrSelf.path, parentOrSelf.creator, parentOrSelf.parent);
   }

diff --git a/criteria/common/src/org/immutables/criteria/matcher/Matchers.java b/criteria/common/src/org/immutables/criteria/matcher/Matchers.java
@@ -20,6 +20,8 @@
 import org.immutables.criteria.Criterias;
 import org.immutables.criteria.Criterion;
 import org.immutables.criteria.expression.Expression;
+import org.immutables.criteria.expression.Expressions;
+import org.immutables.criteria.expression.Operator;
 
 import java.lang.reflect.Method;
 import java.lang.reflect.ParameterizedType;
@@ -51,6 +53,25 @@ static List<Expression> concat(Expression existing, Criterion<?> first, Criterio
 
   }
 
+  static <R extends Criterion<?>> R combine(R left, R right, Operator operator) {
+    CriteriaContext context = Matchers.extract(left);
+    Expression leftExpression = context.expression();
+    Expression rightExpression = Matchers.extract(right).expression();
+    Expression expression = null;
+    if (leftExpression != null && rightExpression != null) {
+      expression = Expressions.call(operator, leftExpression, rightExpression);
+    } else if (rightExpression != null) {
+      expression = rightExpression;
+    } else if (leftExpression != null) {
+      expression = leftExpression;
+    }
+
+    CriteriaContext root = context.root();
+    CriteriaCreator<R> creator = root.creator();
+    return creator.create(new CriteriaContext(root.entityClass, expression, root.path(), creator, null));
+
+  }
+
   /**
    * Gets generic type variable of aggregation interface.
    */

diff --git a/criteria/common/src/org/immutables/criteria/matcher/OrMatcher.java b/criteria/common/src/org/immutables/criteria/matcher/OrMatcher.java
@@ -19,8 +19,7 @@
 import org.immutables.criteria.Criterion;
 import org.immutables.criteria.expression.Expression;
 import org.immutables.criteria.expression.Expressions;
-
-import java.util.function.UnaryOperator;
+import org.immutables.criteria.expression.Operators;
 
 /**
  * Combines matchers using logical {@code OR}
@@ -35,9 +34,9 @@ public interface OrMatcher<R extends Criterion<?>> extends Matcher {
    * @param other other matcher
    * @return new root criteria with updated expression
    */
+  @SuppressWarnings("unchecked")
   default R or(R other) {
-    final UnaryOperator<Expression> expr = e -> Expressions.or(Matchers.concat(e, other));
-    return Matchers.extract(this).apply(expr).or().root().create();
+    return Matchers.combine((R) this, other, Operators.OR);
   }
 
 }
diff --git a/criteria/common/test/org/immutables/criteria/ExpressionAsStringTest.java b/criteria/common/test/org/immutables/criteria/ExpressionAsStringTest.java
@@ -32,10 +32,10 @@
  * Tests that expression is built correctly by "serializing" it to string.
  * It is easier to debug expression when it is presented as human-readable tree
  */
-public class ExpressionAsStringTest {
+class ExpressionAsStringTest {
 
   @Test
-  public void string() {
+  void string() {
     PersonCriteria crit = PersonCriteria.person;
 
     assertExpressional(crit.bestFriend.isPresent(), "call op=IS_PRESENT path=bestFriend");
@@ -55,7 +55,7 @@ public void string() {
   }
 
   @Test
-  public void with() {
+  void with() {
     PersonCriteria crit = PersonCriteria.person;
 
     assertExpressional(crit.with(c -> c.fullName.is("John")),
@@ -117,7 +117,7 @@ public void with() {
   }
 
   @Test
-  public void with2() {
+  void with2() {
     // nested
     assertExpressional(PersonCriteria.person.with(p1 -> p1.with(p2-> p2.fullName.is("a"))),
             "call op=EQUAL path=fullName constant=a");
@@ -148,7 +148,7 @@ public void with2() {
   }
 
   @Test
-  public void not() {
+  void not() {
     PersonCriteria crit = PersonCriteria.person;
 
     assertExpressional(crit.fullName.not(n -> n.is("John")),
@@ -169,15 +169,15 @@ public void not() {
   }
 
   @Test
-  public void debug() {
+  void debug() {
     assertExpressional(PersonCriteria.person.bestFriend.value().hobby.with(h -> h.is("a").is("b")),
             "call op=AND",
             "  call op=EQUAL path=bestFriend.hobby constant=a",
             "  call op=EQUAL path=bestFriend.hobby constant=b");
   }
 
   @Test
-  public void and() {
+  void and() {
     PersonCriteria crit = PersonCriteria.person;
 
     PersonCriteria other = PersonCriteria.person
@@ -189,19 +189,21 @@ public void and() {
 
     assertExpressional(other.and(crit.age.is(3)),
             "call op=AND",
-                    "  call op=LESS_THAN_OR_EQUAL path=age constant=1",
-                    "  call op=GREATER_THAN_OR_EQUAL path=age constant=2",
+                    "  call op=AND",
+                    "    call op=LESS_THAN_OR_EQUAL path=age constant=1",
+                    "    call op=GREATER_THAN_OR_EQUAL path=age constant=2",
                     "  call op=EQUAL path=age constant=3");
 
     assertExpressional(other.and(crit.age.is(3)),
             "call op=AND",
-                    "  call op=LESS_THAN_OR_EQUAL path=age constant=1",
-                    "  call op=GREATER_THAN_OR_EQUAL path=age constant=2",
+                    "  call op=AND",
+                    "    call op=LESS_THAN_OR_EQUAL path=age constant=1",
+                    "    call op=GREATER_THAN_OR_EQUAL path=age constant=2",
                     "  call op=EQUAL path=age constant=3");
   }
 
   @Test
-  public void or() {
+  void or() {
     PersonCriteria crit = PersonCriteria.person;
 
     PersonCriteria other = PersonCriteria.person
@@ -213,23 +215,26 @@ public void or() {
 
     assertExpressional(other.or(crit.age.is(3)),
             "call op=OR",
-                    "  call op=LESS_THAN_OR_EQUAL path=age constant=1",
-                    "  call op=GREATER_THAN_OR_EQUAL path=age constant=2",
+                    "  call op=OR",
+                    "    call op=LESS_THAN_OR_EQUAL path=age constant=1",
+                    "    call op=GREATER_THAN_OR_EQUAL path=age constant=2",
                     "  call op=EQUAL path=age constant=3");
 
     assertExpressional(crit.or(crit.age.atMost(1)).or(crit.age.atLeast(2))
                     .or(crit.age.is(3)),
             "call op=OR",
-            "  call op=LESS_THAN_OR_EQUAL path=age constant=1",
-            "  call op=GREATER_THAN_OR_EQUAL path=age constant=2",
+            "  call op=OR",
+            "    call op=LESS_THAN_OR_EQUAL path=age constant=1",
+            "    call op=GREATER_THAN_OR_EQUAL path=age constant=2",
             "  call op=EQUAL path=age constant=3");
+
   }
 
   /**
    * AND / ORs combined like {@code (A and B) or (C and D)}
    */
   @Test
-  public void andOrCombined() {
+  void andOrCombined() {
     PersonCriteria crit = PersonCriteria.person;
 
     // A or (B and C)
@@ -295,7 +300,49 @@ public void andOrCombined() {
   }
 
   @Test
-  public void next() {
+  void andOrAsCriteriaParams() {
+    PersonCriteria crit = PersonCriteria.person;
+
+    // combination of OR/AND
+    assertExpressional(crit.age.is(1).or(crit.age.is(2).age.is(3)),
+            "call op=OR",
+            "  call op=EQUAL path=age constant=1",
+            "  call op=AND",
+            "    call op=EQUAL path=age constant=2",
+            "    call op=EQUAL path=age constant=3");
+
+    assertExpressional(crit.age.is(1).or(crit.age.is(2).age.is(3)),
+            "call op=OR",
+            "  call op=EQUAL path=age constant=1",
+            "  call op=AND",
+            "    call op=EQUAL path=age constant=2",
+            "    call op=EQUAL path=age constant=3");
+
+    assertExpressional(crit.age.is(1).and(crit.age.is(2).or().age.is(3)),
+            "call op=AND",
+            "  call op=EQUAL path=age constant=1",
+            "  call op=OR",
+            "    call op=EQUAL path=age constant=2",
+            "    call op=EQUAL path=age constant=3");
+
+    assertExpressional(crit.age.is(1).and(crit.age.is(2).or().age.is(3)),
+            "call op=AND",
+            "  call op=EQUAL path=age constant=1",
+            "  call op=OR",
+            "    call op=EQUAL path=age constant=2",
+            "    call op=EQUAL path=age constant=3");
+
+    assertExpressional(crit.age.is(1).or().age.is(2).and(crit.age.is(3)),
+            "call op=AND",
+            "  call op=OR",
+            "    call op=EQUAL path=age constant=1",
+            "    call op=EQUAL path=age constant=2",
+            "  call op=EQUAL path=age constant=3");
+
+  }
+
+  @Test
+  void next() {
     PersonCriteria crit = PersonCriteria.person;
     assertExpressional(crit.bestFriend.value().hobby.is("ski"), "call op=EQUAL path=bestFriend.hobby constant=ski");
 
@@ -317,7 +364,7 @@ public void next() {
   }
 
   @Test
-  public void inner() {
+  void inner() {
     assertExpressional(PersonCriteria.person.bestFriend.value().with(f -> f.hobby.is("hiking")),
             "call op=EQUAL path=bestFriend.hobby constant=hiking");
 

diff --git a/criteria/common/test/org/immutables/criteria/matcher/MatchersTest.java b/criteria/common/test/org/immutables/criteria/matcher/MatchersTest.java
@@ -0,0 +1,40 @@
+/*
+ * Copyright 2019 Immutables Authors and Contributors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.immutables.criteria.matcher;
+
+import org.immutables.criteria.expression.Expression;
+import org.immutables.criteria.expression.Path;
+import org.immutables.criteria.personmodel.PersonCriteria;
+import org.junit.jupiter.api.Test;
+
+import static org.immutables.check.Checkers.check;
+
+class MatchersTest {
+
+  @Test
+  void projection() {
+    Expression expr1 = Matchers.toExpression(PersonCriteria.person.fullName);
+    check(expr1 instanceof Path);
+    check(((Path) expr1).toStringPath()).is("fullName");
+
+    Expression expr2 = Matchers.toExpression(PersonCriteria.person.bestFriend.value().hobby);
+    check(expr2 instanceof Path);
+    check(((Path) expr2).toStringPath()).is("bestFriend.hobby");
+  }
+
+
+}