From 1109d412e7809c3fc1ac6a6daf59535554d7df80 Mon Sep 17 00:00:00 2001 From: Rob Bygrave Date: Thu, 26 Jun 2014 00:37:31 +1200 Subject: [PATCH] Fix for #154 - @OrderBy on a @OneToMany property is not used when lazy loading --- .../server/core/DefaultBeanLoader.java | 5 +- .../server/deploy/BeanPropertyAssocMany.java | 31 +++++++ .../basic/TestSharedInstancePropagation.java | 5 +- .../query/orderby/TestOrderByWithMany.java | 34 +++++++- .../avaje/ebeantest/LoggedSqlCollector.java | 87 +++++++++++++++++++ 5 files changed, 157 insertions(+), 5 deletions(-) create mode 100644 src/test/java/org/avaje/ebeantest/LoggedSqlCollector.java diff --git a/src/main/java/com/avaje/ebeaninternal/server/core/DefaultBeanLoader.java b/src/main/java/com/avaje/ebeaninternal/server/core/DefaultBeanLoader.java index 362c91aec..4c9d234c3 100644 --- a/src/main/java/com/avaje/ebeaninternal/server/core/DefaultBeanLoader.java +++ b/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); diff --git a/src/main/java/com/avaje/ebeaninternal/server/deploy/BeanPropertyAssocMany.java b/src/main/java/com/avaje/ebeaninternal/server/deploy/BeanPropertyAssocMany.java index ca3844be4..156b3a1b2 100644 --- a/src/main/java/com/avaje/ebeaninternal/server/deploy/BeanPropertyAssocMany.java +++ b/src/main/java/com/avaje/ebeaninternal/server/deploy/BeanPropertyAssocMany.java @@ -60,8 +60,16 @@ public class BeanPropertyAssocMany extends BeanPropertyAssoc { */ 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 extends BeanPropertyAssoc { 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 extends BeanPropertyAssoc { } /** + * 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() { diff --git a/src/test/java/com/avaje/tests/basic/TestSharedInstancePropagation.java b/src/test/java/com/avaje/tests/basic/TestSharedInstancePropagation.java index 811795a9b..bae674110 100644 --- a/src/test/java/com/avaje/tests/basic/TestSharedInstancePropagation.java +++ b/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) diff --git a/src/test/java/com/avaje/tests/query/orderby/TestOrderByWithMany.java b/src/test/java/com/avaje/tests/query/orderby/TestOrderByWithMany.java index 2a62e1b35..51df03423 100644 --- a/src/test/java/com/avaje/tests/query/orderby/TestOrderByWithMany.java +++ b/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 query = Ebean.find(Order.class); + + // a query that ensures we are going to lazy load on the details + List orders = query.findList(); + + for (Order order : orders) { + // invoke lazy loading + List details = order.getDetails(); + details.size(); + } + + // first one is the main query and others are lazy loading queries + List 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 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 query = Ebean.find(Order.class).fetch("details").order().desc("customer.name"); diff --git a/src/test/java/org/avaje/ebeantest/LoggedSqlCollector.java b/src/test/java/org/avaje/ebeantest/LoggedSqlCollector.java new file mode 100644 index 000000000..f3d1f0ca4 --- /dev/null +++ b/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. + *

+ * Used {@link #start()} and {@link #stop()} to collect the logged messages that contain the + * executed SQL statements. + *

+ * 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 start() { + return basicAppender.collectStart(); + } + + /** + * Stop collection of the logged SQL statements return the list of captured messages that contain + * the SQL. + */ + public static List stop() { + return basicAppender.collectEnd(); + } + + private static class BasicAppender extends UnsynchronizedAppenderBase { + + List messages = new ArrayList(); + + @Override + protected void append(ILoggingEvent eventObject) { + if (started) { + messages.add(eventObject.getMessage()); + } + } + + /** + * Start collection. + */ + List collectStart() { + List tempMessages = messages; + messages = new ArrayList(); + // set started flag + start(); + return tempMessages; + } + + /** + * End collection. + */ + List collectEnd() { + // set stopped state + stop(); + List tempMessages = messages; + messages = new ArrayList(); + return tempMessages; + } + + } +}