Community
Participate
Working Groups
The existing re-attachment logic assumes that the state after reattachment must always be dirty. This isn't necessarily the case; the object could be perfectly CLEAN after re-attachment. Trivial patch in a minute.
[NoMagicNote: SVR-2889]
Created attachment 194230 [details] Patch
Created attachment 194234 [details] Patch v2 I just simplified the if condition (a little bit).
Committed revision 7642.
Committed revision 7643. (Didn't realize you modified the patch, so the previous commit was my own original patch.)
Seems as if this patch breaks Legacy: https://hudson.eclipse.org/hudson/job/emf-cdo-integration/1314/ Is this intentional? Or is this something which does not occur while running the legacy tests locally? Is investigation required? ;)
(In reply to comment #6) > Is investigation required? ;) Yes! Please clarify with Caspar who feels responsibility first ;-)
> Please clarify with Caspar who feels responsibility first ;-) Due to my limited time at the momment (there are some significant Dawn issues) it would be nice if Caspar could clarify whether this behavior is EMF compliant or not. 1.) If it is, I would jump in because this means an implementation problem in Legacy 2.) If not, we should discuss whether it is o.k. to differ from EMF in this case o.k.?
The problem (in Legacy) is this: if you call getContainerID on a clean object's revision you get the real ID object, e.g. a CDOIDObjectLongImpl. Then, if you detach the object from its container, and reattach it to the same container, and call getContainerID again, you don't get the real ID object anymore, but a CDOLegacyAdapter instead. And since calling cdoRevision().data().getContainerID() is exactly what BaseCDORevision.compare(...) does, the comparison yields a spurious CDOFeatureDelta that is interpreted by the reattachment logic as an indication that the object is DIRTY, while in fact the comparison should have yielded no feature deltas and the object should have been put in state CLEAN. I'm not sure which logic is "most at fault" here. I don't understand why an ID object is sometimes wrapped in a CDOLegacyAdapter, and even if we do have good reasons for that, I don't know why our compare logic isn't prepared to deal with that possibility.
Created attachment 194548 [details] Testcase Testcase demonstrating the legacy problem (It's a straight Java source file, not a patch.)
Created attachment 194750 [details] Legacy Patch v1 Hi Caspar, thanks for your investigation. The problem you’ve showed in the test case seems indeed not to be correct. The fix for it was actually quite trivial. I had to adjust Bugzilla_24662_Test, but I think the previous test logic was not 100% correct. Eike, could you please verify this? But I do not really understand what this has to do with the failing tests, because they are still failing.
Marked Patch v2 obsolete as it looks as if it has been applied already.
(In reply to comment #11) > The problem you’ve showed in the test case seems indeed not to be correct. The > fix for it was actually quite trivial. I had to adjust Bugzilla_24662_Test, but > I think the previous test logic was not 100% correct. Now I'm confused. I was concerned that Bugzilla_283985_1_Test started failing in LEGACY but you changed Bugzilla_246622_Test and that in a way that it now fails too for NATIVE. Can you explain why you did that?
Committed revision 7650: - trunk/plugins/org.eclipse.emf.cdo.tests
Created attachment 194803 [details] Patch v3 The root cause seems to be in the compare() method of CDORevisionDeltaImpl as Caspar pointed out. Revision.containerID can in fact be an EObject under certain circumstances in a dirty client transaction. I've added an interface to cdo.common so that the common mechansims can detect this situation and use the CDOID of the EObject instead. Now the correct if branch in the Reattach transition is used and CDOState.CLEAN ends up in the EObject (legacy adapter). Unfortunately the test still fails because some time later and up in the stack cdoInternalSetState() is called again, making the object DIRTY again. It's my impression that this is a pure LEGACY issue now. Martin, can you have a look again? It will help to set a breakpoint in CDOLegacyWrapper.cdoInternalSetState(). I'm currently running all tests. If no other regressions pop up I'll commit this patch...
Committed revision 7651: - trunk/plugins/org.eclipse.emf.cdo - trunk/plugins/org.eclipse.emf.cdo.common
> Martin, can you have a look again? O.k. Assign this one to me. But I've no glue when there is time for it.
Committed revision 7653: - trunk/plugins/org.eclipse.emf.cdo.tests
Ok, I've added this to the 4 offending tests: skipConfig(LEGACY); // TODO Fix bug 344072
Committed revision 7807
Should we resolve this one and open a new one for LEGACY?
Good idea.
The LEGACY apsect of this problem will be tracked in bug 347135.
Available in R20110608-1407
