Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 333299 - Legacy fails when EClasses containing references are removed from an EPackage
Summary: Legacy fails when EClasses containing references are removed from an EPackage
Status: CLOSED FIXED
Alias: None
Product: EMF
Classification: Modeling
Component: cdo.legacy (show other bugs)
Version: 4.0   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Martin Fluegge CLA
QA Contact: Eike Stepper CLA
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 333291
  Show dependency tree
 
Reported: 2010-12-29 06:13 EST by Martin Fluegge CLA
Modified: 2012-06-19 07:08 EDT (History)
0 users

See Also:
stepper: review-


Attachments
Test v1 (4.02 KB, patch)
2010-12-29 06:14 EST, Martin Fluegge CLA
no flags Details | Diff
Test v2 (3.88 KB, patch)
2010-12-29 11:22 EST, Martin Fluegge CLA
no flags Details | Diff
Patch v1 (1.02 KB, patch)
2010-12-29 11:33 EST, Martin Fluegge CLA
no flags Details | Diff
Combined patch v1 (4.82 KB, patch)
2010-12-31 08:34 EST, Eike Stepper CLA
no flags Details | Diff
Patch v2 (10.20 KB, patch)
2011-01-01 15:50 EST, Martin Fluegge CLA
no flags Details | Diff
Patch vA - code improvements (11.08 KB, patch)
2011-01-02 06:15 EST, Martin Fluegge CLA
no flags Details | Diff
Patch A v2 - ready to be committed (10.25 KB, patch)
2011-01-02 07:14 EST, Eike Stepper CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Fluegge CLA 2010-12-29 06:13:08 EST
When removing an EClass from an EPackage the exception below is thrown. Seems that there is something wrong in retrieving the correct list.

java.lang.ClassCastException: org.eclipse.emf.internal.cdo.CDOLegacyAdapter
	at org.eclipse.emf.cdo.spi.common.revision.BaseCDORevision.getList(BaseCDORevision.java:583)
	at org.eclipse.emf.cdo.spi.common.revision.BaseCDORevision.getList(BaseCDORevision.java:577)
	at org.eclipse.emf.cdo.spi.common.revision.BaseCDORevision.size(BaseCDORevision.java:437)
	at org.eclipse.emf.internal.cdo.CDOLegacyWrapper.cdoInternalPostDetach(CDOLegacyWrapper.java:201)
	at org.eclipse.emf.internal.cdo.CDOStateMachine.detach(CDOStateMachine.java:280)
	at org.eclipse.emf.cdo.eresource.impl.CDOResourceImpl.detached(CDOResourceImpl.java:1172)
	at org.eclipse.emf.ecore.impl.BasicEObjectImpl.eBasicSetContainer(BasicEObjectImpl.java:1334)
	at org.eclipse.emf.ecore.impl.EClassImpl.eInverseRemove(EClassImpl.java:1412)
	at org.eclipse.emf.ecore.impl.BasicEObjectImpl.eInverseRemove(BasicEObjectImpl.java:1447)
	at org.eclipse.emf.ecore.util.EcoreEList.inverseRemove(EcoreEList.java:318)
	at org.eclipse.emf.common.notify.impl.NotifyingListImpl.remove(NotifyingListImpl.java:716)
	at org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_XXX_Test.testMoveEcoreElement(Bugzilla_XXX_Test.java:67)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
	at java.lang.reflect.Method.invoke(Method.java:585)
	at junit.framework.TestCase.runTest(TestCase.java:168)
	at org.eclipse.net4j.util.tests.AbstractOMTest.runBare(AbstractOMTest.java:150)
	at org.eclipse.emf.cdo.tests.config.impl.ConfigTest.runBare(ConfigTest.java:465)
	at junit.framework.TestResult$1.protect(TestResult.java:110)
	at junit.framework.TestResult.runProtected(TestResult.java:128)
	at junit.framework.TestResult.run(TestResult.java:113)
	at junit.framework.TestCase.run(TestCase.java:124)
	at org.eclipse.net4j.util.tests.AbstractOMTest.run(AbstractOMTest.java:196)
	at junit.framework.TestSuite.runTest(TestSuite.java:232)
	at org.eclipse.emf.cdo.tests.config.impl.ConfigTestSuite$TestWrapper.runTest(ConfigTestSuite.java:126)
	at junit.framework.TestSuite.run(TestSuite.java:227)
	at junit.framework.TestSuite.runTest(TestSuite.java:232)
	at junit.framework.TestSuite.run(TestSuite.java:227)
	at junit.framework.TestSuite.runTest(TestSuite.java:232)
	at junit.framework.TestSuite.run(TestSuite.java:227)
	at org.eclipse.jdt.internal.junit.runner.junit3.JUnit3TestReference.run(JUnit3TestReference.java:130)
	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:197)
Comment 1 Martin Fluegge CLA 2010-12-29 06:14:48 EST
Created attachment 185874 [details]
Test v1

This test makes the problem reproducible.
Comment 2 Martin Fluegge CLA 2010-12-29 11:22:19 EST
Created attachment 185879 [details]
Test v2

Updated test case.
Comment 3 Martin Fluegge CLA 2010-12-29 11:33:48 EST
Created attachment 185881 [details]
Patch v1

It turned out the CDOClassInfoImpl return a wrong featureID. Since all non persistent features are mapped to index 0 in the revisions values list it happened that the 'containingClass' feature (which is not persistent and has the featureId 17) was mapped to index 0 because the idMapping mapped from 17 to 0. This in return overwrote the value in the revisions value list at position 0 (which was the eAnnotions list) with the containingClass, when store.set() was called. For this reason a ClassCastExption occured when the revisoin tried to get the size of the eAnnotaions list, because the value was wrongly overwritten with the LegacyWrapper the mapped to the containingClass.

I attached a patch that fixed the problem for legacy by not aligning none-persistent opposite features while an object is detached. But it might be that the root of the problem is in the CDO Core itself, to be exact in the id mapping of CDOClassInfoImpl.

Eike, please let us discuss this.
Comment 4 Eike Stepper CLA 2010-12-31 08:34:47 EST
Created attachment 185925 [details]
Combined patch v1

I added skipUnlessConfig(LEGACY) to the test.
I removed the line with the warning from the test.
Comment 5 Eike Stepper CLA 2010-12-31 08:35:32 EST
Fix is good, but more places may be missing this fix...
Comment 6 Martin Fluegge CLA 2010-12-31 08:52:16 EST
> Fix is good, but more places may be missing this fix...

o.k. I'll commit the patch to fix the issue and then I'll care for the other places. 

I suggest to keep track of the other changes on this bugzilla since the changes are somehow related.
Comment 7 Martin Fluegge CLA 2010-12-31 08:53:17 EST
Combined patch v1 committed to HEAD.
Comment 8 Eike Stepper CLA 2010-12-31 08:53:43 EST
> I suggest to keep track of the other changes on this bugzilla since the changes
> are somehow related.

+1
Comment 9 Martin Fluegge CLA 2011-01-01 15:50:41 EST
Created attachment 185929 [details]
Patch v2

I took care for the other calls of store.set() and optimized the CDOLegacyWrapper a bit.

Happy new year ;)
Comment 10 Eike Stepper CLA 2011-01-02 03:23:51 EST
Patch v2 is v2 of what? How does it relate to the previous non-obsolete patch? Where are the tests?
Comment 11 Martin Fluegge CLA 2011-01-02 05:30:01 EST
>Patch v2 is v2 of what? How does it relate to the previous non-obsolete patch?

Patch v2 is an enhancement of the changes in the CDOLegacyWrapper provided in 'Combined patch v1' which was an enchancement of patch v1. Since 'Combined patch v1' had been already committed to Head and 'Patch v2' does not contain test (it is not combined), I called it just 'Patch v2'. So 'Patch v2' patched the committed state which was provided by 'Combined Patch v2'.

>Where are the tests?

Most parts of the patch are a cleanup of the legacy wrapper. So I tested agains all legacy tests.
Comment 12 Eike Stepper CLA 2011-01-02 05:37:03 EST
If there were just a single patch active there would be no confusion but youforgot to obsolete my patch. Still "Patch v2" is not a good name as it suggests that it *replaces* "Patch v1"
Comment 13 Martin Fluegge CLA 2011-01-02 06:15:30 EST
Created attachment 185934 [details]
Patch vA - code improvements

>If there were just a single patch active there would be no confusion but youforgot to obsolete my patch. 

Guilty. I already fixed it. ;)

>Still "Patch v2" is not a good name as it suggests that it replaces "Patch v1"

Agreed. But neither would had been something like 'Combined Patch v2' since the patch was not combining anything - a vicious circle.

Anyway, since the CDOLegacyWrapper has been moved to another package I have to apply an updated patch. 

To avoid any further confusion I named it 'Patch vA - code improvements'. Hope this makes the separation clear ;)
Comment 14 Eike Stepper CLA 2011-01-02 07:14:30 EST
Created attachment 185936 [details]
Patch A v2 - ready to be committed
Comment 15 Eike Stepper CLA 2011-01-02 07:15:26 EST
(In reply to comment #13)
> To avoid any further confusion I named it 'Patch vA - code improvements'. Hope
> this makes the separation clear ;)

No, Patch A v1 would have :P
Comment 16 Martin Fluegge CLA 2011-01-02 11:42:28 EST
Patch vA committed to HEAD ;)
Comment 17 Eike Stepper CLA 2011-06-23 03:38:02 EDT
Available in R20110608-1407