Fix for #154 - @OrderBy on a @OneToMany property is not used when lazy loading

src/main/java/com/avaje/ebeaninternal/server/core/DefaultBeanLoader.java

@@ -108,7 +108,10 @@ public class DefaultBeanLoader {
     BeanDescriptor<?> desc = ctx.getBeanDescriptor();
 
     SpiQuery<?> query = (SpiQuery<?>) server.createQuery(many.getTargetType());
-
+    String orderBy = many.getLazyFetchOrderBy();
+    if (orderBy != null) {
+      query.orderBy(orderBy);
+    }
     query.setLazyLoadForParents(idList, many);
     many.addWhereParentIdIn(query, idList);
 

src/main/java/com/avaje/ebeaninternal/server/deploy/BeanPropertyAssocMany.java

@@ -60,8 +60,16 @@ public class BeanPropertyAssocMany<T> extends BeanPropertyAssoc<T> {
    */
   private final boolean manyToMany;
 
+  /**
+   * Order by used when fetch joining the associated many.
+   */
   private final String fetchOrderBy;
 
+  /**
+   * Order by used when lazy loading the associated many.
+   */
+  private String lazyFetchOrderBy;
+
   private final String mapKey;
 
   /**
@@ -137,6 +145,22 @@ public class BeanPropertyAssocMany<T> extends BeanPropertyAssoc<T> {
 			if (exportedProperties.length > 0){
 				embeddedExportedProperties = exportedProperties[0].isEmbedded();
 				exportedPropertyBindProto = deriveExportedPropertyBindProto();
+				
+				if (fetchOrderBy != null) {
+  				// derive lazyFetchOrderBy
+  				StringBuilder sb = new StringBuilder(50);
+  	      for (int i = 0; i < exportedProperties.length; i++) {
+  	        if (i > 0) {
+  	          sb.append(", ");
+  	        }
+  	        // these fkcolumns always on base table hence t0 as alias
+  	        sb.append("t0.").append(exportedProperties[i].getForeignDbColumn()); 
+  	      }
+  	      if (fetchOrderBy != null) {
+  	        sb.append(", ").append(fetchOrderBy);
+  	      }
+  	      lazyFetchOrderBy = sb.toString().trim();
+				}
 			}
 			
 			String delStmt;
@@ -510,6 +534,13 @@ public class BeanPropertyAssocMany<T> extends BeanPropertyAssoc<T> {
 	}
 
 	/**
+	 * Return the order by for use when lazy loading the associated collection.
+	 */
+	public String getLazyFetchOrderBy() {
+    return lazyFetchOrderBy;
+  }
+
+  /**
 	 * Return the default mapKey when returning a Map.
 	 */
 	public String getMapKey() {

src/test/java/com/avaje/tests/basic/TestSharedInstancePropagation.java

@@ -2,8 +2,7 @@ package com.avaje.tests.basic;
 
 import java.util.List;
 
-import junit.framework.Assert;
-
+import org.junit.Assert;
 import org.junit.Test;
 
 import com.avaje.ebean.BaseTestCase;
@@ -25,6 +24,8 @@ public class TestSharedInstancePropagation extends BaseTestCase {
 
 		ResetBasicData.reset();
 		
+		Ebean.getServerCacheManager().clearAll();
+
 		Order order = Ebean.find(Order.class)
 			.setAutofetch(false)
 			.setUseCache(true)

src/test/java/com/avaje/tests/query/orderby/TestOrderByWithMany.java

@@ -1,5 +1,8 @@
 package com.avaje.tests.query.orderby;
 
+import java.util.List;
+
+import org.avaje.ebeantest.LoggedSqlCollector;
 import org.junit.Assert;
 import org.junit.Test;
 
@@ -7,6 +10,7 @@ import com.avaje.ebean.BaseTestCase;
 import com.avaje.ebean.Ebean;
 import com.avaje.ebean.Query;
 import com.avaje.tests.model.basic.Order;
+import com.avaje.tests.model.basic.OrderDetail;
 import com.avaje.tests.model.basic.ResetBasicData;
 
 public class TestOrderByWithMany extends BaseTestCase {
@@ -16,6 +20,7 @@ public class TestOrderByWithMany extends BaseTestCase {
 
     ResetBasicData.reset();
 
+    checkWithLazyLoadingOnBuiltInMany();
     checkWithBuiltInManyBasic();
     checkWithBuiltInMany();
     checkAppendId();
@@ -26,17 +31,42 @@ public class TestOrderByWithMany extends BaseTestCase {
     checkAlreadyIncluded2();
   }
 
+  private void checkWithLazyLoadingOnBuiltInMany() {
+
+    LoggedSqlCollector.start();
+
+    Query<Order> query = Ebean.find(Order.class);
+
+    // a query that ensures we are going to lazy load on the details
+    List<Order> orders = query.findList();
+
+    for (Order order : orders) {
+      // invoke lazy loading
+      List<OrderDetail> details = order.getDetails();
+      details.size();
+    }
+
+    // first one is the main query and others are lazy loading queries
+    List<String> loggedSql = LoggedSqlCollector.stop();
+    Assert.assertTrue(loggedSql.size() > 1);
+
+    String lazyLoadSql = loggedSql.get(1);
+    // contains the foreign key back to the parent bean (t0.order_id)
+    Assert.assertTrue(lazyLoadSql.contains("select t0.order_id c0, t0.id"));
+    Assert.assertTrue(lazyLoadSql.contains("order by t0.order_id, t0.id, t0.order_qty, t0.cretime desc"));
+
+  }
+
   private void checkWithBuiltInManyBasic() {
 
     Query<Order> query = Ebean.find(Order.class).fetch("details");
     query.findList();
 
     String sql = query.getGeneratedSql();
-    
+
     Assert.assertTrue(sql.contains("order by t0.id, t1.id asc, t1.order_qty asc, t1.cretime desc"));
   }
 
-  
   private void checkWithBuiltInMany() {
 
     Query<Order> query = Ebean.find(Order.class).fetch("details").order().desc("customer.name");

src/test/java/org/avaje/ebeantest/LoggedSqlCollector.java

@@ -0,0 +1,87 @@
+package org.avaje.ebeantest;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.slf4j.LoggerFactory;
+
+import ch.qos.logback.classic.Level;
+import ch.qos.logback.classic.Logger;
+import ch.qos.logback.classic.LoggerContext;
+import ch.qos.logback.classic.spi.ILoggingEvent;
+import ch.qos.logback.core.UnsynchronizedAppenderBase;
+
+/**
+ * Helper that can collect the SQL that is logged via SLF4J.
+ * <p>
+ * Used {@link #start()} and {@link #stop()} to collect the logged messages that contain the
+ * executed SQL statements.
+ * <p>
+ * Internally this uses a Logback Appender to collect messages for org.avaje.ebean.SQL.
+ */
+public class LoggedSqlCollector {
+
+  private static BasicAppender basicAppender = new BasicAppender();
+
+  static {
+
+    LoggerContext lc = (LoggerContext) LoggerFactory.getILoggerFactory();
+
+    basicAppender.setContext(lc);
+
+    Logger logger = (Logger) LoggerFactory.getLogger("org.avaje.ebean.SQL");
+    logger.addAppender(basicAppender);
+    logger.setLevel(Level.TRACE);
+    logger.setAdditive(true);
+  }
+
+  /**
+   * Start collection of the logged SQL statements.
+   */
+  public static List<String> start() {
+    return basicAppender.collectStart();
+  }
+
+  /**
+   * Stop collection of the logged SQL statements return the list of captured messages that contain
+   * the SQL.
+   */
+  public static List<String> stop() {
+    return basicAppender.collectEnd();
+  }
+
+  private static class BasicAppender extends UnsynchronizedAppenderBase<ILoggingEvent> {
+
+    List<String> messages = new ArrayList<String>();
+
+    @Override
+    protected void append(ILoggingEvent eventObject) {
+      if (started) {
+        messages.add(eventObject.getMessage());
+      }
+    }
+
+    /**
+     * Start collection.
+     */
+    List<String> collectStart() {
+      List<String> tempMessages = messages;
+      messages = new ArrayList<String>();
+      // set started flag
+      start();
+      return tempMessages;
+    }
+
+    /**
+     * End collection.
+     */
+    List<String> collectEnd() {
+      // set stopped state
+      stop();
+      List<String> tempMessages = messages;
+      messages = new ArrayList<String>();
+      return tempMessages;
+    }
+
+  }
+}