persistence@glassfish.java.net

Re: code reivew for issue 269

From: Michael Bouschen <Michael.Bouschen_at_Sun.COM>
Date: Fri, 06 Oct 2006 15:37:22 +0200

Hi Jielin,

this is an interesting issue, because I think we have a more general
validation problem here. Today the compiler does not check whether the
operand types of a comparison expression (=, <>, <, ...) are compatible.
This is caught by the underlying query runtime. I think we should add
this validation to the query compiler. But this is not part of issue
269, it should be a separate issue.

I see two problems with the proposed fix:

(1) The fix only catches the case where the enum value is on the left
and the non-enum is on the right, e.g.
  SELECT e FROM Employee e WHERE e.status = 1
We also need to catch the swapped case:
  SELECT e FROM Employee e WHERE 1 = e.status

I propose to change the code in EqualsNode.validate to something like:
  if (typeHelper.isEnumType(leftType) &&
!typeHelper.isEnumType(rightType)) {
    throw
EJBQLException.invalidEnumEqualExpression(typeHelper.getTypeName(leftType));
  } else if (typeHelper.isEnumType(rightType) &&
!typeHelper.isEnumType(leftType)) {
    throw
EJBQLException.invalidEnumEqualExpression(typeHelper.getTypeName(rightType));
  }

(2) The new error message says: "Invalid enum equal expression, expected
{0} constants by using parameter binding.". This sounds like I have to
compare an enum type field with an input parameter. But the spec allows
several other enum comparisons:
- An enum literal, e.g.
  SELECT e FROM Employee e WHERE e.status =
mypackage.EmployeeStatus.FULL_TIME
- Another enum type field.
- A subquery returning a single enum value.
Maybe you can change the error message to something like:
Invalid enum equal expression, cannot compare enum value of type {0}
with a non enum value of type {1}.

What do you think?

Regards Michael

> Hi, Michael,
>
> Here is the modified code for
> https://glassfish.dev.java.net/issues/show_bug.cgi?id=269. It does not
> throw correct error message when compare enum with wrong data type in
> the query.
>
> Please review the change.
>
> Thanks.
>
> Jielin
>
>
>