Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ExpressionUtils.isUnique(..., PathExpression) always returns false #569

Closed
jwgmeligmeyling opened this issue May 15, 2018 · 5 comments · Fixed by #611
Closed

ExpressionUtils.isUnique(..., PathExpression) always returns false #569

jwgmeligmeyling opened this issue May 15, 2018 · 5 comments · Fixed by #611
Assignees
Milestone

Comments

@jwgmeligmeyling
Copy link
Collaborator

ExpressionUtils.isUnique(..., PathExpression) always returns false. This isn't always correct. For example: MIN(id), MAX(id), SUM(id) are still guaranteed to be unique, if id is unique. Related to #568 as this could have been an easy work around.

Description

Expected behavior

Actual behavior

Steps to reproduce

Environment

Version:
JPA-Provider:
DBMS:
Application Server:

@jwgmeligmeyling
Copy link
Collaborator Author

I'm wondering though how this could easily be changed. While uniqueness is preserved using the MIN, MAX and SUM functions, it wouldn't work for COUNT and potentially others.

@beikov
Copy link
Member

beikov commented May 15, 2018

Also see #418

@jwgmeligmeyling
Copy link
Collaborator Author

I think it also fails for compound keys. Paginating on orderByAsc("entity.compoundKeyA").orderByAsc("entity.compoundKeyB"), which is actually unique when used together, but given the current implementation I think it "works" because it assumes entity.compoundKeyB is unique, which it is not. But this may just be related to the lacking support for @IdClass in general, I haven't tried it with EmbeddedIds.

@beikov
Copy link
Member

beikov commented May 18, 2018

Not sure about @IdClass support, but at least embedded ids work. We have quite a few models with embedded ids and they work all fine. Note that keyset-pagination with compound keys is currently broken, maybe you are hitting a problem there?
Anyway, the uniqueness analysis is more or less broken and currently only catches the most obvious usage errors. You got to be careful for now with @IdClass uses

@jwgmeligmeyling
Copy link
Collaborator Author

jwgmeligmeyling commented May 18, 2018

Didn't run into it yet, just scribbled down that, while fixing this, we should address:

  • isUnique seems to return true for parts of compound keys, if this is so, this needs addressing (unfortunately this will break valid paginations that now work, so ideally this is fixed in conjunction with the following)
  • Rather than checking whether the last column is unique, it probably should check whether the last columns form a unique tuple (either because the order by clause is a super set of the roots primary key column set or equal to the columns in the group by clause (at least the primary keys for the columns referenced in the group by clause)

@beikov beikov added this to the 1.3.0 milestone Jul 8, 2018
@beikov beikov self-assigned this Jul 8, 2018
beikov added a commit to beikov/blaze-persistence that referenced this issue Jul 25, 2018
…nce handling when order by item is not nullable. Fixes Blazebit#418. Fixes Blazebit#569. Fixes Blazebit#194
beikov added a commit to beikov/blaze-persistence that referenced this issue Jul 25, 2018
…nce handling when order by item is not nullable. Fixes Blazebit#418. Fixes Blazebit#569. Fixes Blazebit#194
beikov added a commit to beikov/blaze-persistence that referenced this issue Jul 25, 2018
…nce handling when order by item is not nullable. Fixes Blazebit#418. Fixes Blazebit#569. Fixes Blazebit#194
beikov added a commit to beikov/blaze-persistence that referenced this issue Jul 27, 2018
…nce handling when order by item is not nullable. Fixes Blazebit#418. Fixes Blazebit#569. Fixes Blazebit#194
beikov added a commit to beikov/blaze-persistence that referenced this issue Jul 30, 2018
…nce handling when order by item is not nullable. Fixes Blazebit#418. Fixes Blazebit#569. Fixes Blazebit#194
beikov added a commit to beikov/blaze-persistence that referenced this issue Jul 30, 2018
…nce handling when order by item is not nullable. Fixes Blazebit#418. Fixes Blazebit#569. Fixes Blazebit#194
beikov added a commit to beikov/blaze-persistence that referenced this issue Jul 31, 2018
…nce handling when order by item is not nullable. Fixes Blazebit#418. Fixes Blazebit#569. Fixes Blazebit#194
beikov added a commit to beikov/blaze-persistence that referenced this issue Aug 1, 2018
…nce handling when order by item is not nullable. Fixes Blazebit#418. Fixes Blazebit#569. Fixes Blazebit#194
beikov added a commit to beikov/blaze-persistence that referenced this issue Aug 1, 2018
…nce handling when order by item is not nullable. Fixes Blazebit#418. Fixes Blazebit#569. Fixes Blazebit#194
beikov added a commit to beikov/blaze-persistence that referenced this issue Aug 1, 2018
…y null precedence handling when order by item is not nullable
beikov added a commit that referenced this issue Aug 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment