Merge branch 'fix-order-by-on-prop' of github.com:FOCONIS/ebean into FOCONIS-fix-order-by-on-prop

ebean-api/src/main/java/io/ebean/OrderBy.java

@@ -231,18 +231,6 @@ public class OrderBy<T> 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<T> 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) {

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()));
         }

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 <code>sql</code> contains the <code>column</code>.
+   * <p>
+   * 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;
+    }
+  }
+
 }

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);
       }
     }

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<EBasicTree> 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<EBasicTree> getChildren() {
+    return children;
+  }
+
+  public void setChildren(List<EBasicTree> children) {
+    this.children = children;
+  }
+
+  public EBasic getRef() {
+    return ref;
+  }
+
+  public void setRef(EBasic ref) {
+    this.ref = ref;
+  }
+}

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 ");

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<EBasicTree> 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);

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");

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