spring-data-jpa icon indicating copy to clipboard operation
spring-data-jpa copied to clipboard

Published 20 hours ago • verified publisher icon spring-projects

Supports scrolling base on keyset without id

Open quaff opened this issue 2 years ago • 12 comments
trafficstars

id shouldn't be added to sort if provided sort contains unique property. See GH-2996

quaff avatar Jun 12 '23 06:06 quaff

Removing the primary key isn't going to work, as Keyset scrolling must enforce uniqueness. Just accepting Sort isn't sufficient. What we could do is verify whether a unique property is a part of Sort. If not, we fall back to the primary key.

mp911de avatar Jun 12 '23 07:06 mp911de

Removing the primary key isn't going to work, as Keyset scrolling must enforce uniqueness. Just accepting Sort isn't sufficient. What we could do is verify whether a unique property is a part of Sort. If not, we fall back to the primary key.

IMO, we should add a section in document to elaborate advantage and limitation, guide developers to make best decision, avoid making code more complex.

quaff avatar Jun 12 '23 12:06 quaff

I agree that we should improve the documentation in Commons to explain the relationship of Sort to the query and how we amend the Sort object.

mp911de avatar Jun 12 '23 12:06 mp911de

I mean it is the responsibility of developer to make sure keys combination unique if the document emphasize that.

quaff avatar Jun 12 '23 13:06 quaff

Test improved to cover such cases:

  1. unsorted (implicit sorted by id asc)
  2. explicit sorted by id asc and desc
  3. explicit sorted by seqNo asc and desc
  4. explicit sorted by (id, seqNo) asc and desc

all cases are tested against FORWARD and BACKWARD

quaff avatar Jun 13 '23 05:06 quaff

Please, let's keep pull requests focused instead of mixing concerns within a single pull request.

mp911de avatar Jun 13 '23 07:06 mp911de

Please, let's keep pull requests focused instead of mixing concerns within a single pull request.

Sorry to disturb you, those changes are related and necessary to make the test pass.

quaff avatar Jun 13 '23 07:06 quaff

We don't work towards making tests pass that combine various aspects but rather include coordinated changes. I applied the directional retention via #2999 already. Including unique sort arguments and checking for these requires a bit more changes than we have in this PR.

Towards the sort requirements, we want the calling code to specify sort properties without the requirement to include always properties to make rows unique. Making rows unique is primarily a concern of our keyset scrolling code. If calling code wants to specify sort properties to make the result unique, then we can do so in the course of this pull request.

mp911de avatar Jun 13 '23 09:06 mp911de

KeysetScrollIntegrationTests is extracted to https://github.com/spring-projects/spring-data-jpa/pull/3030 after https://github.com/spring-projects/spring-data-jpa/issues/2999 resolved.

quaff avatar Jun 16 '23 02:06 quaff

A viable proposal is adding a boolean property (eg. keysetQualified) to Sort, let developer hint that based on unique columns.

quaff avatar Jun 18 '23 03:06 quaff

We could introduce a global property indicate that sort is unique ensured by developer.

quaff avatar Jul 03 '23 02:07 quaff

Commit updated, I introduce method JpaEntityInformation.isKeysetQualified() and default implementation is check whether unique property present, then skip include id in keyset, please take a look @mp911de

quaff avatar Oct 24 '23 05:10 quaff