auomatically append "order by id" when paging and query.orderById = true (#1275)

Add ServerConfig.setDefaultOrderById() and query.orderById(boolean) ... to control automatically append "order by id" with paging query

src/main/java/io/ebean/Query.java

@@ -1507,4 +1507,12 @@ public interface Query<T> {
    */
   Set<String> validate();
 
+  /**
+   * Controls, if paginated queries should always append an 'order by id' statement at the end to
+   * guarantee a deterministic sort result. This may affect performance.
+   * If this is not enabled, and an orderBy is set on the query, it's up to the programmer that
+   * this query provides a deterministic result.
+   */
+  Query<T> orderById(boolean orderById);
+
 }

src/main/java/io/ebean/config/ServerConfig.java

@@ -488,6 +488,11 @@ public class ServerConfig {
 
   private ProfilingConfig profilingConfig = new ProfilingConfig();
 
+  /**
+   * Controls the default order by id setting of queries. See {@link Query#orderById(boolean)}
+   */
+  private boolean defaultOrderById = false;
+
   /**
    * Construct a Server Configuration for programmatically creating an EbeanServer.
    */
@@ -523,6 +528,21 @@ public class ServerConfig {
     this.slowQueryListener = slowQueryListener;
   }
 
+
+  /**
+   * Sets the default orderById setting for queries.
+   */
+  public void setDefaultOrderById(boolean defaultOrderById) {
+    this.defaultOrderById = defaultOrderById;
+  }
+
+  /**
+   * Returns the default orderById setting for queries.
+   */
+  public boolean isDefaultOrderById() {
+    return defaultOrderById;
+  }
+
   /**
    * Put a service object into configuration such that it can be passed to a plugin.
    * <p>
@@ -2767,6 +2787,7 @@ public class ServerConfig {
     databaseBooleanTrue = p.get("databaseBooleanTrue", databaseBooleanTrue);
     databaseBooleanFalse = p.get("databaseBooleanFalse", databaseBooleanFalse);
     databasePlatformName = p.get("databasePlatformName", databasePlatformName);
+    defaultOrderById = p.getBoolean("defaultOrderById", defaultOrderById);
 
     DbUuid dbUuid = p.getEnum(DbUuid.class, "dbuuid", null);
     if (dbUuid != null) {

src/main/java/io/ebeaninternal/api/SpiQuery.java

@@ -1,7 +1,6 @@
 package io.ebeaninternal.api;
 
 import io.ebean.CacheMode;
-import io.ebean.EbeanServer;
 import io.ebean.ExpressionList;
 import io.ebean.OrderBy;
 import io.ebean.PersistenceContextScope;
@@ -338,7 +337,7 @@ public interface SpiQuery<T> extends Query<T>, TxnProfileEventCodes {
   /**
    * Return a copy of the query attaching to a different EbeanServer.
    */
-  SpiQuery<T> copy(EbeanServer server);
+  SpiQuery<T> copy(SpiEbeanServer server);
 
   /**
    * Return the type of query (List, Set, Map, Bean, rowCount etc).
@@ -593,6 +592,11 @@ public interface SpiQuery<T> extends Query<T>, TxnProfileEventCodes {
    */
   boolean checkPagingOrderBy();
 
+  /**
+   * Return true if there is no Order By clause.
+   */
+  boolean orderByIsEmpty();
+
   /**
    * Return the Order By clause or null if there is none defined.
    */

src/main/java/io/ebeaninternal/server/deploy/BeanDescriptor.java

@@ -3053,13 +3053,8 @@ public class BeanDescriptor<T> implements BeanType<T> {
    */
   public void appendOrderById(SpiQuery<T> query) {
 
-    if (idProperty != null && !idProperty.isEmbedded()) {
-      SpiRawSql rawSql = query.getRawSql();
-      if (rawSql != null) {
-        query.order(rawSql.getSql().getOrderBy());
-      } else {
-        query.order().asc(idProperty.getName());
-      }
+    if (idProperty != null && !idProperty.isEmbedded() && !query.order().containsProperty(idProperty.getName())) {
+      query.order().asc(idProperty.getName());
     }
   }
 

src/main/java/io/ebeaninternal/server/query/CQueryEngine.java

@@ -190,7 +190,6 @@ public class CQueryEngine {
    */
   public <T> QueryIterator<T> findIterate(OrmQueryRequest<T> request) {
 
-    prepareForPaging(request);
     CQuery<T> cquery = queryBuilder.buildQuery(request);
     request.setCancelableQuery(cquery);
 
@@ -326,24 +325,12 @@ public class CQueryEngine {
     return historySupport.getSysPeriodLower(rootTableAlias);
   }
 
-  /**
-   * 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 but only in
-   * case, this is not a distinct query
-   */
-  private <T> void prepareForPaging(OrmQueryRequest<T> request) {
-    SpiQuery<T> query = request.getQuery();
-    if (query.checkPagingOrderBy()) {
-      request.getBeanDescriptor().appendOrderById(query);
-    }
-  }
+
   /**
    * Find a list/map/set of beans.
    */
   <T> BeanCollection<T> findMany(OrmQueryRequest<T> request) {
 
-    prepareForPaging(request);
-
     CQuery<T> cquery = queryBuilder.buildQuery(request);
     request.setCancelableQuery(cquery);
 

src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java

@@ -1,7 +1,6 @@
 package io.ebeaninternal.server.querydefn;
 
 import io.ebean.CacheMode;
-import io.ebean.EbeanServer;
 import io.ebean.Expression;
 import io.ebean.ExpressionFactory;
 import io.ebean.ExpressionList;
@@ -32,6 +31,7 @@ import io.ebeaninternal.api.CQueryPlanKey;
 import io.ebeaninternal.api.HashQuery;
 import io.ebeaninternal.api.ManyWhereJoins;
 import io.ebeaninternal.api.NaturalKeyQueryData;
+import io.ebeaninternal.api.SpiEbeanServer;
 import io.ebeaninternal.api.SpiExpression;
 import io.ebeaninternal.api.SpiExpressionList;
 import io.ebeaninternal.api.SpiExpressionValidation;
@@ -39,6 +39,7 @@ import io.ebeaninternal.api.SpiNamedParam;
 import io.ebeaninternal.api.SpiQuery;
 import io.ebeaninternal.api.SpiQuerySecondary;
 import io.ebeaninternal.server.autotune.ProfilingListener;
+import io.ebeaninternal.server.core.OrmQueryRequest;
 import io.ebeaninternal.server.deploy.BeanDescriptor;
 import io.ebeaninternal.server.deploy.BeanPropertyAssocMany;
 import io.ebeaninternal.server.deploy.TableJoin;
@@ -75,7 +76,7 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
 
   private final BeanDescriptor<T> beanDescriptor;
 
-  private final EbeanServer server;
+  private final SpiEbeanServer server;
 
   private final ExpressionFactory expressionFactory;
 
@@ -254,6 +255,8 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
 
   private String nativeSql;
 
+  private boolean orderById;
+
   /**
    * Identity the query for profiling purposes (expected to be unique for a bean type).
    */
@@ -261,10 +264,11 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
 
   private ProfileLocation profileLocation;
 
-  public DefaultOrmQuery(BeanDescriptor<T> desc, EbeanServer server, ExpressionFactory expressionFactory) {
+  public DefaultOrmQuery(BeanDescriptor<T> desc, SpiEbeanServer server, ExpressionFactory expressionFactory) {
     this.beanDescriptor = desc;
     this.beanType = desc.getBeanType();
     this.server = server;
+    this.orderById = server.getServerConfig().isDefaultOrderById();
     this.expressionFactory = expressionFactory;
     this.detail = new OrmQueryDetail();
   }
@@ -279,6 +283,7 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
   }
 
 
+  @Override
   public boolean isFindAll() {
     return whereExpressions == null && nativeSql == null && rawSql == null;
   }
@@ -304,6 +309,7 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
     }
   }
 
+  @Override
   public short getProfileId() {
     return profileId;
   }
@@ -717,8 +723,7 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
   }
 
   @Override
-  public DefaultOrmQuery<T> copy(EbeanServer server) {
-
+  public DefaultOrmQuery<T> copy(SpiEbeanServer server) {
     DefaultOrmQuery<T> copy = new DefaultOrmQuery<>(beanDescriptor, server, expressionFactory);
     copy.m2mIncludeJoin = m2mIncludeJoin;
     copy.profilingListener = profilingListener;
@@ -742,6 +747,7 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
     if (orderBy != null) {
       copy.orderBy = orderBy.copy();
     }
+    copy.orderById = orderById;
     if (bindParams != null) {
       copy.bindParams = bindParams.copy();
     }
@@ -1037,6 +1043,7 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
   public CQueryPlanKey prepare(BeanQueryRequest<?> request) {
 
     prepareExpressions(request);
+    prepareForPaging((OrmQueryRequest) request);
     queryPlanKey = createQueryPlanKey();
     return queryPlanKey;
   }
@@ -1054,6 +1061,24 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
     }
   }
 
+  /**
+   * 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 but only in
+   * case, this is not a distinct query
+   */
+  private void prepareForPaging(OrmQueryRequest<T> request) {
+
+    // add the rawSql statement - if any
+    if (orderByIsEmpty()) {
+      SpiRawSql rawSql = getRawSql();
+      if (rawSql != null && rawSql.getSql() != null) {
+        order(rawSql.getSql().getOrderBy());
+      }
+    }
+    if (checkPagingOrderBy()) {
+      request.getBeanDescriptor().appendOrderById(this);
+    }
+  }
   /**
    * Calculate a hash based on the bind values used in the query.
    * <p>
@@ -1388,10 +1413,11 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
 
   @Override
   public boolean checkPagingOrderBy() {
-    return (maxRows > 1 || firstRow > 0) && !distinct && orderByIsEmpty();
+    return (maxRows > 1 || firstRow > 0) && !distinct && (orderByIsEmpty() || isOrderById());
   }
 
-  private boolean orderByIsEmpty() {
+  @Override
+  public boolean orderByIsEmpty() {
     return orderBy == null || orderBy.isEmpty();
   }
 
@@ -1776,4 +1802,14 @@ public class DefaultOrmQuery<T> implements SpiQuery<T> {
   public ProfileLocation getProfileLocation() {
     return profileLocation;
   }
+
+  @Override
+  public Query<T> orderById(boolean orderById) {
+    this.orderById = orderById;
+    return this;
+  }
+
+  public boolean isOrderById() {
+    return orderById;
+  }
 }

src/test/java/io/ebean/EbeanServer_eqlTest.java

@@ -1,5 +1,6 @@
 package io.ebean;
 
+import org.ebeantest.LoggedSqlCollector;
 import org.junit.Test;
 import org.tests.model.basic.Customer;
 import org.tests.model.basic.ResetBasicData;
@@ -8,6 +9,8 @@ import javax.persistence.PersistenceException;
 
 import static org.assertj.core.api.Assertions.assertThat;
 
+import java.util.List;
+
 public class EbeanServer_eqlTest extends BaseTestCase {
 
 
@@ -85,6 +88,40 @@ public class EbeanServer_eqlTest extends BaseTestCase {
     } else {
       assertThat(query.getGeneratedSql()).endsWith("order by t0.name limit 10 offset 3");
     }
+
+    // check also select count(*)
+    LoggedSqlCollector.start();
+    query.findCount();
+    List<String>sql = LoggedSqlCollector.stop();
+    assertThat(sql.get(0)).startsWith("select count(*) from o_customer t0;");
+  }
+
+  @Test
+  public void basic_limit_offset2_with_id() {
+
+    ResetBasicData.reset();
+
+    Query<Customer> query = Ebean.createQuery(Customer.class, "order by name");
+    query.setMaxRows(10);
+    query.setFirstRow(3);
+    query.orderById(true);
+    query.findList();
+
+    if (isSqlServer()) {
+      assertThat(query.getGeneratedSql()).endsWith("order by t0.name, t0.id offset 3 rows fetch next 10 rows only");
+    } else if (isOracle()) {
+      assertThat(query.getGeneratedSql()).contains("where rownum <= 13");
+      assertThat(query.getGeneratedSql()).contains("where rn_ > 3");
+    } else {
+      assertThat(query.getGeneratedSql()).endsWith("order by t0.name, t0.id limit 10 offset 3");
+    }
+
+    // check also select count(*)
+    LoggedSqlCollector.start();
+    query.findCount();
+    List<String>sql = LoggedSqlCollector.stop();
+    assertThat(sql.get(0)).startsWith("select count(*) from o_customer t0;");
+
   }
 
   @Test

src/test/java/org/tests/query/orderby/TestOrderByWithDistinct.java

@@ -37,6 +37,32 @@ public class TestOrderByWithDistinct extends BaseTestCase {
 
   }
 
+
+  @Test
+  public void testOrderByWithDistinct() {
+    Query<MUser> query = Ebean.find(MUser.class);
+    query.findList();
+
+    assertThat(query.getGeneratedSql()).doesNotContain("order by");
+    assertThat(query.getGeneratedSql()).doesNotContain("select distinct");
+
+    query.setMaxRows(1000);
+    query.findList();
+    assertThat(query.getGeneratedSql()).contains("order by t0.userid");
+
+    query = Ebean.find(MUser.class)
+        .where()
+        .eq("roles.roleName", "A")
+        .query();
+    query.findList();
+    assertThat(query.getGeneratedSql()).doesNotContain("order by");
+    assertThat(query.getGeneratedSql()).contains("select distinct");
+
+    query.setMaxRows(1000);
+    query.findList();
+    assertThat(query.getGeneratedSql()).contains("order by t0.userid");
+  }
+
   @Test
   public void test() {
     /*

src/test/java/org/tests/query/other/TestQuerySingleAttribute.java

@@ -244,11 +244,11 @@ public class TestQuerySingleAttribute extends BaseTestCase {
 
     List<String> ids = query.findSingleAttributeList();
     if (isSqlServer()) {
-      assertThat(sqlOf(query)).contains("select top 100 t0.id from o_customer t0");
+      assertThat(sqlOf(query)).contains("select top 100 t0.id from o_customer t0 order by t0.id");
     } else if (isOracle()) {
       assertThat(sqlOf(query)).contains("where rownum <= 100");
     } else {
-      assertThat(sqlOf(query)).contains("select t0.id from o_customer t0 limit 100");
+      assertThat(sqlOf(query)).contains("select t0.id from o_customer t0 order by t0.id limit 100");
     }
     assertThat(ids).isNotEmpty();
   }

src/test/java/org/tests/rawsql/TestRawSqlOrmQuery.java

@@ -10,6 +10,8 @@ import io.ebean.RawSql;
 import io.ebean.RawSqlBuilder;
 import io.ebean.annotation.IgnorePlatform;
 import io.ebean.annotation.Platform;
+
+import org.ebeantest.LoggedSqlCollector;
 import org.junit.Assert;
 import org.junit.Test;
 import org.tests.model.basic.Customer;
@@ -67,6 +69,13 @@ public class TestRawSqlOrmQuery extends BaseTestCase {
 
     List<Customer> list = query.findList();
     Assert.assertNotNull(list);
+
+    // check also select count(*)
+    LoggedSqlCollector.start();
+    assertThat(query.findCount()).isEqualTo(list.size());
+    List<String>sql = LoggedSqlCollector.stop();
+    assertThat(sql.get(0)).startsWith("select count(*) from ( select r.id, r.name from o_customer r");
+    assertThat(sql.get(0)).doesNotContain("order by");
   }
 
   @Test
@@ -104,7 +113,6 @@ public class TestRawSqlOrmQuery extends BaseTestCase {
 
     query.setFirstRow(1);
     query.setMaxRows(2);
-    query.order().asc("id");
 
     List<Customer> list = query.findList();
 
@@ -170,12 +178,54 @@ public class TestRawSqlOrmQuery extends BaseTestCase {
 
     if (isSqlServer()) {
       assertThat(query.getGeneratedSql()).contains("top 100 ");
-      assertThat(query.getGeneratedSql()).contains("order by o.ship_date desc");
+      assertThat(query.getGeneratedSql()).contains("order by o.ship_date desc ");
     } else if (isOracle()) {
       assertThat(query.getGeneratedSql()).contains("a  where rownum <= 100 )");
     } else {
       assertThat(query.getGeneratedSql()).contains("order by o.ship_date desc limit 100");
     }
+
+    // check also select count(*)
+    LoggedSqlCollector.start();
+    query.findCount();
+    List<String>sql = LoggedSqlCollector.stop();
+    assertThat(sql.get(0)).startsWith("select count(*) from ( select o.id, o.order_date, o.ship_date from o_order o");
+    assertThat(sql.get(0)).doesNotContain("order by");
+  }
+
+  @Test
+  public void testPaging_with_existingRawSqlOrderBy_expect_id_appendToOrderBy_with_id() {
+
+    ResetBasicData.reset();
+
+    RawSql rawSql = RawSqlBuilder.parse("select o.id, o.order_date, o.ship_date from o_order o order by o.ship_date desc")
+      .columnMapping("o.id", "id")
+      .columnMapping("o.order_date", "orderDate")
+      .columnMapping("o.ship_date", "shipDate")
+      .create();
+
+    Query<Order> query = Ebean.find(Order.class);
+    query.setRawSql(rawSql);
+    query.orderById(true);
+
+    query.setMaxRows(100);
+    query.findList();
+
+    if (isSqlServer()) {
+      assertThat(query.getGeneratedSql()).contains("top 100 ");
+      assertThat(query.getGeneratedSql()).contains("order by o.ship_date desc, o.id ");
+    } else if (isOracle()) {
+      assertThat(query.getGeneratedSql()).contains("a  where rownum <= 100 )");
+    } else {
+      assertThat(query.getGeneratedSql()).contains("order by o.ship_date desc, o.id limit 100");
+    }
+
+    // check also select count(*)
+    LoggedSqlCollector.start();
+    query.findCount();
+    List<String>sql = LoggedSqlCollector.stop();
+    assertThat(sql.get(0)).startsWith("select count(*) from ( select o.id, o.order_date, o.ship_date from o_order o");
+    assertThat(sql.get(0)).doesNotContain("order by");
   }
 
   @IgnorePlatform(Platform.ORACLE)
@@ -199,7 +249,7 @@ public class TestRawSqlOrmQuery extends BaseTestCase {
       query.order("coalesce(shipDate, getdate()) desc");
       query.findList();
 
-      assertThat(sqlOf(query)).contains("order by coalesce(o.ship_date, getdate()) desc");
+      assertThat(sqlOf(query)).contains("order by coalesce(o.ship_date, getdate()) desc ");
       assertThat(sqlOf(query)).contains("select top 100");
 
     } else {
@@ -210,6 +260,39 @@ public class TestRawSqlOrmQuery extends BaseTestCase {
     }
   }
 
+  @IgnorePlatform(Platform.ORACLE)
+  @Test
+  public void testPaging_when_setOrderBy_expect_id_appendToOrderBy_with_id() {
+
+    ResetBasicData.reset();
+
+    RawSql rawSql = RawSqlBuilder.parse("select o.id, o.order_date, o.ship_date from o_order o order by o.ship_date desc nulls last")
+      .columnMapping("o.id", "id")
+      .columnMapping("o.order_date", "orderDate")
+      .columnMapping("o.ship_date", "shipDate")
+      .create();
+
+    Query<Order> query = Ebean.find(Order.class);
+    query.setRawSql(rawSql);
+
+    query.setMaxRows(100);
+    query.orderById(true);
+
+    if (isSqlServer()) {
+      query.order("coalesce(shipDate, getdate()) desc");
+      query.findList();
+
+      assertThat(sqlOf(query)).contains("order by coalesce(o.ship_date, getdate()) desc, o.id");
+      assertThat(sqlOf(query)).contains("select top 100");
+
+    } else {
+      query.order("coalesce(shipDate, now()) desc");
+      query.findList();
+
+      assertThat(query.getGeneratedSql()).contains("order by coalesce(o.ship_date, now()) desc, o.id limit 100");
+    }
+  }
+
   @Test
   public void testPaging_when_setOrderBy_containsId_expect_leaveAsIs() {
 

src/test/java/org/tests/update/TestSqlUpdateUpsert.java

@@ -13,7 +13,7 @@ public class TestSqlUpdateUpsert extends BaseTestCase {
 
   @ForPlatform(Platform.H2)
   @Test
-  public void h2Merge() {
+  public void h2Merge() throws InterruptedException {
 
     String sql = "merge into e_person_online (email, online_status, when_updated) key(email) values (?, ?, now())";
 
@@ -30,6 +30,7 @@ public class TestSqlUpdateUpsert extends BaseTestCase {
     assertThat(found.getEmail()).isEqualTo(email);
     assertThat(found.isOnlineStatus()).isTrue();
 
+    Thread.sleep(50); // have to wait some millis here
     String sqlNamed = "merge into e_person_online (email, online_status, when_updated) key(email) values (:email, :online, now())";
 
     SqlUpdate sqlUpdate2 = Ebean.createSqlUpdate(sqlNamed)
@@ -46,7 +47,7 @@ public class TestSqlUpdateUpsert extends BaseTestCase {
     assertThat(found2.getId()).isEqualTo(key);
     assertThat(found2.getEmail()).isEqualTo(email);
     assertThat(found2.isOnlineStatus()).isFalse();
-    assertThat(found2.getWhenUpdated()).isGreaterThan(found.getWhenUpdated());
+    assertThat(found2.getWhenUpdated()).isGreaterThan(found.getWhenUpdated()); // otherwise this test fails on my machine
   }
 
   @ForPlatform(Platform.POSTGRES)