Bug 343046

Comment 1 Rimvydas 2011-04-16 13:17:56 EDT

Created attachment 193420 [details]
test case

Comment 2 Ed Merks 2011-04-16 20:01:38 EDT

Oh, so what you're saying is that container references aren't resolved as expected.

Comment 3 Rimvydas 2011-04-17 02:47:25 EDT

To be more precise:
1. Resolving of the container reference leaves the model in inconsistent state. The container contains the object but the object's container is not the containing container.
2. The object's container is a proxy. After EcoreUtil.resolveAll(ResourceSet) I do not expect any proxies.

Comment 4 Ed Merks 2011-04-17 12:25:33 EDT

Created attachment 193437 [details]
Patches to resolve the container proxy during resolve all.

I'm not sure what you say is more precise:

  Resolving of the container reference leaves the model in inconsistent state.

I think what your test case demonstrates is that the container reference isn't resolved at all by EcoreUtil.resolveAll.  After all, the patches I've attached merely resolve the container reference and that results in the tests passing.

Maybe you meant the "container's containment reference".  Note also the characterization that the model is in an inconsistent state is also not accurate. Resolving a proxy resolve that proxy only at one end, not at both ends, so having a bidirectional reference with a resolve reference and one end and an unresolved proxy at the other end is not an invalid state; it just one you don't expect after calling resolveAll which is documented to resolve all proxies, not just containment proxies and cross reference proxies.

So with the simple changes in this patch, you're getting what you're expecting, right?

Comment 5 Rimvydas 2011-04-17 13:50:27 EDT

What I meant by inconsistent:

folderA.eIsProxy(); // false
folderB.eIsProxy(); // false
((InternalEList<?>)folderA.getSubfolders()).basicContains(folderB); //true
((InternalEObject)folderB).eInternalContainer() == folderA; // false

Do you say that this is legal ?

Comment 6 Ed Merks 2011-04-17 20:23:02 EDT

Yes, that state is perfectly legal.  It's the state before the container proxy from B to A is resolved but after the containment proxy from A to B is resolved.  As I said, the two proxies on the two ends are resolved independently.

So I think the only problem is the fact that resolveAll ignores container proxies whereas the documentation would lead one to expect all proxies, including container proxies are resolved.  That's why I believe adding the call to eContainer is all that's needed.  It certainly makes your tests pass.

So is all good with that one change (in two places)?

Comment 7 Rimvydas 2011-04-18 03:12:54 EDT

OK. If this state is legal then lets go further.

We have:

[folderA : Folder]--------------subfolders--[folderB : Folder]
                                                    |
                                                    |
[proxy : Folder]--parent----------------------------+

and I want to replace the folderB with a new folder (folderC).


Folder folderC = TestFactory.eINSTANCE.createFolder();
folderA.setName("C");
EcoreUtil.replace(folderB, folderC);


folderA.getSubfolders()).contains(folderC); // returns false. Is this OK ?

After replace:
folderA still has reference to folderB.
folderB.eContainer() == null

Now the model is in inconsistent state. Is not it ?

Comment 8 Ed Merks 2011-04-18 12:13:37 EDT

I don't think you're asking questions about EcoreUtil.resolveAll anymore. Now it's a question about replace...  

Replace uses eInternalContainer so it literally replaces the B's parent proxy's reference to B with a reference to C.  So it's operating at opposite ends independently so nothing at all has happened to A.  It's interesting if you turn this into a real test case though.  (Note that I eliminated CDO from the picture and just used a plain old EMF generated model for the model you supplied.

        ////////////////////////////		
		
		resourceA.unload();
		resourceB.unload();
		
		resourceA.load(null);
		resourceB.load(null);
		
		// EcoreUtil.resolveAll(resourceA);
		
		folderA = (Folder) resourceA.getContents().get(0);
		folderB = (Folder) resourceB.getContents().get(0);

		Folder folderC = TestFactory.eINSTANCE.createFolder();
		folderC.setName("C");
		EcoreUtil.replace(folderB, folderC);
		assertTrue(folderA.getSubfolders().contains(folderC));
		assertFalse(folderA.getSubfolders().contains(folderB));
		assertTrue(folderB.eContainer() == null);
		assertTrue(folderC.eContainer() == folderA);
		

This works properly because folderA contains a proxy that's resolves to folderC after the replace.  Perhaps CDO has a stronger object identity that causes the proxy to fail to resolve after the operation.

So the new question you've brought up here (totally separate from questions about resolveAll) is whether replace should be using eInternalContainer or eContainer. Probably it should be changed because it's certainly possible to get into an inconsistent state accidentally...

Comment 9 Rimvydas 2011-04-19 02:36:45 EDT

EcoreUtil.resolveAll() should resolve all proxies and with the patch you have provided it does that. Without the patch EcoreUtil.resolveAll() allows to create the model's state that is legal as you explained but it was unexpected to me. BTW, In which version of EMF the EcoreUtil.resolveAll() patch will be included?

It is not clear to me what you want to say by "Perhaps CDO has a stronger object identity that causes the proxy to fail to resolve after the operation."

I have run the test case with CDO based and plain EMF model and the test case fails despite CDO or plain EMF model is used if EcoreUtil.resolveAll(resourceSet) is invoked before replace. If EcoreUtil.resolveAll(resourceSet) is removed then the test case passes.

Regarding EcoreUtil.replace(). Do you say that this is a bug that its implementation uses eInternalContainer() but not eContainer() ? Are you going to fix it ? Should I fill another bug report ?

Comment 10 Ed Merks 2011-04-19 10:12:29 EDT

I'll commit the patch for EMF 2.7.

I was surprised that the test ever passed until I realized why it was passing.  That made me think that a resource implementation that used UUIDs would never pass because the new object would have a different UUID and hence a different proxy URI.  I'm just not sure if CDO's would use a fragment path or an ID in this type of scenario...

I think EcoreUtil.replace's current behavior is error prone and should probably be changed.  A separate bug for that would be better.

Comment 11 Ed Merks 2011-05-12 10:48:02 EDT

The fix is committed to CVS for 2.7; I also changed the implementation of replace to ensure the container proxy is resolved.

Comment 12 Ed Merks 2011-06-02 11:40:18 EDT

The fix is available in 3.7RC3 and later.