persistence@glassfish.java.net

Re: code review for issue 136

From: Michael Bouschen <Michael.Bouschen_at_Sun.COM>
Date: Mon, 27 Mar 2006 18:20:30 +0200

Hi Jielin,

please note, I checked in changes of classes EJBQLException and
EJBQLExceptionResource today. Please merge these changes into your
workspace. You need to increment the error number of the new error
message you added, I think it needs to be 8022.

Regards Michael

> Hi Jielin,
>
> Michael is right, it looks like there will be a merge required for your
> exception changes.
>
> -Tom
>
> Michael Bouschen wrote:
>
>> Hi Jielin,
>>
>> the changes look good. Two remarks:
>>
>> - I propose to add a method isString type to the TypeHelper interface
>> and call this in InNode.validate.
>> This should go into TypeHelper.java:
>> /** Returns true if the specified type represents java.lang.String. */
>> public boolean isStringType(Object type);
>> and this should be added to BasicTypeHelper.java:
>> /** Returns true if the specified type represents java.lang.String. */
>> public boolean isStringType(Object type) {
>> return type == getStringType();
>> }
>>
>> - Are you planning to add a test to JUnitEJBQLValidationTestSuite in
>> entity-persistence-tests? I propose to add two test cases: one where
>> the in clause consists of a list of value and another using a subquery.
>>
>> Just a heads-up: I'm also planning to change EJBQLException, so we
>> need to check for conflicts (e.g. using the same number for the
>> constant etc.).
>>
>> Regards Michael
>>
>>
>>
>>> Hi, Michael, Tom,
>>>
>>> Attached is the fix for issue 136 - JPQL validation: Parser not
>>> catching invalid expression type with IN expression.
>>>
>>> According to spec,
>>> in_expression ::= state_field_path_expression [NOT] IN (in_item {,
>>> in_item}* | subquery)
>>> The state_field_path_expression must have a string, numeric or enum
>>> value.
>>> The in_item and the result of subquery must be like the same abstrct
>>> schema type of the state_field_path_expression.
>>>
>>> Added two validation checks in InNode.java.
>>>
>>> Please review the change.
>>>
>>> Thanks.
>>>
>>> Jielin
>>>
>>>
>>>
>
>