diff --git a/ebean-api/src/main/java/io/ebean/OrderBy.java b/ebean-api/src/main/java/io/ebean/OrderBy.java index 8b39fc19e..d083387b3 100644 --- a/ebean-api/src/main/java/io/ebean/OrderBy.java +++ b/ebean-api/src/main/java/io/ebean/OrderBy.java @@ -231,18 +231,6 @@ public 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. */ @@ -403,12 +391,6 @@ public class OrderBy implements Serializable { this.ascending = ascending; } - /** - * Support use in select clause if no collation or nulls ordering. - */ - boolean supportsSelect() { - return nulls == null; - } } private void parse(String orderByClause) { diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryBuilder.java b/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryBuilder.java index b0fcd7f17..bf5ecc1d6 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryBuilder.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryBuilder.java @@ -585,16 +585,11 @@ final class CQueryBuilder { } if (distinct && dbOrderBy != null) { // add the orderBy columns to the select clause (due to distinct) - final OrderBy orderBy = query.getOrderBy(); - if (orderBy != null && orderBy.supportsSelect()) { - String trimmed = DbOrderByTrim.trim(dbOrderBy); - if (query.isSingleAttribute() && select.selectSql().startsWith(trimmed)) { - // NOP, already in SQL - // TODO: what to do if we select("id").orderBy("prop,id")? - // Can we live with a query like "select t0.id, t0.prop, t0.id from" - // or should we elliminate the second "t0.id" from select - } else { - sb.append(", ").append(trimmed); + String[] tokens = DbOrderByTrim.trim(dbOrderBy).split(","); + for (String token : tokens) { + token = token.trim(); + if (!DbOrderByTrim.contains(select.selectSql(), token)) { + sb.append(", ").append(token); } } } @@ -643,7 +638,7 @@ final class CQueryBuilder { private void appendHistoryAsOfPredicate() { if (query.isAsOfBaseTable()) { query.incrementAsOfTableCount(); - if (!historySupport.isStandardsBased()){ + if (!historySupport.isStandardsBased()) { appendAndOrWhere(); sb.append(historySupport.asOfPredicate(request.baseTableAlias())); } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/query/DbOrderByTrim.java b/ebean-core/src/main/java/io/ebeaninternal/server/query/DbOrderByTrim.java index 72eb81db4..1deefab10 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/query/DbOrderByTrim.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/query/DbOrderByTrim.java @@ -4,7 +4,7 @@ import java.util.regex.Pattern; final class DbOrderByTrim { - private static final Pattern orderByTrim = Pattern.compile("(?i) asc\\b| desc\\b|\\b nulls first\\b|\\b nulls last\\b"); + private static final Pattern orderByTrim = Pattern.compile("(?i) asc\\b| desc\\b| nulls first\\b| nulls last\\b"); /** * Convert the dbOrderBy clause to be safe for adding to select or distinct on. @@ -14,4 +14,21 @@ final class DbOrderByTrim { return orderByTrim.matcher(dbOrderBy).replaceAll(""); } + /** + * Checks, if sql contains the column. + *

+ * SQL is normally comma separated: "t0.id, t1.id, t2.name" + */ + static boolean contains(String sql, String column) { + if (sql.endsWith(column)) { // simplest way. sql ends with the column + return true; + } else if (sql.contains(column)) { + // We need to check, if it is really the correct column, + // sql="t0.name_short, t0.name_long" will match on column="t0.name" + return sql.contains(column + ","); + } else { + return false; + } + } + } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeBuilder.java b/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeBuilder.java index fd7bbfeaf..36647fda4 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeBuilder.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/query/SqlTreeBuilder.java @@ -172,7 +172,7 @@ public final class SqlTreeBuilder { String[] split = idCols.split(","); for (String col : split) { col = col.trim(); - if (!dbOrderBy.contains(col)) { + if (!DbOrderByTrim.contains(dbOrderBy, col)) { sb.append(", ").append(col); } } diff --git a/ebean-test/src/test/java/org/tests/model/basic/EBasicTree.java b/ebean-test/src/test/java/org/tests/model/basic/EBasicTree.java new file mode 100644 index 000000000..eaefd65fb --- /dev/null +++ b/ebean-test/src/test/java/org/tests/model/basic/EBasicTree.java @@ -0,0 +1,59 @@ +package org.tests.model.basic; + +import javax.persistence.Entity; +import javax.persistence.Id; +import javax.persistence.ManyToOne; +import javax.persistence.OneToMany; +import javax.persistence.OrderBy; +import javax.persistence.Table; +import java.util.List; + +@Entity +@Table(name = "e_basic_tree") +public class EBasicTree { + + @Id + private int id; + + @ManyToOne + private EBasicTree parent; + + @OneToMany + @OrderBy("ref.name") + private List children; + + @ManyToOne + private EBasic ref; + + public int getId() { + return id; + } + + public void setId(int id) { + this.id = id; + } + + public EBasicTree getParent() { + return parent; + } + + public void setParent(EBasicTree parent) { + this.parent = parent; + } + + public List getChildren() { + return children; + } + + public void setChildren(List children) { + this.children = children; + } + + public EBasic getRef() { + return ref; + } + + public void setRef(EBasic ref) { + this.ref = ref; + } +} diff --git a/ebean-test/src/test/java/org/tests/query/aggregation/TestAggregationCount.java b/ebean-test/src/test/java/org/tests/query/aggregation/TestAggregationCount.java index 8636d48d6..7d2bc6779 100644 --- a/ebean-test/src/test/java/org/tests/query/aggregation/TestAggregationCount.java +++ b/ebean-test/src/test/java/org/tests/query/aggregation/TestAggregationCount.java @@ -149,9 +149,9 @@ public class TestAggregationCount extends BaseTestCase { String sql = sqlOf(query, 5); if (isH2()) { - assertThat(sql).contains("select distinct t0.id, t0.name, count(u1.id), sum(u1.my_units), sum(u1.my_units * u1.amount), sum(u1.my_units), t0.name from tevent_one t0 "); + assertThat(sql).contains("select distinct t0.id, t0.name, count(u1.id), sum(u1.my_units), sum(u1.my_units * u1.amount) from tevent_one t0 "); } else if (isPostgresCompatible()) { - assertThat(sql).contains("t0.name, count(u1.id), sum(u1.my_units), sum(u1.my_units * u1.amount), sum(u1.my_units), t0.name from tevent_one t0 "); + assertThat(sql).contains("t0.name, count(u1.id), sum(u1.my_units), sum(u1.my_units * u1.amount) from tevent_one t0 "); } assertThat(sql).contains("from tevent_one t0 join tevent_many u1 on u1.event_id = t0.id "); assertThat(sql).contains(" group by t0.id, t0.name "); diff --git a/ebean-test/src/test/java/org/tests/query/orderby/TestOrderByWithDistinct.java b/ebean-test/src/test/java/org/tests/query/orderby/TestOrderByWithDistinct.java index 8e6d13893..c8a911da6 100644 --- a/ebean-test/src/test/java/org/tests/query/orderby/TestOrderByWithDistinct.java +++ b/ebean-test/src/test/java/org/tests/query/orderby/TestOrderByWithDistinct.java @@ -1,12 +1,18 @@ package org.tests.query.orderby; -import io.ebean.xtest.BaseTestCase; import io.ebean.DB; import io.ebean.Query; -import io.ebean.xtest.IgnorePlatform; import io.ebean.annotation.Platform; +import io.ebean.xtest.BaseTestCase; +import io.ebean.xtest.IgnorePlatform; import org.junit.jupiter.api.Test; -import org.tests.model.basic.*; +import org.tests.model.basic.Customer; +import org.tests.model.basic.EBasic; +import org.tests.model.basic.EBasicTree; +import org.tests.model.basic.MRole; +import org.tests.model.basic.MUser; +import org.tests.model.basic.MUserType; +import org.tests.model.basic.ResetBasicData; import java.util.List; import java.util.Set; @@ -57,6 +63,30 @@ public class TestOrderByWithDistinct extends BaseTestCase { } + @Test + public void testOrderByOnPropWithDistinct() { + Query query = DB.find(EBasicTree.class) + .fetch("children") + .where().eq("children.ref.status", EBasic.Status.ACTIVE).query(); + + query.findList(); + // we expect t2.name in this query + if (platformDistinctOn()) { + assertSql(query).startsWith("select distinct on (t0.id, t2.name, t1.id) t0.id, t0.parent_id, t0.ref_id, t1.id, t1.parent_id, t1.ref_id, t2.name " + + "from e_basic_tree t0 " + + "left join e_basic_tree t1 on t1.parent_id = t0.id " + + "join e_basic_tree u1 on u1.parent_id = t0.id " + + "join e_basic u2 on u2.id = u1.ref_id left " + + "join e_basic t2 on t2.id = t1.ref_id where u2.status = ? order by t0.id, t2.name"); + } else { + assertSql(query).startsWith("select distinct t0.id, t0.parent_id, t0.ref_id, t1.id, t1.parent_id, t1.ref_id, t2.name " + + "from e_basic_tree t0 " + + "left join e_basic_tree t1 on t1.parent_id = t0.id " + + "join e_basic_tree u1 on u1.parent_id = t0.id " + + "join e_basic u2 on u2.id = u1.ref_id left " + + "join e_basic t2 on t2.id = t1.ref_id where u2.status = ? order by t0.id, t2.name"); + } + } @Test public void testOrderByWithDistinct() { @@ -72,9 +102,9 @@ public class TestOrderByWithDistinct extends BaseTestCase { assertSql(query).contains("from muser t0 limit 1000"); } query = DB.find(MUser.class) - .where() - .eq("roles.roleName", "A") - .query(); + .where() + .eq("roles.roleName", "A") + .query(); query.findList(); assertSql(query).doesNotContain("order by"); assertSql(query).contains("select distinct"); @@ -89,12 +119,12 @@ public class TestOrderByWithDistinct extends BaseTestCase { @Test public void test() { /* - * Original conversation: - * https://groups.google.com/forum/?fromgroups=#!topic/ebean/uuvi1btdCDQ%5B1-25-false%5D - * - * This test exposes what may be a general problem with columns required by the order by phrase being omitted from the select. - * I'm not sure this exposes all causes of the problem. - */ + * Original conversation: + * https://groups.google.com/forum/?fromgroups=#!topic/ebean/uuvi1btdCDQ%5B1-25-false%5D + * + * This test exposes what may be a general problem with columns required by the order by phrase being omitted from the select. + * I'm not sure this exposes all causes of the problem. + */ MUserType ut = new MUserType("md"); DB.save(ut); diff --git a/ebean-test/src/test/java/org/tests/query/orderby/TestOrderByWithDistinctTake2.java b/ebean-test/src/test/java/org/tests/query/orderby/TestOrderByWithDistinctTake2.java index 8d559f67f..7195fba57 100644 --- a/ebean-test/src/test/java/org/tests/query/orderby/TestOrderByWithDistinctTake2.java +++ b/ebean-test/src/test/java/org/tests/query/orderby/TestOrderByWithDistinctTake2.java @@ -60,9 +60,9 @@ public class TestOrderByWithDistinctTake2 extends BaseTestCase { String generatedSql = sqlOf(query); if (platformDistinctOn()) { - assertThat(generatedSql).contains("select distinct on (t0.name, t0.id) t0.id, t0.name, t0.id"); + assertThat(generatedSql).contains("select distinct on (t0.name, t0.id) t0.id, t0.name"); } else { - assertThat(generatedSql).contains("select distinct t0.id, t0.name, t0.id"); + assertThat(generatedSql).contains("select distinct t0.id, t0.name"); } assertThat(generatedSql).contains("order by t0.name, t0.id desc"); assertThat(generatedSql).contains("from o_customer t0 join contact u1 on u1.customer_id = t0.id"); diff --git a/ebean-test/src/test/java/org/tests/unitinternal/TestOrderByParse.java b/ebean-test/src/test/java/org/tests/unitinternal/TestOrderByParse.java index 4fb45fef6..2a23f7273 100644 --- a/ebean-test/src/test/java/org/tests/unitinternal/TestOrderByParse.java +++ b/ebean-test/src/test/java/org/tests/unitinternal/TestOrderByParse.java @@ -20,7 +20,6 @@ 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 @@ -31,7 +30,6 @@ 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()); @@ -63,7 +61,6 @@ public class TestOrderByParse extends BaseTestCase { assertEquals("id", o1.getProperties().get(0).getProperty()); assertFalse(o1.getProperties().get(0).isAscending()); assertEquals("id desc nulls high", o1.toStringFormat()); - assertFalse(o1.supportsSelect()); } @Test @@ -99,7 +96,6 @@ 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,7 +174,6 @@ 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()); - assertTrue(o1.supportsSelect()); o1 = new OrderBy<>(); o1.desc("id", "latin_1"); @@ -186,7 +181,6 @@ public class TestOrderByParse extends BaseTestCase { assertEquals("id", o1.getProperties().get(0).getProperty()); assertFalse(o1.getProperties().get(0).isAscending()); assertEquals("id collate latin_1 desc", o1.toStringFormat()); - assertTrue(o1.supportsSelect()); o1 = new OrderBy<>(); o1.desc("id", "latin_1"); @@ -197,7 +191,6 @@ public class TestOrderByParse extends BaseTestCase { assertFalse(o1.getProperties().get(0).isAscending()); assertTrue(o1.getProperties().get(1).isAscending()); assertEquals("id collate latin_1 desc, date", o1.toStringFormat()); - assertTrue(o1.supportsSelect()); o1 = new OrderBy<>(); o1.desc("id", "latin_1"); @@ -208,7 +201,6 @@ public class TestOrderByParse extends BaseTestCase { assertFalse(o1.getProperties().get(0).isAscending()); assertTrue(o1.getProperties().get(1).isAscending()); assertEquals("id collate latin_1 desc, name collate latin_2", o1.toStringFormat()); - assertTrue(o1.supportsSelect()); // functional (DB2) syntax o1 = new OrderBy<>(); @@ -217,7 +209,6 @@ public class TestOrderByParse extends BaseTestCase { assertEquals("id", o1.getProperties().get(0).getProperty()); assertFalse(o1.getProperties().get(0).isAscending()); assertEquals("COLLATION_KEY(id, 'latin_1') desc", o1.toStringFormat()); - assertTrue(o1.supportsSelect()); } @Test