Repository: jakartaee/data
Pull request #1075 (2025-04-24 07:43:03)

sort out issues with in()
I think that using `Set` here was okay when we only had literal values. But now that we also have `Expression`s, I think it should be a `List`. Otherwise we commit to always having well-defined equality for expressions, which I would not want to do. Also. the generated SQL would be (in principle) non-deterministic which is (in principle) a problem for prepared statement caching.

I've also added appropriate calls to `unmodifiableList()`.


Patch:
diff --git a/api/src/main/java/jakarta/data/metamodel/Expression.java b/api/src/main/java/jakarta/data/metamodel/Expression.java
index 345cf3d7..5b4b8f05 100644
--- a/api/src/main/java/jakarta/data/metamodel/Expression.java
+++ b/api/src/main/java/jakarta/data/metamodel/Expression.java
@@ -27,7 +27,7 @@ import jakarta.data.metamodel.constraint.Null;
 import jakarta.data.metamodel.restrict.BasicRestriction;
 import jakarta.data.metamodel.restrict.Restriction;
 
-import java.util.Set;
+import java.util.Collection;
 
 public interface Expression<T,V> {
 
@@ -39,20 +39,20 @@ public interface Expression<T,V> {
         return BasicRestriction.of(this, EqualTo.expression(expression));
     }
 
-    default Restriction<T> in(Set<V> values) {
+    default Restriction<T> in(Collection<V> values) {
         if (values == null || values.isEmpty())
             throw new IllegalArgumentException("values are required");
 
         return BasicRestriction.of(this, In.values(values));
     }
 
-    default Restriction<T> in(
-            @SuppressWarnings("unchecked") V... values) {
+    @SuppressWarnings("unchecked")
+    default Restriction<T> in(V... values) {
         return BasicRestriction.of(this, In.values(values));
     }
 
-    default Restriction<T> in(
-            @SuppressWarnings("unchecked") Expression<? super T,V>... expressions) {
+    @SuppressWarnings("unchecked")
+    default Restriction<T> in(Expression<? super T,V>... expressions) {
         return BasicRestriction.of(this, In.expressions(expressions));
     }
 
@@ -68,20 +68,20 @@ public interface Expression<T,V> {
         return BasicRestriction.of(this, NotEqualTo.expression(expression));
     }
 
-    default Restriction<T> notIn(Set<V> values) {
+    default Restriction<T> notIn(Collection<V> values) {
         if (values == null || values.isEmpty())
             throw new IllegalArgumentException("values are required");
 
         return BasicRestriction.of(this, NotIn.values(values));
     }
 
-    default Restriction<T> notIn(
-            @SuppressWarnings("unchecked") V... values) {
+    @SuppressWarnings("unchecked")
+    default Restriction<T> notIn(V... values) {
         return BasicRestriction.of(this, NotIn.values(values));
     }
 
-    default Restriction<T> notIn(
-            @SuppressWarnings("unchecked") Expression<? super T,V>... expressions) {
+    @SuppressWarnings("unchecked")
+    default Restriction<T> notIn(Expression<? super T,V>... expressions) {
         return BasicRestriction.of(this, NotIn.expressions(expressions));
     }
 
diff --git a/api/src/main/java/jakarta/data/metamodel/constraint/In.java b/api/src/main/java/jakarta/data/metamodel/constraint/In.java
index 50779b76..740ae063 100644
--- a/api/src/main/java/jakarta/data/metamodel/constraint/In.java
+++ b/api/src/main/java/jakarta/data/metamodel/constraint/In.java
@@ -17,12 +17,16 @@
  */
 package jakarta.data.metamodel.constraint;
 
-import java.util.LinkedHashSet;
-import java.util.Objects;
-import java.util.Set;
-
 import jakarta.data.metamodel.Expression;
 import jakarta.data.metamodel.expression.Literal;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Objects;
+
+import static java.util.Collections.unmodifiableList;
+
 public interface In<V> extends Constraint<V> {
 
     @SafeVarargs
@@ -33,32 +37,32 @@ public interface In<V> extends Constraint<V> {
             throw new IllegalArgumentException("Array of values must not be empty.");
         }
 
-        Set<Expression<?, V>> expressions = new LinkedHashSet<>(values.length);
+        List<Expression<?, V>> expressions = new ArrayList<>(values.length);
         for (V value : values) {
             Objects.requireNonNull(value, "Value must not be null.");
             expressions.add(Literal.of(value));
         }
 
-        return new InRecord<>(expressions);
+        return new InRecord<>(unmodifiableList(expressions));
     }
 
-    static <V> In<V> values(Set<V> values) {
+    static <V> In<V> values(Collection<V> values) {
         Objects.requireNonNull(values, "Values are required.");
 
         if (values.isEmpty()) {
             throw new IllegalArgumentException("Values must not be empty.");
         }
 
-        Set<Expression<?, V>> expressions = new LinkedHashSet<>(values.size());
+        List<Expression<?, V>> expressions = new ArrayList<>(values.size());
         for (V value : values) {
             Objects.requireNonNull(value, "Value must not be null.");
             expressions.add(Literal.of(value));
         }
 
-        return new InRecord<>(expressions);
+        return new InRecord<>(unmodifiableList(expressions));
     }
 
-    static <V> In<V> expressions(Set<Expression<?, V>> expressions) {
+    static <V> In<V> expressions(List<Expression<?, V>> expressions) {
         Objects.requireNonNull(expressions, "Value expressions are required.");
 
         if (expressions.isEmpty()) {
@@ -70,7 +74,7 @@ public interface In<V> extends Constraint<V> {
             Objects.requireNonNull(expression, "Value expression must not be null.");
         }
 
-        return new InRecord<>(expressions);
+        return new InRecord<>(List.copyOf(expressions));
     }
 
     @SafeVarargs
@@ -86,8 +90,8 @@ public interface In<V> extends Constraint<V> {
             Objects.requireNonNull(expression, "Value expression must not be null.");
         }
 
-        return new InRecord<>(Set.of(expressions));
+        return new InRecord<>(List.of(expressions));
     }
 
-    Set<Expression<?, V>> expressions();
+    List<Expression<?, V>> expressions();
 }
diff --git a/api/src/main/java/jakarta/data/metamodel/constraint/InRecord.java b/api/src/main/java/jakarta/data/metamodel/constraint/InRecord.java
index 33a792aa..805bac14 100644
--- a/api/src/main/java/jakarta/data/metamodel/constraint/InRecord.java
+++ b/api/src/main/java/jakarta/data/metamodel/constraint/InRecord.java
@@ -17,15 +17,15 @@
  */
 package jakarta.data.metamodel.constraint;
 
-import java.util.Set;
+import java.util.List;
 
 import jakarta.data.metamodel.Expression;
 
-record InRecord<V>(Set<Expression<?, V>> expressions) implements In<V> {
+record InRecord<V>(List<Expression<?, V>> expressions) implements In<V> {
 
     @Override
     public NotIn<V> negate() {
-        return NotIn.expressions(expressions);
+        return new NotInRecord<>(expressions);
     }
 
     @Override
diff --git a/api/src/main/java/jakarta/data/metamodel/constraint/NotIn.java b/api/src/main/java/jakarta/data/metamodel/constraint/NotIn.java
index e043f1a9..c9f00c84 100644
--- a/api/src/main/java/jakarta/data/metamodel/constraint/NotIn.java
+++ b/api/src/main/java/jakarta/data/metamodel/constraint/NotIn.java
@@ -17,13 +17,16 @@
  */
 package jakarta.data.metamodel.constraint;
 
-import java.util.LinkedHashSet;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
 import java.util.Objects;
-import java.util.Set;
 
 import jakarta.data.metamodel.Expression;
 import jakarta.data.metamodel.expression.Literal;
 
+import static java.util.Collections.unmodifiableList;
+
 public interface NotIn<V> extends Constraint<V> {
 
     @SafeVarargs
@@ -34,32 +37,32 @@ public interface NotIn<V> extends Constraint<V> {
             throw new IllegalArgumentException("Array of values must not be empty.");
         }
 
-        Set<Expression<?, V>> expressions = new LinkedHashSet<>(values.length);
+        final List<Expression<?, V>> expressions = new ArrayList<>(values.length);
         for (V value : values) {
             Objects.requireNonNull(value, "Value must not be null.");
             expressions.add(Literal.of(value));
         }
 
-        return new NotInRecord<>(expressions);
+        return new NotInRecord<>(unmodifiableList(expressions));
     }
 
-    static <V> NotIn<V> values(Set<V> values) {
+    static <V> NotIn<V> values(Collection<V> values) {
         Objects.requireNonNull(values, "Values are required.");
 
         if (values.isEmpty()) {
             throw new IllegalArgumentException("Values must not be empty.");
         }
 
-        Set<Expression<?, V>> expressions = new LinkedHashSet<>(values.size());
+        final List<Expression<?, V>> expressions = new ArrayList<>(values.size());
         for (V value : values) {
             Objects.requireNonNull(value, "Value must not be null.");
             expressions.add(Literal.of(value));
         }
 
-        return new NotInRecord<>(expressions);
+        return new NotInRecord<>(unmodifiableList(expressions));
     }
 
-    static <V> NotIn<V> expressions(Set<Expression<?, V>> expressions) {
+    static <V> NotIn<V> expressions(List<Expression<?, V>> expressions) {
         Objects.requireNonNull(expressions, "Value expressions are required.");
 
         if (expressions.isEmpty()) {
@@ -70,7 +73,7 @@ public interface NotIn<V> extends Constraint<V> {
             Objects.requireNonNull(expression, "Value expression must not be null.");
         }
 
-        return new NotInRecord<>(expressions);
+        return new NotInRecord<>(List.copyOf(expressions));
     }
 
     @SafeVarargs
@@ -86,8 +89,8 @@ public interface NotIn<V> extends Constraint<V> {
             Objects.requireNonNull(expression, "Value expression must not be null.");
         }
 
-        return new NotInRecord<>(Set.of(expressions));
+        return new NotInRecord<>(List.of(expressions));
     }
 
-    Set<Expression<?, V>> expressions();
+    List<Expression<?, V>> expressions();
 }
diff --git a/api/src/main/java/jakarta/data/metamodel/constraint/NotInRecord.java b/api/src/main/java/jakarta/data/metamodel/constraint/NotInRecord.java
index 9022f923..362e1b7a 100644
--- a/api/src/main/java/jakarta/data/metamodel/constraint/NotInRecord.java
+++ b/api/src/main/java/jakarta/data/metamodel/constraint/NotInRecord.java
@@ -17,15 +17,15 @@
  */
 package jakarta.data.metamodel.constraint;
 
-import java.util.Set;
+import java.util.List;
 
 import jakarta.data.metamodel.Expression;
 
-record NotInRecord<V>(Set<Expression<?, V>> expressions) implements NotIn<V> {
+record NotInRecord<V>(List<Expression<?, V>> expressions) implements NotIn<V> {
 
     @Override
     public In<V> negate() {
-        return In.expressions(expressions);
+        return new InRecord<>(expressions);
     }
 
     @Override
diff --git a/api/src/main/java/jakarta/data/metamodel/restrict/BasicRestriction.java b/api/src/main/java/jakarta/data/metamodel/restrict/BasicRestriction.java
index 7b85198a..42fe095d 100644
--- a/api/src/main/java/jakarta/data/metamodel/restrict/BasicRestriction.java
+++ b/api/src/main/java/jakarta/data/metamodel/restrict/BasicRestriction.java
@@ -28,7 +28,7 @@ public interface BasicRestriction<T, V> extends Restriction<T> {
     Expression<T,V> expression();
     Constraint<V> constraint();
 
-    static <T,V> BasicRestriction<T, V> of(Expression<T,V> expression,  Constraint<V> constraint) {
+    static <T,V> BasicRestriction<T, V> of(Expression<T,V> expression, Constraint<V> constraint) {
         return new BasicRestrictionRecord<>(expression, constraint);
     }
 }


Test patch:
diff --git a/api/src/test/java/jakarta/data/metamodel/AttributeTest.java b/api/src/test/java/jakarta/data/metamodel/AttributeTest.java
index 154a9be6..ec57ec3c 100644
--- a/api/src/test/java/jakarta/data/metamodel/AttributeTest.java
+++ b/api/src/test/java/jakarta/data/metamodel/AttributeTest.java
@@ -77,7 +77,7 @@ class AttributeTest {
     void shouldCreateInRestriction() {
         @SuppressWarnings("unchecked")
         BasicRestriction<Person, String> restriction =
-                (BasicRestriction<Person, String>) testAttribute.in(Set.of("value1", "value2"));
+                (BasicRestriction<Person, String>) testAttribute.in("value1", "value2");
 
         SoftAssertions.assertSoftly(soft -> {
             soft.assertThat(restriction).isInstanceOf(BasicRestriction.class);
@@ -98,7 +98,7 @@ class AttributeTest {
     void shouldCreateNotInRestriction() {
         @SuppressWarnings("unchecked")
         BasicRestriction<Person, String> restriction =
-                (BasicRestriction<Person, String>) testAttribute.notIn(Set.of("value1", "value2"));
+                (BasicRestriction<Person, String>) testAttribute.notIn("value1", "value2");
 
         SoftAssertions.assertSoftly(soft -> {
             soft.assertThat(restriction).isInstanceOf(BasicRestriction.class);
diff --git a/api/src/test/java/jakarta/data/metamodel/restrict/BasicRestrictionRecordTest.java b/api/src/test/java/jakarta/data/metamodel/restrict/BasicRestrictionRecordTest.java
index bad7ba20..c264b221 100644
--- a/api/src/test/java/jakarta/data/metamodel/restrict/BasicRestrictionRecordTest.java
+++ b/api/src/test/java/jakarta/data/metamodel/restrict/BasicRestrictionRecordTest.java
@@ -26,7 +26,7 @@ import org.assertj.core.api.SoftAssertions;
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 
-import java.util.Set;
+import java.util.List;
 
 import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
 
@@ -197,7 +197,7 @@ class BasicRestrictionRecordTest {
     @DisplayName("should create In constraint correctly")
     @Test
     void shouldCreateInRestriction() {
-        var restriction = (BasicRestriction<Book, String>) _Book.author.in(Set.of("Alice", "Bob"));
+        var restriction = (BasicRestriction<Book, String>) _Book.author.in(List.of("Alice", "Bob"));
 
         SoftAssertions.assertSoftly(soft -> {
             soft.assertThat(restriction.expression()).isEqualTo(_Book.author);
@@ -209,7 +209,7 @@ class BasicRestrictionRecordTest {
     @DisplayName("should create NotIn constraint correctly")
     @Test
     void shouldCreateNotInRestriction() {
-        var restriction = (BasicRestriction<Book, String>) _Book.author.in(Set.of("Alice", "Bob")).negate();
+        var restriction = (BasicRestriction<Book, String>) _Book.author.in(List.of("Alice", "Bob")).negate();
 
         SoftAssertions.assertSoftly(soft -> {
             soft.assertThat(restriction.expression()).isEqualTo(_Book.author);