persistence@glassfish.java.net

Re: code review for issue 136

From: Tom Ware <tom.ware_at_oracle.com>
Date: Mon, 27 Mar 2006 08:53:51 -0500

It would be my preference if either validation did not disallow things
that will work on some databases or validation could be turned off.

What does everyone else think?

-Tom

jie leng wrote:

>I test it agianst oracle, the test is working and the IN with dates
>return the correct values.
>
>Thanks.
>
>Jielin
>
>Tom Ware wrote:
>
>
>
>>The spec says the following:
>>
>>4.6.8 In Expressions
>>The syntax for the use of the comparison operator [NOT] IN in a
>>conditional expression is as follows:
>>in_expression ::=
>>state_field_path_expression [NOT] IN ( in_item {, in_item}* | subquery)
>>in_item ::= literal | input_parameter
>>The state_field_path_expression must have a string, numeric, or enum
>>value
>>
>>So, I guess the test can be removed, but I think the fact that it
>>works on some databases opens up the debate of whether it should be
>>disallowed by validation. I know it's a fine line, but, did the test
>>actually work, or just not fail? (i.e. did the IN with dates return
>>correct values?)
>>
>>-Tom
>>
>>jie leng wrote:
>>
>>
>>
>>>Hi, Michael, Tom,
>>>
>>>I ran entity-persistence-tests on the change I made for InNode. The
>>>JUnitEJBQLDateTimeTestSuite.testTimestampIn() failed because the
>>>validation in InNode.java only allows string, numeric and enum type
>>>according to the spec. Is the test for timestamp using IN still
>>>valid? Should the test be removed?
>>>
>>>Thanks.
>>>
>>>Jielin
>>>
>>>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
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>

-- 
Tom Ware
Principal Software Engineer
Oracle Canada Inc.
Direct: (613) 783-4598
Email: tom.ware_at_oracle.com