#410 - Require findPagedList() query to specify an order by clause - throw exception if no order by supplied

src/main/java/com/avaje/ebean/OrderBy.java

@@ -28,7 +28,7 @@ public final class OrderBy<T> implements Serializable {
    * Create an empty OrderBy with no associated query.
    */
   public OrderBy() {
-    this.list = new ArrayList<Property>(2);
+    this.list = new ArrayList<Property>(3);
   }
   
   private OrderBy(List<Property> list) {
@@ -52,7 +52,7 @@ public final class OrderBy<T> implements Serializable {
    */
   public OrderBy(Query<T> query, String orderByClause) {
     this.query = query;
-    this.list = new ArrayList<Property>(2);
+    this.list = new ArrayList<Property>(3);
     parse(orderByClause);
   }
 
@@ -83,6 +83,19 @@ public final class OrderBy<T> 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.
    */

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

@@ -1363,14 +1363,7 @@ public final class DefaultServer implements SpiServer, SpiEbeanServer {
   @Override
   public <T> PagedList<T> findPagedList(Query<T> query, Transaction transaction, int pageIndex, int pageSize) {
 
-    SpiQuery spiQuery = (SpiQuery<T>)query;
-    OrderBy orderBy = spiQuery.getOrderBy();
-    if (orderBy == null || orderBy.isEmpty()) {
-      // add a default order by for paging queries
-      BeanDescriptor<T> desc = beanDescriptorManager.getBeanDescriptor(spiQuery.getBeanType());
-      query.orderBy(desc.getDefaultOrderBy());
-    }
-    return new LimitOffsetPagedList<T>(this, spiQuery, pageIndex, pageSize);
+    return new LimitOffsetPagedList<T>(this, (SpiQuery<T>)query, pageIndex, pageSize);
   }
 
   public <T> void findEach(Query<T> query, QueryEachConsumer<T> consumer, Transaction t) {

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<T> implements MetaBeanInfo, SpiBeanType<T> {
     }
   }
 
+  /**
+   * Appends the Id property to the OrderBy clause if it is not believed
+   * to be already contained in the order by.
+   * <p>
+   * 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).
+   * </p>
+   */
+  public void appendOrderById(SpiQuery<T> query) {
+
+    if (idProperty != null) {
+      OrderBy<T> 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.

src/main/java/com/avaje/ebeaninternal/server/query/CQueryEngine.java

@@ -277,6 +277,14 @@ public class CQueryEngine {
    */
   public <T> BeanCollection<T> findMany(OrmQueryRequest<T> request) {
 
+
+    SpiQuery<T> 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<T> cquery = queryBuilder.buildQuery(request);
     request.setCancelableQuery(cquery);
 
@@ -293,7 +301,7 @@ public class CQueryEngine {
 
       BeanCollection<T> 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!");

src/test/java/com/avaje/tests/query/TestAddOrderByWithFirstRowsMaxRows.java

@@ -50,6 +50,7 @@ public class TestAddOrderByWithFirstRowsMaxRows extends BaseTestCase {
     List<String> 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<String> 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<String> 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<String> loggedSql = LoggedSqlCollector.stop();
+
+    assertThat(loggedSql).hasSize(1);
+    assertThat(loggedSql.get(0)).contains("order by t0.order_date, t0.id desc");
+  }
 }

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<Object> o1 = new OrderBy<Object>("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<Object>("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<Object>("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<Object>(" 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<Object>("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<Object>("  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<Object>("  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<Object>("  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<Object>();
     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<Object>();
     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());
 
   }
 }