Hi, Tom, Michael,
I just conferenced Michael and Marina. For issue 136, there are two
parts of validation: first part validates the allowed data type, second
part checks for compatibility of IN expression data type. So to allow
user still use timestamp or other type that supported by databases, the
first part of validation will not be checked in until we fix issue 481 -
add query hint to disable jpql validation :
https://glassfish.dev.java.net/issues/show_bug.cgi?id=481
I will check in the second part of fix. So no tests will be removed.
issue 136 will be adding all the discussion we had and downgrade to p4 .
Attached the code including InNode change and one added error message
and one validation test for compatibility. Please review it.
Thanks.
Jielin
Tom Ware wrote:
> 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
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>
>>
>>
>