jakartaee / data

Data-API
Apache License 2.0
105 stars 29 forks source link

Precedence on the Sort Features #116

Closed otaviojava closed 1 year ago

otaviojava commented 1 year ago

I'm trying to fix the Sort annotation and appoint it here:

https://github.com/jakartaee/data/issues/94

Currently, we have three ways to define Sort:

Once thrown, an exception is not a good API for developers; it might be better to define precedence.

The first two are more accessible, and we have samples in the market. So, the query by method and appends from the question dynamically.

We could add the Sort annotation as optional and third in the precedence.

I agree with Markus, and it does not look intuitive; furthermore, we need a case for it. Another option would be to remove it and wait until we collect feedback from a vendor who does it first.

njr-11 commented 1 year ago

The first question to answer with respect to establishing a precedence is whether the higher precedence is defined as replacing the lower precedence criteria, or whether the higher precedence is defined to sort by that criteria first before sorting by the lower precedence criteria only if needed.

Here's an example that will illustrate. You could have:

Pageable p = Pageable.ofSize(10).sortBy(Sort.asc("price"));
products.findByNameLikeAndPriceBetweenOrderByName(pattern, minPrice, maxPrice, p);

If we say that sort criteria from Pageable has higher precedence than sort criteria from OrderBy, in the above example it could mean either of:

Let's determine this question first, and then we can consider what precedence to assign to each of the 4 ways to specify sort criteria.

otaviojava commented 1 year ago

This one looks like an append to me:

products.findByNameLikeAndPriceBetweenOrderByName(pattern, minPrice, maxPrice, p);

Thus, sort by name and what is on the pageable, so price.

njr-11 commented 1 year ago

I think I said there are 4 different ways to do sorting above. There are actually 5:

To simplify this, I suggest that it continue to remain an error to combine the first with the second (having multiple different static OrderBy mechanisms), or the third with the fourth (having multiple different dynamic Sort mechanisms) on a single method.

Then the problem reduces to determining order of append for:

Hopefully we can all agree that the last of these needs to go first (to avoid the expectation for providers to parse user-provided query language). I'd suggest that the other static approach be next, followed by dynamic. This will have the advantage of making it so that you can read the precedence from code:

products.findBy...OrderByName(..., Sort.desc("price"), Sort.desc("numAvailable"));
otaviojava commented 1 year ago

Ignoring the Sort annotation. How does it work in Spring and Micronaut?

@graemerocher @odrotbohm @mp911de

odrotbohm commented 1 year ago

In Spring Data, there's a "static" sort component, defined in either the manually declared query or in the method name. That usually gets pushed down into a cached instance of the store's query abstraction. The dynamic one, handed in as method parameter, then augments that query instance, i.e. gets appended to the statically defined sort, to build up the query that's brought to execution eventually. There's no sort annotation or the like.

That said, we usually see queries defined without any static sort reference and users declaring default methods without a Sort to then call the actual query method with some default. Also, we generally reject query methods that declare both a Sort and a Pageable. If you declare a query to be a paging one, the Sort you want to apply needs to be handed in as part of the Pageable.

HTH

njr-11 commented 1 year ago

That's good, it sounds like Spring Data behavior is consistent with what I was expecting. @Query with ORDER BY in it can just be treated as another form of static, so then a method can have at most one static mechanism and at most one dynamic mechanism for specifying sort criteria, with the static taking precedence and the dynamic being appended on to that. I think that will be easy to explain in the spec in a way that makes sense.

mp911de commented 1 year ago

@Query with ORDER BY

While this sounds simple, it's all but that. Consider @Query("SELECT … FROM foo ORDER BY x UNION SELECT … ORDER BY y) as one of the easier cases that would conflict with amending the static ORDER BY part.

I honestly have no good recommendation, as a provider for SQL-based queries would have to parse and rewrite the query. At the same time, stores like MongoDB allow specifying the sort part in a more accessible machine-readable form.

otaviojava commented 1 year ago

Markus, I have the same feelings about the @Query

@odrotbohm @graemerocher @njr-11 what do you think about it?

mp911de commented 1 year ago

Let me add a bit more opinion from my side: Not having Sort support for SQL @Query queries results in not having pagination support for these which imposes quite some constraints on users.

Also, our current thinking goes into the direction that we could well specify the WHERE part in a query as this wouldn't impose limitations on pagination and sorting augmentation for queries.

Another possibility is giving users a mechanism to augment/rewrite a query. Clearly, these limitations apply only to particular stores.

As a way out, @Query could serve as a meta-annotation marker. We can leave the actual @Query handling to implementation modules which effectively turns @Query into implementation details instead of part of the spec.

njr-11 commented 1 year ago

I found the following in Spring Data documentation giving examples where @Query can be combined with Sort,

  @Query("select u from User u where u.lastname like ?1%")
  List<User> findByAndSort(String lastname, Sort sort);

  @Query("select u.id, LENGTH(u.firstname) as fn_len from User u where u.lastname like ?1%")
  List<Object[]> findByAsArrayAndSort(String lastname, Sort sort);

For the sake of users who are already doing that, I think it does make sense for the spec to allow it to continue to be valid. I like the idea of allowing for Sort/Pagination with @Query that is limited to the WHERE clause and doesn't already contain an ORDER BY clause in it. I think we can come up with some spec language that identifies the limitation and makes it the application's responsibility if trying to combine @Query with other sorting to write their query in such a way that an ORDER BY clause can be appended without requiring the provider to parse query language. I've made an attempt at this in commit https://github.com/jakartaee/data/pull/117/commits/bfbefedcc25672e65f7df202d98211397169609a to pull #117.

otaviojava commented 1 year ago

@njr-11, what do you think about putting several samples and their precedence on it? I guess it will get clear the proposal:

List<User> findByNameOrderType(String name);

@Query("my native query here order by name")
List<User> findByNameOrderType(String name);

@Sort("age")
@Query("my native query here order by name")
List<User> findByNameOrderType(String name);

@Query("my native query here order by name")
List<User> findByNameOrderType(String name, Pageable pageable);

@Sort("age")
List<User> findByNameOrderType(String name, Pageable pageable);
odrotbohm commented 1 year ago

Can someone elaborate what the point of the @Sort annotation is? Either you have a query defined explicitly, in which you better define the sort directly in the query (second method). Or you use query derivation, in which you'd include all static query information in the query method name (method one)? I don't quite get which problem methods three and five solve.

njr-11 commented 1 year ago

@otaviojava - that's an excellent idea to add examples. I did so here. @odrotbohm - I think those examples were written quickly and have a few errors in them. I cleaned that up when adding it to the pull, so they'll hopefully make sense now. There's a separate issue for discussing the topic of why we have the multiple mechanisms in the first place and whether we should keep them all, so I'll leave that sort of discussion to that issue. This issue here is only to document the precedence of what we currently have.

odrotbohm commented 1 year ago

I've commented on the other ticket. I'd vote to get to a decision whether to introduce the annotation in the first place as you (we) wouldn't want to spend time specifying and thinking about precedence of programming model means that might not even make it.

otaviojava commented 1 year ago

I agree with Oliver's point, but as discussed in the meeting, let's keep it.

We have a lengthy discussion about this annotation, and I'm okay with the team's decision to keep it.

The last question I want to ask about this feature:

Can we make it at least optional on the spec?

Thus, Spring and Eclipse JNoSQL do not need to implement and are still compatible with the spec.

@njr-11 @mpredli01 @KyleAure @keilw

njr-11 commented 1 year ago

We could add language that says something along the lines of: "MappingException is raised when the database is incapable of ordering with the requested sort criteria." That way, Jakarta Data providers for any NoSQL databases that can't do sorting (or some aspect of it) are compliant with the spec by raising the exception.