[TCK Challenge]: testCursoredPageOf7FromCursor() and testCursoredPageWithoutTotalOf9FromCursor()

[gavinking]

Specification

PageRequest.afterKey()

Assertion

line 977 of EntityTests

TCK Version

1.0.0-M3

Implementation being tested

Hibernate

Challenge Scenario

Claims that a test is not portable or depends on a particular implementation.

Full Description

The assertion here makes little sense and depends on an inconsistently constructed PageRequest. Thus, it appears to depend on implementation details of a particular product.

Additional Context

The following line:

PageRequest middle7 = PageRequest.ofSize(7) .afterKey((short) 5, 5L, 26L)

produces a PageRequest whose internal state is inconsistent with the data in the database. It manifestly represents a "middle" page, but has a page number of 1.

Thus, later assertions can fail (in particular the one at line 977).

Now, sure, a certain implementation of Jakarta Data might pass these assertions by luck, but nothing in the specification or API documentation requires that an implementation be this lucky.

Is there an existing challenge for this?

• I have searched the existing issues

[gavinking]

The same problem affects testCursoredPageWithoutTotalOf9FromCursor().

[njr-11]

produces a PageRequest whose internal state is inconsistent with the data in the database. It manifestly represents a "middle" page, but has a page number of 1.

With cursor-based pagination, the page numbers are meaningless unless you start from the beginning of the results and disallow updating the data. Instead of stating this in the spec, the CursoredPage javadoc has the more general statement that

• Page numbers, total numbers of elements across all pages, and total count

• of pages are not accurate when cursor-based pagination is used and should not
• be relied upon.

In other words, don't rely on page number having any meaning when using cursor-based pagination. I don't see a problem with test in this regard, but if you would like to have it assign a more meaningful page number to reflect being in the middle, I'd be fine with that.

Thus, later assertions can fail (in particular the one at line 977).

I don't understand this part. Line 977 doesn't assert a page number, it asserts the content of the previous page that it obtained on preceding lines,

    assertEquals(Arrays.toString(new Long[] { 16L, // 4 bits required, square root rounds down to 4
                                             31L, 30L, 29L, 28L, 27L, 26L // 5 bits required, square root rounds down to 5
    }),
                Arrays.toString(previousPage.stream().map(number -> number.getId()).toArray()));

I'm not sure what is wrong with that other than poorly formatted code.

[gavinking]

With cursor-based pagination, the page numbers are meaningless

If that's the case then the current API is terrible, and we need to do as @otaviojava suggests and split this thing into two classes.

But I'm not sure that it needs to be the case. In the implementation I have in Hibernate, I assume that it is the responsibility of the caller to construct a sensible PageRequest object.

[gavinking]

In other words, don't rely on page number having any meaning when using cursor-based pagination.

You cannot find this statement anywhere in the spec or Javadoc, and therefore the TCK can't assert it.

Besides, this is a huge red flag screaming that we have a badly designed PageRequest object.

[gavinking]

Like, @njr-11, seriously. I hope we agree on this: the TCK is only permitted to assert things explicitly stated in the spec. It is absolutely not permitted to assert things that you implicitly understand to be "the right way to implement this".

[njr-11]

The TCK isn't aiming to assert anything unusual or implicit here. It is aiming to test that you can specify a cursor at some random point within the results and either move next or previous from it. It doesn't specify or assert anything about page numbers, which is consistent with statements in the spec about page numbers being inaccurate for cursor-based pagination. I'm still trying to understand what is really the problem here. I've already stated, that it would be fine updating the test to assign a page number to the initial request if you would like.

[gavinking]

The problem is that right at the beginning of the test you construct a Pagerequest whose page number is inconsistent with its Cursor.

[gavinking]

"page numbers being inaccurate" does not mean "page numbers are completely incorrect and wrong".

And that's an incredibly vague statement to hang this test on. Until this moment I had absolutely no clue that this is what you meant by that.

[gavinking]

I had interpreted it to mean: that if an insert, update, or delete happens during navigation, then phantom reads might cause the cursor to become unsynchronized with the page number.

[njr-11]

The problem is that right at the beginning of the test you construct a Pagerequest whose page number is inconsistent with its Cursor.

The intent of the TCK test is to specify the starting point with a cursor, NOT with a page number. I understand that the default page number of 1 ends up applying. The test is not intending to cover having a page number of 1 for a cursor in the middle of the results - it only wants to specify the cursor, not the page number. I think we can solve this in the spec by making the page number not default to 1. We can have pageRequest.page() return an empty Optional when no page number is specified, and the spec can say that for offset pagination, the page number defaults to 1, and for cursor-based pagination, the provider computes one. I'll look into a pull with this. We should also be able to add more clarification around page numbers for cursor-based pagination that will make them more useful.

[gavinking]

Nathan honestly man, just change the test to construct a sensible PageRequest. It's a literal one-character change.

[njr-11]

Nathan honestly man, just change the test to construct a sensible PageRequest. It's a literal one-character change.

All this time I've still been trying to figure out what you are really wanting here. If all you need is to have it ask for page 4 instead of unspecified and getting a default of 1, I can easily do that.

[gavinking]

If all you need is to have it ask for page 4 instead of unspecified and getting a default of 1

Yes, exactly.

[njr-11]

If all you need is to have it ask for page 4 instead of unspecified and getting a default of 1

Yes, exactly.

Done in #678

[gavinking]

The problem is that when you have a "first" page, you have to do something special when navigating backwards. navigating backwards from the "first" page should return the true first page of results (after accounting for phantoms). That is, you can't just ask for previous results before the current value of the cursor. You have to switch to requesting the first size results from offset zero.

So if a PageRequest looks like a request for the first page, and actually isn't, then you're going to do something wrong.

[gavinking]

If all you need is to have it ask for page 4 instead of unspecified and getting a default of 1

Yes, exactly.

Done in #678

Thanks, that's perfect.