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

Blazebit / blaze-persistence

Rich Criteria API for JPA providers

https://persistence.blazebit.com

Apache License 2.0

727 stars 85 forks source link

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

Closed jwgmeligmeyling closed 6 years ago

jwgmeligmeyling commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Also see https://github.com/Blazebit/blaze-persistence/issues/418

jwgmeligmeyling commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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)