diff --git a/src/main/java/com/avaje/ebean/OrderBy.java b/src/main/java/com/avaje/ebean/OrderBy.java index 7ef6ff8ee..c39617551 100644 --- a/src/main/java/com/avaje/ebean/OrderBy.java +++ b/src/main/java/com/avaje/ebean/OrderBy.java @@ -28,7 +28,7 @@ public final class OrderBy implements Serializable { * Create an empty OrderBy with no associated query. */ public OrderBy() { - this.list = new ArrayList(2); + this.list = new ArrayList(3); } private OrderBy(List list) { @@ -52,7 +52,7 @@ public final class OrderBy implements Serializable { */ public OrderBy(Query query, String orderByClause) { this.query = query; - this.list = new ArrayList(2); + this.list = new ArrayList(3); parse(orderByClause); } @@ -83,6 +83,19 @@ public final class OrderBy implements Serializable { return query; } + /** + * Return true if the property is known to be contained in the order by clause. + */ + public boolean containsProperty(String propertyName) { + + for (int i = 0; i < list.size(); i++) { + if (propertyName.equals(list.get(i).getProperty())) { + return true; + } + } + return false; + } + /** * Return a copy of this OrderBy with the path trimmed. */ diff --git a/src/main/java/com/avaje/ebeaninternal/server/core/DefaultServer.java b/src/main/java/com/avaje/ebeaninternal/server/core/DefaultServer.java index 13afc23be..548a1e0ec 100644 --- a/src/main/java/com/avaje/ebeaninternal/server/core/DefaultServer.java +++ b/src/main/java/com/avaje/ebeaninternal/server/core/DefaultServer.java @@ -1363,14 +1363,7 @@ public final class DefaultServer implements SpiServer, SpiEbeanServer { @Override public PagedList findPagedList(Query query, Transaction transaction, int pageIndex, int pageSize) { - SpiQuery spiQuery = (SpiQuery)query; - OrderBy orderBy = spiQuery.getOrderBy(); - if (orderBy == null || orderBy.isEmpty()) { - // add a default order by for paging queries - BeanDescriptor desc = beanDescriptorManager.getBeanDescriptor(spiQuery.getBeanType()); - query.orderBy(desc.getDefaultOrderBy()); - } - return new LimitOffsetPagedList(this, spiQuery, pageIndex, pageSize); + return new LimitOffsetPagedList(this, (SpiQuery)query, pageIndex, pageSize); } public void findEach(Query query, QueryEachConsumer consumer, Transaction t) { diff --git a/src/main/java/com/avaje/ebeaninternal/server/deploy/BeanDescriptor.java b/src/main/java/com/avaje/ebeaninternal/server/deploy/BeanDescriptor.java index e2fa6399c..628bfcfa7 100644 --- a/src/main/java/com/avaje/ebeaninternal/server/deploy/BeanDescriptor.java +++ b/src/main/java/com/avaje/ebeaninternal/server/deploy/BeanDescriptor.java @@ -1,5 +1,6 @@ package com.avaje.ebeaninternal.server.deploy; +import com.avaje.ebean.OrderBy; import com.avaje.ebean.SqlUpdate; import com.avaje.ebean.Transaction; import com.avaje.ebean.ValuePair; @@ -2098,6 +2099,26 @@ public class BeanDescriptor implements MetaBeanInfo, SpiBeanType { } } + /** + * Appends the Id property to the OrderBy clause if it is not believed + * to be already contained in the order by. + *

+ * This is primarily used for paging queries to ensure that an order by clause is provided and that the order by + * provides unique ordering of the rows (so that the paging is predicable). + *

+ */ + public void appendOrderById(SpiQuery query) { + + if (idProperty != null) { + OrderBy orderBy = query.getOrderBy(); + if (orderBy == null || orderBy.isEmpty()) { + query.order().asc(idProperty.getName()); + } else if (!orderBy.containsProperty(idProperty.getName())){ + query.order().asc(idProperty.getName()); + } + } + } + /** * All the BeanPropertyAssocOne that are not embedded. These are effectively * joined beans. For ManyToOne and OneToOne associations. diff --git a/src/main/java/com/avaje/ebeaninternal/server/query/CQueryEngine.java b/src/main/java/com/avaje/ebeaninternal/server/query/CQueryEngine.java index 5a309180b..8b832230b 100644 --- a/src/main/java/com/avaje/ebeaninternal/server/query/CQueryEngine.java +++ b/src/main/java/com/avaje/ebeaninternal/server/query/CQueryEngine.java @@ -277,6 +277,14 @@ public class CQueryEngine { */ public BeanCollection findMany(OrmQueryRequest request) { + + SpiQuery query = request.getQuery(); + if (query.getMaxRows() > 1 || query.getFirstRow() > 0) { + // deemed to be a be a paging query - check that the order by contains + // the id property to ensure unique row ordering for predicable paging + request.getBeanDescriptor().appendOrderById(query); + } + CQuery cquery = queryBuilder.buildQuery(request); request.setCancelableQuery(cquery); @@ -293,7 +301,7 @@ public class CQueryEngine { BeanCollection beanCollection = cquery.readCollection(); - BeanCollectionTouched collectionTouched = request.getQuery().getBeanCollectionTouched(); + BeanCollectionTouched collectionTouched = query.getBeanCollectionTouched(); if (collectionTouched != null) { // register a listener that wants to be notified when the // bean collection is first used @@ -319,7 +327,7 @@ public class CQueryEngine { if (cquery != null) { cquery.close(); } - if (request.getQuery().isFutureFetch()) { + if (query.isFutureFetch()) { // end the transaction for futureFindIds // as it had it's own transaction logger.debug("Future fetch completed!"); diff --git a/src/test/java/com/avaje/tests/query/TestAddOrderByWithFirstRowsMaxRows.java b/src/test/java/com/avaje/tests/query/TestAddOrderByWithFirstRowsMaxRows.java index dc8ea1d9c..f22855c32 100644 --- a/src/test/java/com/avaje/tests/query/TestAddOrderByWithFirstRowsMaxRows.java +++ b/src/test/java/com/avaje/tests/query/TestAddOrderByWithFirstRowsMaxRows.java @@ -50,6 +50,7 @@ public class TestAddOrderByWithFirstRowsMaxRows extends BaseTestCase { List loggedSql = LoggedSqlCollector.stop(); assertThat(loggedSql).hasSize(1); + assertThat(loggedSql.get(0)).contains("order by t0.id"); } @@ -72,6 +73,25 @@ public class TestAddOrderByWithFirstRowsMaxRows extends BaseTestCase { assertThat(loggedSql.get(0)).contains("order by t0.id"); } + @Test + public void test_maxRows1() { + + ResetBasicData.reset(); + + LoggedSqlCollector.start(); + + // maxRows 1 with no first rows means Ebean does not automatically + // add the order by id to the query + Ebean.find(Order.class) + .setMaxRows(1) + .findList(); + + List loggedSql = LoggedSqlCollector.stop(); + + assertThat(loggedSql).hasSize(1); + assertThat(loggedSql.get(0)).doesNotContain("order by t0.id"); + } + @Test public void test_pagingOne() { @@ -107,4 +127,42 @@ public class TestAddOrderByWithFirstRowsMaxRows extends BaseTestCase { assertThat(loggedSql).hasSize(1); assertThat(loggedSql.get(0)).contains("order by t0.id"); } + + + @Test + public void test_pagingAppendToExistingOrderBy() { + + ResetBasicData.reset(); + + LoggedSqlCollector.start(); + + Ebean.find(Order.class) + .order().asc("orderDate") + .findPagedList(0, 10) + .getList(); + + List loggedSql = LoggedSqlCollector.stop(); + + assertThat(loggedSql).hasSize(1); + assertThat(loggedSql.get(0)).contains("order by t0.order_date, t0.id"); + } + + @Test + public void test_pagingExistingOrderByWithId() { + + ResetBasicData.reset(); + + LoggedSqlCollector.start(); + + Ebean.find(Order.class) + .order().asc("orderDate") + .order().desc("id") + .findPagedList(0, 10) + .getList(); + + List loggedSql = LoggedSqlCollector.stop(); + + assertThat(loggedSql).hasSize(1); + assertThat(loggedSql.get(0)).contains("order by t0.order_date, t0.id desc"); + } } diff --git a/src/test/java/com/avaje/tests/unitinternal/TestOrderByParse.java b/src/test/java/com/avaje/tests/unitinternal/TestOrderByParse.java index a7337c114..f87015977 100644 --- a/src/test/java/com/avaje/tests/unitinternal/TestOrderByParse.java +++ b/src/test/java/com/avaje/tests/unitinternal/TestOrderByParse.java @@ -1,10 +1,12 @@ package com.avaje.tests.unitinternal; -import org.junit.Assert; -import org.junit.Test; - import com.avaje.ebean.BaseTestCase; import com.avaje.ebean.OrderBy; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; /** * Test the OrderBy object and especially its parsing. @@ -15,105 +17,109 @@ public class TestOrderByParse extends BaseTestCase { public void testParsingOne() { OrderBy o1 = new OrderBy("id"); - Assert.assertTrue(o1.getProperties().size() == 1); - Assert.assertTrue(o1.getProperties().get(0).getProperty().equals("id")); - Assert.assertTrue(o1.getProperties().get(0).isAscending()); - Assert.assertTrue(o1.toStringFormat().equals("id")); + assertTrue(o1.getProperties().size() == 1); + assertTrue(o1.getProperties().get(0).getProperty().equals("id")); + assertTrue(o1.getProperties().get(0).isAscending()); + assertTrue(o1.toStringFormat().equals("id")); o1 = new OrderBy("id asc"); - Assert.assertTrue(o1.getProperties().size() == 1); - Assert.assertTrue(o1.getProperties().get(0).getProperty().equals("id")); - Assert.assertTrue(o1.getProperties().get(0).isAscending()); - Assert.assertTrue(o1.toStringFormat().equals("id")); + assertTrue(o1.getProperties().size() == 1); + assertTrue(o1.getProperties().get(0).getProperty().equals("id")); + assertTrue(o1.getProperties().get(0).isAscending()); + assertTrue(o1.toStringFormat().equals("id")); o1 = new OrderBy("id desc"); - Assert.assertTrue(o1.getProperties().size() == 1); - Assert.assertTrue(o1.getProperties().get(0).getProperty().equals("id")); - Assert.assertTrue(!o1.getProperties().get(0).isAscending()); - Assert.assertTrue(o1.toStringFormat().equals("id desc")); + assertTrue(o1.getProperties().size() == 1); + assertTrue(o1.getProperties().get(0).getProperty().equals("id")); + assertTrue(!o1.getProperties().get(0).isAscending()); + assertTrue(o1.toStringFormat().equals("id desc")); o1 = new OrderBy(" id asc "); - Assert.assertTrue(o1.getProperties().size() == 1); - Assert.assertTrue(o1.getProperties().get(0).getProperty().equals("id")); - Assert.assertTrue(o1.getProperties().get(0).isAscending()); - Assert.assertTrue(o1.toStringFormat().equals("id")); + assertTrue(o1.getProperties().size() == 1); + assertTrue(o1.getProperties().get(0).getProperty().equals("id")); + assertTrue(o1.getProperties().get(0).isAscending()); + assertTrue(o1.toStringFormat().equals("id")); + assertTrue(o1.containsProperty("id")); + assertFalse(o1.containsProperty("junk")); } + @Test public void testParsingTwo() { OrderBy o1 = new OrderBy("id,name"); - Assert.assertTrue(o1.getProperties().size() == 2); - Assert.assertTrue(o1.getProperties().get(0).getProperty().equals("id")); - Assert.assertTrue(o1.getProperties().get(0).isAscending()); - Assert.assertTrue(o1.getProperties().get(1).getProperty().equals("name")); - Assert.assertTrue(o1.getProperties().get(1).isAscending()); - Assert.assertEquals("id, name", o1.toStringFormat()); + assertTrue(o1.getProperties().size() == 2); + assertTrue(o1.getProperties().get(0).getProperty().equals("id")); + assertTrue(o1.getProperties().get(0).isAscending()); + assertTrue(o1.getProperties().get(1).getProperty().equals("name")); + assertTrue(o1.getProperties().get(1).isAscending()); + assertEquals("id, name", o1.toStringFormat()); o1 = new OrderBy(" id , name "); - Assert.assertTrue(o1.getProperties().size() == 2); - Assert.assertTrue(o1.getProperties().get(0).getProperty().equals("id")); - Assert.assertTrue(o1.getProperties().get(0).isAscending()); - Assert.assertTrue(o1.getProperties().get(1).getProperty().equals("name")); - Assert.assertTrue(o1.getProperties().get(1).isAscending()); - Assert.assertEquals("id, name", o1.toStringFormat()); + assertTrue(o1.getProperties().size() == 2); + assertTrue(o1.getProperties().get(0).getProperty().equals("id")); + assertTrue(o1.getProperties().get(0).isAscending()); + assertTrue(o1.getProperties().get(1).getProperty().equals("name")); + assertTrue(o1.getProperties().get(1).isAscending()); + assertEquals("id, name", o1.toStringFormat()); o1 = new OrderBy(" id desc , name desc "); - Assert.assertTrue(o1.getProperties().size() == 2); - Assert.assertTrue(o1.getProperties().get(0).getProperty().equals("id")); - Assert.assertTrue(!o1.getProperties().get(0).isAscending()); - Assert.assertTrue(o1.getProperties().get(1).getProperty().equals("name")); - Assert.assertTrue(!o1.getProperties().get(1).isAscending()); - Assert.assertEquals("id desc, name desc", o1.toStringFormat()); + assertTrue(o1.getProperties().size() == 2); + assertTrue(o1.getProperties().get(0).getProperty().equals("id")); + assertTrue(!o1.getProperties().get(0).isAscending()); + assertTrue(o1.getProperties().get(1).getProperty().equals("name")); + assertTrue(!o1.getProperties().get(1).isAscending()); + assertEquals("id desc, name desc", o1.toStringFormat()); o1 = new OrderBy(" id ascending, name asc"); - Assert.assertTrue(o1.getProperties().size() == 2); - Assert.assertTrue(o1.getProperties().get(0).getProperty().equals("id")); - Assert.assertTrue(o1.getProperties().get(0).isAscending()); - Assert.assertTrue(o1.getProperties().get(1).getProperty().equals("name")); - Assert.assertTrue(o1.getProperties().get(1).isAscending()); - Assert.assertEquals("id, name", o1.toStringFormat()); + assertTrue(o1.getProperties().size() == 2); + assertTrue(o1.getProperties().get(0).getProperty().equals("id")); + assertTrue(o1.getProperties().get(0).isAscending()); + assertTrue(o1.getProperties().get(1).getProperty().equals("name")); + assertTrue(o1.getProperties().get(1).isAscending()); + assertEquals("id, name", o1.toStringFormat()); } + @Test public void testAddMethods() { OrderBy o1 = new OrderBy(); o1.asc("id"); o1.asc("name"); - Assert.assertTrue(o1.getProperties().size() == 2); - Assert.assertTrue(o1.getProperties().get(0).getProperty().equals("id")); - Assert.assertTrue(o1.getProperties().get(0).isAscending()); - Assert.assertTrue(o1.getProperties().get(1).getProperty().equals("name")); - Assert.assertTrue(o1.getProperties().get(1).isAscending()); - Assert.assertEquals("id, name", o1.toStringFormat()); + assertTrue(o1.getProperties().size() == 2); + assertTrue(o1.getProperties().get(0).getProperty().equals("id")); + assertTrue(o1.getProperties().get(0).isAscending()); + assertTrue(o1.getProperties().get(1).getProperty().equals("name")); + assertTrue(o1.getProperties().get(1).isAscending()); + assertEquals("id, name", o1.toStringFormat()); o1 = new OrderBy(); o1.desc("id"); o1.desc("name"); - Assert.assertTrue(o1.getProperties().size() == 2); - Assert.assertTrue(o1.getProperties().get(0).getProperty().equals("id")); - Assert.assertTrue(!o1.getProperties().get(0).isAscending()); - Assert.assertTrue(o1.getProperties().get(1).getProperty().equals("name")); - Assert.assertTrue(!o1.getProperties().get(1).isAscending()); - Assert.assertEquals("id desc, name desc", o1.toStringFormat()); + assertTrue(o1.getProperties().size() == 2); + assertTrue(o1.getProperties().get(0).getProperty().equals("id")); + assertTrue(!o1.getProperties().get(0).isAscending()); + assertTrue(o1.getProperties().get(1).getProperty().equals("name")); + assertTrue(!o1.getProperties().get(1).isAscending()); + assertEquals("id desc, name desc", o1.toStringFormat()); o1.reverse(); - Assert.assertTrue(o1.getProperties().size() == 2); - Assert.assertTrue(o1.getProperties().get(0).getProperty().equals("id")); - Assert.assertTrue(o1.getProperties().get(0).isAscending()); - Assert.assertTrue(o1.getProperties().get(1).getProperty().equals("name")); - Assert.assertTrue(o1.getProperties().get(1).isAscending()); - Assert.assertEquals("id, name", o1.toStringFormat()); + assertTrue(o1.getProperties().size() == 2); + assertTrue(o1.getProperties().get(0).getProperty().equals("id")); + assertTrue(o1.getProperties().get(0).isAscending()); + assertTrue(o1.getProperties().get(1).getProperty().equals("name")); + assertTrue(o1.getProperties().get(1).isAscending()); + assertEquals("id, name", o1.toStringFormat()); OrderBy copy = o1.copy(); - Assert.assertTrue(copy != o1); - Assert.assertTrue(copy.getProperties().size() == 2); - Assert.assertTrue(copy.getProperties().get(0).getProperty().equals("id")); - Assert.assertTrue(copy.getProperties().get(0).isAscending()); - Assert.assertTrue(copy.getProperties().get(1).getProperty().equals("name")); - Assert.assertTrue(copy.getProperties().get(1).isAscending()); - Assert.assertEquals("id, name", copy.toStringFormat()); + assertTrue(copy != o1); + assertTrue(copy.getProperties().size() == 2); + assertTrue(copy.getProperties().get(0).getProperty().equals("id")); + assertTrue(copy.getProperties().get(0).isAscending()); + assertTrue(copy.getProperties().get(1).getProperty().equals("name")); + assertTrue(copy.getProperties().get(1).isAscending()); + assertEquals("id, name", copy.toStringFormat()); } }