[Bug]: PageRequest doesn't know whether it's a Builder

[gavinking]

Specification Version

1.0.0-M3

Bug report

PageRequest has multiple operations capable of producing an object in an inconsistent state. The most dangerous of these is page(long), which looks harmless enough but actually results in something completely broken if you call it an a PageRequest with a Cursor. And there's no warning of this in the Javadoc, even. On the other hand, it also has operations like next() which are written to be called by the application.

It is in no way obvious to the user that these operations page(long) and next() are so different in nature. So much so that even I have tripped up twice over the difference and I am far from a casual user of this API.

We need to remove these dangerous operations to a Builder-type object, or do what I suggested a while ago, and expose the constructor of Pagination to Jakarta Data implementations and simply remove the "dangerous" methods. (This is the simplest solution.)

Additional information

No response

[otaviojava]

I saw this this weekend.

Doing a ballparking implementation, since we have two paginations, I would create specializations:

Doing a ball park implementation:

classDiagram

    PageRequest <|-- OffSetPagination
    PageRequest <|-- CursoredPagination
    PageRequest: long size()
    PageRequest: CursoredPagination beforeKey()
    PageRequest: CursoredPagination afterCursor()
    OffSetPagination: page()
    CursoredPagination: next()
    CursoredPagination: previous()
    CursoredPagination: cursor()

[gavinking]

@otaviojava Yes, that's an option indeed. Though I'm also looking for options which have a smaller blast radius.

[njr-11]

We should get rid of the next() and previous() methods from PageRequest. The right way to create a request for a next or previous page is to invoke nextPageRequest() and previousPageRequest() on a Page or CursoredPage so that you are getting a next/previous relative to a page.

[gavinking]

The right way to create a request for a next or previous page is to invoke nextPageRequest() and previousPageRequest() on a Page or CursoredPage so that you are getting a next/previous relative to a page.

Ah. See I did not understand that.

[gavinking]

So the question is: who is the client of those operations of PageRequest. If it is not usually an application program, but rather the provider itself, or some sophisticated framework, then I think they should be somehow moved to some spi package to make that clear.

[njr-11]

So the question is: who is the client of those operations of Page. If it is not usually an application program, but rather the provider itself, or some sophisticated framework, then I think they should be somehow moved to some spi package to make that clear.

The purpose of page.nextPageRequest and page.previousPageRequest is for application usage. They are used in many places in the TCK, mirroring actual application usage patterns.

[gavinking]

I meant to write PageRequest, sorry. I will edit my comment.

[njr-11]

I meant to write PageRequest, sorry. I will edit my comment.

Oh, you question makes a lot more sense now. As far as I'm aware, there is no purpose whatsoever for the next() and previous() methods of PageRequest. For API or SPI. I don't think the TCK even uses them. I'll open a pull to get rid of them.

[gavinking]

Nono, I know you want to get rid of those ones, and that's fine.

My question is: who is the called of all the other methods like page(long) and beforeCursor()?

Because if the answer is: usually a provider, or a library, then I think we need to move all those operations to an SPI.

[njr-11]

My question is: who is the called of all the other methods like page(long) and beforeCursor()?

Because if the answer is: usually a provider, or a library, then I think we need to move all those operations to an SPI.

page(long) is for the application to request a specific page number. It has exactly the same pattern as size(int). The application can either do,

PageRequest.ofSize(20).page(5);

or it can do

PageRequest.ofPage(5).size(20);

Similarly, beforeCursor is for the application as well, although it is likely more advanced usage.

Here is one example,

PageRequest.ofSize(20).beforeCursor(Cursor.forKey(product.id));

Here is an example of requesting a previous page with intentional overlap of 2 results:

PageRequest.ofSize(20).beforeCursor(page.cursor(2));

[gavinking]

page(long) is for the application to request a specific page number.

OK, well today it's broken for that usage. If you call page(long) on a PageRequest you got from a CursoredPage, you get something completely wrong.

So we need to either fix it or remove it.

[gavinking]

Similarly, beforeCursor is for the application as well, although it is likely more advanced usage.

Both of your examples show usage of PageRequest as a builder to construct a brand-new PageRequest. They don't show mutation of an existing PageRequest. What I'm saying is that such operations should be moved to a PageRequestBuilder object.

[njr-11]

Both of your examples show usage of PageRequest as a builder to construct a brand-new PageRequest. They don't show mutation of an existing PageRequest.

That's good. PageRequest is meant to be a builder, and PageRequests are immutable.

What I'm saying is that such operations should be moved to a PageRequestBuilder object.

Technically you could put builder in the name, but that doesn't look very nice in application code and it is better to keep its current name. The entire purpose of PageRequest is to be a builder. I opened a pull to get rid of two methods that don't make sense with it being a builder.

[gavinking]

PageRequest is meant to be a builder.

Then it shouldn't be the object that is passed to a repository method, and returned by CursoredPage.nextPagerequest().

and PageRequests are immutable.

This would be fine if the copy-on-write methods always returned a PageRequest in a consistent state, but they don't. PageRequest.page(2) returns nonsense when called on a PageRequest with a Cursor.

[njr-11]

PageRequest is meant to be a builder.

Then it shouldn't be the object that is passed to a repository method, and returned by CursoredPage.nextPagerequest().

I don't see what is wrong with that. It is fine to supply a builder instance to a method and to receive a builder instance from a method. They are immutable instances so it is impossible to modify them and cause side effects.

It does seem a little awkward that the builder never builds an instance of and the configuration is instead read directly from it. If we wanted, we could have it build something and access the configuration from that. I'd rather not clutter the application's API usage with that, but if we want to isolate that to providers, I'd be fine with it.

This would be fine if the copy-on-write methods always returned a PageRequest in a consistent state, but they don't. PageRequest.page(2) returns nonsense when called on a PageRequest with a Cursor.

Is the combination of page and cursor the only area where you see there being inconsistent state? If so, we should look into solving that specific issue rather than redesigning everything. We could easily define it to raise an error for specific invalid combinations. That said, I don't see what is wrong with page(2) here. That seems like a reasonable thing to do if you want the page you are requesting to be numbered as page 2. Maybe you were already on page 2 but have deleted everything on it while processing it and so you want the next requested page after it to also be numbered as 2. I suppose that is more of an unusual case. If you want that to raise an error, I suppose it's fine and that user can figure out a different way to ask for page 2.

[gavinking]

Is the combination of page and cursor the only area where you see there being inconsistent state?

I believe so, yes.

[gavinking]

That said, I don't see what is wrong with page(2) here. That seems like a reasonable thing to do if you want the page you are requesting to be numbered as page 2.

OK, so look, I'm an application programmer. I get back a CursoredPage from a repository method.

Now let's say the user clicked on the link to go to page 2.

It seems like totally legit to call: page.pageRequest().page(2) and pass that back to the repository method. It's what I would do.

But with the current implementation that just sent you back to the exact same page you were just on.

Because your page() method was never meant to be called by the application programmer, or, at least, not in that way.

[njr-11]

OK, so look, I'm an application programmer. I get back a CursoredPage from a repository method.

Now let's say the user clicked on the link to go to page 2.

It seems like totally legit to call: page.pageRequest().page(2) and pass that back to the repository method. It's what I would do.

But with the current implementation that just sent you back to the exact same page you were just on.

Because your page() method was never meant to be called by the application programmer, or, at least, not in that way.

That's an excellent example - thanks for identifying it! Given that the scenario is a very useful one for offset pagination, I think a good approach to take here is to specify that page(long) raises an error when the PageRequest already has a cursor. Mabye IllegalStateException? I can look into adding that.

[gavinking]

Meh. That's not typesafe. It's easy to come up with a typesafe solution: simply move that operation to an SPI.

[gavinking]

Really, page() has no business being there, luring the user in to calling it, when you actually don't want the user to ever call it.

[njr-11]

Really, page() has no business being there, luring the user in to calling it, when you actually don't want the user to ever call it.

Sure, I'm fine with that. I'll switch over to removing it entirely.

[gavinking]

I'll switch over to removing it entirely.

So then implementations would use:

PageRequest.ofPage(pagination.page()+1).size(pagination.size()).afterCursor(_cursors.get(_cursors.size()-1))

I dunno, would it not be better to just remove the methods like afterCursor() and let implementations use something like:

PageRequest.afterCursor(pagination.size(), pagination.page()+1, _cursors.get(_cursors.size()-1)))

or even just, hear me out:

new Pagination(pagination.size(), pagination.page()+1, _cursors.get(_cursors.size()-1)))

after moving Pagination to an appropriate spi package.

It's just simpler and less confusing.

[njr-11]

would it not be better to just remove the methods like afterCursor() and let implementations use something like:

PageRequest.afterCursor(pagination.size(), pagination.page()+1, _cursors.get(_cursors.size()-1)))

That seems like it would be reasonable. I added a commit to the pull with this (and also rebased on to latest).