diff --git a/src/main/java/io/ebean/Query.java b/src/main/java/io/ebean/Query.java index d8898ae5d..6102926c3 100644 --- a/src/main/java/io/ebean/Query.java +++ b/src/main/java/io/ebean/Query.java @@ -1507,4 +1507,12 @@ public interface Query { */ Set 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 orderById(boolean orderById); + } diff --git a/src/main/java/io/ebean/config/ServerConfig.java b/src/main/java/io/ebean/config/ServerConfig.java index 03fcf0eed..b336029f8 100644 --- a/src/main/java/io/ebean/config/ServerConfig.java +++ b/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. *

@@ -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) { diff --git a/src/main/java/io/ebeaninternal/api/SpiQuery.java b/src/main/java/io/ebeaninternal/api/SpiQuery.java index 2be96bfd9..869f2ca1c 100644 --- a/src/main/java/io/ebeaninternal/api/SpiQuery.java +++ b/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 extends Query, TxnProfileEventCodes { /** * Return a copy of the query attaching to a different EbeanServer. */ - SpiQuery copy(EbeanServer server); + SpiQuery copy(SpiEbeanServer server); /** * Return the type of query (List, Set, Map, Bean, rowCount etc). @@ -593,6 +592,11 @@ public interface SpiQuery extends Query, 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. */ diff --git a/src/main/java/io/ebeaninternal/server/deploy/BeanDescriptor.java b/src/main/java/io/ebeaninternal/server/deploy/BeanDescriptor.java index 9cc43fef5..424c91363 100644 --- a/src/main/java/io/ebeaninternal/server/deploy/BeanDescriptor.java +++ b/src/main/java/io/ebeaninternal/server/deploy/BeanDescriptor.java @@ -3053,13 +3053,8 @@ public class BeanDescriptor implements BeanType { */ public void appendOrderById(SpiQuery 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()); } } diff --git a/src/main/java/io/ebeaninternal/server/query/CQueryEngine.java b/src/main/java/io/ebeaninternal/server/query/CQueryEngine.java index 541a6f1ab..f03efc1a0 100644 --- a/src/main/java/io/ebeaninternal/server/query/CQueryEngine.java +++ b/src/main/java/io/ebeaninternal/server/query/CQueryEngine.java @@ -190,7 +190,6 @@ public class CQueryEngine { */ public QueryIterator findIterate(OrmQueryRequest request) { - prepareForPaging(request); CQuery 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 void prepareForPaging(OrmQueryRequest request) { - SpiQuery query = request.getQuery(); - if (query.checkPagingOrderBy()) { - request.getBeanDescriptor().appendOrderById(query); - } - } + /** * Find a list/map/set of beans. */ BeanCollection findMany(OrmQueryRequest request) { - prepareForPaging(request); - CQuery cquery = queryBuilder.buildQuery(request); request.setCancelableQuery(cquery); diff --git a/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java b/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java index a7aef2d50..6a8648e90 100644 --- a/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java +++ b/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 implements SpiQuery { private final BeanDescriptor beanDescriptor; - private final EbeanServer server; + private final SpiEbeanServer server; private final ExpressionFactory expressionFactory; @@ -254,6 +255,8 @@ public class DefaultOrmQuery implements SpiQuery { 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 implements SpiQuery { private ProfileLocation profileLocation; - public DefaultOrmQuery(BeanDescriptor desc, EbeanServer server, ExpressionFactory expressionFactory) { + public DefaultOrmQuery(BeanDescriptor 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 implements SpiQuery { } + @Override public boolean isFindAll() { return whereExpressions == null && nativeSql == null && rawSql == null; } @@ -304,6 +309,7 @@ public class DefaultOrmQuery implements SpiQuery { } } + @Override public short getProfileId() { return profileId; } @@ -717,8 +723,7 @@ public class DefaultOrmQuery implements SpiQuery { } @Override - public DefaultOrmQuery copy(EbeanServer server) { - + public DefaultOrmQuery copy(SpiEbeanServer server) { DefaultOrmQuery copy = new DefaultOrmQuery<>(beanDescriptor, server, expressionFactory); copy.m2mIncludeJoin = m2mIncludeJoin; copy.profilingListener = profilingListener; @@ -742,6 +747,7 @@ public class DefaultOrmQuery implements SpiQuery { if (orderBy != null) { copy.orderBy = orderBy.copy(); } + copy.orderById = orderById; if (bindParams != null) { copy.bindParams = bindParams.copy(); } @@ -1037,6 +1043,7 @@ public class DefaultOrmQuery implements SpiQuery { public CQueryPlanKey prepare(BeanQueryRequest request) { prepareExpressions(request); + prepareForPaging((OrmQueryRequest) request); queryPlanKey = createQueryPlanKey(); return queryPlanKey; } @@ -1054,6 +1061,24 @@ public class DefaultOrmQuery implements SpiQuery { } } + /** + * 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 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. *

@@ -1388,10 +1413,11 @@ public class DefaultOrmQuery implements SpiQuery { @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 implements SpiQuery { public ProfileLocation getProfileLocation() { return profileLocation; } + + @Override + public Query orderById(boolean orderById) { + this.orderById = orderById; + return this; + } + + public boolean isOrderById() { + return orderById; + } } diff --git a/src/test/java/io/ebean/EbeanServer_eqlTest.java b/src/test/java/io/ebean/EbeanServer_eqlTest.java index 42dfc0905..a29e00bad 100644 --- a/src/test/java/io/ebean/EbeanServer_eqlTest.java +++ b/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(); + Listsql = LoggedSqlCollector.stop(); + assertThat(sql.get(0)).startsWith("select count(*) from o_customer t0;"); + } + + @Test + public void basic_limit_offset2_with_id() { + + ResetBasicData.reset(); + + Query 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(); + Listsql = LoggedSqlCollector.stop(); + assertThat(sql.get(0)).startsWith("select count(*) from o_customer t0;"); + } @Test diff --git a/src/test/java/org/tests/query/orderby/TestOrderByWithDistinct.java b/src/test/java/org/tests/query/orderby/TestOrderByWithDistinct.java index 4b5b2292a..2090a0566 100644 --- a/src/test/java/org/tests/query/orderby/TestOrderByWithDistinct.java +++ b/src/test/java/org/tests/query/orderby/TestOrderByWithDistinct.java @@ -37,6 +37,32 @@ public class TestOrderByWithDistinct extends BaseTestCase { } + + @Test + public void testOrderByWithDistinct() { + Query 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() { /* diff --git a/src/test/java/org/tests/query/other/TestQuerySingleAttribute.java b/src/test/java/org/tests/query/other/TestQuerySingleAttribute.java index 125c01373..27305412c 100644 --- a/src/test/java/org/tests/query/other/TestQuerySingleAttribute.java +++ b/src/test/java/org/tests/query/other/TestQuerySingleAttribute.java @@ -244,11 +244,11 @@ public class TestQuerySingleAttribute extends BaseTestCase { List 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(); } diff --git a/src/test/java/org/tests/rawsql/TestRawSqlOrmQuery.java b/src/test/java/org/tests/rawsql/TestRawSqlOrmQuery.java index ffa8b83c8..fe0850c4d 100644 --- a/src/test/java/org/tests/rawsql/TestRawSqlOrmQuery.java +++ b/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 list = query.findList(); Assert.assertNotNull(list); + + // check also select count(*) + LoggedSqlCollector.start(); + assertThat(query.findCount()).isEqualTo(list.size()); + Listsql = 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 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(); + Listsql = 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 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(); + Listsql = 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 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() { diff --git a/src/test/java/org/tests/update/TestSqlUpdateUpsert.java b/src/test/java/org/tests/update/TestSqlUpdateUpsert.java index eeb761b4b..fe8669c82 100644 --- a/src/test/java/org/tests/update/TestSqlUpdateUpsert.java +++ b/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)