From 105f08cca1aaa736c948aafc12d74746b0dde6db Mon Sep 17 00:00:00 2001 From: Roland Praml Date: Tue, 13 Nov 2018 11:41:36 +0100 Subject: [PATCH] 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 | 3 +- .../api/NaturalKeyQueryData.java | 28 +++++++++---------- .../server/expression/InExpression.java | 12 ++++++-- .../server/expression/InPairsExpression.java | 17 +++++++---- .../cache/TestCacheViaComplexNaturalKey3.java | 14 ++++++---- 5 files changed, 47 insertions(+), 27 deletions(-) diff --git a/src/main/java/io/ebean/Pairs.java b/src/main/java/io/ebean/Pairs.java index d32d1c986..3c99bceed 100644 --- a/src/main/java/io/ebean/Pairs.java +++ b/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 getEntries() { - return entries; + return Collections.unmodifiableList(entries); } /** diff --git a/src/main/java/io/ebeaninternal/api/NaturalKeyQueryData.java b/src/main/java/io/ebeaninternal/api/NaturalKeyQueryData.java index d542404f4..ddd02d768 100644 --- a/src/main/java/io/ebeaninternal/api/NaturalKeyQueryData.java +++ b/src/main/java/io/ebeaninternal/api/NaturalKeyQueryData.java @@ -25,7 +25,7 @@ public class NaturalKeyQueryData { private List inPairs; // IN clause - only one allowed - private Collection inValues; + private List inValues; private String inProperty; // normal EQ expressions @@ -52,36 +52,36 @@ public class NaturalKeyQueryData { /** * Match for In Pairs expression. We only allow one IN clause. */ - public boolean matchInPairs(Pairs pairs) { + public List matchInPairs(String property0, String property1, List 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 matchIn(String propName, List 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; } /** diff --git a/src/main/java/io/ebeaninternal/server/expression/InExpression.java b/src/main/java/io/ebeaninternal/server/expression/InExpression.java index c1101b9a2..ccb1a5862 100644 --- a/src/main/java/io/ebeaninternal/server/expression/InExpression.java +++ b/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 copy = data.matchIn(propName, bindValues); + if (copy == null) { + return false; + } + bindValues = copy; + return true; } @Override diff --git a/src/main/java/io/ebeaninternal/server/expression/InPairsExpression.java b/src/main/java/io/ebeaninternal/server/expression/InPairsExpression.java index 2147af1a2..07dbc2945 100644 --- a/src/main/java/io/ebeaninternal/server/expression/InPairsExpression.java +++ b/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 entries; + private List 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 copy = data.matchInPairs(property0, property1, entries); + if (copy == null) { + return false; + } + entries = copy; + return true; } @Override diff --git a/src/test/java/org/tests/model/basic/cache/TestCacheViaComplexNaturalKey3.java b/src/test/java/org/tests/model/basic/cache/TestCacheViaComplexNaturalKey3.java index 34a484ce9..1f1073116 100644 --- a/src/test/java/org/tests/model/basic/cache/TestCacheViaComplexNaturalKey3.java +++ b/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 stores = new ArrayList<>(Arrays.asList("abc", "def")); + List stores =Arrays.asList("abc", "def"); for (String store : stores) { - List skus = new ArrayList<>(Arrays.asList("1", "2", "3")); + List 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 codes = new ArrayList<>(Arrays.asList(1001, 1000, 1002, 1003)); + List codes = Arrays.asList(1001, 1000, 1002, 1003); LoggedSqlCollector.start(); @@ -132,7 +132,7 @@ public class TestCacheViaComplexNaturalKey3 extends BaseTestCase { setup(); loadSomeIntoCache(); - List skus = new ArrayList<>(Arrays.asList("2", "3")); + List skus = Arrays.asList("2", "3"); LoggedSqlCollector.start(); @@ -162,7 +162,7 @@ public class TestCacheViaComplexNaturalKey3 extends BaseTestCase { loadSomeIntoCache(); String storeId = "abc"; - List skus = new ArrayList<>(Arrays.asList("3", "2", "4")); + List skus = Arrays.asList("3", "2", "4"); LoggedSqlCollector.start(); @@ -272,6 +272,8 @@ public class TestCacheViaComplexNaturalKey3 extends BaseTestCase { List 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 sql = LoggedSqlCollector.stop(); + assertThat(pairs.getEntries()).hasSize(3); + assertThat(list).hasSize(3); assertNaturalKeyHitMiss(1, 2); assertBeanCacheHitMiss(1, 0);