Issue Details (XML | Word | Printable)

Key: NUCCORE-525
Type: Bug Bug
Status: Closed Closed
Resolution: Fixed
Priority: Major Major
Assignee: Peter Dettman
Reporter: Peter Dettman
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
DataNucleus Core

JDOPersistenceManager/Factory addInstanceLifecycleListener methods inconsistent with JDO javadoc/spec

Created: 17/May/10 06:49 AM   Updated: 04/Oct/12 05:02 PM   Resolved: 07/Jun/10 04:40 PM
Component/s: Persistence
Affects Version/s: 2.1.0.m2
Fix Version/s: 2.1.0.release


 Description  « Hide
JDO spec: "11.9 Registering for life cycle events", "12.15 Life-cycle callbacks" (http://people.apache.org/~clr/jdo-2010-04-09.pdf)
Javadoc: http://db.apache.org/jdo/api23/apidocs/javax/jdo/PersistenceManagerFactory.html#addInstanceLifecycleListener(javax.jdo.listener.InstanceLifecycleListener, java.lang.Class[]), http://db.apache.org/jdo/api23/apidocs/javax/jdo/PersistenceManager.html#addInstanceLifecycleListener(javax.jdo.listener.InstanceLifecycleListener, java.lang.Class...)

1. "11.9 If the listener is already registered for life cycle events, the classes are added to the set of classes being listened to". If the same listener is added in multiple calls, the classes from each call should be merged into a single LifecycleListenerForClass object.

2. "11.9 The addInstanceLifecycleListener and removeInstanceLifecycleListener methods are considered to be configuration methods and can only be called when the PersistenceManagerFactory is configurable (before the first getPersistenceManager is called)". With the missing "assertConfigurable" guards in place, the current code for auto-adding the new listeners to existing PMs is also superfluous.

3. The spec says what to do if the 'classes' parameter of PMF.addInstanceLifecycleListener is itself null, but not what to do in case individual elements of the array are null (or if the array is empty). I propose to ignore null elements and empty arrays (currently there would probably be NPEs in some cases).

4. The considerations for 1 & 3 above also apply to the PM.addInstanceLifecycleListener method (12.15).

5. We should make defensive copies of the 'classes' parameter as necessary to guard against subsequent modification of the elements.

6. Relevant test cases should also be contributed to the JDO TCK.


Peter Dettman made changes - 17/May/10 06:57 AM
Field Original Value New Value
Description JDO spec: "11.9 Registering for life cycle events", "12.15 Life-cycle callbacks" (http://people.apache.org/~clr/jdo-2010-04-09.pdf)
Javadoc: http://db.apache.org/jdo/api23/apidocs/javax/jdo/PersistenceManagerFactory.html#addInstanceLifecycleListener(javax.jdo.listener.InstanceLifecycleListener, java.lang.Class[]), http://db.apache.org/jdo/api23/apidocs/javax/jdo/PersistenceManager.html#addInstanceLifecycleListener(javax.jdo.listener.InstanceLifecycleListener, java.lang.Class...)

1. "11.9 If the listener is already registered for life cycle events, the classes are added to the set of classes being listened to". If the same listener is added in multiple calls, the classes from each call should be merged into a single LifecycleListenerForClass object.

2. "11.9 The addInstanceLifecycleListener and removeInstanceLifecycleListener methods are considered to be configuration methods and can only be called when the PersistenceManagerFactory is configurable (before the first getPersistenceManager is called)". With the missing "assertConfigurable" guards in place, the current code for auto-adding the new listeners to existing PMs is also superfluous.

3. The spec says what to do if the 'classes' parameter of PMF.addInstanceLifecycleListener is itself null, but not what to do in case individual elements of the array are null (or if the array is empty). I propose to ignore null elements and empty arrays (currently there would probably be NPEs in some cases).

4. The considerations for 1 & 3 above also apply to the PM.addInstanceLifecycleListener method (12.15).

5. Relevant test cases should also be contributed to the JDO TCK.
JDO spec: "11.9 Registering for life cycle events", "12.15 Life-cycle callbacks" (http://people.apache.org/~clr/jdo-2010-04-09.pdf)
Javadoc: http://db.apache.org/jdo/api23/apidocs/javax/jdo/PersistenceManagerFactory.html#addInstanceLifecycleListener(javax.jdo.listener.InstanceLifecycleListener, java.lang.Class[]), http://db.apache.org/jdo/api23/apidocs/javax/jdo/PersistenceManager.html#addInstanceLifecycleListener(javax.jdo.listener.InstanceLifecycleListener, java.lang.Class...)

1. "11.9 If the listener is already registered for life cycle events, the classes are added to the set of classes being listened to". If the same listener is added in multiple calls, the classes from each call should be merged into a single LifecycleListenerForClass object.

2. "11.9 The addInstanceLifecycleListener and removeInstanceLifecycleListener methods are considered to be configuration methods and can only be called when the PersistenceManagerFactory is configurable (before the first getPersistenceManager is called)". With the missing "assertConfigurable" guards in place, the current code for auto-adding the new listeners to existing PMs is also superfluous.

3. The spec says what to do if the 'classes' parameter of PMF.addInstanceLifecycleListener is itself null, but not what to do in case individual elements of the array are null (or if the array is empty). I propose to ignore null elements and empty arrays (currently there would probably be NPEs in some cases).

4. The considerations for 1 & 3 above also apply to the PM.addInstanceLifecycleListener method (12.15).

5. We should make defensive copies of the 'classes' parameter as necessary to guard against subsequent modification of the elements.

6. Relevant test cases should also be contributed to the JDO TCK.
Peter Dettman made changes - 17/May/10 06:57 AM
Comment [ 6. We should make defensive copies of the 'classes' parameter to guard against subsequent modification of the elements.
]
Peter Dettman added a comment - 07/Jun/10 06:54 AM
Re: 3., section 12.6 Null Management of the spec:

"In the APIs that follow, Object[] and Collection are permitted parameter types. As
these may contain nulls, the following rules apply. A12.6-3 [Null arguments to APIs that take an Object parameter cause the API to have noeffect. ] A12.6-4 [Null arguments to APIs that take Object[] or Collection will cause the API to throw NullPointerException. ] A12.6-5 [Non-null Object[] or Collec-
tion arguments that contain null elements will have the documented behavior for non-
null elements, and the null elements will be ignored.]"

Even though this does not seem directly applicable to 11.9, it gives me confidence that what I propose in 3. above is reasonable.

Peter Dettman added a comment - 07/Jun/10 02:18 PM
Re: 2. The sense of "configurable" appears to be slightly different in this context ("before getPersistenceManager is called") vs. JDOHelper.getPMF (not configurable = "the setXXX methods will throw an exception").

Peter Dettman added a comment - 07/Jun/10 04:40 PM
Fixed in trunk.

Note: I have taken "configurable" here to mean "no PMs created yet", as per previous comment.

Peter Dettman made changes - 07/Jun/10 04:40 PM
Status Open [ 1 ] Resolved [ 5 ]
Fix Version/s 2.1.0.release [ 10836 ]
Resolution Fixed [ 1 ]
Andy Jefferson made changes - 11/Jun/10 01:38 PM
Status Resolved [ 5 ] Closed [ 6 ]
Andy Jefferson made changes - 04/Oct/12 05:02 PM
Component/s Persistence [ 10200 ]
Component/s JDO [ 10201 ]