From 4aa8a0dc63cfc47b4613e92f1e032a1a69290dee Mon Sep 17 00:00:00 2001 From: rob bygrave Date: Fri, 3 May 2019 14:33:42 +1200 Subject: [PATCH] #1694 - findList() with firstRows/maxRows adds "order by id" (even with orderById(false)) --- .../java/io/ebean/config/ServerConfig.java | 5 ++- .../server/querydefn/DefaultOrmQuery.java | 2 +- .../java/io/ebean/EbeanServer_eqlTest.java | 8 ++-- .../server/grammer/EqlParserTest.java | 2 +- .../server/rawsql/TestRawSqlParsing.java | 30 ++++++++++++++- .../model/history/TestHistoryOneToMany.java | 4 +- .../TestAddOrderByWithFirstRowsMaxRows.java | 12 ++++-- .../org/tests/query/TestQueryOrderById.java | 37 +++++++++++++++++++ .../orderby/TestOrderByWithDistinct.java | 9 +++-- .../query/other/TestQuerySingleAttribute.java | 7 +++- .../softdelete/TestSoftDeletePagingList.java | 4 +- 11 files changed, 98 insertions(+), 22 deletions(-) create mode 100644 src/test/java/org/tests/query/TestQueryOrderById.java diff --git a/src/main/java/io/ebean/config/ServerConfig.java b/src/main/java/io/ebean/config/ServerConfig.java index eaf40eb50..d5fb6092e 100644 --- a/src/main/java/io/ebean/config/ServerConfig.java +++ b/src/main/java/io/ebean/config/ServerConfig.java @@ -518,7 +518,7 @@ public class ServerConfig { /** * Controls the default order by id setting of queries. See {@link Query#orderById(boolean)} */ - private boolean defaultOrderById = false; + private boolean defaultOrderById; /** * The mappingLocations for searching xml mapping. @@ -585,8 +585,9 @@ public class ServerConfig { /** - * Sets the default orderById setting for queries. + * Deprecated - look to have explicit order by. Sets the default orderById setting for queries. */ + @Deprecated public void setDefaultOrderById(boolean defaultOrderById) { this.defaultOrderById = defaultOrderById; } diff --git a/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java b/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java index 49140b07e..56a7c35c2 100644 --- a/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java +++ b/src/main/java/io/ebeaninternal/server/querydefn/DefaultOrmQuery.java @@ -1615,7 +1615,7 @@ public class DefaultOrmQuery implements SpiQuery { @Override public boolean checkPagingOrderBy() { - return !useDocStore && (maxRows > 1 || firstRow > 0) && !distinct && (orderByIsEmpty() || isOrderById()); + return orderById && !useDocStore; } @Override diff --git a/src/test/java/io/ebean/EbeanServer_eqlTest.java b/src/test/java/io/ebean/EbeanServer_eqlTest.java index 75c0c1f4f..1e160484d 100644 --- a/src/test/java/io/ebean/EbeanServer_eqlTest.java +++ b/src/test/java/io/ebean/EbeanServer_eqlTest.java @@ -135,12 +135,12 @@ public class EbeanServer_eqlTest extends BaseTestCase { query.findList(); if (isSqlServer()) { - assertThat(query.getGeneratedSql()).endsWith("order by t0.id offset 3 rows fetch next 10 rows only"); + assertThat(query.getGeneratedSql()).endsWith("from o_customer t0 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.id limit 10 offset 3"); + assertThat(query.getGeneratedSql()).endsWith("from o_customer t0 limit 10 offset 3"); } } @@ -155,12 +155,10 @@ public class EbeanServer_eqlTest extends BaseTestCase { if (isSqlServer()) { assertThat(query.getGeneratedSql()).startsWith("select top 10 "); - assertThat(query.getGeneratedSql()).endsWith("order by t0.id"); } else if (isOracle()) { - assertThat(query.getGeneratedSql()).contains("t0 order by t0.id"); assertThat(query.getGeneratedSql()).contains(" a where rownum <= 10"); } else { - assertThat(query.getGeneratedSql()).endsWith("order by t0.id limit 10"); + assertThat(query.getGeneratedSql()).endsWith("limit 10"); } } diff --git a/src/test/java/io/ebeaninternal/server/grammer/EqlParserTest.java b/src/test/java/io/ebeaninternal/server/grammer/EqlParserTest.java index 2360772af..6e63066e2 100644 --- a/src/test/java/io/ebeaninternal/server/grammer/EqlParserTest.java +++ b/src/test/java/io/ebeaninternal/server/grammer/EqlParserTest.java @@ -370,7 +370,7 @@ public class EqlParserTest extends BaseTestCase { Query query = parse("select name fetch billingAddress (line1, city) fetch shippingAddress (line1) limit 10"); query.findList(); - assertThat(sqlOf(query, 12)).contains("select t0.id, t0.name, t1.id, t1.line_1, t1.city, t2.id, t2.line_1 from o_customer t0 left join o_address t1 on t1.id = t0.billing_address_id left join o_address t2 on t2.id = t0.shipping_address_id order by t0.id"); + assertThat(sqlOf(query, 12)).contains("select t0.id, t0.name, t1.id, t1.line_1, t1.city, t2.id, t2.line_1 from o_customer t0 left join o_address t1 on t1.id = t0.billing_address_id left join o_address t2 on t2.id = t0.shipping_address_id"); } @Test diff --git a/src/test/java/io/ebeaninternal/server/rawsql/TestRawSqlParsing.java b/src/test/java/io/ebeaninternal/server/rawsql/TestRawSqlParsing.java index e868cb9bd..89b2f6fb0 100644 --- a/src/test/java/io/ebeaninternal/server/rawsql/TestRawSqlParsing.java +++ b/src/test/java/io/ebeaninternal/server/rawsql/TestRawSqlParsing.java @@ -2,6 +2,7 @@ package io.ebeaninternal.server.rawsql; import io.ebean.BaseTestCase; import io.ebean.Ebean; +import io.ebean.Query; import io.ebean.RawSql; import io.ebean.RawSqlBuilder; import io.ebean.annotation.ForPlatform; @@ -29,7 +30,6 @@ public class TestRawSqlParsing extends BaseTestCase { RawSql rawSql = RawSqlBuilder .parse(sql) .columnMapping("order_id", "order.id") - //.columnMapping("sum(order_qty*unit_price)","totalAmount") .create(); Sql rs = ((SpiRawSql)rawSql).getSql(); @@ -72,4 +72,32 @@ public class TestRawSqlParsing extends BaseTestCase { assertThat(customers).isNotEmpty(); } + + @ForPlatform({Platform.H2, Platform.POSTGRES}) + @Test + public void testUnion() { + + ResetBasicData.reset(); + + String sql = + "select id, name from (" + + " select id, name from o_customer where name <= 'f' " + + " union all " + + " select id, name from o_customer where name > 'f' " + + ") all_split"; + + + RawSql rawSql = RawSqlBuilder.parse(sql).create(); + + Query query = Ebean.createQuery(Customer.class) + .setRawSql(rawSql) + .setFirstRow(1) + .setMaxRows(5); + //.orderById(false); + + query.findList(); + + assertThat(sqlOf(query)).contains(") all_split limit 5 offset 1"); + } + } diff --git a/src/test/java/org/tests/model/history/TestHistoryOneToMany.java b/src/test/java/org/tests/model/history/TestHistoryOneToMany.java index 7332b81e2..7ca93fbe7 100644 --- a/src/test/java/org/tests/model/history/TestHistoryOneToMany.java +++ b/src/test/java/org/tests/model/history/TestHistoryOneToMany.java @@ -50,8 +50,8 @@ public class TestHistoryOneToMany extends BaseTestCase { if (isH2()) { assertThat(sql).hasSize(2); - assertThat(sql.get(0)).contains("from hi_tone_with_history t0 where (t0.sys_period_start <= ? and (t0.sys_period_end is null or t0.sys_period_end > ?)) and lower(t0.name) like ? escape'' order by t0.id limit 10"); - assertThat(sql.get(1)).contains("from hi_ttwo_with_history t0 left join hi_tthree_with_history t1 on t1.hi_ttwo_id = t0.id and (t1.sys_period_start <= ? and (t1.sys_period_end is null or t1.sys_period_end > ?)) where (t0.sys_period_start <= ? and (t0.sys_period_end is null or t0.sys_period_end > ?)) and (t0.hi_tone_id) in (? ) order by t0.id"); + assertThat(sql.get(0)).contains("from hi_tone_with_history t0 where (t0.sys_period_start <= ? and (t0.sys_period_end is null or t0.sys_period_end > ?)) and lower(t0.name) like ? escape'' limit 10"); + assertThat(sql.get(1)).contains("from hi_ttwo_with_history t0 left join hi_tthree_with_history t1 on t1.hi_ttwo_id = t0.id and (t1.sys_period_start <= ? and (t1.sys_period_end is null or t1.sys_period_end > ?)) where (t0.sys_period_start <= ? and (t0.sys_period_end is null or t0.sys_period_end > ?)) and (t0.hi_tone_id) in (? )"); } assertThat(list).hasSize(1); diff --git a/src/test/java/org/tests/query/TestAddOrderByWithFirstRowsMaxRows.java b/src/test/java/org/tests/query/TestAddOrderByWithFirstRowsMaxRows.java index b6a9d0a01..d6c7c66b6 100644 --- a/src/test/java/org/tests/query/TestAddOrderByWithFirstRowsMaxRows.java +++ b/src/test/java/org/tests/query/TestAddOrderByWithFirstRowsMaxRows.java @@ -50,7 +50,9 @@ public class TestAddOrderByWithFirstRowsMaxRows extends BaseTestCase { List loggedSql = LoggedSqlCollector.stop(); assertThat(loggedSql).hasSize(1); - assertThat(loggedSql.get(0)).contains("order by t0.id"); + if (isH2()) { + assertThat(loggedSql.get(0)).contains("from o_order t0 join o_customer t1 on t1.id = t0.kcustomer_id limit 10"); + } } @@ -109,7 +111,9 @@ public class TestAddOrderByWithFirstRowsMaxRows extends BaseTestCase { List loggedSql = LoggedSqlCollector.stop(); assertThat(loggedSql).hasSize(1); - assertThat(loggedSql.get(0)).contains("order by t0.id"); + if (isH2()) { + assertThat(loggedSql.get(0)).contains("join o_customer t1 on t1.id = t0.kcustomer_id limit 10"); + } } @Test @@ -128,7 +132,9 @@ public class TestAddOrderByWithFirstRowsMaxRows extends BaseTestCase { List loggedSql = LoggedSqlCollector.stop(); assertThat(loggedSql).hasSize(1); - assertThat(loggedSql.get(0)).contains("order by t0.id"); + if (isH2()) { + assertThat(loggedSql.get(0)).contains(" limit 10 offset 10"); + } } diff --git a/src/test/java/org/tests/query/TestQueryOrderById.java b/src/test/java/org/tests/query/TestQueryOrderById.java new file mode 100644 index 000000000..7d1c402f1 --- /dev/null +++ b/src/test/java/org/tests/query/TestQueryOrderById.java @@ -0,0 +1,37 @@ +package org.tests.query; + +import io.ebean.BaseTestCase; +import io.ebean.DB; +import io.ebean.Query; +import org.junit.Test; +import org.tests.model.basic.Customer; + +import static org.assertj.core.api.StrictAssertions.assertThat; + +public class TestQueryOrderById extends BaseTestCase { + + @Test + public void orderById_default_expectNotOrderById() { + + Query query = DB.find(Customer.class) + .select("id,name") + .setFirstRow(1) + .setMaxRows(5); + + query.findList(); + assertThat(sqlOf(query)).isEqualTo("select t0.id, t0.name from o_customer t0 limit 5 offset 1"); + } + + @Test + public void orderById_whenTrue_expectOrderById() { + + Query query = DB.find(Customer.class) + .select("id,name") + .setFirstRow(1) + .setMaxRows(5) + .orderById(true); + + query.findList(); + assertThat(sqlOf(query)).isEqualTo("select t0.id, t0.name from o_customer t0 order by t0.id limit 5 offset 1"); + } +} diff --git a/src/test/java/org/tests/query/orderby/TestOrderByWithDistinct.java b/src/test/java/org/tests/query/orderby/TestOrderByWithDistinct.java index 733af34a0..684678e1f 100644 --- a/src/test/java/org/tests/query/orderby/TestOrderByWithDistinct.java +++ b/src/test/java/org/tests/query/orderby/TestOrderByWithDistinct.java @@ -73,8 +73,9 @@ public class TestOrderByWithDistinct extends BaseTestCase { query.setMaxRows(1000); query.findList(); - assertThat(query.getGeneratedSql()).contains("order by t0.userid"); - + if (isH2()) { + assertThat(query.getGeneratedSql()).contains("from muser t0 limit 1000"); + } query = Ebean.find(MUser.class) .where() .eq("roles.roleName", "A") @@ -85,7 +86,9 @@ public class TestOrderByWithDistinct extends BaseTestCase { query.setMaxRows(1000); query.findList(); - assertThat(query.getGeneratedSql()).contains("order by t0.userid"); + if (isH2()) { + assertThat(query.getGeneratedSql()).contains("where u1.role_name = ? limit 1000"); + } } @Test diff --git a/src/test/java/org/tests/query/other/TestQuerySingleAttribute.java b/src/test/java/org/tests/query/other/TestQuerySingleAttribute.java index 4e25732bf..ae8ca0e98 100644 --- a/src/test/java/org/tests/query/other/TestQuerySingleAttribute.java +++ b/src/test/java/org/tests/query/other/TestQuerySingleAttribute.java @@ -258,17 +258,20 @@ public class TestQuerySingleAttribute extends BaseTestCase { // hmm - same problem when not using distinct @Test public void findSingleOnIdProperty() { + + ResetBasicData.reset(); + Query query = Ebean.find(Customer.class) .select("id") .setMaxRows(100); List ids = query.findSingleAttributeList(); if (isSqlServer()) { - assertThat(sqlOf(query)).contains("select top 100 t0.id from o_customer t0 order by t0.id"); + assertThat(sqlOf(query)).contains("select top 100 t0.id from o_customer t0"); } else if (isOracle()) { assertThat(sqlOf(query)).contains("where rownum <= 100"); } else { - assertThat(sqlOf(query)).contains("select t0.id from o_customer t0 order by t0.id limit 100"); + assertThat(sqlOf(query)).contains("select t0.id from o_customer t0 limit 100"); } assertThat(ids).isNotEmpty(); } diff --git a/src/test/java/org/tests/query/softdelete/TestSoftDeletePagingList.java b/src/test/java/org/tests/query/softdelete/TestSoftDeletePagingList.java index 1942f0adb..c3fc05b9a 100644 --- a/src/test/java/org/tests/query/softdelete/TestSoftDeletePagingList.java +++ b/src/test/java/org/tests/query/softdelete/TestSoftDeletePagingList.java @@ -50,9 +50,9 @@ public class TestSoftDeletePagingList extends TransactionalTestCase { assertThat(sql.get(1)).contains("where t0.s3_url like "); if (isPlatformBooleanNative()) { - assertThat(sql.get(1)).contains("and t0.deleted = false order by t0.id"); + assertThat(sql.get(1)).contains("and t0.deleted = false"); } else { - assertThat(sql.get(1)).contains("and t0.deleted = 0 order by t0.id"); + assertThat(sql.get(1)).contains("and t0.deleted = 0"); } } }