persistence@glassfish.java.net

Re: code review for issue 136

From: Tom Ware <tom.ware_at_oracle.com>
Date: Tue, 28 Mar 2006 09:28:53 -0500

I agree that the default configuration should be for Glassfish to
validate according to the way this feature is described in the spec.

I would prefer a more general flag than:
toplink.query.allowINDate=true. I am concerned that we will find quite
a number of these areas where the specification disallows things that
work on specific vendors and think adding a property for each one is
overkill.

-Tom


Craig L Russell wrote:

>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!
>
>
>