Expressions toString method serializes incorrectly #638

Closed
Ghoughpteighbteau opened this Issue Jan 24, 2014 · 4 comments

Comments

Projects
None yet
2 participants
@Ghoughpteighbteau

If you say A.a.id.eq(theId).and(B.b.on.eq(false).or(B.b.id.eq(otherId)));

then the generated condition for JPQL will be incorrect

the expected result is a.id = 1 && ( b.on = true || b.id = 5 ) but instead a.id = 1 && b.on = true || b.id = 5 is being generated

based on bracketing rules, that would be evaluated like this: (a.id = 1 && b.on = true) || b.id = 5

my I'm seeing this bug by running a sort of "Does this problem handler deal with these problem causers?" function

qProblemCauser.id.eq(problemCauser.getId())
    .and(qProblemSolver.causersIHandle.isEmpty()
          .or(qProblemCauser.in(qProblemSolver.causersIHandle)));

which should return the problem causer if he matches the criteria, but if the problemSolver has a list of specific causers it handles, then it discriminates. Otherwise it catches everything.

the queries a little more complex, there are other filters at play but this is the basic problem. The generated clause is getting flattened out and evaluates incorrectly.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Jan 24, 2014

Member

Did you look at the generated JPQL or the toString() result of the Expression? I created a test for it:

@Test
public void And_Or() {
    //A.a.id.eq(theId).and(B.b.on.eq(false).or(B.b.id.eq(otherId)));
    QCat cat = QCat.cat;
    Predicate pred = cat.id.eq(1).and(cat.name.eq("Kitty").or(cat.name.eq("Boris")));
    JPQLSerializer serializer = new JPQLSerializer(HQLTemplates.DEFAULT);
    serializer.handle(pred);
    assertEquals("cat.id = ?1 and (cat.name = ?2 or cat.name = ?3)", serializer.toString());
    assertEquals("cat.id = 1 && cat.name = Kitty || cat.name = Boris", pred.toString());
}

So it appears the toString() serialization should be tuned to take the operator precedence correctly into account, but the JPQL serialization works.

Member

timowest commented Jan 24, 2014

Did you look at the generated JPQL or the toString() result of the Expression? I created a test for it:

@Test
public void And_Or() {
    //A.a.id.eq(theId).and(B.b.on.eq(false).or(B.b.id.eq(otherId)));
    QCat cat = QCat.cat;
    Predicate pred = cat.id.eq(1).and(cat.name.eq("Kitty").or(cat.name.eq("Boris")));
    JPQLSerializer serializer = new JPQLSerializer(HQLTemplates.DEFAULT);
    serializer.handle(pred);
    assertEquals("cat.id = ?1 and (cat.name = ?2 or cat.name = ?3)", serializer.toString());
    assertEquals("cat.id = 1 && cat.name = Kitty || cat.name = Boris", pred.toString());
}

So it appears the toString() serialization should be tuned to take the operator precedence correctly into account, but the JPQL serialization works.

timowest added a commit that referenced this issue Jan 24, 2014

@Ghoughpteighbteau

This comment has been minimized.

Show comment
Hide comment
@Ghoughpteighbteau

Ghoughpteighbteau Jan 24, 2014

huh. Maybe my complicated little query has a logical error, and the toString bug threw me off?

oh crud! I must have a bug somewhere else...

select asset
from ProblemHandler qPh, ProblemCauser qPc
where qPh = ?1 and (qPc.id = ?2 and (qPh.watchAreaSet is empty or qPc member of qPh.watchAssetSet) and ...

ok, so I guess the bug report is that the toString is misleading. 😅

huh. Maybe my complicated little query has a logical error, and the toString bug threw me off?

oh crud! I must have a bug somewhere else...

select asset
from ProblemHandler qPh, ProblemCauser qPc
where qPh = ?1 and (qPc.id = ?2 and (qPh.watchAreaSet is empty or qPc member of qPh.watchAssetSet) and ...

ok, so I guess the bug report is that the toString is misleading. 😅

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Jan 24, 2014

Member

Ok, I can take a look at the toString() logic. Could you update the issue title?

Member

timowest commented Jan 24, 2014

Ok, I can take a look at the toString() logic. Could you update the issue title?

@timowest timowest added the fixed label May 29, 2014

@timowest timowest added this to the 3.4.0 milestone May 29, 2014

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Jun 9, 2014

Member

Released in 3.4.0

Member

timowest commented Jun 9, 2014

Released in 3.4.0

@timowest timowest closed this Jun 9, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment