From 52398317bc3270237bd93eb4820987e88b3f2cd5 Mon Sep 17 00:00:00 2001 From: rob bygrave Date: Mon, 1 Jul 2019 22:50:44 +1200 Subject: [PATCH] #1746 - Wrong SQL generated when using setDistinct and order by on aggregated column --- src/main/java/io/ebean/OrderBy.java | 27 ++++++++++++++----- .../server/query/CQueryBuilder.java | 6 ++++- .../TestAggregateFormula.java | 24 +++++++++++++++++ .../orderby/TestOrderByWithDistinct.java | 2 +- .../tests/unitinternal/TestOrderByParse.java | 9 +++++++ 5 files changed, 59 insertions(+), 9 deletions(-) diff --git a/src/main/java/io/ebean/OrderBy.java b/src/main/java/io/ebean/OrderBy.java index 10e6baf37..6b59e12bf 100644 --- a/src/main/java/io/ebean/OrderBy.java +++ b/src/main/java/io/ebean/OrderBy.java @@ -80,7 +80,6 @@ public final class OrderBy implements Serializable { * Add a property with ascending order to this OrderBy. */ public Query asc(String propertyName, String collation) { - list.add(new Property(propertyName, true, collation)); return query; } @@ -89,7 +88,6 @@ public final class OrderBy implements Serializable { * Add a property with descending order to this OrderBy. */ public Query desc(String propertyName) { - list.add(new Property(propertyName, false)); return query; } @@ -98,7 +96,6 @@ public final class OrderBy implements Serializable { * Add a property with descending order to this OrderBy. */ public Query desc(String propertyName, String collation) { - list.add(new Property(propertyName, false, collation)); return query; } @@ -108,7 +105,6 @@ public final class OrderBy implements Serializable { * Return true if the property is known to be contained in the order by clause. */ public boolean containsProperty(String propertyName) { - for (Property aList : list) { if (propertyName.equals(aList.getProperty())) { return true; @@ -161,10 +157,9 @@ public final class OrderBy implements Serializable { * Return a copy of the OrderBy. */ public OrderBy copy() { - OrderBy copy = new OrderBy<>(); - for (Property aList : list) { - copy.add(aList.copy()); + for (Property property : list) { + copy.add(property.copy()); } return copy; } @@ -241,6 +236,18 @@ public final class OrderBy implements Serializable { return this; } + /** + * Return true if this order by can be used in select clause. + */ + public boolean supportsSelect() { + for (Property property : list) { + if (!property.supportsSelect()) { + return false; + } + } + return true; + } + /** * A property and its ascending descending order. */ @@ -401,6 +408,12 @@ public final class OrderBy implements Serializable { this.ascending = ascending; } + /** + * Support use in select clause if no collation or nulls ordering. + */ + boolean supportsSelect() { + return nulls == null && collation == null; + } } private void parse(String orderByClause) { diff --git a/src/main/java/io/ebeaninternal/server/query/CQueryBuilder.java b/src/main/java/io/ebeaninternal/server/query/CQueryBuilder.java index 8fa4bc712..2212e6b87 100644 --- a/src/main/java/io/ebeaninternal/server/query/CQueryBuilder.java +++ b/src/main/java/io/ebeaninternal/server/query/CQueryBuilder.java @@ -1,6 +1,7 @@ package io.ebeaninternal.server.query; import io.ebean.CountDistinctOrder; +import io.ebean.OrderBy; import io.ebean.Query; import io.ebean.RawSql; import io.ebean.RawSqlBuilder; @@ -600,7 +601,10 @@ class CQueryBuilder { } if (distinct && dbOrderBy != null && !query.isSingleAttribute()) { // add the orderBy columns to the select clause (due to distinct) - sb.append(", ").append(DbOrderByTrim.trim(dbOrderBy)); + final OrderBy orderBy = query.getOrderBy(); + if (orderBy != null && orderBy.supportsSelect()) { + sb.append(", ").append(DbOrderByTrim.trim(dbOrderBy)); + } } } diff --git a/src/test/java/org/tests/aggregateformula/TestAggregateFormula.java b/src/test/java/org/tests/aggregateformula/TestAggregateFormula.java index 16be122b1..5d75c444a 100644 --- a/src/test/java/org/tests/aggregateformula/TestAggregateFormula.java +++ b/src/test/java/org/tests/aggregateformula/TestAggregateFormula.java @@ -17,6 +17,30 @@ import static org.junit.Assert.assertNotNull; public class TestAggregateFormula extends BaseTestCase { + @Test + public void minDistinctOrderByNulls() { + + ResetBasicData.reset(); + + LoggedSqlCollector.start(); + + List contacts = Ebean.find(Contact.class) + .setDistinct(true) + .select("lastName, min(customer)") + .orderBy("min(customer) asc nulls last") + .findList(); + + List sql = LoggedSqlCollector.stop(); + assertThat(sql.get(0)).contains("select distinct t0.last_name, min(t0.customer_id) from contact t0 group by t0.last_name order by min(t0.customer_id) nulls last"); + + assertThat(contacts).isNotEmpty(); + + Contact contact = contacts.get(0); + assertThat(contact.getLastName()).isNotNull(); + assertThat(contact.getCustomer()).isNotNull(); + assertThat(contact.getCustomer().getId()).isNotNull(); + } + @Test public void minOnManyToOne() { diff --git a/src/test/java/org/tests/query/orderby/TestOrderByWithDistinct.java b/src/test/java/org/tests/query/orderby/TestOrderByWithDistinct.java index 684678e1f..7e03b1b6f 100644 --- a/src/test/java/org/tests/query/orderby/TestOrderByWithDistinct.java +++ b/src/test/java/org/tests/query/orderby/TestOrderByWithDistinct.java @@ -57,7 +57,7 @@ public class TestOrderByWithDistinct extends BaseTestCase { if (isPostgres()) { assertThat(sql).contains("select distinct on (t0.user_name, t0.userid) t0.userid,"); } else if (isH2()) { - assertThat(sql).contains("select distinct t0.userid, t0.user_name, t0.user_type_id,"); + assertThat(sql).contains("select distinct t0.userid, t0.user_name, t0.user_type_id from muser t0"); } } diff --git a/src/test/java/org/tests/unitinternal/TestOrderByParse.java b/src/test/java/org/tests/unitinternal/TestOrderByParse.java index 6cd43c5be..362086508 100644 --- a/src/test/java/org/tests/unitinternal/TestOrderByParse.java +++ b/src/test/java/org/tests/unitinternal/TestOrderByParse.java @@ -24,6 +24,7 @@ public class TestOrderByParse extends BaseTestCase { assertTrue(o1.getProperties().get(0).isAscending()); assertThat(o1.toStringFormat()).isEqualTo("case when status='N' then 1 when status='F' then 2 else 99 end"); assertThat(o1.getProperties().get(0).getProperty()).isEqualTo("case when status='N' then 1 when status='F' then 2 else 99 end"); + assertTrue(o1.supportsSelect()); } @Test @@ -34,6 +35,7 @@ public class TestOrderByParse extends BaseTestCase { assertEquals("id", o1.getProperties().get(0).getProperty()); assertTrue(o1.getProperties().get(0).isAscending()); assertEquals("id", o1.toStringFormat()); + assertTrue(o1.supportsSelect()); o1 = new OrderBy<>("id asc"); assertEquals(1, o1.getProperties().size()); @@ -65,6 +67,7 @@ public class TestOrderByParse extends BaseTestCase { assertEquals("id", o1.getProperties().get(0).getProperty()); assertTrue(!o1.getProperties().get(0).isAscending()); assertEquals("id desc nulls high", o1.toStringFormat()); + assertFalse(o1.supportsSelect()); } @Test @@ -100,6 +103,7 @@ public class TestOrderByParse extends BaseTestCase { assertEquals("name", o1.getProperties().get(1).getProperty()); assertTrue(o1.getProperties().get(1).isAscending()); assertEquals("id, name", o1.toStringFormat()); + assertTrue(o1.supportsSelect()); o1 = new OrderBy<>(" id , name "); assertEquals(2, o1.getProperties().size()); @@ -178,6 +182,7 @@ public class TestOrderByParse extends BaseTestCase { assertEquals("id", o1.getProperties().get(0).getProperty()); assertTrue(o1.getProperties().get(0).isAscending()); assertEquals("id collate latin_1", o1.toStringFormat()); + assertFalse(o1.supportsSelect()); o1 = new OrderBy<>(); o1.desc("id", "latin_1"); @@ -185,6 +190,7 @@ public class TestOrderByParse extends BaseTestCase { assertEquals("id", o1.getProperties().get(0).getProperty()); assertTrue(!o1.getProperties().get(0).isAscending()); assertEquals("id collate latin_1 desc", o1.toStringFormat()); + assertFalse(o1.supportsSelect()); o1 = new OrderBy<>(); o1.desc("id", "latin_1"); @@ -195,6 +201,7 @@ public class TestOrderByParse extends BaseTestCase { assertTrue(!o1.getProperties().get(0).isAscending()); assertTrue(o1.getProperties().get(1).isAscending()); assertEquals("id collate latin_1 desc, date", o1.toStringFormat()); + assertFalse(o1.supportsSelect()); o1 = new OrderBy<>(); o1.desc("id", "latin_1"); @@ -205,6 +212,7 @@ public class TestOrderByParse extends BaseTestCase { assertTrue(!o1.getProperties().get(0).isAscending()); assertTrue(o1.getProperties().get(1).isAscending()); assertEquals("id collate latin_1 desc, name collate latin_2", o1.toStringFormat()); + assertFalse(o1.supportsSelect()); // functional (DB2) syntax o1 = new OrderBy<>(); @@ -213,6 +221,7 @@ public class TestOrderByParse extends BaseTestCase { assertEquals("id", o1.getProperties().get(0).getProperty()); assertTrue(!o1.getProperties().get(0).isAscending()); assertEquals("COLLATION_KEY(id, 'latin_1') desc", o1.toStringFormat()); + assertFalse(o1.supportsSelect()); }