Bugzilla – Full Text Bug Listing
| Summary: | CommitIntegrityCheck fails for object moved to different resource | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Caspar D. <caspar_d> | ||||||||||||
| Component: | cdo.core | Assignee: | Caspar D. <caspar_d> | ||||||||||||
| Status: | CLOSED FIXED | QA Contact: | Eike Stepper <stepper> | ||||||||||||
| Severity: | normal | ||||||||||||||
| Priority: | P3 | CC: | martin.fluegge, saulius.tvarijonas | ||||||||||||
| Version: | 4.0 | Flags: | stepper:
review+
stepper: review+ |
||||||||||||
| Target Milestone: | --- | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | All | ||||||||||||||
| Whiteboard: | |||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Caspar D.
Created attachment 193779 [details]
Patch v1 (including testcase)
Developing this patch I discovered that resource-containment behavior
is different in Legacy. I added an explicit assertion to demonstrate
this. It'll fail in legacy.
Perhaps Martin can comment?
[NoMagicNote: SVR-2804] Created attachment 193805 [details]
Patch v2
Just inserted two NLs.
Was I too quick to approve the patch? Bugzilla_343471_Test fails in the legacy suites. Or was that something we asked Martin to care for? Caspar already aske me in comment 1 to have a look at the problem. ;) I am currently a bit busy. How urgent is this patch? Would it be enough if I look at it next week? Created attachment 193842 [details]
pure EMF example
Hi Casper,
I had I quick look at the problem. In this case Legacy behaves as EMF itself would do, actually it is EMF which acts. When adding *c2* to *resource2* internally BasicEOjectImpl.eSetResource() is performed on c2. This implementation checks whether isResolveProxy is set for the containment feature (which it is not in our case) and thus the container will be removed. If resolveProxy would had been enabled on the containment feature everything would be o.k. (see. pure EMF example attached)
I hope I do not miss the obvious, but from my point of view the new implementation leads to a non-EMF conform behavior. I think we should discuss whether this is desirable in the case of CDO or not. Maybe Eike has some ideas on this?
(In reply to comment #6) Hi Martin, thanks for your investigation. I don't follow everything you're saying though. Your test demonstrates that indeed the Legacy behavior is standard EMF behavior, and so it follows that it is the native CDO behavior that is questionable. But here's where I lose the plot: > ... checks whether isResolveProxy is set for the containment feature > (which it is not in our case) In model1.ecore, Category.categories has resolveProxies=true. So I don't understand why at run-time eContainmentFeature().isResolveProxies() returns false. (But I'm aware that it does, as you said.) > I hope I do not miss the obvious, but from my point of view the new > implementation leads to a non-EMF conform behavior. Not sure what you mean by "new implementation". My patch doesn't change any containment behavior; it only attempts to fix the partial commit checks to deal with existing containment behavior. Perhaps a separate Bugzilla for sorting out the containment behavior is in order? Please confirm and I'll open one. Thanks. Some experimenting gives me the impression that what is relevant here, is whether the model was generated with 'Containment proxies' = true or not. (With CP=false, resolveProxies=true has no effect -- at least not where cross-resource containment (CRC) is concerned.) Considering that, one way of looking at this issue is as follows: currently, CDO's CRC behavior for native models is as if 'Containment proxies' = true, while in fact it's set to false in the genmodel. Has this behavior somehow been hardcoded? And would the problem be solved if we made it dependent on this config parameter in the genmodel? Hi Caspar, sorry for not being that clear. I admit that when I was talking about the “new implementation” the word “behavior” was a bit misplaced. What I meant was that before your patch the CommitEntegrityCheck at least ensured that the CDOResource could not be in a different “state” than the pure EMF resource, because it threw an exception when the object moved. You are right, model1.ecore claims that *resolveProxy* for the categories features is set to true, but debugging and the generated code shows that it is not. Even regeneration does not change the EPackge implementation. After a while I hunted the problem down to the CDOObjectImpl.eSetResource() line 729. Here the implementation is different to the BasicEObjectImpl and thus the behavior is too. In fact the whole container alignment is only performed if the object has moved to another view. This means that if two resources are opened on the same view and an object moves from one resource to the other, we are not compliant to the native EMF implementation. That is exactly what the happens in the test. When removing this check the behavior between legacy and CDO becomes equals, but your test fails. I think because it makes a wrong assumption about the state of the object. I think that’s why it seams that CDO implementation behaves as if containment proxies is set to true while in fact it is not. I do not know why the CDOObjectImpl.eSetResource was changed in this way, but I would say that the currently implementation contains a bug. I would say we should open a new bug on this issue. Maybe Eike can support us with some information about the reason for the changed implementation in CDOObjectImpl? Created attachment 194038 [details]
Patch v3
Patch v2 contained a bug; attaching v3.
Committed revision 7638: - trunk/plugins/org.eclipse.emf.cdo.tests.mango Created attachment 194134 [details]
Patch v4
Added skipLegacy to the test. Justification is that we have agreed not
to change either Native or Legacy, because no complaints have been raised.
So, Native will continue to behave as if 'Containment proxies' = true,
and Legacy as if CP = false, which means the behavior verified by this
testcase is only available for Native models.
Please do not hit me for been to accurate. But I need to make one small correction to previous statement. Legacy does not behave as if containment proxy is set to *true* but simply follows the settings from genmodel. So if containment proxy is set to *false* Legacy would reflect this in its behaviour while Native simple ignores the CP setting completely. (In reply to comment #13) > [...] while Native simple ignores the CP setting completely. There's no other way because after generation the GenModel settings are not preserved as API ;-( Committed revision 7812 Available in R20110608-1407 |