Comment 1 Christian Damus 2013-03-18 15:34:01 EDT

Sorry, had the wrong version number.

This blocks the integration of CDO in Papyrus (bug 290952).

Comment 2 Eike Stepper 2013-03-18 15:57:58 EDT

Can I help? I'm off now because it's late here, but I could react early tomorrow...

Comment 3 Eike Stepper 2013-03-18 15:59:39 EDT

Please also note that in a feature branch (and in the context of bug 403661) I've deprecated the ability to control the enablement of the legacy mode completely!

Comment 4 Christian Damus 2013-03-18 16:18:34 EDT

(In reply to comment #2)
> Can I help? I'm off now because it's late here, but I could react early
> tomorrow...

Uh, yes.  I was actually hoping you would fix it. :-)  This regression is caused by your commit

   b5a193491935f7c23ddf1da34dc6ea0561290b65
   [400389] Eliminate CDOObjectImpl.cdoSettings

At least, so says git bisect.  :-)

I don't know what the elimination of cdoSettings is trying to do, nor why it would cause EStructuralFeature::eType no longer to be persisted whereas UML metamodel is unaffected.  I suppose it's the special case for the EGenericType again ...

Comment 5 Eike Stepper 2013-03-19 08:40:32 EDT

I wanted to run your test case but it fails to initialize properly. The following is null:

  private final EPackage stuffPackage = EPackage.Registry.INSTANCE
      .getEPackage("http://www.eclipse.org/cdo/tests/schema/stuff/1.0");

Comment 6 Christian Damus 2013-03-19 09:03:26 EDT

(In reply to comment #5)
> I wanted to run your test case but it fails to initialize properly. The
> following is null:
> 
>   private final EPackage stuffPackage = EPackage.Registry.INSTANCE
>       .getEPackage("http://www.eclipse.org/cdo/tests/schema/stuff/1.0");

Thanks for looking into this, Eike!  Anything I can do to help, I shall try to.

That shouldn't be null:  it's registered on the org.eclipse.emf.ecore.dynamic_package extension point.  How could it be null?  I'll try running it here to see whether maybe I broke something in the test project.

Comment 7 Christian Damus 2013-03-19 09:12:16 EDT

Created attachment 228631 [details]
JUnit launch config

Eike, the test suite runs OK for me, except of course for the assertion failure on the missing eType references, which is the point.  In particular, I don't get a null pointer on that test package.

I have attached the launch config that I run, in case that helps.  It may have some Mac-isms.

Comment 8 Christian Damus 2013-03-19 11:14:26 EDT

I'm debugging the test case.

I see that for most eReferences, the following code converts the EObject value to a CDOLegacyAdapter (being the CDOObject):

    class CDOObjectImpl
    {

        ...

        public static void instanceToRevisionFeature(InternalCDOView view, InternalCDOObject object,
              EStructuralFeature feature, Object setting)
        {

            ...

            setting = cdoStore.convertToCDO(object, feature, setting);

            ...

But, in the case of EGenericType instances, the same EGenericType is returned by the store, instead of a CDOLegacyAdapter wrapper.

Could it be that, because the EReference::eGenericType reference is a containment reference, the view doesn't yet know about the contained eGenericType?

Comment 9 Christian Damus 2013-03-19 11:45:55 EDT

Created attachment 228657 [details]
Patch that fixes the problem

I've attached a patch that fixes the problem.

The root cause of the issue is that the CDOLegacyAdapter is not being attached to EGenericType instances by the CDO state machine.  The reason for that is that the CDOStateMachine::PrepareTransition::getPersistentContents(InternalEObject) method iterates an EContentsEList, which only includes features for which eIsSet() is true.

The target of the eIsSet() calls in this case is a CDOLegacyAdapter wrapping an EStructuralFeature, and CDOObjectWrapperBase implements eIsSet() as a pass-through to the wrapped instance.  So, my patch updates CDOObjectWrapperBase to delegate to the same static isSetInstanceValue(...) utility used by the wrapper for persistence.

Comment 10 Christian Damus 2013-03-19 11:50:07 EDT

Hi, Eike,

Could I please ask you to review my patch?

My first thought was that I could update the PrepareTransition to use a custom EContentsEList that would let the CDOLegacyWrapper override the is-set check, but that would also apply to native models and seemed to me a scary place to make changes.

On the other hand, changing the CDOLegacyAdapter's implementation of eIsSet() could affect clients that are using it not to test whether the value should be persisted, but for whatever other reasons.  However, I don't think the risk of that is great because clients shouldn't be working with the adapter in the first place, but rather with the object that it wraps.

Your insight on these concerns will be most helpful.

Comment 11 Christian Damus 2013-03-19 11:50:44 EDT

I have a fix in review, so I should take this bug.

Comment 12 Christian Damus 2013-03-19 12:13:09 EDT

Created attachment 228660 [details]
UML-free test case

I've attached a better test case that

  * doesn't require UML
  * is integrated in the main CDO regression test suite

This test case, too, passes with my fix.

Comment 13 Eike Stepper 2013-03-19 12:22:31 EDT

> Could I please ask you to review my patch?

I would, but the patch does not apply at all. Can you go on Skype?

Comment 14 Eike Stepper 2013-03-19 12:31:58 EDT

Strange, although the apply patch wizard complained about conflicts everywhere and I canceled, the patch was actually applied. Anyway. But testInstancesOfRegisteredDynamicUMLProfile() still fails with an IllegalArgumentException:

org.eclipse.emf.cdo.tests.config.impl.ConfigTestException: Error in DynamicProfileTest.testInstancesOfRegisteredDynamicUMLProfile [Combined, MEM-branching, JVM, Legacy]
	at org.eclipse.emf.cdo.tests.config.impl.ConfigTest.runBare(ConfigTest.java:603)
	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:307)
	at junit.framework.TestSuite.runTest(TestSuite.java:243)
	at junit.framework.TestSuite.run(TestSuite.java:238)
	at org.junit.internal.runners.JUnit38ClassRunner.run(JUnit38ClassRunner.java:83)
	at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:50)
	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.pde.internal.junit.runtime.RemotePluginTestRunner.main(RemotePluginTestRunner.java:62)
	at org.eclipse.pde.internal.junit.runtime.CoreTestApplication.run(CoreTestApplication.java:23)
	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:597)
	at org.eclipse.equinox.internal.app.EclipseAppContainer.callMethodWithException(EclipseAppContainer.java:587)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:198)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:353)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:180)
	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:597)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:629)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:584)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1438)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1414)
Caused by: java.lang.IllegalArgumentException: org.eclipse.uml2.uml.internal.impl.StereotypeImpl@6197cc (name: Concept, visibility: <unset>) (isLeaf: false, isAbstract: false, isFinalSpecialization: false) (isActive: false)
	at org.eclipse.uml2.uml.internal.operations.ElementOperations.applyStereotype(ElementOperations.java:1413)
	at org.eclipse.uml2.uml.internal.impl.ElementImpl.applyStereotype(ElementImpl.java:510)
	at org.eclipse.emf.cdo.tests.uml.DynamicProfileTest.testInstancesOfRegisteredDynamicUMLProfile(DynamicProfileTest.java:83)
	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:597)
	at junit.framework.TestCase.runTest(TestCase.java:168)
	at org.eclipse.net4j.util.tests.AbstractOMTest.access$1(AbstractOMTest.java:1)
	at org.eclipse.net4j.util.tests.AbstractOMTest$2.execute(AbstractOMTest.java:259)
	at org.eclipse.net4j.internal.util.test.TestExecuter.execute(TestExecuter.java:40)
	at org.eclipse.net4j.util.tests.AbstractOMTest.runBare(AbstractOMTest.java:246)
	at org.eclipse.emf.cdo.tests.config.impl.ConfigTest.runBare(ConfigTest.java:594)
	... 33 more

Trying your new test case now...

Comment 15 Eike Stepper 2013-03-19 12:40:50 EDT

The new test case you've provided fails without your patch and succeeds with it. I'm currently running the entire MEMStore test suite. If that doesn't show other regressions I'll commit Bugzilla_403681_Test and CDOObjectWrapperBase with you as author. Then I have a chance to get it into M6 ;-)

I leave it to you whether/how to include the UML test case as I couldn't make it succeed. Thanks!

Comment 16 Eike Stepper 2013-03-19 12:53:16 EDT

commit 57564f9b21425b1e63542c157562747ded6e93f5

Comment 17 Christian Damus 2013-03-19 13:51:25 EDT

Thanks, Eike!

(sorry for the poor responsiveness, today.  Dealing with a snowstorm)

I don't need the UML test; the new one totally covers the problem as will run in every build.  :-)

Though it's still puzzling why it wouldn't work for you.  Must be Windows.  :-P

Comment 18 Eike Stepper 2013-03-19 14:08:46 EDT

> Though it's still puzzling why it wouldn't work for you.  Must be Windows. 

As usually :P

Comment 19 Eike Stepper 2013-06-27 03:34:28 EDT

Available in R20130613-1157 (4.2)