Fix nondeterministic constant ordering causing possible incorrect in-memory evaluation (#1803)

* Fix nondeterministic constant ordering causing incorrect in-memory evaluation

Problem:

`DefaultEvaluatorFactory` builds compiled evaluators with from a non-deterministic
ordering of constants. This can lead to mismatches between the cached generated
java method and the runtime argument order, producing incorrect evaluation results.

`AbstractEvaluatorFactory#toId` only includes constant types in the cache key,
not their values. This is safe only if argument ordering is stable, which is
not guaranteed with the current constant resolution logic.

For example, the predicate `QEntity.entity.prop1.eq(true).or(QEntity.entity.prop2.eq(false))`
may result in a mismatch where the same compiled method is reused, but
arguments are supplied in a different order. The method may be invoked with
`[true, false]` (correct) or `[false, true]` (incorrect), leading to inverted
semantics at evaluation time.

Solution:

This change ensures deterministic constant ordering so that compiled methods
and runtime arguments always remain aligned.

* Apply code formatting to DefaultEvaluatorFactory

Signed-off-by: Marvin Froeder <velo.br@gmail.com>

* Add regression test for deterministic constant ordering

Signed-off-by: Marvin Froeder <velo.br@gmail.com>

---------

Signed-off-by: Marvin Froeder <velo.br@gmail.com>
Co-authored-by: Alexandre Potvin Latreille <alexandre.potvinlatreille@cra-arc.gc.ca>
Co-authored-by: Marvin Froeder <velo.br@gmail.com>

Author: plalx

querydsl-libraries/querydsl-collections/src/main/java/com/querydsl/collections/DefaultEvaluatorFactory.java

@@ -37,7 +37,7 @@
 import com.querydsl.core.util.PrimitiveUtils;
 import java.net.URLClassLoader;
 import java.util.ArrayList;
-import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import javax.tools.JavaCompiler;
@@ -112,8 +112,8 @@ public <T> Evaluator<T> create(
     }
     serializer.append(";");
 
-    var constantToLabel = serializer.getConstantToLabel();
-    var constants = getConstants(metadata, constantToLabel);
+    var constants =
+        getConstants(metadata, serializer.getConstants(), serializer.getConstantToLabel());
     var types = new Class<?>[sources.size()];
     var names = new String[sources.size()];
     for (var i = 0; i < sources.size(); i++) {
@@ -155,8 +155,7 @@ public <T> Evaluator<List<T>> createEvaluator(
     ser.append("}\n");
     ser.append("return rv;");
 
-    var constantToLabel = ser.getConstantToLabel();
-    var constants = getConstants(metadata, constantToLabel);
+    var constants = getConstants(metadata, ser.getConstants(), ser.getConstantToLabel());
 
     Type sourceType = new ClassType(TypeCategory.SIMPLE, source.getType());
     var sourceListType = new ClassType(TypeCategory.SIMPLE, Iterable.class, sourceType);
@@ -269,8 +268,7 @@ public Evaluator<List<Object[]>> createEvaluator(
     }
     ser.append("return rv;");
 
-    var constantToLabel = ser.getConstantToLabel();
-    var constants = getConstants(metadata, constantToLabel);
+    var constants = getConstants(metadata, ser.getConstants(), ser.getConstantToLabel());
 
     var projectionType = new ClassType(TypeCategory.LIST, List.class, Types.OBJECTS);
     return factory.createEvaluator(
@@ -283,19 +281,25 @@ public Evaluator<List<Object[]>> createEvaluator(
   }
 
   private Map<String, Object> getConstants(
-      QueryMetadata metadata, Map<Object, String> constantToLabel) {
-    Map<String, Object> constants = new HashMap<>();
-    for (Map.Entry<Object, String> entry : constantToLabel.entrySet()) {
-      if (entry.getKey() instanceof ParamExpression<?>) {
-        var value = metadata.getParams().get(entry.getKey());
+      QueryMetadata metadata, List<Object> constants, Map<Object, String> constantToLabel) {
+    var result = new LinkedHashMap<String, Object>();
+
+    for (var constant : constants) {
+      var paramName = constantToLabel.get(constant);
+
+      if (constant instanceof ParamExpression<?>) {
+        var value = metadata.getParams().get(constant);
         if (value == null) {
-          throw new ParamNotSetException((ParamExpression<?>) entry.getKey());
+          throw new ParamNotSetException((ParamExpression<?>) constant);
         }
-        constants.put(entry.getValue(), value);
-      } else {
-        constants.put(entry.getValue(), entry.getKey());
+
+        result.put(paramName, value);
+        continue;
       }
+
+      result.put(paramName, constant);
     }
-    return constants;
+
+    return result;
   }
 }

querydsl-libraries/querydsl-collections/src/test/java/com/querydsl/collections/DefaultEvaluatorFactoryTest.java

@@ -0,0 +1,72 @@
+package com.querydsl.collections;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import com.querydsl.codegen.utils.Evaluator;
+import com.querydsl.codegen.utils.EvaluatorFactory;
+import com.querydsl.codegen.utils.model.ClassType;
+import com.querydsl.codegen.utils.model.Type;
+import com.querydsl.core.DefaultQueryMetadata;
+import com.querydsl.core.types.dsl.BooleanExpression;
+import java.util.ArrayList;
+import java.util.Map;
+import org.junit.jupiter.api.Test;
+
+public class DefaultEvaluatorFactoryTest extends AbstractQueryTest {
+
+  /**
+   * The compiled evaluator is cached by a key that does not encode constant ordering, and its
+   * arguments are supplied positionally. The factory must therefore emit constants in a
+   * deterministic order matching the AST traversal so that a cached method is always invoked with
+   * its arguments in the same positions it was compiled with.
+   */
+  @Test
+  public void constants_follow_ast_order() {
+    var capturing = new CapturingEvaluatorFactory();
+    var factory = new DefaultEvaluatorFactory(CollQueryTemplates.DEFAULT, capturing);
+
+    BooleanExpression filter = cat.name.eq("c1");
+    for (var i = 2; i <= 12; i++) {
+      filter = filter.or(cat.name.eq("c" + i));
+    }
+
+    factory.createEvaluator(new DefaultQueryMetadata(), cat, filter);
+
+    var expectedKeys = new ArrayList<String>();
+    for (var i = 1; i <= 12; i++) {
+      expectedKeys.add("a" + i);
+    }
+    assertThat(capturing.constants.keySet()).containsExactlyElementsOf(expectedKeys);
+    for (var i = 1; i <= 12; i++) {
+      assertThat(capturing.constants.get("a" + i)).isEqualTo("c" + i);
+    }
+  }
+
+  private static final class CapturingEvaluatorFactory implements EvaluatorFactory {
+
+    private Map<String, Object> constants;
+
+    @Override
+    public <T> Evaluator<T> createEvaluator(
+        String source,
+        Class<? extends T> projectionType,
+        String[] names,
+        Class<?>[] classes,
+        Map<String, Object> constants) {
+      this.constants = constants;
+      return null;
+    }
+
+    @Override
+    public <T> Evaluator<T> createEvaluator(
+        String source,
+        ClassType projection,
+        String[] names,
+        Type[] types,
+        Class<?>[] classes,
+        Map<String, Object> constants) {
+      this.constants = constants;
+      return null;
+    }
+  }
+}