Looks good to me (with Michael's comment included).
-Tom
Michael Bouschen wrote:
>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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>
>>>>
>>>>
>>>>