Hi Jielin,
looks good, just one comment: in method InNode.validate we should only
call isAssignableFrom if leftType is not null:
if ((leftType != null) &&
!typeHelper.isAssignableFrom(leftType, nodeType))
Please call typeHelper.getTypeName(leftType) instead of
leftType.toString in order to get the name of the type.
Regards Michael
> 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
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>
>>>
>>>
>>
>