Merge pull request #2816 from ebean-orm/feature/2813-subQueryCompile-raceCondition

#2813 - Sometimes subquery use wrong alias in SQL

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<String> getPropertyNames() {
-
     TreeSet<String> propertyNames = new TreeSet<>();
     for (PropertyJoin join : joins.values()) {
       propertyNames.add(join.getProperty());

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;
+  }
 }

ebean-core/src/main/java/io/ebeaninternal/server/expression/DefaultExpressionList.java

@@ -76,11 +76,7 @@ public class DefaultExpressionList<T> implements SpiExpressionList<T> {
   }
 
   public DefaultExpressionList(Query<T> query, ExpressionList<T> parentExprList) {
-    this(query, query.getExpressionFactory(), parentExprList);
-  }
-
-  DefaultExpressionList(Query<T> query, ExpressionFactory expr, ExpressionList<T> parentExprList) {
-    this(query, expr, parentExprList, new ArrayList<>());
+    this(query, query.getExpressionFactory(), parentExprList, new ArrayList<>());
   }
 
   DefaultExpressionList(Query<T> query, ExpressionFactory expr, ExpressionList<T> parentExprList, List<SpiExpression> list) {
@@ -243,13 +239,12 @@ public class DefaultExpressionList<T> implements SpiExpressionList<T> {
 
   /**
    * Return a copy of the expression list.
-   * <p>
-   * Each of the expressions are expected to be immutable and safe to reference.
-   * </p>
    */
   public DefaultExpressionList<T> copy(Query<T> query) {
-    DefaultExpressionList<T> copy = new DefaultExpressionList<>(query, expr, null);
-    copy.list.addAll(list);
+    DefaultExpressionList<T> copy = new DefaultExpressionList<>(query, expr, null, new ArrayList<>(list.size()));
+    for (SpiExpression expr : list) {
+      copy.list.add(expr.copy());
+    }
     return copy;
   }
 

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

ebean-core/src/main/java/io/ebeaninternal/server/expression/FilterExpressionList.java

@@ -27,7 +27,7 @@ public final class FilterExpressionList<T> extends DefaultExpressionList<T> {
   }
 
   public FilterExpressionList(FilterExprPath pathPrefix, ExpressionFactory expr, Query<T> rootQuery) {
-    super(null, expr, null);
+    super(null, expr, null, new ArrayList<>());
     this.pathPrefix = pathPrefix;
     this.rootQuery = rootQuery;
   }

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);
     }

ebean-core/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java

@@ -412,21 +412,22 @@ public class DefaultOrmQuery<T> extends AbstractQuery implements SpiQuery<T> {
   }
 
   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;
   }
 
   /**

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 <T> DefaultExpressionList<T> spi(ExpressionList<T> 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();
+  }
 }

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() {
 

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() {
 

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<Object> list = new DefaultExpressionList<>(null, new DefaultExpressionFactory(true, false), null);
+    DefaultExpressionList<Object> list = new DefaultExpressionList<>(null, new DefaultExpressionFactory(true, false), null, new ArrayList<>());
     for (Expression ex : expressions) {
       list.add(ex);
     }

ebean-test/src/test/java/org/tests/query/TestQueryFindPagedList.java

@@ -70,11 +70,12 @@ public class TestQueryFindPagedList extends BaseTestCase {
       .setMaxRows(3)
       .findPagedList();
 
-    Future<Integer> rowCount = pagedList.getFutureCount();
+    pagedList.loadCount();
     List<Order> orders = pagedList.getList();
 
     // these are each getting the total row count
     int totalRowCount = pagedList.getTotalCount();
+    Future<Integer> rowCount = pagedList.getFutureCount();
     Integer totalRowCountWithTimeout = rowCount.get(30, TimeUnit.SECONDS);
     Integer totalRowCountViaFuture = rowCount.get();