From 70cfc77b5bfcfcd4374baac0aaf82b1d92264311 Mon Sep 17 00:00:00 2001 From: Rob Bygrave Date: Thu, 4 Jul 2024 00:01:00 +1200 Subject: [PATCH] #3433 - Entity property called desc used with query order by desc generates bad sql --- ebean-api/src/main/java/io/ebean/OrderBy.java | 96 +++++++++++++------ .../server/query/CQueryOrderBy.java | 72 ++++++++++---- .../server/query/CQueryPredicates.java | 34 +++---- .../org/tests/basic/type/BSimpleWithGen.java | 9 ++ .../tests/basic/type/TestTransientMap.java | 15 ++- .../query/orderby/TestOrderByWithMany.java | 5 +- 6 files changed, 158 insertions(+), 73 deletions(-) diff --git a/ebean-api/src/main/java/io/ebean/OrderBy.java b/ebean-api/src/main/java/io/ebean/OrderBy.java index 68e5acbb53..f7b5ebcfe0 100644 --- a/ebean-api/src/main/java/io/ebean/OrderBy.java +++ b/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 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); } } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryOrderBy.java b/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryOrderBy.java index 5c832f4430..692993580e 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryOrderBy.java +++ b/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 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(); } } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryPredicates.java b/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryPredicates.java index 2f4823f196..821c9ea318 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/query/CQueryPredicates.java +++ b/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."; diff --git a/ebean-test/src/test/java/org/tests/basic/type/BSimpleWithGen.java b/ebean-test/src/test/java/org/tests/basic/type/BSimpleWithGen.java index 75326d3b3b..97d3fbb526 100644 --- a/ebean-test/src/test/java/org/tests/basic/type/BSimpleWithGen.java +++ b/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> someMap; @@ -45,4 +47,11 @@ public void setSomeMap(Map> someMap) { this.someMap = someMap; } + public String getDesc() { + return desc; + } + + public void setDesc(String desc) { + this.desc = desc; + } } diff --git a/ebean-test/src/test/java/org/tests/basic/type/TestTransientMap.java b/ebean-test/src/test/java/org/tests/basic/type/TestTransientMap.java index fb38d7b454..ab54d46e8b 100644 --- a/ebean-test/src/test/java/org/tests/basic/type/TestTransientMap.java +++ b/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> 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 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); } } diff --git a/ebean-test/src/test/java/org/tests/query/orderby/TestOrderByWithMany.java b/ebean-test/src/test/java/org/tests/query/orderby/TestOrderByWithMany.java index 028b80d2d9..b21249f4a9 100644 --- a/ebean-test/src/test/java/org/tests/query/orderby/TestOrderByWithMany.java +++ b/ebean-test/src/test/java/org/tests/query/orderby/TestOrderByWithMany.java @@ -11,6 +11,7 @@ import java.util.List; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertTrue; public class TestOrderByWithMany extends BaseTestCase { @@ -64,7 +65,7 @@ private void checkWithBuiltInManyBasic() { String sql = query.getGeneratedSql(); - assertTrue(sql.contains("order by t0.id, t1.id asc, t1.order_qty asc, t1.cretime desc")); + assertThat(sql).contains("order by t0.id, t1.id asc, t1.order_qty asc, t1.cretime desc"); } private void checkWithBuiltInMany() { @@ -151,6 +152,6 @@ private void checkAlreadyIncluded2() { String sql = query.getGeneratedSql(); // prepend id in order by - assertTrue(sql.contains("order by t0.order_date, t0.id, t1.ship_time desc")); + assertThat(sql).contains("order by t0.order_date, t0.id, t1.ship_time desc"); } }