Bugzilla – Full Text Bug Listing
| Summary: | [Legacy] Issues with non-containment opposite references in legacy mode | ||
|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Alex Lagarde <alex.lagarde> |
| Component: | cdo.legacy | Assignee: | Eike Stepper <stepper> |
| Status: | CLOSED FIXED | QA Contact: | Eike Stepper <stepper> |
| Severity: | normal | ||
| Priority: | P3 | CC: | alex.lagarde, give.a.damus, stepper |
| Version: | 4.2 | ||
| Target Milestone: | --- | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| Attachments: | |||
Created attachment 209836 [details]
A testcase reproducing issues when modifying bi-directional references
I think these issues can be explained by the fact that during the integration of changes commited by user A, the invalidate() of User B's Transaction calls CDOLegacyWrapper.revisionToInstance(), which calls CDOLegacyWrapper.revisionToInstanceFeature().
When dealing with the "H.correspondingG" reference, and its opposite "G.listOfHs" reference, to determine if it has changed it tests if "object != instance.eContainer". This test is only valid for containment based references : if the reference is not containment based, test should be "object != instance.eGet(feature)".
However, updating the CDOLegacyWrapper does not fix the tests, so maybe I'm missing something here.
Moving all open bug reports to 4.1 because the release is very near and it's hghly unlikely that there will be spare time to address 4.0 problems. Please make sure that your patches can be applied against the master branch and that your problem is not already fixed there!!! Moving all open issues to 4.2. Open bugs can be ported to 4.1 maintenance after they've been fixed in master. Created attachment 220907 [details]
Proposed fix
This problem happens in UML models: an association has member ends, which are the association-end properties. The "memberEnd" reference of the Association EClass is not a containment reference. It has an opposite, the "association" reference of the Property Eccles.
Some of the ends of an association are objects that it contains (in the "ownedEnd" containment reference). For these, CDO does not set the "association" reference because the guard condition in CDOLegacyWrapper::revisionToInstanceFeature(EStructuralFeature) method sees that the object it would inverse-add to the instance is the instance's container.
The problem is that, as the comment in the code indicates, we need the guard to protect against problems with containment references. But, the opposite reference in this case isn't a containment, so the guard shouldn't apply. The attached patch adds this condition to the guard.
The patch is relative to today's 4.1 master branch and is in Git's e-mail patch format.
(In reply to comment #4) > > The patch is relative to today's 4.1 master branch and is in Git's e-mail > patch format. Oops ... I meant the 4.1 *maintenance* branch, of course, as of 10 September. Created attachment 222654 [details]
Updated patch with tests
I've attached an updated patch that
- rebases the fix onto a more recent master (4.2 release)
- adds a JUnit test in a new org.eclipse.emf.cdo.tests.uml plug-in
Created attachment 223011 [details]
New patch in Eclipse workspace format with integrated tests
I've attached a new patch that
- is in Eclipse workspace patch format, not Git e-mail patch
- doesn't add a new UML-based tests plug-in, but has integrated tests
I added new Parent and Child eclasses to the model5 test model, which have both
- bidirectional containment/container references: children <--> parent
- bidirectional cross-references: favourite <--> preferredBy
A new test case is added to the CrossReferenceTest suite to verify that cross-references from an object to its container that are not the opposite of a containment (e.g., Child::preferredBy) are correctly persisted and re-loaded. The last assertion of the test fails without my fix; passes with my fix.
All other tests pass.
NOTE some other non-obvious changes in the generated test model5:
- lots of formatting changes (I followed the steps in the wiki)
- EMF now won't reload a generator model if the file extension is different
from "*.genmodel". So, I renamed the legacy genmodel file from
"model5.legacy-genmodel" to "model5-legacy.genmodel"
Good catch! And small fix + good test ;-) I've preserved your author name in these Git commits: 10a7b6b99cf6eaa8f35000320ccb8b0544d2bca8 9e9b1aac1fd89d2bbf7ac39c823ee996e4f3d832 They contain all changes but the re-generation results (which are mostly comment reformattings, strange). For the sake of making the IP Log easier (no CQ needed) can you please attach a new patch that only contains the changes to these 4 files? I've regenerated and committed the reult under my user ID ;-) (In reply to comment #1) > Created attachment 209836 [details] > A testcase reproducing issues when modifying bi-directional references Hi Alex, You've invested substantially into good test cases and I'd like to commit them. Unfortunately I can't apply your patch to the test model anymore. Is it possible that you provide me with a updated patch of just model6.ecore? I guess that would be easier than switching to the latest state of model5.ecore (with Christian's changes)... Created attachment 223036 [details]
Reduced patch for IP review
Attached a new patch reduced to just the "intellectually significant" (if I may phrase it thus) content, including:
- fix in the CDOLegacyWrapper
- new test case in CrossReferenceTest class
- updated model5.ecore
- updated model5.genmodel
The renaming of the legacy genmodel makes noise in a patch and its changes are substantially the same as in model5.genmodel.
Thanks, Eike!
Comment on attachment 223036 [details]
Reduced patch for IP review
Awesome, thx!
Available in R20130613-1157 (4.2) |
Created attachment 209833 [details] An updated version of model 6 introducing bi-directionnal references Hi everyone ! I've detected issues when dealing with legacy models having non-containment opposite references. I've written some tests reproducing the issue, trying to identify problematic use cases.