Hi,
Comments inline.
Tom Ware wrote:
> comment inline
>
> Michael Bouschen wrote:
>
>> Hi Jielin,
>>
>> I agree with Tom's comments, you could do something like
>> int index = getDatabaseQuery().getArguments().indexOf(pos);
>> if (index == -1) {
>> throw new IllegalArgumentException(...
>> } else { ...
>>
I will change the code like that.
>> About the tests added to entity-persistence-testing module: how about
>> adding a new test class to the ejbqltesting package that includes
>> input parameter tests: JUnitEJBQLParameterTestSuite. We could then
>> add a couple of other parameter-related tests later. What do you think?
>>
>>
>
> Sounds like a great idea to me.
Okay, I will add it.
Thanks.
Jielin
>
> -Tom
>
>> Regards Michael
>>
>>
>>
>>> Hi Jie,
>>>
>>> One minor coment and one question.
>>>
>>> Comment: In the following lines of code:
>>> --
>>> if (!getDatabaseQuery().getArguments().contains(pos)) {
>>> throw new
>>> IllegalArgumentException(ExceptionLocalization.buildMessage("ejb30-wrong-argument-index",
>>> new Object[]{position, getDatabaseQuery().getEJBQLString()}));
>>> }
>>> int index = getDatabaseQuery().getArguments().indexOf(pos);
>>> --
>>>
>>> Is it possible to do the look-up for 'index' before the if and then
>>> use the value of index to determine if we throw the exception? This
>>> will avoid one lookup.
>>>
>>> Question:
>>>
>>> Can regression tests be added to the entity-persistence-testing
>>> module for these bug fixes. I woudl suggest adding tests in either
>>> in one of the EJBQL test suites defined in:
>>> oracle.toplink.essentials.testing.tests.ejb.ejbqltesting or adding a
>>> EJBQuery test suite to
>>> oracle.toplink.essentials.testing.tests.cmp3.advanced
>>>
>>> Other than that, everything looks good.
>>>
>>> Thanks,
>>> Tom
>>>
>>> jie leng wrote:
>>>
>>>
>>>
>>>> Hi, Tom/Michael,
>>>>
>>>> Here is the fix for glassfish issue 273 and 214.
>>>>
>>>> https://glassfish.dev.java.net/issues/show_bug.cgi?id=273
>>>>
>>>> and
>>>> https://glassfish.dev.java.net/issues/show_bug.cgi?id=214
>>>>
>>>> Please review the change. The change is in EJBQueryImpl.java to
>>>> handle validation check correctly for position query such as:
>>>>
>>>> 1. String ejbql = "SELECT i FROM Item i WHERE i.name = ?1 AND
>>>> i.itemId = ?3);
>>>>
>>>> Query query = em.createQuery(ejbql);
>>>>
>>>> query.setParameter(1, "Jie Leng");
>>>>
>>>> query.setParameter(2, "");
>>>>
>>>> query.setParameter(3, new Integer(1));
>>>>
>>>> It should throw:
>>>>
>>>> [java] Exception in thread "main"
>>>> java.lang.IllegalArgumentException: You have attempted to set a
>>>> parameter at position 2 which does not exist in this query string
>>>> SELECT i FROM Item i WHERE i.name = ?1 AND i.itemId = ?3.
>>>>
>>>> 2. String ejbql = "SELECT i FROM Item i WHERE i.name = ?1 AND
>>>> i.itemId = ?3);
>>>>
>>>> Query query = em.createQuery(ejbql);
>>>>
>>>> query.setParameter(1, "Jie Leng");
>>>>
>>>> query.setParameter(3, new Integer(1));
>>>>
>>>> This query should be processed successfully even though ?2 is missing.
>>>>
>>>>
>>>> Thanks.
>>>>
>>>> Jielin
>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>>
>