Merge pull request #3469 from ebean-orm/feature/orderBy2

#3466 - Duplicated entities in findMany when a ManyToOne relation is fetched

ebean-core/src/main/java/io/ebeaninternal/api/SpiQuery.java

@@ -580,7 +580,12 @@ public static TemporalMode of(SpiQuery<?> query) {
   /**
    * Convert joins as necessary to query joins etc.
    */
-  SpiQuerySecondary convertJoins();
+  SpiQueryManyJoin convertJoins();
+
+  /**
+   * Return secondary queries if required.
+   */
+  SpiQuerySecondary secondaryQuery();
 
   /**
    * Return the TransactionContext.

ebean-core/src/main/java/io/ebeaninternal/api/SpiQueryManyJoin.java

@@ -0,0 +1,17 @@
+package io.ebeaninternal.api;
+
+/**
+ * A SQL Join for a ToMany property included in the query.
+ */
+public interface SpiQueryManyJoin {
+
+  /**
+   * The full path of the many property to include in the query via join.
+   */
+  String path();
+
+  /**
+   * Order by clause defined via mapping on the ToMany property.
+   */
+  String fetchOrderBy();
+}

ebean-core/src/main/java/io/ebeaninternal/server/core/OrmQueryRequest.java

@@ -41,9 +41,9 @@ public final class OrmQueryRequest<T> extends BeanRequest implements SpiOrmQuery
   private CQueryPlanKey queryPlanKey;
   private SpiQuerySecondary secondaryQueries;
   private List<T> cacheBeans;
-  private BeanPropertyAssocMany<?> manyProperty;
   private boolean inlineCountDistinct;
   private Set<String> dependentTables;
+  private SpiQueryManyJoin manyJoin;
 
   public OrmQueryRequest(SpiEbeanServer server, OrmQueryEngine queryEngine, SpiQuery<T> query, SpiTransaction t) {
     super(server, t);
@@ -168,7 +168,8 @@ private void adapterPreQuery() {
    */
   @Override
   public void prepareQuery() {
-    secondaryQueries = query.convertJoins();
+    manyJoin = query.convertJoins();
+    secondaryQueries = query.secondaryQuery();
     beanDescriptor.prepareQuery(query);
     adapterPreQuery();
     queryPlanKey = query.prepare(this);
@@ -446,19 +447,19 @@ public SpiQuery<T> query() {
     return query;
   }
 
-  /**
-   * Determine and return the ToMany property that is included in the query.
-   */
-  public BeanPropertyAssocMany<?> determineMany() {
-    manyProperty = beanDescriptor.manyProperty(query);
-    return manyProperty;
+  public SpiQueryManyJoin manyJoin() {
+    return manyJoin;
   }
 
   /**
-   * Return the many property that is fetched in the query or null if there is not one.
+   * Return true if there is a manyJoin that should be included with this query.
    */
-  public BeanPropertyAssocMany<?> manyPropertyForOrderBy() {
-    return query.isSingleAttribute() ? null : manyProperty;
+  public boolean includeManyJoin() {
+    if (manyJoin == null || query.isSingleAttribute()) {
+      return false;
+    }
+    final Type type = query.type();
+    return type != Type.SQ_EX && type != Type.COUNT;
   }
 
   /**

ebean-core/src/main/java/io/ebeaninternal/server/deploy/BeanDescriptor.java

@@ -1601,19 +1601,6 @@ private BeanProperty findWhenModifiedProperty() {
     return null;
   }
 
-  /**
-   * Return the many property included in the query or null if one is not.
-   */
-  public BeanPropertyAssocMany<?> manyProperty(SpiQuery<?> query) {
-    OrmQueryDetail detail = query.detail();
-    for (BeanPropertyAssocMany<?> many : propertiesMany) {
-      if (detail.includesPath(many.name())) {
-        return many;
-      }
-    }
-    return null;
-  }
-
   /**
    * Return a raw expression for 'where parent id in ...' clause.
    */

ebean-core/src/main/java/io/ebeaninternal/server/el/ElPropertyDeploy.java

@@ -1,5 +1,6 @@
 package io.ebeaninternal.server.el;
 
+import io.ebeaninternal.api.SpiQueryManyJoin;
 import io.ebeaninternal.server.deploy.BeanProperty;
 
 /**
@@ -10,7 +11,7 @@
  * joins and set place holders for table alias'.
  * </p>
  */
-public interface ElPropertyDeploy {
+public interface ElPropertyDeploy extends SpiQueryManyJoin {
 
   /**
    * This is the elPrefix for all root level properties.
@@ -80,4 +81,14 @@ public interface ElPropertyDeploy {
    * is left as a 'join' and which get converted to query join.
    */
   int fetchPreference();
+
+  @Override
+  default String path() {
+    return elName();
+  }
+
+  @Override
+  default String fetchOrderBy() {
+    return beanProperty().fetchOrderBy();
+  }
 }

ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryPredicates.java

@@ -1,13 +1,9 @@
 package io.ebeaninternal.server.query;
 
 import io.ebean.OrderBy;
-import io.ebeaninternal.api.BindParams;
-import io.ebeaninternal.api.CoreLog;
-import io.ebeaninternal.api.SpiExpressionList;
-import io.ebeaninternal.api.SpiQuery;
+import io.ebeaninternal.api.*;
 import io.ebeaninternal.server.bind.DataBind;
 import io.ebeaninternal.server.core.OrmQueryRequest;
-import io.ebeaninternal.server.deploy.BeanPropertyAssocMany;
 import io.ebeaninternal.server.deploy.DeployParser;
 import io.ebeaninternal.server.expression.DefaultExpressionRequest;
 import io.ebeaninternal.server.persist.Binder;
@@ -175,9 +171,8 @@ public void prepare(boolean buildSql) {
     buildUpdateClause(buildSql, deployParser);
     buildBindWhereRawSql(buildSql);
 
-    BeanPropertyAssocMany<?> manyProperty = request.determineMany();
     if (buildSql) {
-      dbOrderBy = deriveOrderByWithMany(deployParser, request.manyPropertyForOrderBy());
+      dbOrderBy = deriveOrderByWithMany(deployParser);
       // create a copy of the includes required to support the orderBy
       orderByIncludes = new HashSet<>(deployParser.includes());
     }
@@ -188,13 +183,16 @@ public void prepare(boolean buildSql) {
         dbWhere = where.buildSql();
       }
     }
+    SpiQueryManyJoin manyProperty = request.manyJoin();
     if (manyProperty != null) {
-      OrmQueryProperties chunk = query.detail().getChunk(manyProperty.name(), false);
-      SpiExpressionList<?> filterManyExpr = chunk.getFilterMany();
-      if (filterManyExpr != null) {
-        this.filterMany = new DefaultExpressionRequest(request, deployParser, binder, filterManyExpr);
-        if (buildSql) {
-          dbFilterMany = filterMany.buildSql();
+      OrmQueryProperties chunk = query.detail().getChunk(manyProperty.path(), false);
+      if (chunk != null) {
+        SpiExpressionList<?> filterManyExpr = chunk.getFilterMany();
+        if (filterManyExpr != null) {
+          this.filterMany = new DefaultExpressionRequest(request, deployParser, binder, filterManyExpr);
+          if (buildSql) {
+            dbFilterMany = filterMany.buildSql();
+          }
         }
       }
     }
@@ -248,19 +246,20 @@ private String parseOrderBy(DeployParser parser) {
   /**
    * There is a many property we need to make sure the ordering is appropriate.
    */
-  private String deriveOrderByWithMany(DeployParser parser, BeanPropertyAssocMany<?> manyProp) {
+  private String deriveOrderByWithMany(DeployParser parser) {
     String orderBy = parseOrderBy(parser);
-    if (manyProp == null) {
+    if (!request.includeManyJoin()) {
       return orderBy;
     }
     String orderById = parser.parse(request.descriptor().defaultOrderBy());
     if (orderBy == null) {
       orderBy = orderById;
     }
     // check for default ordering on the many property...
+    SpiQueryManyJoin manyProp = request.manyJoin();
     String manyOrderBy = manyProp.fetchOrderBy();
     if (manyOrderBy != null) {
-      orderBy = orderBy + ", " + parser.parse(CQueryBuilder.prefixOrderByFields(manyProp.name(), manyOrderBy));
+      orderBy = orderBy + ", " + parser.parse(CQueryBuilder.prefixOrderByFields(manyProp.path(), manyOrderBy));
     }
     if (request.isFindById()) {
       // only one master bean so should be fine...
@@ -273,7 +272,7 @@ private String deriveOrderByWithMany(DeployParser parser, BeanPropertyAssocMany<
     if (idPos == 0) {
       return orderBy;
     }
-    int manyPos = orderBy.indexOf("${" + manyProp.name() + "}");
+    int manyPos = orderBy.indexOf("${" + manyProp.path() + "}");
     if (manyPos == -1) {
       // no ordering of the many
       if (idPos == -1) {
@@ -286,10 +285,10 @@ private String deriveOrderByWithMany(DeployParser parser, BeanPropertyAssocMany<
     if (idPos == -1 || idPos >= manyPos) {
       if (idPos > manyPos) {
         // there was an error with the order by...
-        String msg = "A Query on [" + request.descriptor() + "] includes a join to a 'many' association [" + manyProp.name()
-        + "] with an incorrect orderBy [" + orderBy + "]. The id property [" + orderById
-        + "] must come before the many property [" + manyProp.name() + "] in the orderBy."
-        + " Ebean has automatically modified the orderBy clause to do this.";
+        String msg = "A Query on [" + request.descriptor() + "] includes a join to a 'many' association [" + manyProp.path()
+          + "] with an incorrect orderBy [" + orderBy + "]. The id property [" + orderById
+          + "] must come before the many property [" + manyProp.path() + "] in the orderBy."
+          + " Ebean has automatically modified the orderBy clause to do this.";
         CoreLog.log.log(WARNING, msg);
       }
       // the id needs to come before the manyPropName

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

@@ -504,22 +504,26 @@ public final void setLazyLoadManyPath(String lazyLoadManyPath) {
   }
 
   @Override
-  public final SpiQuerySecondary convertJoins() {
+  public final SpiQueryManyJoin convertJoins() {
     if (!useDocStore) {
       createExtraJoinsToSupportManyWhereClause();
     }
-    markQueryJoins();
+    return markQueryJoins();
+  }
+
+  @Override
+  public SpiQuerySecondary secondaryQuery() {
     return new OrmQuerySecondary(removeQueryJoins(), removeLazyJoins());
   }
 
   /**
    * Limit the number of fetch joins to Many properties, mark as query joins as needed.
+   *
+   * @return The query join many property or null.
    */
-  private void markQueryJoins() {
-    if (distinctOn == null) {
-      // no automatic join to query join conversion when distinctOn is used
-      detail.markQueryJoins(beanDescriptor, lazyLoadManyPath, isAllowOneManyFetch(), type.defaultSelect());
-    }
+  private SpiQueryManyJoin markQueryJoins() {
+    // no automatic join to query join conversion when distinctOn is used
+    return distinctOn != null ? null : detail.markQueryJoins(beanDescriptor, lazyLoadManyPath, isAllowOneManyFetch(), type.defaultSelect());
   }
 
   private boolean isAllowOneManyFetch() {

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

@@ -3,6 +3,7 @@
 import io.ebean.FetchConfig;
 import io.ebean.event.BeanQueryRequest;
 import io.ebean.util.SplitName;
+import io.ebeaninternal.api.SpiQueryManyJoin;
 import io.ebeaninternal.server.deploy.BeanDescriptor;
 import io.ebeaninternal.server.deploy.BeanPropertyAssoc;
 import io.ebeaninternal.server.el.ElPropertyDeploy;
@@ -327,12 +328,15 @@ private void sortFetchPaths(BeanDescriptor<?> d, OrmQueryProperties p, LinkedHas
 
   /**
    * Mark 'fetch joins' to 'many' properties over to 'query joins' where needed.
+   *
+   * @return The fetch join many property or null
    */
-  void markQueryJoins(BeanDescriptor<?> beanDescriptor, String lazyLoadManyPath, boolean allowOne, boolean addIds) {
+  SpiQueryManyJoin markQueryJoins(BeanDescriptor<?> beanDescriptor, String lazyLoadManyPath, boolean allowOne, boolean addIds) {
     if (fetchPaths.isEmpty()) {
-      return;
+      return null;
     }
 
+    ElPropertyDeploy many = null;
     // the name of the many fetch property if there is one
     String manyFetchProperty = null;
     // flag that is set once the many fetch property is chosen
@@ -353,13 +357,15 @@ void markQueryJoins(BeanDescriptor<?> beanDescriptor, String lazyLoadManyPath, b
             fetchJoinFirstMany = false;
             manyFetchProperty = pair.getPath();
             chunk.filterManyInline();
+            many = elProp;
           } else {
             // convert this one over to a 'query join'
             chunk.markForQueryJoin();
           }
         }
       }
     }
+    return many;
   }
 
   /**