persistence@glassfish.java.net

Re: code review for issue 136

From: Craig L Russell <Craig.Russell_at_Sun.COM>
Date: Mon, 27 Mar 2006 18:28:14 -0800

I think that if the specification disallows IN with Dates, then the
default configuration for Glassfish should disallow it. This allows
for maximum portability of applications. If users run into this and
think it's an issue, then there should be some vendor-specific
configuration, e.g. toplink.query.allowINDate=true that enables the
non-standard behavior. Then users will know that they are writing non-
portable applications.

For me, turning off validation is an overly broad brush that will
encourage users to turn off validation because they have to in order
to access vendor-specific features. Then they don't get validation
for user errors either. But perhaps I misunderstand your preference.

The danger in just allowing this without changing the specification
is that a CTS test can be written to test this assertion ("The
state_field_path_expression must have a string, numeric, or enum
value")and the RI won't pass.

Craig

On Mar 27, 2006, at 5:53 AM, 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

Craig Russell
Architect, Sun Java Enterprise System http://java.sun.com/products/jdo
408 276-5638 mailto:Craig.Russell_at_sun.com
P.S. A good JDO? O, Gasp!