FIX: Pairs must not be modified by query (#1539)

* FIX: Pairs must not be modified by query

* Modification can also occur on inValues.

* fixed tests

* removed obsolete line

src/main/java/io/ebean/Pairs.java

@@ -1,6 +1,7 @@
 package io.ebean;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 
 /**
@@ -125,7 +126,7 @@ public class Pairs {
    * Return all the value pairs.
    */
   public List<Entry> getEntries() {
-    return entries;
+    return Collections.unmodifiableList(entries);
   }
 
   /**

src/main/java/io/ebeaninternal/api/NaturalKeyQueryData.java

@@ -25,7 +25,7 @@ public class NaturalKeyQueryData<T> {
   private List<Pairs.Entry> inPairs;
 
   // IN clause - only one allowed
-  private Collection<?> inValues;
+  private List<Object> inValues;
   private String inProperty;
 
   // normal EQ expressions
@@ -52,36 +52,36 @@ public class NaturalKeyQueryData<T> {
   /**
    * Match for In Pairs expression. We only allow one IN clause.
    */
-  public boolean matchInPairs(Pairs pairs) {
+  public List<Pairs.Entry> matchInPairs(String property0, String property1, List<Pairs.Entry> inPairs) {
     if (hasIn) {
       // only 1 IN allowed (to project naturalIds)
-      return false;
+      return null;
     }
-    if (matchProperty(pairs.getProperty0()) && matchProperty(pairs.getProperty1())) {
+    if (matchProperty(property0) && matchProperty(property1)) {
       this.hasIn = true;
-      this.inProperty0 = pairs.getProperty0();
-      this.inProperty1 = pairs.getProperty1();
-      this.inPairs = pairs.getEntries();
-      return true;
+      this.inProperty0 = property0;
+      this.inProperty1 = property1;
+      this.inPairs = new ArrayList<>(inPairs); // will be modified
+      return this.inPairs;
     }
-    return false;
+    return null;
   }
 
   /**
    * Match for IN expression. We only allow one IN clause.
    */
-  public boolean matchIn(String propName, Collection<?> sourceValues) {
+  public List<Object> matchIn(String propName, List<Object> sourceValues) {
     if (hasIn) {
       // only 1 IN allowed (to project naturalIds)
-      return false;
+      return null;
     }
     if (matchProperty(propName)) {
       this.hasIn = true;
       this.inProperty = propName;
-      this.inValues = sourceValues;
-      return true;
+      this.inValues = new ArrayList<>(sourceValues);
+      return this.inValues;
     }
-    return false;
+    return null;
   }
 
   /**

src/main/java/io/ebeaninternal/server/expression/InExpression.java

@@ -48,8 +48,16 @@ class InExpression extends AbstractExpression {
 
   @Override
   public boolean naturalKey(NaturalKeyQueryData<?> data) {
-    // can't use naturalKey cache for NOT IN
-    return !not && data.matchIn(propName, bindValues);
+    // can't use naturalKey cache for NOT IN or if multi values are used
+    if (not || multiValueSupported) {
+      return false;
+    }
+    List<Object> copy = data.matchIn(propName, bindValues);
+    if (copy == null) {
+      return false;
+    }
+    bindValues = copy;
+    return true;
   }
 
   @Override

src/main/java/io/ebeaninternal/server/expression/InPairsExpression.java

@@ -1,6 +1,7 @@
 package io.ebeaninternal.server.expression;
 
 import io.ebean.Pairs;
+import io.ebean.Pairs.Entry;
 import io.ebean.event.BeanQueryRequest;
 import io.ebeaninternal.api.NaturalKeyQueryData;
 import io.ebeaninternal.api.SpiExpression;
@@ -15,11 +16,9 @@ class InPairsExpression extends AbstractExpression {
 
   private final boolean not;
 
-  private final Pairs pairs;
-
   private final String property0, property1;
 
-  private final List<Pairs.Entry> entries;
+  private List<Pairs.Entry> entries;
 
   private boolean multiValueSupported;
 
@@ -31,9 +30,9 @@ class InPairsExpression extends AbstractExpression {
 
   InPairsExpression(Pairs pairs, boolean not) {
     super(pairs.getProperty0());
-    this.pairs = pairs;
     this.property0 = pairs.getProperty0();
     this.property1 = pairs.getProperty1();
+    // the entries might be modified on cache hit.
     this.entries = pairs.getEntries();
     this.not = not;
     this.separator = pairs.getConcatSeparator();
@@ -42,7 +41,15 @@ class InPairsExpression extends AbstractExpression {
 
   @Override
   public boolean naturalKey(NaturalKeyQueryData<?> data) {
-    return !not && data.matchInPairs(pairs);
+    if (not) {
+      return false;
+    }
+    List<Entry> copy = data.matchInPairs(property0, property1, entries);
+    if (copy == null) {
+      return false;
+    }
+    entries = copy;
+    return true;
   }
 
   @Override

src/test/java/org/tests/model/basic/cache/TestCacheViaComplexNaturalKey3.java

@@ -31,9 +31,9 @@ public class TestCacheViaComplexNaturalKey3 extends BaseTestCase {
     if (!loadOnce) {
       Ebean.find(OCachedNatKeyBean3.class).delete();
 
-      List<String> stores = new ArrayList<>(Arrays.asList("abc", "def"));
+      List<String> stores =Arrays.asList("abc", "def");
       for (String store : stores) {
-        List<String> skus = new ArrayList<>(Arrays.asList("1", "2", "3"));
+        List<String> skus = Arrays.asList("1", "2", "3");
         for (String sku : skus) {
           int[] codes = {1000,1001,1002,1003,1004};
           for (int code : codes) {
@@ -99,7 +99,7 @@ public class TestCacheViaComplexNaturalKey3 extends BaseTestCase {
     loadSomeIntoCache();
 
 
-    List<Integer> codes = new ArrayList<>(Arrays.asList(1001, 1000, 1002, 1003));
+    List<Integer> codes = Arrays.asList(1001, 1000, 1002, 1003);
 
     LoggedSqlCollector.start();
 
@@ -132,7 +132,7 @@ public class TestCacheViaComplexNaturalKey3 extends BaseTestCase {
     setup();
     loadSomeIntoCache();
 
-    List<String> skus = new ArrayList<>(Arrays.asList("2", "3"));
+    List<String> skus = Arrays.asList("2", "3");
 
     LoggedSqlCollector.start();
 
@@ -162,7 +162,7 @@ public class TestCacheViaComplexNaturalKey3 extends BaseTestCase {
     loadSomeIntoCache();
 
     String storeId = "abc";
-    List<String> skus = new ArrayList<>(Arrays.asList("3", "2", "4"));
+    List<String> skus = Arrays.asList("3", "2", "4");
 
     LoggedSqlCollector.start();
 
@@ -272,6 +272,8 @@ public class TestCacheViaComplexNaturalKey3 extends BaseTestCase {
 
     List<String> sql = LoggedSqlCollector.stop();
 
+    assertThat(pairs.getEntries()).hasSize(3);
+
     assertThat(list).hasSize(3);
     assertNaturalKeyHitMiss(1, 2);
     assertBeanCacheHitMiss(1, 0);
@@ -313,6 +315,8 @@ public class TestCacheViaComplexNaturalKey3 extends BaseTestCase {
 
     List<String> sql = LoggedSqlCollector.stop();
 
+    assertThat(pairs.getEntries()).hasSize(3);
+
     assertThat(list).hasSize(3);
     assertNaturalKeyHitMiss(1, 2);
     assertBeanCacheHitMiss(1, 0);