[clarification]: should PageRequest<?> really be allowed

[gavinking]

Specification

PageRequest, Order, Sort

I need clarification on ...

whether we should really be accepting parameters of type PageRequest<?>, Sort<?>, and Order<?>.

Additional information

Strictly-speaking, the correct type for a parameter of type PageRequest is PageRequest<? super E> or PageRequest<E> where E is the queried entity type.

However, in section 4.7 we don't specify any restrictions at all on the type arguments in Sort, Order, or PageRequest, and we have a couple of examples in the spec and javadoc where PageRequest<?> is used as the type of a repository method parameter. Worse, the TCK actually tests it.

The problem with this is it limits the ability of compile-time tooling to provide feedback on method signatures of repository methods. I would like to be able to reject nonsensical things like:

@Find List<Book> books(PageRequest<Author> request)

But the spec doesn't really give me permission to do that.

I would even much prefer to reject:

@Find List<Book> books(PageRequest<?> request)

(Note that this does not prevent the method being called via the untypesafe string-based mode, since PageRequest.ofSize(40).sortBy(Sort.desc("name"), Sort.asc("id")) is actually assignable to PageRequest<? super Author>, which is strictly-speaking wrong in itself but it's a kind of wrong I guess I can live with.)

Whatever, I think it's important that 4.7 be explicit about what is allowed and what can be rejected by the provider.

[njr-11]

The problem with this is it limits the ability of compile-time tooling to provide feedback on method signatures of repository methods. I would like to be able to reject nonsensical things like:

@Find List<Book> books(PageRequest<Author> request)

But the spec doesn't really give me permission to do that.

It would make sense to add language to the spec that the user must not mix incompatible type parameters, which would allow implementations to validly reject it. However, it would need to be done very precisely because there are cases where an implementation might have a valid use for that example as an extension to the spec. For example, if Author is the entity and List<Book> books is a field of Author, and there is a @Query annotation on the method rather than @Find causing books to be selected, then it might be valid to write a repository method like that. I don't know if anyone is planning to allow that as a vendor-specific extension, but it they were, it would be prevented if the spec language isn't precise enough to only rule out the incompatible types when the return value is the entity type or multiple of the entity type.

I would even much prefer to reject:

@Find List<Book> books(PageRequest<?> request)

Disallowing the above wouldn't be consistent with the previous compromise that allowed us to add type parameters to the spec. An implementation would be free to issue a warning for this usage without any spec language. Would that be good enough?

[gavinking]

It would make sense to add language to the spec that the user must not mix incompatible type parameters, which would allow implementations to validly reject it. However, it would need to be done very precisely because there are cases where an implementation might have a valid use for that example as an extension to the spec. For example, if Author is the entity and List<Book> books is a field of Author, and there is a @Query annotation on the method rather than @Find causing books to be selected, then it might be valid to write a repository method like that.

In JDQL we have the notion of a unique "queried entity", so this doesn't arise.

In full JPQL, all this is much more complicated since the cursor might need to include fields of multiple entities referred to from the select clause. But we don't and shouldn't be getting into that for 1.0.

I don't know if anyone is planning to allow that as a vendor-specific extension, but it they were, it would be prevented if the spec language isn't precise enough to only rule out the incompatible types when the return value is the entity type or multiple of the entity type.

The spec can say what vendors must support, without restricting what vendors may support as an extension. This is the approach we have always taken in the JPA spec.

Disallowing the above wouldn't be consistent with the previous compromise that allowed us to add type parameters to the spec.

I figured you might say that. But as I tried to point out above, PageRequest<Object> or PageRequest<? super Book>both work just as well here, since PageRequest.ofSize(40).sortBy(Sort.desc("name"), Sort.asc("id")) is assigned the type PageRequest<Object>. And Object is a svpertype of Book. We don't actually need to allow PageRequest<?> to support this usage.

An implementation would be free to issue a warning for this usage without any spec language. Would that be good enough?

That might be OK, but I would much prefer to make it an error.

It's not that it's hard for me to implement; I've already implemented it. But in the implementation I had to use a cast to a raw type, which is your red flag that this is something that's unsound. I would sorta prefer that we not bake in things that are unsound as required features.

[gavinking]

So a more radical idea would be to revisit the whole idea of hanging Sorts off a PageRequest. Hear me out.

History (from my perspective)

When I had originally modeled this in my head, a PageRequest didn't carry any sorting information—that was passed separately—and so PageRequest wasn't a generic type. But when I saw what you guys had done, I sorta saw why you would want to package the sorting criteria with a request for a page, it seemed convenient and reasonable, and so I just went along with that. The idea being, of course, that you want to be able package up everything describing a page of results and just pass it around the system. Sounds compelling right?

Well, except, it doesn't actually work. a PageRequest doesn't package arguments to the parameters of the query, and so you still need to carry them around separately. And while only some paged queries have dynamic sorting, almost every paged query has parameters! So this solution is at best incomplete. It's not even 50% complete. This was a disappointing realization.

But anyway, continuing along, when I saw that PageRequest carried around Sorts, just like an Order does, it seemed to make sense to parameterize it for typesafety, just like we parameterize Sort and Order. A first hint that there was something wrong with that thinking came up when Nathan had to add pageRequest(Class) and nextPageRequest(Class) overloads to Page. Still, it apparently worked nicely for @Find, at least, and I while this was a wart, I didn't really focus on it, especially since we had much bigger fish to fry at the time.

In summary

But now I'm wondering whether this was all a big mistake, and an unnecessary one.

• Sorts are in some sense just a kind of query parameter, and why do we treat them in a privileged way by letting them be packaged into the PageRequest object when we don't provide that facility for other arguments?
• For queries with a select clause, the ostensible typesafety provided by PageRequest is broken, at least when you use CursoredPage. Worse, the mere presence of the generic type makes the API much clunkier.
• It's not really very hard to write record PageOfBooks(String title, PageRequest page, Order<Book> order) {} if you want to pass a page request around the system, with its arguments.

Proposal

So, I think we should discuss simplifying PageRequest to:

• no longer carry any Sort objects,
• no longer be a generic type.

And, to partially compensate for this loss, we could add Order<?> order() to Page.

We would then be able to simplify Page by removing pageRequest(Class), nextPageRequest(Class), and previousPageRequest(Class).

The question raised by this issue would be resolved by side-effect, since PageRequest would no longer be a generic type. (A similar sort of question can be asked about Sort and Order but I think the discomfort there is much less acute.)

On the TCK and implementation side, there would now be fewer things to implement, and fewer things to test.

I understand it's late to be making API changes, but it seems to me that if a simplification like this really makes sense—and we're all comfortable with it—then not doing it would just be saddling ourselves with unnecessary technical debt.

[njr-11]

When I had originally modeled this in my head, a PageRequest didn't carry any sorting information—that was passed separately

That's actually how I originally wrote it - with the Sorts decoupled from the page request. The decision was reversed and the coupling added back in #37 where I was outnumbered in the voting.

[gavinking]

I was outnumbered in the voting.

OK. Well, I would say that it wasn't a bad idea to add Sorts to PageRequest. At least it didn't seem like a bad idea to me at first, nor even at second.

But now we have more experience actually programming with the APIs, and actual solid implementations of these things, I guess we are better-placed than you guys were back in 2022.

[gavinking]

Actually, I think that in #618, @otaviojava is right that currently the rules surrounding order by, @OrderBy and Order.by() really are too complicated and confusing. (Even I get muddled up and have to check the javadoc every time to see what's allowed.) But I think Otavio was wrong to point at @OrderBy as the culprit.

Actually the true culprit is PageRequest and its sorts(). If a PageRequest didn't carry sorting criteria, then I could accept this:

@Query("from Book order by title")
Page<Book> allBooks(PageRequest page);

and reject this:

@Query("from Book order by title")
Page<Book> allBooks(PageRequest page, Order<Book> order);

at build time (or at startup). And everything would be hunky-dory.

But since a PageRequest may or may not carry sorting criteria, that type-safety is lost, and I as a user of Jakarta Data have to be very careful about what instance of PageRequest I pass to which method. And that's why we have weird carve-outs like these, which can only be enforced at runtime, and not at compile-time:

A repository method throws IllegalArgumentException if it is called with an argument of type PageRequest with nonempty sort criteria, and a separate argument or arguments of type Sort or Order

If the annotated repository method accepts other forms of sorting (such as a parameter of type Sort), it is the responsibility of the application programmer to compose the query so that an ORDER BY clause can be validly appended to the text of the query.

So I'm increasingly convinced that PageRequests should not have sorting criteria.

[otaviojava]

I was outnumbered in the voting.

OK. Well, I would say that it wasn't a bad idea to add Sorts to PageRequest. At least it didn't seem like a bad idea to me at first, nor even at second.

But now we have more experience actually programming with the APIs, and actual solid implementations of these things, I guess we are better-placed than you guys were back in 2022.

I have the same feeling.
Given a new context, it is okay to review what we did.

We currently have Jakarta Data Query Language, Order annotation, etc., which we did not have at that time.

[otaviojava]

Actually, I think that in #618, @otaviojava is right that currently the rules surrounding order by, @OrderBy and Order.by() really are too complicated and confusing. (Even I get muddled up and have to check the javadoc every time to see what's allowed.) But I think Otavio was wrong to point at @OrderBy as the culprit.

Now, we have thousands of ways to represent the sort in the query.

I like the single responsibility principle, I mean, the pagination request handle with pagination.

[gavinking]

We have made the proposed change. Closing.