similar to #485, add convenience impls of Page and Slice

[gavinking]

This is, again, to ease the job of implementors.

I hope I got the semantics right here; please check!

[njr-11]

I added one comment for a total page computation that would need to be fixed, but then I also realized that nextPageRequest can have some different behavior if there is known to not be a next page. I'm starting to think maybe this part of the implementation shouldn't go into the spec because it is starting to go beyond just directly returning values.

[gavinking]

I also realized that nextPageRequest can have some different behavior if there is known to not be a next page.

Yes, I knew about this wrinkle which is one reason I left this in Draft, because I knew I needed to think about that and I didn't have time yesterday.

But actually it's well defined by the Javadoc:

Returns a request for the next page, or null if it is known that there is no next page.

So I implemented that.

Fixed!

[gavinking]

Also, yes, sure, some fancier implementation could retrieve N+1 rows from the database to be absolutely sure that there really is a next page. But I guess if you want to be fancy you can provide your own implementation of the interface.

[gavinking]

But I guess if you want to be fancy you can provide your own implementation of the interface.

Scratch that, let's give fancy people what they want :)

[gavinking]

I have added a new commit to this PR which makes the internal implementation of Cursor available as a record in the same package as the Page and Slice implementations. I don't believe there was any good reason to hide this from clients.

[gavinking]

I've now also added implementations of KeysetAwarePage and KeysetAwareSlice, based on my best understanding of the semantics here.

@otaviojava @njr-11 Please take a look.

[njr-11]

I have added a new commit to this PR which makes the internal implementation of Cursor available as a record in the same package as the Page and Slice implementations. I don't believe there was any good reason to hide this from clients.

I would say it's a good practice not to externalize unless we have a good reason to believe that clients (or the provider) will need it. It can make sense for a common implementation of Page/Slice to be shared by providers and be accessed from an impl package, but the implementation of Cursor only seems to be needed internally to Jakarta Data. I would not want to see client code constructing cursors by accessing API in our impl package, but if we externalize it, they will be able to do that. I think we should remove the second commit from this pull.

[gavinking]

the implementation of Cursor only seems to be needed internally to Jakarta Data

OK if that's true I made a mistake somewhere. Let me recheck why I thought I needed one.

[gavinking]

OK if that's true I made a mistake somewhere.

Yep, OK, I see, when I saw it being instantiated directly all through the TCK tests, I guess I just assumed it was needed by implementors too.

But I just misunderstood.

I have put it back where it belongs, and changed its visibility back to package.

[gavinking]

(I force-pushed to clean up the history.)

[njr-11]

Yep, OK, I see, when I saw it being instantiated directly all through the TCK tests, I guess I just assumed it was needed by implementors too.

The TCK tests don't directly create Cursor themselves anywhere, but the Unit Tests do a lot of that, so that is probably where you saw it.

[gavinking]

Yep, OK, I see, when I saw it being instantiated directly all through the TCK tests, I guess I just assumed it was needed by implementors too.

The TCK tests don't directly create Cursor themselves anywhere, but the Unit Tests do a lot of that, so that is probably where you saw it.

Yeahright. That.

[gavinking]

I think we should remove the second commit from this pull.

Done.

[njr-11]

Sorry for the two separate reviews. I think the pull was updated while I was reviewing and git needed to refresh. Fortunately I was suspicious of whether it would be able to handle that and had saved my final comment to the clipboard. Here it is again because it didn't seem to go through on the previous review,

[njr-11]

Are you sure it is a good idea to precompute all of the cursors? When I implemented this, I created each cursor only upon request, under the assumption that users will typically only want 1 or 2 cursors per page (usually for the next or previous page), and not a cursor for every result on the page.

[gavinking]

It's a good question. At least in our implementation I believe it makes no difference because you have already done the work of retrieving that information from the server.

But if you think that other implementations might want to do it lazily, I can replace the List with a function ref.

[gavinking]

FTR, when I first implemented this I threw away all the keys except for first and last. But then when I saw that the API seems to require that I can send back the key of any row, and @beikov told me that's useful, I changed it to retain all the keys. I already have them so...

[njr-11]

It's a good question. At least in our implementation I believe it makes no difference because you have already done the work of retrieving that information from the server.

But if you think that other implementations might want to do it lazily, I can replace the List with a function ref.

On the other hand, these are just convenience implementations. If a provider wants a different pattern they can easily supply their own implementation, so this is probably fine to go with the simpler one.

[gavinking]

they can easily supply their own implementation

Yup. And furthermore it's something we can very easily add later without breaking the existing SPI.

[gavinking]

Force pushed again to fix the checkstyle violations.

[gavinking]

the implementation of Cursor only seems to be needed internally to Jakarta Data. I would not want to see client code constructing cursors by accessing API in our impl package

Ugh, OK, after all that, I found where it's needed. The issue is that KeysetAwareSliceRecord needs to be able to return a Cursor for each row of the result list. That's the contract of KeysetAwareSliceRecord.getKeysetCursor(). The implementation needs to instantiate those objects. It has nowhere else to get them from, AFAICT.

So if you don't want to make KeysetCursor public, I would propose adding a static method somewhere to instantiate it.

[gavinking]

So if you don't want to make KeysetCursor public, I would propose adding a static method somewhere to instantiate it.

See @2fa8ef2

[gavinking]

The implementation needs to instantiate those objects. It has nowhere else to get them from, AFAICT.

An alternative solution would be to remove KeysetAwareSlice.getKeysetCursor(int) for now.

[njr-11]

An alternative solution would be to remove KeysetAwareSlice.getKeysetCursor(int) for now.

That method is the most convenient way for a user to obtain a cursor from any starting point when they already have some results. I would say that wanting to be able to share a Cursor implementation among providers doesn't justify taking that away from users.

I like your other approach of adding Cursor.forKeyset,

So if you don't want to make KeysetCursor public, I would propose adding a static method somewhere to instantiate it.

See @2fa8ef2

[gavinking]

I would say that wanting to be able to share a Cursor implementation among providers doesn't justify taking that away from users.

Oh, that wasn't really the idea: the idea was that as per our other discussion it would have relieved us from having to materialize Cursor instances for anything other than the first/last row, and by side effect providers would never need to instiate a Cursor.

But I wasn't arguing for it, just mapping out the solution space.

I like your other approach of adding Cursor.forKeyset

Good, that solves the problem.

[njr-11]

Approving - Everything looks good.

[gavinking]

Folks please review a6b6c1c which I just pushed, a better implementation of "more results" for both PageRecord and SliceRecord.

[gavinking]

Folks please review a6b6c1c which I just pushed, a better implementation of "more results" for both PageRecord and SliceRecord.

Oh well, Otavio just merged it, as I was typing this message.

[njr-11]

Folks please review a6b6c1c which I just pushed, a better implementation of "more results" for both PageRecord and SliceRecord.

Oh well, Otavio just merged it, as I was typing this message.

I suppose that means he was very strongly in favor of it.
The updates look good to me.

[gavinking]

Yeah it would be great except for the fact that it had a bug in it (fixed in the new PR).