From ae261d378249fa0c55939d17fb198ff6c5b19e6a Mon Sep 17 00:00:00 2001 From: Robin Bygrave Date: Tue, 26 Apr 2016 22:54:34 +1200 Subject: [PATCH] #660 - findPagedList() does not handle raw sql order by --- src/main/java/com/avaje/ebean/OrderBy.java | 53 ++++++++++---- .../server/deploy/BeanDescriptor.java | 5 ++ .../java/com/avaje/ebean/PropertyTest.java | 43 ++++++++++++ .../tests/rawsql/TestRawSqlOrmQuery.java | 70 +++++++++++++++++++ .../tests/unitinternal/TestOrderByParse.java | 22 ++++++ 5 files changed, 181 insertions(+), 12 deletions(-) create mode 100644 src/test/java/com/avaje/ebean/PropertyTest.java diff --git a/src/main/java/com/avaje/ebean/OrderBy.java b/src/main/java/com/avaje/ebean/OrderBy.java index a045d3812..4ee0f5b5c 100644 --- a/src/main/java/com/avaje/ebean/OrderBy.java +++ b/src/main/java/com/avaje/ebean/OrderBy.java @@ -221,24 +221,39 @@ public final class OrderBy implements Serializable { private boolean ascending; + private String nulls; + + private String highLow; + public Property(String property, boolean ascending) { this.property = property; this.ascending = ascending; } + public Property(String property, boolean ascending, String nulls, String highLow) { + this.property = property; + this.ascending = ascending; + this.nulls = nulls; + this.highLow = highLow; + } + /** * Return a copy of this Property with the path trimmed. */ public Property copyWithTrim(String path) { - return new Property(property.substring(path.length() + 1), ascending); + return new Property(property.substring(path.length() + 1), ascending, nulls, highLow); } + @Override public int hashCode() { int hc = property.hashCode(); hc = hc * 31 + (ascending ? 0 : 1); + hc = hc * 31 + (nulls == null ? 0 : nulls.hashCode()); + hc = hc * 31 + (highLow == null ? 0 : highLow.hashCode()); return hc; } - + + @Override public boolean equals(Object obj) { if (obj == this) { return true; @@ -246,10 +261,11 @@ public final class OrderBy implements Serializable { if (!(obj instanceof Property)) { return false; } - Property e = (Property) obj; - return e.ascending == ascending - && e.property.equals(property); + if (ascending != e.ascending) return false; + if (!property.equals(e.property)) return false; + if (nulls != null ? !nulls.equals(e.nulls) : e.nulls != null) return false; + return highLow != null ? highLow.equals(e.highLow) : e.highLow == null; } public String toString() { @@ -257,10 +273,20 @@ public final class OrderBy implements Serializable { } public String toStringFormat() { - if (ascending) { - return property; + if (nulls == null) { + if (ascending) { + return property; + } else { + return property + " desc"; + } } else { - return property + " desc"; + StringBuilder sb = new StringBuilder(); + sb.append(property); + if (!ascending) { + sb.append(" ").append("desc"); + } + sb.append(" ").append(nulls).append(" ").append(highLow); + return sb.toString(); } } @@ -282,7 +308,7 @@ public final class OrderBy implements Serializable { * Return a copy of this property. */ public Property copy() { - return new Property(property, ascending); + return new Property(property, ascending, nulls, highLow); } /** @@ -323,7 +349,6 @@ public final class OrderBy implements Serializable { String[] chunks = orderByClause.split(","); for (int i = 0; i < chunks.length; i++) { - String[] pairs = chunks[i].split(" "); Property p = parseProperty(pairs); if (p != null) { @@ -353,8 +378,12 @@ public final class OrderBy implements Serializable { boolean asc = isAscending(wordList.get(1)); return new Property(wordList.get(0), asc); } - String m = "Expecting a max of 2 words in [" + Arrays.toString(pairs) - + "] but got " + wordList.size(); + if (wordList.size() == 4) { + // nulls high or nulls low as 3rd and 4th + boolean asc = isAscending(wordList.get(1)); + return new Property(wordList.get(0), asc, wordList.get(2), wordList.get(3)); + } + String m = "Expecting a 1, 2 or 4 words in [" + Arrays.toString(pairs) + "] but got " + wordList; throw new RuntimeException(m); } diff --git a/src/main/java/com/avaje/ebeaninternal/server/deploy/BeanDescriptor.java b/src/main/java/com/avaje/ebeaninternal/server/deploy/BeanDescriptor.java index a45cf92b6..98b069dbd 100644 --- a/src/main/java/com/avaje/ebeaninternal/server/deploy/BeanDescriptor.java +++ b/src/main/java/com/avaje/ebeaninternal/server/deploy/BeanDescriptor.java @@ -3,6 +3,7 @@ package com.avaje.ebeaninternal.server.deploy; import com.avaje.ebean.OrderBy; import com.avaje.ebean.PersistenceContextScope; import com.avaje.ebean.Query; +import com.avaje.ebean.RawSql; import com.avaje.ebean.SqlUpdate; import com.avaje.ebean.Transaction; import com.avaje.ebean.ValuePair; @@ -2719,6 +2720,10 @@ public class BeanDescriptor implements MetaBeanInfo, BeanType { if (idProperty != null && !idProperty.isEmbedded()) { OrderBy orderBy = query.getOrderBy(); if (orderBy == null || orderBy.isEmpty()) { + RawSql rawSql = query.getRawSql(); + if (rawSql != null) { + query.order(rawSql.getSql().getOrderBy()); + } query.order().asc(idProperty.getName()); } else if (!orderBy.containsProperty(idProperty.getName())){ query.order().asc(idProperty.getName()); diff --git a/src/test/java/com/avaje/ebean/PropertyTest.java b/src/test/java/com/avaje/ebean/PropertyTest.java new file mode 100644 index 000000000..9a89f231f --- /dev/null +++ b/src/test/java/com/avaje/ebean/PropertyTest.java @@ -0,0 +1,43 @@ +package com.avaje.ebean; + +import org.junit.Test; + +import static org.junit.Assert.*; + +public class PropertyTest { + + @Test + public void equals() throws Exception { + + assertEquals(prop("foo", true), prop("foo", true)); + } + + @Test + public void diff_basic() throws Exception { + + assertNotEquals(prop("foo", true), prop("bar", true)); + assertNotEquals(prop("foo", true), prop("foo", false)); + assertNotEquals(prop("foo", false), prop("foo", true)); + } + + @Test + public void diff_nulls() throws Exception { + + assertEquals(prop("foo", true, "nulls", "high"), prop("foo", true, "nulls", "high")); + assertEquals(prop("foo", true, "nulls", "low"), prop("foo", true, "nulls", "low")); + + assertNotEquals(prop("foo", true), prop("foo", true, "nulls", "high")); + assertNotEquals(prop("foo", true, "nulls", "high"), prop("foo", true)); + assertNotEquals(prop("foo", true, "nulls", "high"), prop("foo", true, "nulls", "low")); + assertNotEquals(prop("foo", true, "nulls", "low"), prop("foo", true, "nulls", "high")); + } + + private OrderBy.Property prop(String name, boolean asc) { + return new OrderBy.Property(name, asc, null, null); + } + + private OrderBy.Property prop(String name, boolean asc, String nulls, String highLow) { + return new OrderBy.Property(name, asc, nulls, highLow); + } + +} \ No newline at end of file diff --git a/src/test/java/com/avaje/tests/rawsql/TestRawSqlOrmQuery.java b/src/test/java/com/avaje/tests/rawsql/TestRawSqlOrmQuery.java index 71d90a4cf..44152f309 100644 --- a/src/test/java/com/avaje/tests/rawsql/TestRawSqlOrmQuery.java +++ b/src/test/java/com/avaje/tests/rawsql/TestRawSqlOrmQuery.java @@ -3,6 +3,7 @@ package com.avaje.tests.rawsql; import java.util.List; import java.util.concurrent.ExecutionException; +import com.avaje.tests.model.basic.Order; import org.junit.Assert; import org.junit.Test; @@ -17,6 +18,8 @@ import com.avaje.ebean.RawSqlBuilder; import com.avaje.tests.model.basic.Customer; import com.avaje.tests.model.basic.ResetBasicData; +import static org.assertj.core.api.Assertions.assertThat; + public class TestRawSqlOrmQuery extends BaseTestCase { @Test @@ -107,4 +110,71 @@ public class TestRawSqlOrmQuery extends BaseTestCase { } } + + @Test + public void testPaging_with_existingRawSqlOrderBy_expect_id_appendToOrderBy() { + + ResetBasicData.reset(); + + RawSql rawSql = RawSqlBuilder.parse("select o.id, o.order_date, o.ship_date from o_order o order by o.ship_date desc nulls last") + .columnMapping("o.id", "id") + .columnMapping("o.order_date", "orderDate") + .columnMapping("o.ship_date", "shipDate") + .create(); + + Query query = Ebean.find(Order.class); + query.setUseCache(false).setUseQueryCache(false); + query.setRawSql(rawSql); + + query.setMaxRows(100); + query.findList(); + + assertThat(query.getGeneratedSql()).contains("order by o.ship_date desc nulls last, o.id limit 100"); + } + + @Test + public void testPaging_when_setOrderBy_expect_id_appendToOrderBy() { + + ResetBasicData.reset(); + + RawSql rawSql = RawSqlBuilder.parse("select o.id, o.order_date, o.ship_date from o_order o order by o.ship_date desc nulls last") + .columnMapping("o.id", "id") + .columnMapping("o.order_date", "orderDate") + .columnMapping("o.ship_date", "shipDate") + .create(); + + Query query = Ebean.find(Order.class); + query.setUseCache(false).setUseQueryCache(false); + query.setRawSql(rawSql); + + query.setMaxRows(100); + query.order("coalesce(shipDate, now()) desc"); + query.findList(); + + assertThat(query.getGeneratedSql()).contains("order by coalesce(o.ship_date, now()) desc, o.id limit 100"); + } + + @Test + public void testPaging_when_setOrderBy_containsId_expect_leaveAsIs() { + + ResetBasicData.reset(); + + RawSql rawSql = RawSqlBuilder.parse("select o.id, o.order_date, o.ship_date from o_order o order by o.ship_date desc nulls last") + .columnMapping("o.id", "id") + .columnMapping("o.order_date", "orderDate") + .columnMapping("o.ship_date", "shipDate") + .create(); + + Query query = Ebean.find(Order.class); + query.setUseCache(false).setUseQueryCache(false); + query.setRawSql(rawSql); + + query.setMaxRows(100); + query.order("id desc"); + PagedList pagedList = query.findPagedList(); + pagedList.getList(); + pagedList.getTotalRowCount(); + + assertThat(query.getGeneratedSql()).contains("order by o.id desc limit 100"); + } } diff --git a/src/test/java/com/avaje/tests/unitinternal/TestOrderByParse.java b/src/test/java/com/avaje/tests/unitinternal/TestOrderByParse.java index f87015977..882128e81 100644 --- a/src/test/java/com/avaje/tests/unitinternal/TestOrderByParse.java +++ b/src/test/java/com/avaje/tests/unitinternal/TestOrderByParse.java @@ -44,6 +44,28 @@ public class TestOrderByParse extends BaseTestCase { assertFalse(o1.containsProperty("junk")); } + @Test + public void parseNullsHigh() { + + OrderBy o1 = new OrderBy("id desc nulls high"); + assertTrue(o1.getProperties().size() == 1); + assertTrue(o1.getProperties().get(0).getProperty().equals("id")); + assertTrue(!o1.getProperties().get(0).isAscending()); + assertTrue(o1.toStringFormat().equals("id desc nulls high")); + } + + @Test + public void parseNullsHigh_with_second() { + + OrderBy o1 = new OrderBy("id desc nulls high, name"); + assertTrue(o1.getProperties().size() == 2); + assertTrue(o1.getProperties().get(0).getProperty().equals("id")); + assertTrue(!o1.getProperties().get(0).isAscending()); + assertTrue(o1.toStringFormat().equals("id desc nulls high, name")); + assertTrue(o1.getProperties().get(1).getProperty().equals("name")); + assertTrue(o1.getProperties().get(1).isAscending()); + } + @Test public void testParsingTwo() {