persistence@glassfish.java.net

Re: code review for issue 1388

From: jielin <Jie.Leng_at_Sun.COM>
Date: Thu, 09 Nov 2006 10:38:47 -0800

Michael Bouschen wrote:

> Hi Jielin,
>
> great you fixed the problem.
>
>> Hi, Tom,
>>
>> Attahced is the fix for issue 1388. Could you please review the fix?
>> https://glassfish.dev.java.net/issues/show_bug.cgi?id=1388.
>>
>> The problem is that a bulk UPDATE statement setting a field of an
>> embedded instance changes a column mapped to a field of the entity
>> instance, if the field of the embedded instance has the same name as
>> a field of the entity. So
>>
>> Query "UPDATE Customer c SET c.country.name = 'CHANGED'" generates
>> "UPDATE CUSTOMER_TABLE SET NAME = 'CHANGED'"
>> instead of "UPDATE CUSTOMER_TABLE SET COUNTRY_NAME = 'CHANGED'"
>>
>> The causing is in ExpressionQueryMechanism:prepareUpdateAll(). When
>> mapping field to column, the following code is only taking field name
>> as a string argument.
>> field =
>> getDescriptor().getObjectBuilder().getFieldForQueryKeyName(fieldExpression.getName());
>>
>> So 'c.country.name' is only passing 'name', once there is field name
>> matching at the class level, it will return the column.
>>
>> The fix for this is before calling the above code, check if the field
>> is at class level.
>
> Just out of curiosity:
> with your fix the above code is only executed if the base expression
> of fieldExpression is an ExpressionBuilder (which basically means a
> direct field access). The code then looks up the field using the
> descriptor of the DatabaseQuery instance and the DatabaseQuery
> instance is bound to a specific entity class. Can it happen that the
> ExpressionBuilder is bound to a different class such that the lookup
> checks the wrong class? Maybe not with an bulk UPDATE statement,
> because it may only have a single class in the UPDATE clause. But I'm
> not sure whether the code is used in a different context, too.

The fix is in method prepareUpdateAll(). and I double checked that it is
only used for update. Select statement goes through different code path.

Thanks.

Jielin

>
> Thanks!
>
> Regards Michael
>
>> Tested with entity-persistence-tests.
>>
>> Thanks.
>>
>> Jielin
>