#681 Exp: regression in processing (not)in with empty list

agrestio · Jun 19, 2024 · 1606e90 · 1606e90
1 parent b7b6d34
commit 1606e90
Show file tree

Hide file tree

Showing 5 changed files with 82 additions and 3 deletions.
diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md
@@ -15,6 +15,7 @@
 * #678 Upgrading Swagger to 2.2.2
 * #679 Upgrading Jackson to 2.15.4
 * #680 Upgrading SLF4J to 2.0.7
+* #681 Exp: regression in processing (not)in with empty list
 
 ## Release 5.0.M19
 

diff --git a/agrest-cayenne/src/test/java/io/agrest/cayenne/exp/CayenneExpParserTest.java b/agrest-cayenne/src/test/java/io/agrest/cayenne/exp/CayenneExpParserTest.java
@@ -77,6 +77,24 @@ public void parseIn() {
 
         Expression e3 = parser.parse(Exp.in("a", 5, 6, 7));
         assertEquals(ExpressionFactory.exp("a in (5, 6, 7)"), e3);
+
+        Expression e4 = parser.parse(Exp.in("a"));
+        assertEquals(ExpressionFactory.exp("false"), e4);
+    }
+
+    @Test
+    public void parseNotIn() {
+        Expression e1 = parser.parse(Exp.notIn("a", 5, 6, 7));
+        assertEquals(ExpressionFactory.exp("a not in (5, 6, 7)"), e1);
+
+        Expression e2 = parser.parse(Exp.notIn("a", "x", "y", "z"));
+        assertEquals(ExpressionFactory.exp("a not in ('x','y','z')"), e2);
+
+        Expression e3 = parser.parse(Exp.notIn("a", 5, 6, 7));
+        assertEquals(ExpressionFactory.exp("a not in (5, 6, 7)"), e3);
+
+        Expression e4 = parser.parse(Exp.notIn("a"));
+        assertEquals(ExpressionFactory.exp("true"), e4);
     }
 
     @Test

diff --git a/agrest-engine/src/main/java/io/agrest/exp/parser/NamedParamTransformer.java b/agrest-engine/src/main/java/io/agrest/exp/parser/NamedParamTransformer.java
@@ -2,6 +2,7 @@
 
 import java.util.Map;
 import java.util.function.Function;
+import java.util.function.Supplier;
 
 class NamedParamTransformer implements Function<Object, Object> {
 
@@ -17,7 +18,8 @@ class NamedParamTransformer implements Function<Object, Object> {
     public Object apply(Object object) {
 
         if (!(object instanceof ExpNamedParameter)) {
-            return object;
+            // after parameters are resolved, we may need to shake down the tree a bit
+            return optimize(object);
         }
 
         String name = ((ExpNamedParameter) object).getName();
@@ -31,4 +33,43 @@ public Object apply(Object object) {
             return value != null ? SimpleNode.wrapParameterValue(value) : new ExpScalar(null);
         }
     }
+
+    private Object optimize(Object object) {
+        if(object instanceof SimpleNode) {
+            return ((SimpleNode) object).jjtAccept(new OptimizationVisitor(), null);
+        }
+        return object;
+    }
+
+    static class OptimizationVisitor extends AgExpressionParserDefaultVisitor<SimpleNode> {
+
+        @Override
+        public SimpleNode defaultVisit(SimpleNode node, SimpleNode data) {
+            // note, we do not go down to children, just process this node and that's it
+            return node;
+        }
+
+        @Override
+        public SimpleNode visit(ExpIn node, SimpleNode data) {
+            return optimizeIn(node, ExpFalse::new);
+        }
+
+        @Override
+        public SimpleNode visit(ExpNotIn node, SimpleNode data) {
+            return optimizeIn(node, ExpTrue::new);
+        }
+
+        private static SimpleNode optimizeIn(SimpleNode node, Supplier<SimpleNode> supplier) {
+            if(node.jjtGetNumChildren() < 2) {
+                return node;
+            }
+            Node child = node.jjtGetChild(1);
+            if(child instanceof ExpScalarList) {
+                if(((ExpScalarList) child).getValue().isEmpty()) {
+                    return supplier.get();
+                }
+            }
+            return node;
+        }
+    }
 }
diff --git a/agrest-engine/src/main/java/io/agrest/protocol/Exp.java b/agrest-engine/src/main/java/io/agrest/protocol/Exp.java
@@ -101,14 +101,20 @@ static Exp notBetween(String left, Object right1, Object right2) {
     /**
      * @since 5.0
      */
-    static Exp in(String path, Object... values) {
-        return ExpUtils.composeBinary(new ExpIn(), path(path), ExpUtils.scalarArray(values));
+    static Exp in(String path, Object... scalars) {
+        if(scalars.length == 0){
+            return new ExpFalse();
+        }
+        return ExpUtils.composeBinary(new ExpIn(), path(path), ExpUtils.scalarArray(scalars));
     }
 
     /**
      * @since 5.0
      */
     static Exp notIn(String path, Object... scalars) {
+        if(scalars.length == 0){
+            return new ExpTrue();
+        }
         return ExpUtils.composeBinary(new ExpNotIn(), path(path), ExpUtils.scalarArray(scalars));
     }
 

diff --git a/agrest-engine/src/test/java/io/agrest/exp/parser/ExpInTest.java b/agrest-engine/src/test/java/io/agrest/exp/parser/ExpInTest.java
@@ -8,6 +8,7 @@
 import org.junit.jupiter.params.provider.ValueSource;
 
 import java.util.List;
+import java.util.Map;
 
 import static org.junit.jupiter.api.Assertions.*;
 
@@ -53,6 +54,18 @@ public void parseInvalidGrammar(String expString) {
         assertThrows(AgException.class, () -> Exp.parse(expString));
     }
 
+    @Test
+    public void emptyIn() {
+        Exp exp = Exp.parse("a in $a").namedParams(Map.of("a", List.of()));
+        assertEquals("false", exp.toString());
+    }
+
+    @Test
+    public void emptyNotIn() {
+        Exp exp = Exp.parse("a not in $a").namedParams(Map.of("a", List.of()));
+        assertEquals("true", exp.toString());
+    }
+
     @Test
     public void parameterizedToString() {
         assertEquals("a in ('x', 'y')", Exp.parse("a in $l").positionalParams(List.of("x", "y")).toString());