-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support Panache Sort Null Handling #26394
Conversation
I like the idea! @loicmathieu WDYT? |
I like the idea, I'll try to review the PR tomorrow or Friday. On Mongo, we should not create methods that allow to set the NullHandling if not supported, or we should throw an error not log a warning as it's a developer mistake. |
The methods are common for the non-Mongo extensions, but it makes sense to throw an exception instead. PR updated. |
extensions/panache/panache-common/runtime/src/main/java/io/quarkus/panache/common/Sort.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I think it would be nice to have this new feature documented in the Panache guides too. |
Also don't forget to change the snippet in the commit to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add some tests for MongoDB ? In existing test cases.
...ment/src/test/java/io/quarkus/hibernate/orm/panache/deployment/test/PanacheSortTestCase.java
Outdated
Show resolved
Hide resolved
extensions/panache/panache-common/runtime/src/main/java/io/quarkus/panache/common/Sort.java
Outdated
Show resolved
Hide resolved
| Can you also add some tests for MongoDB ? In existing test cases. @loicmathieu do you mean to verify that the exception is thrown when using NullPrecedence in MongoDB ? |
@Sgitario yes |
I've just updated the PR's changes with:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a bunch of JavaDoc suggestions, mainly due to the renaming of handlin -> precedence but overall looks good to me.
Can you please also update the Hibernate reactive guide Sort section: https://quarkus.io/guides/hibernate-reactive-panache#sorting
extensions/panache/panache-common/runtime/src/main/java/io/quarkus/panache/common/Sort.java
Outdated
Show resolved
Hide resolved
extensions/panache/panache-common/runtime/src/main/java/io/quarkus/panache/common/Sort.java
Outdated
Show resolved
Hide resolved
extensions/panache/panache-common/runtime/src/main/java/io/quarkus/panache/common/Sort.java
Outdated
Show resolved
Hide resolved
extensions/panache/panache-common/runtime/src/main/java/io/quarkus/panache/common/Sort.java
Outdated
Show resolved
Hide resolved
extensions/panache/panache-common/runtime/src/main/java/io/quarkus/panache/common/Sort.java
Outdated
Show resolved
Hide resolved
extensions/panache/panache-common/runtime/src/main/java/io/quarkus/panache/common/Sort.java
Outdated
Show resolved
Hide resolved
extensions/panache/panache-common/runtime/src/main/java/io/quarkus/panache/common/Sort.java
Outdated
Show resolved
Hide resolved
extensions/panache/panache-common/runtime/src/main/java/io/quarkus/panache/common/Sort.java
Outdated
Show resolved
Hide resolved
extensions/panache/panache-common/runtime/src/main/java/io/quarkus/panache/common/Sort.java
Outdated
Show resolved
Hide resolved
extensions/panache/panache-common/runtime/src/main/java/io/quarkus/panache/common/Sort.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Documentation for Panache Reactive also updated. |
This comment has been minimized.
This comment has been minimized.
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 quarkusio#26172
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for your contribution
Allow users sorting by either nulls first or nulls last.
Example of usage:
(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 #26172