allow zero-based indexing (offsets) in PageRequest and Limit

[gavinking]

See #1447.

[njr-11]

I'm okay with adding a minimal amount of methods to accommodate users who prefer 0-based. Deprecating some existing methods in favor of introducing new ones with more precise names along with new methods for 0-based index seems like too much footprint in the API.

How about if we add a single method to Limit:

Limit.of(maxResults, offset)

where offset is defined relative to the first result, causing 0 to point to the first result.

and for PageRequest,

pageReq = PageRequest.ofSize(10).pageOffset(offset);

where offset is defined relative to the first page, causing 0 to point to the first page.

and

offset = pageReq.pageOffset();

to obtain the current page offset.

That said, even with paring it down to only these 2 methods for PageRequest, I still don't like how confusing it is to have a pageOffset on a PageRequest that is used or cursor pagination instead of offset pagination. I'm leaning toward also saying we should also have the pageOffset methods raise an error if the PageRequest also has a cursor or if a cursor is added to a page that was requested to have an offset (although unfortunately the latter is indistinguishable from a page that was requested to have a page number, which does not reject adding a cursor)

[gavinking]

Deprecating some existing methods in favor of introducing new ones with more precise names along with new methods for 0-based index seems like too much footprint in the API.

Yeah I dunno, I guess the thing that pushed me in that direction was:

• page() doesn't return a Page
• page(10) was named in a way which seemed inconsistent with the style of the rest of the API, which was all ofPage(), ofSize(), and so on
• I didn't want to double down on this inconsistency when introducing new operations

I agree that it's annoying and embarrassing to deprecate things on APIs which are brand new. But I've also had plenty of experiences where avoiding such embarrassment leads to worse outcomes in the long run. It's better to change APIs before they're in wide use than after.

[FroMage]

Thanks for this change.

So, for pages, we're getting rid of index in favour of number (1-based) and offset (0-based).

But then for limits, we've a choice of startAt and startOffset and I guess at means number here?

This looks like gymnastics to allow two styles of indexing. I am a bit torn on whether this is better or not.

If we switch this to 0-based indices, I can avoid warnings in the Quarkus docs.

If we get two indexing mechanisms here, I will have to add warnings in the Quarkus docs to steer people in the direction of 0-based methods, and mention that there are 1-based methods in the JD API that they should watch out for. Because two indexing methods could be confusing. Perhaps less than the following alternative, but I'm not entirely sure.

If we leave 1-based as it is now, I will have to add warnings in the Quarkus docs to mention this potential pitfall, and also align our API to be 1-based on page numbers, just to make sure we're consistent.

So, in two cases I'll have to write big warnings everywhere page numbers are mentioned in the docs. Given that, I'm not entirely sure this is much better.

I agree that it's annoying and embarrassing to deprecate things on APIs which are brand new. But I've also had plenty of experiences where avoiding such embarrassment leads to worse outcomes in the long run. It's better to change APIs before they're in wide use than after

Totally agree, it's now or never, IMO.

[njr-11]

• page() doesn't return a Page

I agree pageNumber() would have been a better and more descriptive choice.
Mostly this method is used by the Data provider rather than the application because the latter already knows which page it asked for.

• page(10) was named in a way which seemed inconsistent with the style of the rest of the API

It is consistent with size(20).
And it does have at least one advantage in that it makes for very concise preferred usage:

PageRequest.ofSize(50).page(10);

The naming could be improved. The part the bothers me most is the getter and setter/newinstance methods having the exact same name. If this were still Data 1.0, I'd be in favor of switching page() to pageNumber(). But I'm not so sure it adds that much value that it's worth the duplication that would exist followed by breaking change.

[njr-11]

A minimal changes approach (with some text copied from this PR) can be found under #1482
I did not include anything about 'pageNumber()` because whether or not we want that should really be a separate discussion independent of whether we are going to allow for 0-based