#3433 - Entity property called desc used with query order by desc gen…

…erates bad sql

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

@@ -186,15 +186,15 @@ public String toStringFormat() {
     if (list.isEmpty()) {
       return null;
     }
-    StringBuilder sb = new StringBuilder();
+    var append = new StringAppend();
     for (int i = 0; i < list.size(); i++) {
       Property property = list.get(i);
       if (i > 0) {
-        sb.append(", ");
+        append.append(", ");
       }
-      sb.append(property.toStringFormat());
+      property.toStringFormat(append);
     }
-    return sb.toString();
+    return append.toString();
   }
 
   @Override
@@ -231,6 +231,55 @@ public OrderBy<T> clear() {
     return this;
   }
 
+  /**
+   * Append the order by clause.
+   */
+  public interface Append {
+
+    /**
+     * Append a property expression.
+     */
+    Append property(String property);
+
+    /**
+     * Append a literal.
+     */
+    Append append(String literal);
+
+    /**
+     * Parse and append an expression.
+     */
+    Append parse(String expression);
+  }
+
+  private static final class StringAppend implements Append {
+
+    private final StringBuilder builder = new StringBuilder();
+
+    @Override
+    public String toString() {
+      return builder.toString();
+    }
+
+    @Override
+    public Append property(String property) {
+      builder.append(property);
+      return this;
+    }
+
+    @Override
+    public Append append(String literal) {
+      builder.append(literal);
+      return this;
+    }
+
+    @Override
+    public Append parse(String raw) {
+      builder.append(raw);
+      return this;
+    }
+  }
+
   /**
    * A property and its ascending descending order.
    */
@@ -309,36 +358,25 @@ public boolean equals(Object obj) {
 
     @Override
     public String toString() {
-      return toStringFormat();
+      return property;
     }
 
-    public String toStringFormat() {
-      if (nulls == null && collation == null) {
-        if (ascending) {
-          return property;
+    public void toStringFormat(Append append) {
+      if (collation != null)  {
+        if (collation.contains("${}")) {
+          // this is a complex collation, e.g. DB2 - we must replace the property
+          append.parse(collation.replace("${}", property));
         } else {
-          return property + " desc";
+          append.property(property).append(" collate ").append(collation);
         }
       } else {
-        StringBuilder sb = new StringBuilder();
-        if (collation != null)  {
-          if (collation.contains("${}")) {
-            // this is a complex collation, e.g. DB2 - we must replace the property
-            sb.append(collation.replace("${}", property));
-          } else {
-            sb.append(property);
-            sb.append(" collate ").append(collation);
-          }
-        } else {
-          sb.append(property);
-        }
-        if (!ascending) {
-          sb.append(' ').append("desc");
-        }
-        if (nulls != null) {
-          sb.append(' ').append(nulls).append(' ').append(highLow);
-        }
-        return sb.toString();
+        append.property(property);
+      }
+      if (!ascending) {
+        append.append(" desc");
+      }
+      if (nulls != null) {
+        append.append(" ").append(nulls).append(" ").append(highLow);
       }
     }
 

ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryOrderBy.java

@@ -5,6 +5,7 @@
 import io.ebeaninternal.server.deploy.BeanDescriptor;
 import io.ebeaninternal.server.deploy.BeanProperty;
 import io.ebeaninternal.server.deploy.BeanPropertyAssoc;
+import io.ebeaninternal.server.deploy.DeployParser;
 import io.ebeaninternal.server.deploy.id.IdBinder;
 import io.ebeaninternal.server.el.ElPropertyValue;
 
@@ -17,50 +18,81 @@ final class CQueryOrderBy {
 
   private final BeanDescriptor<?> desc;
   private final OrderBy<?> orderBy;
+  private final DeployParser parser;
 
   /**
    * Create the logical order by clause.
    */
-  public static String parse(BeanDescriptor<?> desc, OrderBy<?> orderBy) {
-    return new CQueryOrderBy(desc, orderBy).parseInternal();
+  static String parse(DeployParser parser, BeanDescriptor<?> desc, OrderBy<?> orderBy) {
+    return new CQueryOrderBy(parser, desc, orderBy).parseInternal();
   }
 
-  private CQueryOrderBy(BeanDescriptor<?> desc, OrderBy<?> orderBy) {
+  private CQueryOrderBy(DeployParser parser, BeanDescriptor<?> desc, OrderBy<?> orderBy) {
     this.desc = desc;
+    this.parser = parser;
     this.orderBy = orderBy;
   }
 
   private String parseInternal() {
-    StringBuilder sb = new StringBuilder();
     List<Property> properties = orderBy.getProperties();
     if (properties.isEmpty()) {
       // order by clause removed by filterMany()
       return null;
     }
+    var append = new StringAppend(parser);
     for (int i = 0; i < properties.size(); i++) {
       if (i > 0) {
-        sb.append(", ");
+        append.append(", ");
       }
-      Property p = properties.get(i);
-      String expression = parseProperty(p);
-      sb.append(expression);
+      parseProperty(properties.get(i), append);
     }
-    return sb.toString();
+    return append.toString();
   }
 
-  private String parseProperty(Property p) {
-    String propName = p.getProperty();
-    ElPropertyValue el = desc.elGetValue(propName);
-    if (el == null) {
-      return p.toStringFormat();
+  private void parseProperty(Property p, StringAppend append) {
+    ElPropertyValue el = desc.elGetValue(p.getProperty());
+    if (el != null) {
+      BeanProperty beanProperty = el.beanProperty();
+      if (beanProperty instanceof BeanPropertyAssoc<?>) {
+        BeanPropertyAssoc<?> ap = (BeanPropertyAssoc<?>) beanProperty;
+        IdBinder idBinder = ap.targetDescriptor().idBinder();
+        append.parse(idBinder.orderBy(el.elName(), p.isAscending()));
+        return;
+      }
+    }
+    p.toStringFormat(append);
+  }
+
+  private static class StringAppend implements OrderBy.Append {
+
+    private final StringBuilder builder = new StringBuilder();
+    private final DeployParser parser;
+
+    StringAppend(DeployParser parser) {
+      this.parser = parser;
+    }
+
+    @Override
+    public String toString() {
+      return builder.toString();
+    }
+
+    @Override
+    public OrderBy.Append property(String property) {
+      builder.append(parser.property(property));
+      return this;
+    }
+
+    @Override
+    public OrderBy.Append append(String literal) {
+      builder.append(literal);
+      return this;
     }
 
-    BeanProperty beanProperty = el.beanProperty();
-    if (beanProperty instanceof BeanPropertyAssoc<?>) {
-      BeanPropertyAssoc<?> ap = (BeanPropertyAssoc<?>) beanProperty;
-      IdBinder idBinder = ap.targetDescriptor().idBinder();
-      return idBinder.orderBy(el.elName(), p.isAscending());
+    @Override
+    public OrderBy.Append parse(String raw) {
+      builder.append(parser.parse(raw));
+      return this;
     }
-    return p.toStringFormat();
   }
 }

ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryPredicates.java

@@ -7,7 +7,6 @@
 import io.ebeaninternal.api.SpiQuery;
 import io.ebeaninternal.server.bind.DataBind;
 import io.ebeaninternal.server.core.OrmQueryRequest;
-import io.ebeaninternal.server.deploy.BeanDescriptor;
 import io.ebeaninternal.server.deploy.BeanPropertyAssocMany;
 import io.ebeaninternal.server.deploy.DeployParser;
 import io.ebeaninternal.server.expression.DefaultExpressionRequest;
@@ -178,10 +177,7 @@ public void prepare(boolean buildSql) {
 
     BeanPropertyAssocMany<?> manyProperty = request.determineMany();
     if (buildSql) {
-      String logicalOrderBy = deriveOrderByWithMany(request.manyPropertyForOrderBy());
-      if (logicalOrderBy != null) {
-        dbOrderBy = deployParser.parse(logicalOrderBy);
-      }
+      dbOrderBy = deriveOrderByWithMany(deployParser, request.manyPropertyForOrderBy());
       // create a copy of the includes required to support the orderBy
       orderByIncludes = new HashSet<>(deployParser.includes());
     }
@@ -241,49 +237,47 @@ void parseTableAlias(SqlTreeAlias alias) {
     }
   }
 
-  private String parseOrderBy() {
+  private String parseOrderBy(DeployParser parser) {
     OrderBy<?> orderBy = query.getOrderBy();
     if (orderBy == null) {
       return null;
     }
-    return CQueryOrderBy.parse(request.descriptor(), orderBy);
+    return CQueryOrderBy.parse(parser, request.descriptor(), orderBy);
   }
 
   /**
    * There is a many property we need to make sure the ordering is appropriate.
    */
-  private String deriveOrderByWithMany(BeanPropertyAssocMany<?> manyProp) {
+  private String deriveOrderByWithMany(DeployParser parser, BeanPropertyAssocMany<?> manyProp) {
+    String orderBy = parseOrderBy(parser);
     if (manyProp == null) {
-      return parseOrderBy();
+      return orderBy;
     }
-    String orderBy = parseOrderBy();
-    BeanDescriptor<?> desc = request.descriptor();
-    String orderById = desc.defaultOrderBy();
+    String orderById = parser.parse(request.descriptor().defaultOrderBy());
     if (orderBy == null) {
       orderBy = orderById;
     }
     // check for default ordering on the many property...
     String manyOrderBy = manyProp.fetchOrderBy();
     if (manyOrderBy != null) {
-      orderBy = orderBy + ", " + CQueryBuilder.prefixOrderByFields(manyProp.name(), manyOrderBy);
+      orderBy = orderBy + ", " + parser.parse(CQueryBuilder.prefixOrderByFields(manyProp.name(), manyOrderBy));
     }
     if (request.isFindById()) {
       // only one master bean so should be fine...
       return orderBy;
     }
-    if (orderBy.startsWith(orderById)) {
-      return orderBy;
-    }
     // more than one top level row may be returned so
     // we need to make sure their is an order by on the
     // top level first (to ensure master/detail construction).
-    int manyPos = orderBy.indexOf(manyProp.name());
-    int idPos = orderBy.indexOf(" " + orderById);
+    int idPos = orderBy.indexOf(orderById);
+    if (idPos == 0) {
+      return orderBy;
+    }
+    int manyPos = orderBy.indexOf("${" + manyProp.name() + "}");
     if (manyPos == -1) {
       // no ordering of the many
       if (idPos == -1) {
         // append the orderById so that master level objects are ordered
-        // even if the orderBy is not unique for the master object
         return orderBy + ", " + orderById;
       }
       // orderById is already in the order by clause
@@ -292,7 +286,7 @@ private String deriveOrderByWithMany(BeanPropertyAssocMany<?> manyProp) {
     if (idPos == -1 || idPos >= manyPos) {
       if (idPos > manyPos) {
         // there was an error with the order by...
-        String msg = "A Query on [" + desc + "] includes a join to a 'many' association [" + manyProp.name()
+        String msg = "A Query on [" + request.descriptor() + "] includes a join to a 'many' association [" + manyProp.name()
         + "] with an incorrect orderBy [" + orderBy + "]. The id property [" + orderById
         + "] must come before the many property [" + manyProp.name() + "] in the orderBy."
         + " Ebean has automatically modified the orderBy clause to do this.";

ebean-test/src/test/java/org/tests/basic/type/BSimpleWithGen.java

@@ -14,6 +14,8 @@ public class BSimpleWithGen {
 
   private String name;
 
+  private String desc;
+
   @Transient
   private Map<String, List<String>> someMap;
 
@@ -45,4 +47,11 @@ public void setSomeMap(Map<String, List<String>> someMap) {
     this.someMap = someMap;
   }
 
+  public String getDesc() {
+    return desc;
+  }
+
+  public void setDesc(String desc) {
+    this.desc = desc;
+  }
 }

ebean-test/src/test/java/org/tests/basic/type/TestTransientMap.java

@@ -1,5 +1,6 @@
 package org.tests.basic.type;
 
+import io.ebean.Query;
 import io.ebean.xtest.BaseTestCase;
 import io.ebean.DB;
 import org.junit.jupiter.api.Test;
@@ -11,22 +12,32 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 
-public class TestTransientMap extends BaseTestCase {
+class TestTransientMap extends BaseTestCase {
 
   @Test
-  public void testMe() {
+  void testMe() {
 
     Map<String, List<String>> map = new HashMap<>();
     map.put("foo", new ArrayList<>());
 
     BSimpleWithGen b = new BSimpleWithGen("blah");
     b.setSomeMap(map);
+    b.setDesc("hi");
     DB.save(b);
 
     final BSimpleWithGen found = DB.find(BSimpleWithGen.class, b.getId());
     assertThat(found.getName()).isEqualTo("blah");
     assertThat(found.getSomeMap()).isNull();
 
+    Query<BSimpleWithGen> query = DB.find(BSimpleWithGen.class)
+      .where().startsWith("desc", "h")
+      .orderBy().desc("desc");
+
+    var list = query.findList();
+    assertThat(list).hasSize(1);
+    assertThat(query.getGeneratedSql()).contains("where t0.desc like");
+    assertThat(query.getGeneratedSql()).contains("order by t0.desc desc");
+
     DB.delete(b);
   }
 }