Hi Gordon,
Gordon Yorke wrote:
> Hello Markus,
> Good catch on the updateWriteLockValueForWrite method setting of the
> initialWriteLockValue should only be done for an insert moving the
> update into the setupWriteFieldsForInsert method would be better. If
> setInitialWriteLockValue does not exist then it should be created. It
> should be a simple mutator .
I implemented this behavior, please see the attachment. Note that I did
my changes on the FCS branch. I'll switch my workspace to the trunk
version for the bug fix.
> I do not believe there is current support for detecting if a version
> value has been set to null after it has been created. It will need
> to be implemented and should be done as part of this bug.
Do you want me to work on this? I'll send you the code for review.
I also think that we should change
public int compareWriteLockValues(Object value1, Object value2);
in XXXockingPolicy not to throw aNullPointerException if a null value is
passed. But maybe not as part of the fix for issue 635?
Thanks,
-- markus.
> -- Gordon
>
>
> Markus Fuchs wrote:
>
>> Hi Gordon,
>>
>> This fixes my test case. Great!
>>
>> The new method
>> setInitialWriteLockValue(lockValue)
>> would be implemented in ObjectChangeSet, correct?
>>
>> This solution sets the initialWriteLockValue every time the version
>> is increased. I'm not sure if that's an expected behavior, the
>> initial value should be set only once. But that also depends on the
>> implementation of setInitialWriteLockValue(lockValue) and should be
>> documented in the JavaDoc. Could you send me that implementation?
>>
>> I agree, we absolutely need to detect if a user updates an object to
>> have a null version. Is this implemented?
>>
>> If you still want the method
>> public int compareWriteLockValues(Object value1, Object value2);
>> to throw an exception if a null value is passed, we might want to
>> enhance this behaviour to throw an application error rather than a
>> NullPointerException.
>>
>> Thanks,
>>
>> -- markus.
>>
>>
>> Gordon Yorke wrote:
>>
>>> Hello Markus,
>>> With new objects this problem can be easily solved by adding the
>>> following line to
>>> VersionLockingPolicy.updateWriteLockValueForWrite(...)
>>> if (objectChangeSet != null) {
>>> objectChangeSet.setWriteLockValue(lockValue);
>>> objectChangeSet.setInitialWriteLockValue(lockValue); // <--
>>> *** new line.
>>> }
>>>
>>> If the user updates an object to have a null version value we
>>> should probably detect this and throw an exception that notifies the
>>> user of the error they have made.
>>> What do you think,
>>> --Gordon
>>>
>>> -----Original Message-----
>>> From: Markus.Fuchs_at_Sun.COM [mailto:Markus.Fuchs_at_Sun.COM]On Behalf Of
>>> Markus Fuchs
>>> Sent: Thursday, May 11, 2006 2:15 PM
>>> To: Gordon Yorke
>>> Cc: persistence_at_glassfish.com
>>> Subject: Re: [Fwd: [Issue 635] Toplink's VersionLockingPolicy became to
>>> throw a NullPointerException]
>>>
>>>
>>> Hi Gordon,
>>>
>>> Gordon Yorke wrote:
>>>
>>>
>>>
>>>> Hello Markus,
>>>> Can you explain how initialWriteLockValue is null? As the
>>>> object is inserted into the database TopLink should set an initial
>>>> value into the object and the ChangeSet. Is an instance value
>>>> being inserted into the database?
>>>>
>>>>
>>>
>>> I attached my testbase to the bug report. To run it, please unjar
>>> issue635.jar under entity-persistence-tests. Then run the newly created
>>>
>>> oracle.toplink.essentials.testing.tests.cmp3.advanced.EntityMappingsAdvancedJUnitTestCase
>>>
>>>
>>>
>>> Junit test. The situation is:
>>>
>>> An Employee is persisted, and in the same transaction two previously
>>> created Projects are added to the Employee. The NullPointerException
>>> happens at the second commit.
>>>
>>>
>>>
>>>> Also the method you mention
>>>> "setOptimisticLockingPolicyAndInitialWriteLockValue" that
>>>> initializes the value does not exist in TopLink?
>>>>
>>>>
>>>>
>>>
>>> I was referring to
>>>
>>> public void
>>> setOptimisticLockingPolicyAndInitialWriteLockValue(OptimisticLockingPolicy
>>>
>>> optimisticLockingPolicy, AbstractSession session) in ObjectChangeSet
>>>
>>> Thanks,
>>>
>>> -- markus.
>>>
>>>
>>>
>>>> --Gordon
>>>>
>>>> -----Original Message-----
>>>> From: Markus.Fuchs_at_Sun.COM [mailto:Markus.Fuchs_at_Sun.COM]On Behalf Of
>>>> Markus Fuchs
>>>> Sent: Wednesday, May 10, 2006 10:39 PM
>>>> To: ejb3-toplink-ext_at_sun.com
>>>> Subject: [Fwd: [Issue 635] Toplink's VersionLockingPolicy became to
>>>> throw a NullPointerException]
>>>>
>>>>
>>>> Hi TopLink team,
>>>>
>>>> while investigating issue 635, I noticed that the assumption given in
>>>> ObjectChangeSet (in mergeObjectChanges, line 540 of the FCS
>>>> version), is
>>>> not always true:
>>>>
>>>> // optimisticLockingPolicy != null guarantees
>>>> initialWriteLockValue !=
>>>> null
>>>>
>>>> If the instance has an Object type version field (the spec allows
>>>> Integer, Short, Long, and Timestamp) following can be true:
>>>>
>>>> // optimisticLockingPolicy != null && initialWriteLockValue == null
>>>>
>>>> which leads to the NullPointerException described in issue 635.
>>>>
>>>> The member "initialWriteLockValue" can be null for an Object typed
>>>> version field, as it may be initialized by getting the version field's
>>>> value in
>>>>
>>>> public void
>>>> setOptimisticLockingPolicyAndInitialWriteLockValue(OptimisticLockingPolicy
>>>>
>>>> optimisticLockingPolicy, AbstractSession session)
>>>>
>>>> in class ObjectChangeSet.
>>>>
>>>> ===================================================
>>>>
>>>> I see following the alternative fixes:
>>>>
>>>> 1. Let the two implementations of method
>>>>
>>>> public int compareWriteLockValues(Object value1, Object value2);
>>>>
>>>> defined in the interface OptimisticLockingPolicy handle null values
>>>> correctly. The two implementations are in VersionLockingPolicy and
>>>> TimestampLockingPolicy. This seems the preferred solution as long as
>>>> there are no other problems associated with null values in version
>>>> fields.
>>>>
>>>> 2. Add a method to OptimisticLockingPolicy
>>>>
>>>> public Object getInitialWriteLockValue(Object domainObject,
>>>> java.util.Vector primaryKey, AbstractSession session);
>>>>
>>>> which makes sure the return value is never null. This method would
>>>> only
>>>> be called to initialize the variable
>>>> ObjectChangeSet#initialWriteLockValue
>>>>
>>>> I couldn't think of an appropriate default for Timestamp version
>>>> fields,
>>>> maybe new Timestamp(0)? There might be other problems with
>>>> "initialWriteLockValue" *not* being null in some cases.
>>>>
>>>> What do you think? I would be happy to implement one of the above
>>>> proposals if the TopLink team agrees.
>>>>
>>>> Thanks!
>>>>
>>>> -- markus.
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>