From 29e2b81092cb989df827fc00c101a50dbbebce78 Mon Sep 17 00:00:00 2001 From: Rob Bygrave Date: Wed, 31 Aug 2022 17:24:55 +1200 Subject: [PATCH] #2813 - Sometimes subquery use wrong alias in SQL The reason for this is that as part of DefaultOrmQuery.copy() it uses DefaultExpressionList.copy() and that assumed that expressions were safe to share which is NOT the case for IN and EXISTS sub-query expressions so InQueryExpression and ExistsQueryExpression The effective fix for this is that DefaultExpressionList.copy() changes to call SpiExpression.copy() and for InQueryExpression and ExistsQueryExpression to implement that copy() by creating a copy of the sub-query. A "side-fix" is that in DefaultOrmQuery.createExtraJoinsToSupportManyWhereClause() it was creating an instance of ManyWhereJoins, then mutating it ... and if we change that to only doing the assignment at the end (object assignment is atomic) then racy access reading ManyWhereJoins would always get a fully completed non-mutating instance of ManyWhereJoins. Noting this because it kind of points to where I think the race condition is (in createExtraJoinsToSupportManyWhereClause()) but noting that with the change to DefaultExpressionList.copy() this "side-fix" isn't required per say. --- .../io/ebeaninternal/api/ManyWhereJoins.java | 2 - .../io/ebeaninternal/api/SpiExpression.java | 7 ++++ .../expression/DefaultExpressionList.java | 15 +++---- .../expression/ExistsQueryExpression.java | 5 +++ .../expression/FilterExpressionList.java | 2 +- .../server/expression/InQueryExpression.java | 8 ++-- .../server/querydefn/DefaultOrmQuery.java | 9 +++-- .../expression/DefaultExpressionListTest.java | 40 +++++++++++++++++-- .../expression/ExistsQueryExpressionTest.java | 20 ++++++++++ .../expression/InQueryExpressionTest.java | 21 +++++++++- .../expression/JunctionExpressionTest.java | 6 +-- .../tests/query/TestQueryFindPagedList.java | 3 +- 12 files changed, 110 insertions(+), 28 deletions(-) diff --git a/ebean-core/src/main/java/io/ebeaninternal/api/ManyWhereJoins.java b/ebean-core/src/main/java/io/ebeaninternal/api/ManyWhereJoins.java index 2278d5d11..463f1705e 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/api/ManyWhereJoins.java +++ b/ebean-core/src/main/java/io/ebeaninternal/api/ManyWhereJoins.java @@ -50,7 +50,6 @@ public final class ManyWhereJoins implements Serializable { * Add a many where join. */ public void add(ElPropertyDeploy elProp) { - String join = elProp.elPrefix(); BeanProperty p = elProp.beanProperty(); if (p instanceof BeanPropertyAssocMany) { @@ -111,7 +110,6 @@ public final class ManyWhereJoins implements Serializable { * Return the set of property names for the many where joins. */ public TreeSet getPropertyNames() { - TreeSet propertyNames = new TreeSet<>(); for (PropertyJoin join : joins.values()) { propertyNames.add(join.getProperty()); diff --git a/ebean-core/src/main/java/io/ebeaninternal/api/SpiExpression.java b/ebean-core/src/main/java/io/ebeaninternal/api/SpiExpression.java index e4392bf17..030042d96 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/api/SpiExpression.java +++ b/ebean-core/src/main/java/io/ebeaninternal/api/SpiExpression.java @@ -110,4 +110,11 @@ public interface SpiExpression extends Expression { * Apply property prefix when filterMany expressions included into main query. */ void prefixProperty(String path); + + /** + * Return a copy of the expression (as part of creating a query copy). + */ + default SpiExpression copy() { + return this; + } } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/expression/DefaultExpressionList.java b/ebean-core/src/main/java/io/ebeaninternal/server/expression/DefaultExpressionList.java index 26f225f3a..234fd28c2 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/expression/DefaultExpressionList.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/expression/DefaultExpressionList.java @@ -76,11 +76,7 @@ public class DefaultExpressionList implements SpiExpressionList { } public DefaultExpressionList(Query query, ExpressionList parentExprList) { - this(query, query.getExpressionFactory(), parentExprList); - } - - DefaultExpressionList(Query query, ExpressionFactory expr, ExpressionList parentExprList) { - this(query, expr, parentExprList, new ArrayList<>()); + this(query, query.getExpressionFactory(), parentExprList, new ArrayList<>()); } DefaultExpressionList(Query query, ExpressionFactory expr, ExpressionList parentExprList, List list) { @@ -243,13 +239,12 @@ public class DefaultExpressionList implements SpiExpressionList { /** * Return a copy of the expression list. - *

- * Each of the expressions are expected to be immutable and safe to reference. - *

*/ public DefaultExpressionList copy(Query query) { - DefaultExpressionList copy = new DefaultExpressionList<>(query, expr, null); - copy.list.addAll(list); + DefaultExpressionList copy = new DefaultExpressionList<>(query, expr, null, new ArrayList<>(list.size())); + for (SpiExpression expr : list) { + copy.list.add(expr.copy()); + } return copy; } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/expression/ExistsQueryExpression.java b/ebean-core/src/main/java/io/ebeaninternal/server/expression/ExistsQueryExpression.java index 1e8121e52..4a8717e4c 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/expression/ExistsQueryExpression.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/expression/ExistsQueryExpression.java @@ -27,6 +27,11 @@ final class ExistsQueryExpression implements SpiExpression, UnsupportedDocStoreE this.subQuery = null; } + @Override + public SpiExpression copy() { + return subQuery == null ? this : new ExistsQueryExpression(subQuery.copy(), not); + } + @Override public void prefixProperty(String path) { // do nothing diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/expression/FilterExpressionList.java b/ebean-core/src/main/java/io/ebeaninternal/server/expression/FilterExpressionList.java index 45d308eec..3b8c5005b 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/expression/FilterExpressionList.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/expression/FilterExpressionList.java @@ -27,7 +27,7 @@ public final class FilterExpressionList extends DefaultExpressionList { } public FilterExpressionList(FilterExprPath pathPrefix, ExpressionFactory expr, Query rootQuery) { - super(null, expr, null); + super(null, expr, null, new ArrayList<>()); this.pathPrefix = pathPrefix; this.rootQuery = rootQuery; } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/expression/InQueryExpression.java b/ebean-core/src/main/java/io/ebeaninternal/server/expression/InQueryExpression.java index 20d6a3d16..f922edbfe 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/expression/InQueryExpression.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/expression/InQueryExpression.java @@ -31,6 +31,11 @@ final class InQueryExpression extends AbstractExpression implements UnsupportedD this.bindParams = bindParams; } + @Override + public SpiExpression copy() { + return subQuery == null ? this : new InQueryExpression(propName, subQuery.copy(), not); + } + @Override public void simplify() { // do nothing @@ -43,7 +48,6 @@ final class InQueryExpression extends AbstractExpression implements UnsupportedD @Override public void prepareExpression(BeanQueryRequest request) { - CQuery subQuery = compileSubQuery(request); this.bindParams = subQuery.predicates().whereExprBindValues(); this.sql = subQuery.generatedSql().replace('\n', ' '); @@ -71,7 +75,6 @@ final class InQueryExpression extends AbstractExpression implements UnsupportedD @Override public void addSql(SpiExpressionRequest request) { - request.append(" (").append(propName).append(")"); if (not) { request.append(" not"); @@ -83,7 +86,6 @@ final class InQueryExpression extends AbstractExpression implements UnsupportedD @Override public void addBindValues(SpiExpressionRequest request) { - for (Object bindParam : bindParams) { request.addBindValue(bindParam); } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java b/ebean-core/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java index ceafabbb9..5c5ff7f14 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java @@ -412,21 +412,22 @@ public class DefaultOrmQuery extends AbstractQuery implements SpiQuery { } private void createExtraJoinsToSupportManyWhereClause() { - manyWhereJoins = new ManyWhereJoins(); + final var manyWhere = new ManyWhereJoins(); if (whereExpressions != null) { - whereExpressions.containsMany(beanDescriptor, manyWhereJoins); + whereExpressions.containsMany(beanDescriptor, manyWhere); } if (havingExpressions != null) { - havingExpressions.containsMany(beanDescriptor, manyWhereJoins); + havingExpressions.containsMany(beanDescriptor, manyWhere); } if (orderBy != null) { for (Property orderProperty : orderBy.getProperties()) { ElPropertyDeploy elProp = beanDescriptor.elPropertyDeploy(orderProperty.getProperty()); if (elProp != null && elProp.containsFormulaWithJoin()) { - manyWhereJoins.addFormulaWithJoin(elProp.elPrefix(), elProp.name()); + manyWhere.addFormulaWithJoin(elProp.elPrefix(), elProp.name()); } } } + manyWhereJoins = manyWhere; } /** diff --git a/ebean-core/src/test/java/io/ebeaninternal/server/expression/DefaultExpressionListTest.java b/ebean-core/src/test/java/io/ebeaninternal/server/expression/DefaultExpressionListTest.java index 1fa245832..b89590936 100644 --- a/ebean-core/src/test/java/io/ebeaninternal/server/expression/DefaultExpressionListTest.java +++ b/ebean-core/src/test/java/io/ebeaninternal/server/expression/DefaultExpressionListTest.java @@ -1,16 +1,20 @@ package io.ebeaninternal.server.expression; import io.ebean.ExpressionList; +import io.ebean.Query; +import io.ebeaninternal.api.SpiQuery; import org.junit.jupiter.api.Test; +import java.util.ArrayList; + import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; public class DefaultExpressionListTest extends BaseExpressionTest { - DefaultExpressionList exp() { - - return new DefaultExpressionList<>(null, new DefaultExpressionFactory(true, true), null); + return new DefaultExpressionList<>(null, new DefaultExpressionFactory(true, true), null, new ArrayList<>()); } private DefaultExpressionList spi(ExpressionList list) { @@ -95,4 +99,34 @@ public class DefaultExpressionListTest extends BaseExpressionTest { .isSameByBind(spi(exp().eq("a", 10).eq("b", 20)))).isFalse(); } + @SuppressWarnings("unchecked") + @Test + void copy() { + DefaultExpressionList orig = exp(); + orig.eq("a", 10).in("b", 11); + DefaultExpressionList copy = (DefaultExpressionList)orig.copy(mock(Query.class)); + + assertThat(copy).isNotSameAs(orig); + assertThat(copy.list).hasSize(2); + assertThat(copy.list.get(0)).isSameAs(orig.list.get(0)); + assertThat(copy.list.get(1)).isSameAs(orig.list.get(1)); + } + + @SuppressWarnings("unchecked") + @Test + void copy_withSubQuery() { + DefaultExpressionList orig = exp(); + SpiQuery inSubQuery = mock(SpiQuery.class); + SpiQuery existsSubQuery = mock(SpiQuery.class); + orig.eq("a", 10).in("name", inSubQuery).exists(existsSubQuery); + + DefaultExpressionList copy = (DefaultExpressionList)orig.copy(mock(Query.class)); + + assertThat(copy.list).hasSize(3); + assertThat(copy.list.get(0)).isSameAs(orig.list.get(0)); + assertThat(copy.list.get(1)).isNotSameAs(orig.list.get(1)); + assertThat(copy.list.get(2)).isNotSameAs(orig.list.get(2)); + verify(inSubQuery).copy(); + verify(existsSubQuery).copy(); + } } diff --git a/ebean-core/src/test/java/io/ebeaninternal/server/expression/ExistsQueryExpressionTest.java b/ebean-core/src/test/java/io/ebeaninternal/server/expression/ExistsQueryExpressionTest.java index b869096a1..37433fbdf 100644 --- a/ebean-core/src/test/java/io/ebeaninternal/server/expression/ExistsQueryExpressionTest.java +++ b/ebean-core/src/test/java/io/ebeaninternal/server/expression/ExistsQueryExpressionTest.java @@ -1,10 +1,14 @@ package io.ebeaninternal.server.expression; +import io.ebeaninternal.api.SpiExpression; +import io.ebeaninternal.api.SpiQuery; import org.junit.jupiter.api.Test; import java.util.Arrays; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; public class ExistsQueryExpressionTest extends BaseExpressionTest { @@ -13,6 +17,22 @@ public class ExistsQueryExpressionTest extends BaseExpressionTest { return new ExistsQueryExpression(not, sql, Arrays.asList(bindValues)); } + @Test + void copy_subQuery_expectNewInstance() { + SpiQuery subQuery = mock(SpiQuery.class); + var orig = new ExistsQueryExpression(subQuery, false); + SpiExpression copy = orig.copy(); + assertThat(copy).isNotSameAs(orig); + verify(subQuery).copy(); + } + + @Test + void copy_sqlLiteral_expectSameInstance() { + var orig = exp(true, "sql", 10); + SpiExpression copy = orig.copy(); + assertThat(copy).isSameAs(orig); + } + @Test public void isSameByPlan_when_same() { diff --git a/ebean-core/src/test/java/io/ebeaninternal/server/expression/InQueryExpressionTest.java b/ebean-core/src/test/java/io/ebeaninternal/server/expression/InQueryExpressionTest.java index 1ab4853b5..b8d9fc451 100644 --- a/ebean-core/src/test/java/io/ebeaninternal/server/expression/InQueryExpressionTest.java +++ b/ebean-core/src/test/java/io/ebeaninternal/server/expression/InQueryExpressionTest.java @@ -1,18 +1,37 @@ package io.ebeaninternal.server.expression; +import io.ebeaninternal.api.SpiExpression; +import io.ebeaninternal.api.SpiQuery; import org.junit.jupiter.api.Test; import java.util.Arrays; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; public class InQueryExpressionTest extends BaseExpressionTest { - private InQueryExpression exp(String propertyName, boolean not, String sql, Object... bindValues) { return new InQueryExpression(propertyName, not, sql, Arrays.asList(bindValues)); } + @Test + void copy_subQuery_expectNewInstance() { + SpiQuery subQuery = mock(SpiQuery.class); + var orig = new InQueryExpression("name", subQuery, false); + SpiExpression copy = orig.copy(); + assertThat(copy).isNotSameAs(orig); + verify(subQuery).copy(); + } + + @Test + void copy_sqlLiteral_expectSameInstance() { + var orig = exp("name", true, "sql", 10); + SpiExpression copy = orig.copy(); + assertThat(copy).isSameAs(orig); + } + @Test public void isSameByPlan_when_same() { diff --git a/ebean-core/src/test/java/io/ebeaninternal/server/expression/JunctionExpressionTest.java b/ebean-core/src/test/java/io/ebeaninternal/server/expression/JunctionExpressionTest.java index b28d44c05..b1d4db672 100644 --- a/ebean-core/src/test/java/io/ebeaninternal/server/expression/JunctionExpressionTest.java +++ b/ebean-core/src/test/java/io/ebeaninternal/server/expression/JunctionExpressionTest.java @@ -5,16 +5,16 @@ import io.ebean.Expression; import io.ebean.Junction; import org.junit.jupiter.api.Test; +import java.util.ArrayList; + public class JunctionExpressionTest extends BaseExpressionTest { Expression eq(String propName, int value) { return Expr.eq(propName, value); } - DefaultExpressionList exp(Expression... expressions) { - - DefaultExpressionList list = new DefaultExpressionList<>(null, new DefaultExpressionFactory(true, false), null); + DefaultExpressionList list = new DefaultExpressionList<>(null, new DefaultExpressionFactory(true, false), null, new ArrayList<>()); for (Expression ex : expressions) { list.add(ex); } diff --git a/ebean-test/src/test/java/org/tests/query/TestQueryFindPagedList.java b/ebean-test/src/test/java/org/tests/query/TestQueryFindPagedList.java index 69092641e..629776577 100644 --- a/ebean-test/src/test/java/org/tests/query/TestQueryFindPagedList.java +++ b/ebean-test/src/test/java/org/tests/query/TestQueryFindPagedList.java @@ -70,11 +70,12 @@ public class TestQueryFindPagedList extends BaseTestCase { .setMaxRows(3) .findPagedList(); - Future rowCount = pagedList.getFutureCount(); + pagedList.loadCount(); List orders = pagedList.getList(); // these are each getting the total row count int totalRowCount = pagedList.getTotalCount(); + Future rowCount = pagedList.getFutureCount(); Integer totalRowCountWithTimeout = rowCount.get(30, TimeUnit.SECONDS); Integer totalRowCountViaFuture = rowCount.get();