Supports scrolling base on keyset without id

[quaff]

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

[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.

[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.

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

[mp911de]

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.

[quaff]

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

[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

[mp911de]

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

[quaff]

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.

[mp911de]

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.

[quaff]

KeysetScrollIntegrationTests is extracted to #3030 after #2999 resolved.

[quaff]

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

[quaff]

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

[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

[stupid58fly]

Hi @quaff,

Have you considered nullable unique columns?

In standard SQL, a UNIQUE constraint does not prevent multiple NULL values. If the database contains multiple rows where this column is NULL, relying solely on the unique flag for keyset scrolling without an explicit ID fallback might cause unexpected behavior or result in multiple rows being returned.

It would be great to add a test case to ensure this works fine when multiple NULL values are present in the unique column.

[quaff]

Hi @quaff,

Have you considered nullable unique columns?

In standard SQL, a UNIQUE constraint does not prevent multiple NULL values. If the database contains multiple rows where this column is NULL, relying solely on the unique flag for keyset scrolling without an explicit ID fallback might cause unexpected behavior or result in multiple rows being returned.

It would be great to add a test case to ensure this works fine when multiple NULL values are present in the unique column.

@stupid58fly Updated as you suggested, thanks.

[stupid58fly]

Thanks for the update! I have a couple of thoughts regarding the logic and future extensibility:

1. Loop Exit Logic & nullable handling:
   It looks like the current exit condition in line 316 and line 326 doesn't fully account for the nullable attribute. To ensure the logic behaves correctly when nullable is present, I think it should be rewritten like this:

   if (column != null) {
       return column.unique() && !column.nullable();
   }

2. Extensibility for UniqueConstraint:
   Apart from the unique attribute inside the @Column annotation, JPA also provides the @UniqueConstraint annotation (usually defined at the @Table level). Have you considered architectural space for expanding this feature to support @UniqueConstraint in the future? It might be worth keeping the design flexible enough so we can easily plug in table-level constraint scanning later on.

What do you think?

[quaff]

Thanks for the update! I have a couple of thoughts regarding the logic and future extensibility:

1. Loop Exit Logic & nullable handling:
   It looks like the current exit condition in line 316 and line 326 doesn't fully account for the nullable attribute. To ensure the logic behaves correctly when nullable is present, I think it should be rewritten like this:

   if (column != null) {
       return column.unique() && !column.nullable();
   }

2. Extensibility for UniqueConstraint:
   Apart from the unique attribute inside the @Column annotation, JPA also provides the @UniqueConstraint annotation (usually defined at the @Table level). Have you considered architectural space for expanding this feature to support @UniqueConstraint in the future? It might be worth keeping the design flexible enough so we can easily plug in table-level constraint scanning later on.

What do you think?

1. Updated.
2. I think we shouldn't overcomplicate things, what if a column is nullable in @Column and unique in @UniqueConstraint or missing from annotation but in DDL? It's likely impossible to cover all cases, covering most common case is enough.