Community
Participate
Working Groups
Create a model with 2 objects, x and y with x.f = y, f being a 1:1 containment feature with resolveProxies=true, and y having a feature g that is the eOpposite of x.f. Add x to a resource, thus putting both x and y in the NEW state, but do not commit. Then, call y.eUnset(g). This should detach y from the model. After this call, x.f still gives g while it should give null.
Created attachment 192533 [details] Testcase (as a patch)
I still can't make sense of the code in CDOObjectImpl.cdoInternalPostDetach. At line 364 (rev 7577) there is a check for resolveProxies=true. If true, then the object being detached is added *back* to the containing feature. (It was just removed through eBasicRemoveFromContainer as part of the same operation...) Second, the CDOObjectImpl.adjustOppositeReference that gets called to do the job, has the following JavaDoc: "Adjust the reference ONLY if the opposite reference used CDOID. This is true ONLY if the state of this was not CDOState.NEW." But there is no check to this effect either in the caller or in the method implementation. I'm puzzled as to how all of this is supposed to work. Comments please?
I've run through it once and can also not make a sense out of it. Will think about it again later... I don't think it's related to this issue, but I doubt that it's legal to do this call: ref_parToEl.setResolveProxies(true); Shouldn't a frozen package prevent changes from happening?
Created attachment 192550 [details] Test v2
(In reply to comment #3) > I don't think it's related to this issue, but I doubt that it's legal to do > this call: > ref_parToEl.setResolveProxies(true); > Shouldn't a frozen package prevent changes from happening? I guess so. But for reproducing this bug it was pretty convenient that it doesn't :-)
As I mentioned earlier, the call to #adjustOppositeReference at there is a call to CDOObjectImpl.java:369 (rev. 7589) plays a crucial role in this. When I disable this call, the problem is solved, and all unit test still pass except one: ContainmentTest.testObjectNotSameResourceThanItsContainerCDOANDXMI which fails with a ClassCastException, when, at the bottom of the test case, it receives a proxy object instead of an EObject. Eike and I have looked at the test case and wondered what behavior is supposed to be verified here. Apparently, the expected behavior is that when an object in a non-CDO resource contains an object in a CDO resource, and the latter is removed from its resource, the proxy pointing from the first object to the second, is supposed to remain resolvable, even though the target has been removed from its resource. We don't understand why this behavior is desirable and/or sensible. ** Victor: ** It appears this behavior was added by Simon at your request. Can you comment on this?
Caspar, from your comments, the test-case doesn't seem to make sense to me either. I can't recall why that was added. I can only think of it being related to our usage of GMF diagrams, where EObjects in that resource reference CDOObjects. But I believe in our case diagrams never contain the CDOObject in a CDOResource, but simply reference it. I can't see how such CDOObject should be resolvable even when it has been removed from its container CDOResource. From my side, I would say that test-case is wrong unless someone proves it to be right. If something gets broken in our app, I'll let you know :)
I'd be fine to have the offending code and the test case removed.
Created attachment 193460 [details] Patch (including testcase)
[NoMagic internal note: SVR-2649]
Created attachment 193462 [details] Patch v2 I just sorted all bugzilla tests in AllConfigs.java
Committed revision 7626.
Reopening.. seems this has caused some breakage in the legacy tests.
Created attachment 193540 [details] Test failures
Martin, can you have a look if these failures are caused by bad test logic (e.g. getCDOObject missing) or if the legacy mode is based on some bad assumptions that have changed now?
Sure, I'll habe a look at it. But I wonder that this Legacy problem did not occur before the patch was committed. Did something else change meanwhile or was legacy simply left out when testing this patch?
I admit I didn't run the legacy suite, as the patch seemed straightforward enough... and even now, the incomprehensible thing is that the changes to our logic do not seem to be the cause of the problem, as all legacy tests pass fine. It is, strangely, the presence of Bugzilla_341875_Test in the suite, that (apparently) is causing the failures... The suite passes cleanly when I take it out. Investigating...
Created attachment 193560 [details] Patch (incremental) There are 2 problems. This fixes 1 of them, leaving the other (failure of Bugzilla_341875_Test) to be investigated..
Actually, it looks like it fixed both problems. Flagging for review.
Created attachment 193588 [details] Legacy patch v1 Found it. Actually the legacy wrapper did exactly the same thing Caspar described in comment 2. In CDOLegacyWrapper. cdoInternalPostDetach it checked whether isResolveProxy is true and then added the value back. I also did not understand why this was done, so I just followed your strategy and simply removed these lines, hoping that nothing worse happens ;) It's strange the both CDOObject implementations used this curious alignment. So it must have been import in the past. Since all tests are passing it seems to be a relict from older day ;) I attached a patch for legacy.
Created attachment 193607 [details] Legacy patch v2 I fixed the warning in my previous patch by commenting out the related method. I left a TODO to remove this method if it is ensured that it is definitely not needed anymore.
Committed revision 7629.
Available in R20110608-1407
