From 3c43a1542277960c5adc471f6783bd87250d87cb Mon Sep 17 00:00:00 2001 From: Jose Date: Fri, 1 Jul 2022 16:34:12 +0200 Subject: [PATCH] Support Panache Sort Null Precedence Allow users sorting by either nulls first or nulls last. Example of usage: ```java Sort.by("foo", Sort.Direction.Ascending, Sort.NullPrecedence.NULLS_FIRST); ``` (See more examples in tests) This PR also adds support of nulls handling for the Spring Data JPA Quarkus extension: it translates the Spring Sort object to the new Panache Sort object. Finally, as the Panache Sort API is also used by the Mongo Quarkus extension and Mongo does not easily support sorting by nulls, I decided to simply print a WARNING message if users try to use it. Fix https://github.com/quarkusio/quarkus/issues/26172 --- .../main/asciidoc/hibernate-orm-panache.adoc | 5 +- .../test/JpaOperationsSortTest.java | 12 +++ .../runtime/ReactiveMongoOperations.java | 3 + .../common/runtime/MongoOperations.java | 3 + .../java/io/quarkus/panache/common/Sort.java | 85 ++++++++++++++++++- .../common/runtime/PanacheJpaUtil.java | 12 ++- .../BasicTypeDataRepositoryTest.java | 23 +++++ .../spring/data/runtime/TypesConverter.java | 17 +++- .../io/quarkus/it/panache/TestEndpoint.java | 24 ++++++ .../it/panache/PanacheFunctionalityTest.java | 5 ++ .../it/panache/reactive/TestEndpoint.java | 27 ++++++ .../reactive/PanacheFunctionalityTest.java | 5 ++ .../resources/PersonRepositoryResource.java | 7 ++ .../panache/MongodbPanacheResourceTest.java | 7 ++ 14 files changed, 230 insertions(+), 5 deletions(-) diff --git a/docs/src/main/asciidoc/hibernate-orm-panache.adoc b/docs/src/main/asciidoc/hibernate-orm-panache.adoc index 250b1f18024f45..50520be3c429cc 100644 --- a/docs/src/main/asciidoc/hibernate-orm-panache.adoc +++ b/docs/src/main/asciidoc/hibernate-orm-panache.adoc @@ -606,9 +606,12 @@ List persons = Person.list(Sort.by("name").and("birth")); // and with more restrictions List persons = Person.list("status", Sort.by("name").and("birth"), Status.Alive); + +// and list first the entries with null values in the field "birth" +List persons = Person.list(Sort.by("birth", Sort.NullPrecedence.NULLS_FIRST)); ---- -The `Sort` class has plenty of methods for adding columns and specifying sort direction. +The `Sort` class has plenty of methods for adding columns and specifying sort direction or the null precedence. === Simplified queries diff --git a/extensions/panache/hibernate-orm-panache/deployment/src/test/java/io/quarkus/hibernate/orm/panache/deployment/test/JpaOperationsSortTest.java b/extensions/panache/hibernate-orm-panache/deployment/src/test/java/io/quarkus/hibernate/orm/panache/deployment/test/JpaOperationsSortTest.java index 48e75fd971053f..30e13c7a45c5f6 100644 --- a/extensions/panache/hibernate-orm-panache/deployment/src/test/java/io/quarkus/hibernate/orm/panache/deployment/test/JpaOperationsSortTest.java +++ b/extensions/panache/hibernate-orm-panache/deployment/src/test/java/io/quarkus/hibernate/orm/panache/deployment/test/JpaOperationsSortTest.java @@ -27,4 +27,16 @@ public void testEmptySortEmptyYieldsEmptyString() { assertEquals("", PanacheJpaUtil.toOrderBy(emptySort)); } + @Test + public void testSortByNullsFirst() { + Sort emptySort = Sort.by("foo", Sort.Direction.Ascending, Sort.NullPrecedence.NULLS_FIRST); + assertEquals(" ORDER BY foo NULLS FIRST", PanacheJpaUtil.toOrderBy(emptySort)); + } + + @Test + public void testSortByNullsLast() { + Sort emptySort = Sort.by("foo", Sort.Direction.Descending, Sort.NullPrecedence.NULLS_LAST); + assertEquals(" ORDER BY foo DESC NULLS LAST", PanacheJpaUtil.toOrderBy(emptySort)); + } + } diff --git a/extensions/panache/mongodb-panache-common/runtime/src/main/java/io/quarkus/mongodb/panache/common/reactive/runtime/ReactiveMongoOperations.java b/extensions/panache/mongodb-panache-common/runtime/src/main/java/io/quarkus/mongodb/panache/common/reactive/runtime/ReactiveMongoOperations.java index 1bc547dd25f341..2ba4c3a035b3fa 100644 --- a/extensions/panache/mongodb-panache-common/runtime/src/main/java/io/quarkus/mongodb/panache/common/reactive/runtime/ReactiveMongoOperations.java +++ b/extensions/panache/mongodb-panache-common/runtime/src/main/java/io/quarkus/mongodb/panache/common/reactive/runtime/ReactiveMongoOperations.java @@ -567,6 +567,9 @@ private Document sortToDocument(Sort sort) { Document sortDoc = new Document(); for (Sort.Column col : sort.getColumns()) { sortDoc.append(col.getName(), col.getDirection() == Sort.Direction.Ascending ? 1 : -1); + if (col.getNullPrecedence() != null) { + throw new UnsupportedOperationException("Cannot sort by nulls first or nulls last"); + } } return sortDoc; } diff --git a/extensions/panache/mongodb-panache-common/runtime/src/main/java/io/quarkus/mongodb/panache/common/runtime/MongoOperations.java b/extensions/panache/mongodb-panache-common/runtime/src/main/java/io/quarkus/mongodb/panache/common/runtime/MongoOperations.java index bca0e045f31fbf..27ef1a5c68452d 100644 --- a/extensions/panache/mongodb-panache-common/runtime/src/main/java/io/quarkus/mongodb/panache/common/runtime/MongoOperations.java +++ b/extensions/panache/mongodb-panache-common/runtime/src/main/java/io/quarkus/mongodb/panache/common/runtime/MongoOperations.java @@ -638,6 +638,9 @@ private Document sortToDocument(Sort sort) { Document sortDoc = new Document(); for (Sort.Column col : sort.getColumns()) { sortDoc.append(col.getName(), col.getDirection() == Sort.Direction.Ascending ? 1 : -1); + if (col.getNullPrecedence() != null) { + throw new UnsupportedOperationException("Cannot sort by nulls first or nulls last"); + } } return sortDoc; } diff --git a/extensions/panache/panache-common/runtime/src/main/java/io/quarkus/panache/common/Sort.java b/extensions/panache/panache-common/runtime/src/main/java/io/quarkus/panache/common/Sort.java index 4dfafee123bbb9..19fcb713c1fd8a 100644 --- a/extensions/panache/panache-common/runtime/src/main/java/io/quarkus/panache/common/Sort.java +++ b/extensions/panache/panache-common/runtime/src/main/java/io/quarkus/panache/common/Sort.java @@ -40,9 +40,24 @@ public enum Direction { Descending; } + /** + * Represents the order of null columns. + */ + public enum NullPrecedence { + /** + * Sort by the null columns listed first in the resulting query. + */ + NULLS_FIRST, + /** + * Sort by the null columns listed last in the resulting query. + */ + NULLS_LAST; + } + public static class Column { private String name; private Direction direction; + private NullPrecedence nullPrecedence; public Column(String name) { this(name, Direction.Ascending); @@ -53,6 +68,12 @@ public Column(String name, Direction direction) { this.direction = direction; } + public Column(String name, Direction direction, NullPrecedence nullPrecedence) { + this.name = name; + this.direction = direction; + this.nullPrecedence = nullPrecedence; + } + public String getName() { return name; } @@ -68,6 +89,14 @@ public Direction getDirection() { public void setDirection(Direction direction) { this.direction = direction; } + + public NullPrecedence getNullPrecedence() { + return nullPrecedence; + } + + public void setNullPrecedence(NullPrecedence nullPrecedence) { + this.nullPrecedence = nullPrecedence; + } } private List columns = new ArrayList<>(); @@ -91,7 +120,7 @@ public static Sort by(String column) { * Sort by the given column, in the given order. * * @param column the column to sort on, in the given order. - * @param direction the direction to sort on + * @param direction the direction to sort on. * @return a new Sort instance which sorts on the given column in the given order. * @see #by(String) * @see #by(String...) @@ -100,6 +129,33 @@ public static Sort by(String column, Direction direction) { return new Sort().and(column, direction); } + /** + * Sort by the given column, in the given order and with null handling. + * + * @param column the column to sort on, in the given order. + * @param nullPrecedence the null handling to use. + * @return a new Sort instance which sorts on the given column in the given order. + * @see #by(String) + * @see #by(String...) + */ + public static Sort by(String column, NullPrecedence nullPrecedence) { + return by(column, Direction.Ascending, nullPrecedence); + } + + /** + * Sort by the given column, in the given order and with null handling. + * + * @param column the column to sort on, in the given order. + * @param direction the direction to sort on. + * @param nullPrecedence the null handling to use. + * @return a new Sort instance which sorts on the given column in the given order. + * @see #by(String) + * @see #by(String...) + */ + public static Sort by(String column, Direction direction, NullPrecedence nullPrecedence) { + return new Sort().and(column, direction, nullPrecedence); + } + /** * Sort by the given columns, in ascending order. Equivalent to {@link #ascending(String...)}. * @@ -202,6 +258,7 @@ public Sort and(String name) { * Adds a sort column, in the given order. * * @param name the new column to sort on, in the given order. + * @param direction the direction to sort on. * @return this instance, modified. * @see #and(String) */ @@ -210,6 +267,32 @@ public Sort and(String name, Direction direction) { return this; } + /** + * Adds a sort column, in the given order. + * + * @param name the new column to sort on, in the given order. + * @param nullPrecedence the null handling to use. + * @return this instance, modified. + * @see #and(String) + */ + public Sort and(String name, NullPrecedence nullPrecedence) { + return and(name, Direction.Ascending, nullPrecedence); + } + + /** + * Adds a sort column, in the given order. + * + * @param name the new column to sort on, in the given order. + * @param direction the direction to sort on. + * @param nullPrecedence the null handling to use. + * @return this instance, modified. + * @see #and(String) + */ + public Sort and(String name, Direction direction, NullPrecedence nullPrecedence) { + columns.add(new Column(name, direction, nullPrecedence)); + return this; + } + /** * Get the sort columns * diff --git a/extensions/panache/panache-hibernate-common/runtime/src/main/java/io/quarkus/panache/hibernate/common/runtime/PanacheJpaUtil.java b/extensions/panache/panache-hibernate-common/runtime/src/main/java/io/quarkus/panache/hibernate/common/runtime/PanacheJpaUtil.java index 08a930cbaef3f8..06b6eba3b67216 100644 --- a/extensions/panache/panache-hibernate-common/runtime/src/main/java/io/quarkus/panache/hibernate/common/runtime/PanacheJpaUtil.java +++ b/extensions/panache/panache-hibernate-common/runtime/src/main/java/io/quarkus/panache/hibernate/common/runtime/PanacheJpaUtil.java @@ -177,8 +177,18 @@ public static String toOrderBy(Sort sort) { if (i > 0) sb.append(" , "); sb.append(column.getName()); - if (column.getDirection() != Sort.Direction.Ascending) + if (column.getDirection() != Sort.Direction.Ascending) { sb.append(" DESC"); + } + + if (column.getNullPrecedence() != null) { + if (column.getNullPrecedence() == Sort.NullPrecedence.NULLS_FIRST) { + sb.append(" NULLS FIRST"); + } else { + sb.append(" NULLS LAST"); + } + } + } return sb.toString(); } diff --git a/extensions/spring-data-jpa/deployment/src/test/java/io/quarkus/spring/data/deployment/BasicTypeDataRepositoryTest.java b/extensions/spring-data-jpa/deployment/src/test/java/io/quarkus/spring/data/deployment/BasicTypeDataRepositoryTest.java index 9ad3e63ca66084..512d9a2b574931 100644 --- a/extensions/spring-data-jpa/deployment/src/test/java/io/quarkus/spring/data/deployment/BasicTypeDataRepositoryTest.java +++ b/extensions/spring-data-jpa/deployment/src/test/java/io/quarkus/spring/data/deployment/BasicTypeDataRepositoryTest.java @@ -29,6 +29,7 @@ import org.springframework.data.domain.Page; import org.springframework.data.domain.PageRequest; import org.springframework.data.domain.Slice; +import org.springframework.data.domain.Sort; import io.quarkus.test.QuarkusUnitTest; @@ -124,6 +125,28 @@ public void testMapInterfacesUsingSlice() { assertEquals(0, nextPage.getNumberOfElements()); } + @Test + @Order(8) + @Transactional + public void testListOrderByNullHandling() throws MalformedURLException { + // Insert row with null in url + UUID uuidForTheNullUrlRecord = UUID.randomUUID(); + BasicTypeData item = populateData(new BasicTypeData()); + item.setUuid(uuidForTheNullUrlRecord); + item.setUrl(null); + repo.save(item); + // At this moment, there should be at least two records, one inserted in the first test, + // and the second with the "url" column to null. + + // Check Nulls first + List list = repo.findAll(Sort.by(Sort.Order.by("url").nullsFirst())); + assertEquals(uuidForTheNullUrlRecord, list.get(0).getUuid()); + + // Check Nulls last + list = repo.findAll(Sort.by(Sort.Order.by("url").nullsLast())); + assertEquals(uuidForTheNullUrlRecord, list.get(list.size() - 1).getUuid()); + } + private BasicTypeData populateData(BasicTypeData basicTypeData) throws MalformedURLException { basicTypeData.setDoubleValue(Math.PI); basicTypeData.setBigDecimalValue(BigDecimal.valueOf(Math.PI * 2.0)); diff --git a/extensions/spring-data-jpa/runtime/src/main/java/io/quarkus/spring/data/runtime/TypesConverter.java b/extensions/spring-data-jpa/runtime/src/main/java/io/quarkus/spring/data/runtime/TypesConverter.java index 5963a8e894803e..84bfd77757fb99 100644 --- a/extensions/spring-data-jpa/runtime/src/main/java/io/quarkus/spring/data/runtime/TypesConverter.java +++ b/extensions/spring-data-jpa/runtime/src/main/java/io/quarkus/spring/data/runtime/TypesConverter.java @@ -18,14 +18,14 @@ public static io.quarkus.panache.common.Sort toPanacheSort(org.springframework.d } org.springframework.data.domain.Sort.Order firstOrder = orders.get(0); - Sort result = Sort.by(firstOrder.getProperty(), getDirection(firstOrder)); + Sort result = Sort.by(firstOrder.getProperty(), getDirection(firstOrder), getNullPrecedence(firstOrder)); if (orders.size() == 1) { return result; } for (int i = 1; i < orders.size(); i++) { org.springframework.data.domain.Sort.Order order = orders.get(i); - result = result.and(order.getProperty(), getDirection(order)); + result = result.and(order.getProperty(), getDirection(order), getNullPrecedence(order)); } return result; } @@ -35,6 +35,19 @@ private static Sort.Direction getDirection(org.springframework.data.domain.Sort. : Sort.Direction.Descending; } + private static Sort.NullPrecedence getNullPrecedence(org.springframework.data.domain.Sort.Order order) { + if (order.getNullHandling() != null) { + switch (order.getNullHandling()) { + case NULLS_FIRST: + return Sort.NullPrecedence.NULLS_FIRST; + case NULLS_LAST: + return Sort.NullPrecedence.NULLS_LAST; + } + } + + return null; + } + public static io.quarkus.panache.common.Page toPanachePage(org.springframework.data.domain.Pageable pageable) { // only generate queries with paging if param is actually paged (ex. Unpaged.INSTANCE is a Pageable not paged) if (pageable.isPaged()) { diff --git a/integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/TestEndpoint.java b/integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/TestEndpoint.java index 2daf1aa31ebd70..90ff34b95be9fd 100644 --- a/integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/TestEndpoint.java +++ b/integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/TestEndpoint.java @@ -1519,4 +1519,28 @@ public String testFilterWithCollections() { return "OK"; } + + @GET + @Path("testSortByNullPrecedence") + @Transactional + public String testSortByNullPrecedence() { + Person.deleteAll(); + + Person stefPerson = new Person(); + stefPerson.name = "Stef"; + stefPerson.persist(); + + Person josePerson = new Person(); + josePerson.name = null; + josePerson.persist(); + + List persons = Person.findAll(Sort.by("name", Sort.NullPrecedence.NULLS_FIRST)).list(); + assertEquals(josePerson.id, persons.get(0).id); + persons = Person.findAll(Sort.by("name", Sort.NullPrecedence.NULLS_LAST)).list(); + assertEquals(josePerson.id, persons.get(persons.size() - 1).id); + + Person.deleteAll(); + + return "OK"; + } } diff --git a/integration-tests/hibernate-orm-panache/src/test/java/io/quarkus/it/panache/PanacheFunctionalityTest.java b/integration-tests/hibernate-orm-panache/src/test/java/io/quarkus/it/panache/PanacheFunctionalityTest.java index 9beeb594209dbf..8b8a84ef9d5ed6 100644 --- a/integration-tests/hibernate-orm-panache/src/test/java/io/quarkus/it/panache/PanacheFunctionalityTest.java +++ b/integration-tests/hibernate-orm-panache/src/test/java/io/quarkus/it/panache/PanacheFunctionalityTest.java @@ -85,6 +85,11 @@ public void testFilterWithCollections() { RestAssured.when().get("/test/testFilterWithCollections").then().body(is("OK")); } + @Test + public void testSortByNullPrecedence() { + RestAssured.when().get("/test/testSortByNullPrecedence").then().body(is("OK")); + } + @Test public void testJaxbAnnotationTransfer() { RestAssured.when() diff --git a/integration-tests/hibernate-reactive-panache/src/main/java/io/quarkus/it/panache/reactive/TestEndpoint.java b/integration-tests/hibernate-reactive-panache/src/main/java/io/quarkus/it/panache/reactive/TestEndpoint.java index 4a9ca0cb80f737..6a8931ea36564a 100644 --- a/integration-tests/hibernate-reactive-panache/src/main/java/io/quarkus/it/panache/reactive/TestEndpoint.java +++ b/integration-tests/hibernate-reactive-panache/src/main/java/io/quarkus/it/panache/reactive/TestEndpoint.java @@ -1878,4 +1878,31 @@ public Uni testBug9036() { }).map(v -> "OK"); }); } + + @GET + @Path("testSortByNullPrecedence") + @ReactiveTransactional + public Uni testSortByNullPrecedence() { + return Person.deleteAll() + .flatMap(v -> { + Person stefPerson = new Person(); + stefPerson.name = "Stef"; + stefPerson.uniqueName = "stef"; + + Person josePerson = new Person(); + josePerson.name = null; + josePerson.uniqueName = "jose"; + return Person.persist(stefPerson, josePerson); + }).flatMap(p -> { + return Person.findAll(Sort.by("name", Sort.NullPrecedence.NULLS_FIRST)).list(); + }).flatMap(list -> { + assertEquals("jose", ((Person) list.get(0)).uniqueName); + + return Person.findAll(Sort.by("name", Sort.NullPrecedence.NULLS_LAST)).list(); + }).flatMap(list -> { + assertEquals("jose", ((Person) list.get(list.size() - 1)).uniqueName); + + return Person.deleteAll(); + }).map(v -> "OK"); + } } diff --git a/integration-tests/hibernate-reactive-panache/src/test/java/io/quarkus/it/panache/reactive/PanacheFunctionalityTest.java b/integration-tests/hibernate-reactive-panache/src/test/java/io/quarkus/it/panache/reactive/PanacheFunctionalityTest.java index ca07721d4cb164..bb1746121b4ade 100644 --- a/integration-tests/hibernate-reactive-panache/src/test/java/io/quarkus/it/panache/reactive/PanacheFunctionalityTest.java +++ b/integration-tests/hibernate-reactive-panache/src/test/java/io/quarkus/it/panache/reactive/PanacheFunctionalityTest.java @@ -151,6 +151,11 @@ public void testBug9036() { RestAssured.when().get("/test/9036").then().body(is("OK")); } + @Test + public void testSortByNullPrecedence() { + RestAssured.when().get("/test/testSortByNullPrecedence").then().body(is("OK")); + } + @DisabledOnNativeImage @ReactiveTransactional @Test diff --git a/integration-tests/mongodb-panache/src/main/java/io/quarkus/it/mongodb/panache/person/resources/PersonRepositoryResource.java b/integration-tests/mongodb-panache/src/main/java/io/quarkus/it/mongodb/panache/person/resources/PersonRepositoryResource.java index 3a614f7f29a056..3d0219aa63a5d6 100644 --- a/integration-tests/mongodb-panache/src/main/java/io/quarkus/it/mongodb/panache/person/resources/PersonRepositoryResource.java +++ b/integration-tests/mongodb-panache/src/main/java/io/quarkus/it/mongodb/panache/person/resources/PersonRepositoryResource.java @@ -104,4 +104,11 @@ public Response rename(@QueryParam("previousName") String previousName, @QueryPa personRepository.update("lastname", newName).where("lastname", previousName); return Response.ok().build(); } + + @GET + @Path("/search/by/nulls/precedence") + public Response searchPersonsByNullsPrecedence() { + personRepository.listAll(Sort.by("lastname", Sort.NullPrecedence.NULLS_FIRST)); + return Response.ok().build(); + } } diff --git a/integration-tests/mongodb-panache/src/test/java/io/quarkus/it/mongodb/panache/MongodbPanacheResourceTest.java b/integration-tests/mongodb-panache/src/test/java/io/quarkus/it/mongodb/panache/MongodbPanacheResourceTest.java index 06c8ed04e0b617..b6bfc702e3040c 100644 --- a/integration-tests/mongodb-panache/src/test/java/io/quarkus/it/mongodb/panache/MongodbPanacheResourceTest.java +++ b/integration-tests/mongodb-panache/src/test/java/io/quarkus/it/mongodb/panache/MongodbPanacheResourceTest.java @@ -11,6 +11,7 @@ import java.util.GregorianCalendar; import java.util.List; +import org.apache.http.HttpStatus; import org.hamcrest.CoreMatchers; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -67,6 +68,12 @@ public void testPersonRepository() { callPersonEndpoint("/persons/repository"); } + @Test + public void testShouldThrowExceptionWhenUsingNullsPrecedence() { + get("/persons/repository/search/by/nulls/precedence") + .then().statusCode(HttpStatus.SC_INTERNAL_SERVER_ERROR); + } + private void callBookEndpoint(String endpoint) { RestAssured.defaultParser = Parser.JSON; RestAssured.config