persistence@glassfish.java.net

Re: code review for issue 136

From: Tom Ware <tom.ware_at_oracle.com>
Date: Mon, 27 Mar 2006 09:18:49 -0500

I just had a conversation with Michael Bouschen and we have an idea.

I will enter a task in Issue Tracker to add a query hint that allows
validation to be disabled. For the time being we will continue with the
validation as we have it.

Jielin, you can remove the test.

-Tom

Tom Ware wrote:

>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