Add convenience methods to get persistent instance and detached instance from InstanceLifecycleEvent

• Key: JDO-282
• Type: Improvement
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Craig Russell
• Reporter: Matthew T. Adams
• Component/s: api2
• Fix Version/s: JDO 2 rc1

Description

The source and target object differ as to whether they are the persistent or detached instance in class InstanceLifecycleEvent depending upon whether the event object is given in the preDetach, postDetach, preAttach, or postAttach callbacks. I propose adding two convenience methods that makes it obvious from the API which object the user is getting.

Comments

[Matthew T. Adams]

Here's the patch. This was harder than I thought, which, IMHO, illustrates its value. I created the patch relative to the directory trunk/api20. I would also like to see this included in the upcoming 2.0-beta release if possible.

[Matthew T. Adams]

In case anyone were wondering, the reason for the "boolean isPreCallback" argument on the getPersistentInstance and getDetachedInstance is that the event enums do not indicate whether the event is a preDetach/preAttach or postDetach/postAttach. The event enums for everything are only CREATE, LOAD, STORE, etc. instead of POST_CREATE, POST_LOAD, PRE_STORE, etc.

* Should the enums be refactored to include PRE & POST prefixes, or
* should we add a boolean field to the object indicating whether it's a pre- or post-event object, or
* just do nothing and preserve the boolean isPreCallback arguments on the convenience methods?

[Craig Russell]

I'd prefer to avoid the isPreCallback argumment entirely. I think if we add a line to the specification that the target is only allowed on the postAttach and postDetach events, then the existence of the target tells you what you need to know. So the code doesn't need the argument, but can simply return either the source or target depending on whether target is null.

For example,

+ public Object getDetachedInstance() {
boolean isPreCallback = (target == null);
+ switch (getEventType()) {
+ case DETACH:
+ return isPreCallback
+ ? null // preDetach: no detached instance yet
+ : getSource(); // postDetach: source is detached instance
+ case ATTACH:
+ return isPreCallback
+ ? getSource() // preAttach: source is detached instance
+ : target // postAttach: target is detached instance
+ }
+
+ // for all other events, there is no detached instance
+ return null;
+ }

OK, attached is the refactored patch. Please review and comment. I opted not to use the boolean at all, as isPreCallback and other variants of the name either weren't correct or just weren't descriptive enough.

[Craig Russell]

Looks good to go.

[Matthew T. Adams]

The JDO 2.0 specification needs to be updated to reflect the addition of the methods getPersistentInstance and getDetachedInstance to the class InstanceLifecycleEvent.

[Matthew T. Adams]

Assigning this to Craig, as he is responsible for updating the spec.

[Craig Russell]

The change was committed to the repository and will be released with the Release Candidate 1. The specification has been updated.