[#NUCCORE-24] Detach : some objects, when involved in a graph more than once are not detached to the full FetchDepth - DataNucleus JIRA

DataNucleus Core

Detach : some objects, when involved in a graph more than once are not detached to the full FetchDepth

Created: 19/May/06 10:26 AM Updated: 26/Apr/10 03:37 PM Resolved: 13/Apr/10 09:00 AM

Component/s:

Lifecycles

Affects Version/s:

None

Fix Version/s:

2.1.0.m2

File Attachments:

NUCCORE-24-core.patch (11 kB)
2.

NUCCORE-24-test.jdo.general.patch (5 kB)

Description

« Hide

When we have an object graph that we want to detach and an object is detached early in the detach cycle it is put into a cache of detached objects so that we only return one of that identity per detach operation. This has the unfortunate side effect of causing any further references to that object to only be detached to that fetchDepth.

Please refer to JPOX unit testcase AttachDetachTest.testFetchDepthOnDetachCopyAll

Sort Order:

[ Permalink | « Hide ]

Peter Dettman added a comment - 12/Apr/10 05:59 PM

Proposed patch and test case (both against respective 2.0 branch).

[ Permalink | « Hide ]

Peter Dettman added a comment - 12/Apr/10 06:39 PM

To briefly describe the patch:

1. Fix minor issue relating to the id that detached copy objects were stored against in DetachState: detached copies were being stored against "StateManager.getExternalObjectId", but looked up via "ApiAdapter.getIdForObject". It now consistently uses the ApiAdapter one.

2. DetachState tracks extra information for each detached copy about the state of the FetchPlanState at the time(s) it was detached. Redundant entries (as determined by a "dominance" algorithm) are not kept, so the size of this information ought to be small in practice (and even in the worst-case, bounded by the number of fields for the type).

3. ObjectManagerImpl.detachObjectCopy delegates the detached object cacheing to the StateManager.

4. JDOStateManagerImpl.detachCopy now handles the DetachState interaction. When an already-detached object is encountered again (other than the trivial nested occurrence), the current FetchPlanState is compared to the information from previous detaches to see whether further processing is required. As it stands, the algorithm is conservative. It ensures efficiency for the important case of sibling objects in a collection. It could do better in the general case with access to the FetchPlan information. Some local optimisations remain to be done in DetachState e.g. the full fetchFieldNames list isn't really required in the Entry objects.

5. If the current detached object is not sufficient, detachCopy algorithm is run on the existing object again, being careful to retrieve information about the existing loaded fields so none will be lost.

----------

I've attached a test case, but this patch also fixes "org.datanucleus.tests.knownbugs.AttachDetachTest.testFetchDepthOnDetachCopyAll", which I only belatedly realised was there. They are quite similar.

Issues:

1. Transient objects: in JDOStateManagerImpl, the first time a non-Detachable object is detachCopy'ed, it is made transient. Per this patch, detachCopy may be attempted on that object again. Is it safe to call JDOStateManagerImpl.initializeForDetached on a transient instance? Also, will JDOStateManagerImpl.retrieveDetachState then work on that instance?

2. With the patch applied, the DetachLifecycleListener listeners may be called more than once for a given object during detachCopy. Is this a spec violation? Is it problematic in other ways?

[ Show » ]

Peter Dettman added a comment - 12/Apr/10 06:39 PM To briefly describe the patch: 1. Fix minor issue relating to the id that detached copy objects were stored against in DetachState: detached copies were being stored against "StateManager.getExternalObjectId", but looked up via "ApiAdapter.getIdForObject". It now consistently uses the ApiAdapter one. 2. DetachState tracks extra information for each detached copy about the state of the FetchPlanState at the time(s) it was detached. Redundant entries (as determined by a "dominance" algorithm) are not kept, so the size of this information ought to be small in practice (and even in the worst-case, bounded by the number of fields for the type). 3. ObjectManagerImpl.detachObjectCopy delegates the detached object cacheing to the StateManager. 4. JDOStateManagerImpl.detachCopy now handles the DetachState interaction. When an already-detached object is encountered again (other than the trivial nested occurrence), the current FetchPlanState is compared to the information from previous detaches to see whether further processing is required. As it stands, the algorithm is conservative. It ensures efficiency for the important case of sibling objects in a collection. It could do better in the general case with access to the FetchPlan information. Some local optimisations remain to be done in DetachState e.g. the full fetchFieldNames list isn't really required in the Entry objects. 5. If the current detached object is not sufficient, detachCopy algorithm is run on the existing object again, being careful to retrieve information about the existing loaded fields so none will be lost. ---------- I've attached a test case, but this patch also fixes "org.datanucleus.tests.knownbugs.AttachDetachTest.testFetchDepthOnDetachCopyAll", which I only belatedly realised was there. They are quite similar. Issues: 1. Transient objects: in JDOStateManagerImpl, the first time a non-Detachable object is detachCopy'ed, it is made transient. Per this patch, detachCopy may be attempted on that object again. Is it safe to call JDOStateManagerImpl.initializeForDetached on a transient instance? Also, will JDOStateManagerImpl.retrieveDetachState then work on that instance? 2. With the patch applied, the DetachLifecycleListener listeners may be called more than once for a given object during detachCopy. Is this a spec violation? Is it problematic in other ways?

[ Permalink | « Hide ]

Andy Jefferson added a comment - 13/Apr/10 09:00 AM

Thx for the patch. The one for core is applied to SVN trunk. Not bothered with the testcase, and moved the "knownbugs" case to the main test area now that it passes. DN unit tests pass as before, as does JDO2 TCK and JPA1 TCK.

Re: questions,
1. calling JDOStateManagerImpl.initializeForDetached would likely be a bad thing to do. If the user is calling detachCopy on something non-detachable then it is usually user error anyway (where they forgot to make the class detachable). Not an issue for JPA usage since everything is detachable.

2. whether the call of DetachLifecycleListener causes a JDO spec issue is unknown off the top of my head, but the TCK passes so either is not an issue or is not tested, so leave it alone until the issue comes up.

[ Permalink | « Hide ]

Peter Dettman added a comment - 14/Apr/10 05:31 AM

Thanks for the quick turnaround, and I'm happy with those answers.

Part of the patch was not applied (JDOStateManagerImpl line 3829):
+ // If detached copy already existed, take note of fields previously loaded
+ if (existingDetached != null)
+ {
+ smDetachedPC.retrieveDetachState(smDetachedPC);
+ }

and I'm curious if it was an oversight or deemed to be unnecessary. Granted the current test cases do not reveal a problem, so either way I suppose a failing test case is in order.

[ Permalink | « Hide ]

Andy Jefferson added a comment - 14/Apr/10 05:38 AM

Wasn't applied since your patch did not apply (to JDOStateManagerImpl, and to one of the XXXState classes) and had to be hacked in manually. Will look at it later.