Conversation
| public record Sort<T>(String property, | ||
| boolean isAscending, | ||
| boolean ignoreCase, | ||
| Boolean orderNullsFirst) { |
There was a problem hiding this comment.
I guess you want to include this in Jakarta Persistence, right?
There was a problem hiding this comment.
I guess you want to include this in Jakarta Persistence, right?
Yes, Jakarta Persistence has it. According to an AI search, a few NoSQL databases have it as well, although you will know more the detail on that.
otaviojava
left a comment
There was a problem hiding this comment.
Null ordering is fundamentally non-portable across databases.
Given that Jakarta Data aims to be domain-centric, exposing null ordering as a first-class abstraction would promote an infrastructure concern into the domain model, which seems misaligned with its goals.
A better approach would be to treat null ordering as a provider-specific capability (e.g., via hints or extensions), rather than standardizing it in the core API.
This allows the specification to remain focused on domain concepts while still providing an escape hatch for datastore-specific behavior.
|
Do you have an example of what it would look like if this was left to specific implementations? I'm afraid this will only make user's life more complicated. |
The complexity already exists — the question is where we choose to expose it. I’m not suggesting we ignore this case. I agree there’s a risk of making advanced use cases harder if we don’t support it directly, but there is a trade-off. If we bring these concerns into the core API, we introduce a different kind of complexity: instead of making a few advanced cases harder, we make the entire API heavier for everyone. Sorting itself already has multiple non-portable dimensions, for example:
If we try to standardize all of these in the core API, it quickly becomes harder to understand and reason about than the problem it is trying to solve. IMHO: A better balance is to keep the core API focused on universally meaningful concepts (such as basic ordering), and allow provider-specific extensions or hints for cases like null handling. |
I don't see how what we proposed here makes the API heavier. If you have data without nulls or a database that isn't capable of sorting them, you use the |
| public record Sort<T>(String property, | ||
| boolean isAscending, | ||
| boolean ignoreCase, | ||
| Boolean orderNullsFirst) { |
There was a problem hiding this comment.
Not sure I'm happy with Boolean here.
Because orderNullsFirst = false isn't completely obviously the same thing as "order nulls last".
I think an enum would be more appropriate, as in JPA.
There was a problem hiding this comment.
I was wondering about that, too, when I wrote this up, whether it would be better for clarity to use an enum. I switched to an enum under 9af3253 and also included @since 1.1 which I had missed in my previous changes.
There was a problem hiding this comment.
I am not a huge fan of increasing this class, what do you think of having components instead?
There was a problem hiding this comment.
I am not a huge fan of increasing this class, what do you think of having components instead?
@otaviojava I'm not sure what you mean by having components instead. From the other part of your comment, I am guess that you want the enum moved out of the Sort class. I liked it there because you can write Sort.Nulls.FIRST which reads very well, but if you prefer it moved to the same level as Sort, I could do that. I'm not sure if that is what you are asking for though.
There was a problem hiding this comment.
I want to understand how big this Sort class can become, given that we want to put some Sort configurations in a single place.
What I would suggest is: instead of having it here, we could consider moving it to another class, such as SortNullable, and so on.
I understand we as a team want this option, but I recommend creating classes or specializations for any new configuration type rather than increasing this one.
What do you think?
There was a problem hiding this comment.
The decision to represent Sort configuration as a Java record means that it cannot be subclassed with a SortNullable. That means if we create a SortNullable then it would not be an instance of Sort, and so it would not be usable in Order.by(Sort...), and then you need Order.by(SortNullable...) even though only one out of several items that the user supplies intends to sort a nullable entity attribute and the others should have been fine as just Sort but now need to be SortNullable since one of the other items is a SortNullable. It gets worse. Sorts are obtained from the metamodel and so then the metamodel needs a way to obtain a SortNullable for something you don't actually want to be nullable so that you can combine it with things that are nullable in Order.by(SortNullable...). The end user would need to do something like this:
Order.by(_Product.price.desc().nullsLast(), // this is fine
_Product.name.asc().asSortNullable(), // confusing and annoying to user
_Product.id.asc().asSortNullable()); // confusing and annoying to userThat is just one example of where the specialization makes the API uglier and more burdensome for users instead of better.
I also want to point out that you are rightly referring to Sort as a configuration. It is perfectly normal for configuration to include some options that not all users will take advantage of. It is unnatural to subclass or copy a configuration when you introduce a new configuration property.
@otaviojava I think this is fine, as long as it's clear that support is not required. (And I think this PR already makes it clear.) And this is definitely something people request and use. |
based on discussion I iwll remove my hold
gavinking
left a comment
There was a problem hiding this comment.
There are a couple of @param javadocs which have periods (inconsistently).
Otherwise it's fine.
…ion for param javadoc
Resolves #1416 which requested a way to indicate whether null values should be sorted first or last. This is done with methods
.nullsFirst()and.nullsLast()that can be invoked on aSortinstance to obtain an otherwise equivalent instance that specifies how to sort nulls. This pattern aligns with designedPageRequestin Jakarta Data. I have included language that allows NoSQL databases to reject aSortinstance that hasnullsFirstornullsLastif the NoSQL database is not capable of it.