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

Generated JPA query with incorrect argument binding indexes #2816

Closed
didiez opened this issue May 3, 2021 · 12 comments · Fixed by #2883
Closed

Generated JPA query with incorrect argument binding indexes #2816

didiez opened this issue May 3, 2021 · 12 comments · Fixed by #2883
Assignees
Labels

Comments

@didiez
Copy link

didiez commented May 3, 2021

Observed vs. expected behavior

Given an JPA entity with two boolean fields, one of them annotated with a custom type @Type, when querying by those two fields providing the same boolean value (true, true or false, false) the generated query ends up using ?1 for both arguments (as they are considered a constant).

The generated query is

select user
from User user
where user.active = ?1 and user.admin = ?1  -- note the two arguments as ?1

when the expected would be

select user
from User user
where user.active = ?1 and user.admin = ?2

Steps to reproduce

Here is a sample application to reproduce the problem, with some tests and loggers enabled to show the differences between the generated query and the expected one.

https://github.com/didiez/hibernate-querydsl-type-binding-bug

Environment

Querydsl version: 4.4.0

Querydsl module: querydsl-jpa

Database: tested against oracle 11g and h2 (probably database agnostic)

JDK: 11

Additional details

Might be related to the changes applied to fix #2326 (PR #2354)

@didiez didiez added the bug label May 3, 2021
@jwgmeligmeyling
Copy link
Member

I would expect this issue to have appeared even before #2326 (PR #2354), can you confirm whether this is the case or that the regression was in fact introduced in 4.4.0?

@jwgmeligmeyling jwgmeligmeyling self-assigned this Jun 2, 2021
@jwgmeligmeyling jwgmeligmeyling added this to the 5.0 milestone Jun 2, 2021
@jwgmeligmeyling
Copy link
Member

@F43nd1r should we look into this issue before 5.x?

@didiez
Copy link
Author

didiez commented Jun 2, 2021

I confirm the issue was present before 4.4.0.
I have tested the following versions:

  • 4.4.0
  • 4.3.1
  • 4.2.2
  • 4.1.3
  • 4.0.9
  • 3.7.4 (same behaviour even in 3.x)

@F43nd1r
Copy link
Member

F43nd1r commented Jun 2, 2021

@jwgmeligmeyling looked into it.
Easy fix would be to rip out the optimization for same constants.

Correctly fixing this is very involved.
Maybe a @Type annotated field shouldn't be represented by a BooleanPath at all, would need design for alternatives.
Or Constant could also hold their target type...

I would go ahead with 5.0 first and then someone can look into how to support custom hibernate types.

@jwgmeligmeyling
Copy link
Member

I'm all for ripping out premature optimizations. So I am fine with using multiple parameters for the same value.

ORMs, JDBC drivers and Query Engine Optimizers are perfectly capable of optimizing on their own.

@jwgmeligmeyling
Copy link
Member

An unfortunate side effect of delaying the fix is that we will probably need a 6.0 (due to a signature change in the serializer).

The specific signature change already caused quite a bit of fuzz for people using modern querydsl-sql/querydsl-jpawith olderquerydsl-core` versions (which was easily solved, but generated a bunch of issue reports nonetheless).

@F43nd1r
Copy link
Member

F43nd1r commented Jun 2, 2021

In that case SerializerBase #constantToNamedLabel and #constantToNumberedLabel need to be changed from map to list. That still involves quite a bit of related changes and I won't have time for it today, maybe I could do it this weekend.

@jwgmeligmeyling
Copy link
Member

Or we simply inverse it: labelToConstant. The label will always be unique. I'll see if I can have a look before the weekend.

@jwgmeligmeyling
Copy link
Member

I do see now that such a change would break my personal integrations 😢 So there probably will be others. Not that sure about this change anymore...

@jwgmeligmeyling
Copy link
Member

Closing as duplicate of #1413

@jwgmeligmeyling
Copy link
Member

@F43nd1r apparently someone already did the work in https://github.com/querydsl/querydsl/pull/2000/files (largely outdated PR though, and conflicts with the fix for #2326 provided in #2354 ,

@jwgmeligmeyling jwgmeligmeyling removed this from the 5.0 milestone Jun 3, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 3, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 4, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 4, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 4, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 4, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 4, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 4, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 4, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 4, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 4, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 4, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 4, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 4, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 7, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 7, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 7, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 7, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 7, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 7, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 7, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 7, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 7, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 7, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 8, 2021
jwgmeligmeyling added a commit that referenced this issue Jun 8, 2021
@didiez
Copy link
Author

didiez commented Aug 6, 2021

Working like a charm with the latest and greatest querydsl-5.0.0.
Many thanks @jwgmeligmeyling and @F43nd1r

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants