Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 329869

Summary: Legacy Mode : "Duplicate ID" Exceptions caused by multiple registrations of the same Object
Product: [Modeling] EMF Reporter: Alex Lagarde <alex.lagarde>
Component: cdo.legacyAssignee: Martin Fluegge <martin.fluegge>
Status: CLOSED FIXED QA Contact: Eike Stepper <stepper>
Severity: normal    
Priority: P3 CC: martin.fluegge
Version: 4.0Flags: stepper: review+
Target Milestone: ---   
Hardware: All   
OS: All   
Whiteboard:
Attachments:
Description Flags
JUnit test for Duplicate ID Exceptions
stepper: iplog+
Models use for testCase
none
Patch v1
none
Test v1
none
Test v2
none
Fix v2 none

Description Alex Lagarde CLA 2010-11-10 03:32:18 EST 
Build Identifier: 20100917-0705

please see http://www.eclipse.org/forums/index.php?t=msg&goto=629963&S=c933a859f2691ec9f7b73a2ecce05293#msg_629963.

In Legacy Mode, objects can be registered multiples times when their CDOResource is loaded. 
When trying to modify such objects and commit, a "Duplicate ID" IllegalStateException is thrown. 

Example : 
---------
I've got an EObject A from a MetaModel M1, containing, at several level of containment, an EObject B from a MetaModel M2. B references A (it's the case of cyclic relationship described my Martin Fluegge in one of his post). 

When the EObject A is commited (using Legacy Mode), everything goes well. However, when the resource containing A is loaded (for example when calling getContents() ), a problem occurs : 

- in a first call to "getObject(OID-A, true)" in AbstractCDOView, an instanceof A is created (as localLookupObject is equal to null).
- as A is not CDONative, a CDOLegacyWrapper is created and registered (CDOLegacyWrapper.cdoInternalPostLoad() -> call to registerWrapper()).
- Later, when creating B, the B associated Legacy Adapter tries to get the referenced element A using its OID (CDOViewImpl.convertIDToObject(OID-A).
- This calls the getObject(OID-A,true) method in AbstractCDOView. Clearly, as the LegacyWrapper associated to A has been created and registered, the localLookupObject should be the LegacyWrapper associated to A, but is null. 
- As the localLookupObject is null, A is instanciated twice, and a new Wrapper is created for A, with the same ID. 

Hence the "Duplicate ID" error, as the OID-A is associated to two differents wrappers.

Corrections : 
-------------
The simplest correction would be to modify the getObject() method from AbstractCDOView as above  :

InternalCDOObject localLookupObject = objects.get(id);
         if (localLookupObject == null)
         {
+          localLookupObject = CDOLegacyWrapper.getRegisteredWrapper(id);
+        }
+        if (localLookupObject == null)
+        {
           if (id.isMeta())
		   
But, as stated by Martin Fluegge, we should minimize dependencies between CDO core and the legacy mode. 

A better solution may be to change the registerWrapper() method to make it call registerObject() method of AbstractCDOView.

Test is being written, coming soon.

Reproducible: Always

Steps to Reproduce:
See test
Comment 1 Alex Lagarde CLA 2010-11-10 03:43:35 EST 
Created attachment 182789 [details]
JUnit test for Duplicate ID Exceptions
Comment 2 Alex Lagarde CLA 2010-11-10 03:44:30 EST 
Created attachment 182790 [details]
Models use for testCase
Comment 3 Martin Fluegge CLA 2010-11-27 16:40:48 EST 
Created attachment 183985 [details]
Patch v1
Comment 4 Martin Fluegge CLA 2010-11-27 16:44:55 EST 
Created attachment 183986 [details]
Test v1

Although the fix was quite easy it took a while to figure out what caused the problem. In the end it turned out that the Color<->Graph reference which is unsettable cause the trouble because the legacy wrapper internally called store.isset() while the wrapper is not yet registered in the view. This lead to the creation of a second object. 

Alex, thanks for detecting this bug.

I enhanced to CDOView to detect such errors and fixed the bug. I Also attached a test to verify the issue.
Comment 5 Eike Stepper CLA 2010-11-28 02:05:02 EST 
Martin, the test (without the fix patch) does *not* fail here...
Comment 6 Eike Stepper CLA 2010-11-28 02:09:30 EST 
Created attachment 183991 [details]
Test v2

I've commented out two lines of code that seem redundant. The test does still not fail...
Comment 7 Eike Stepper CLA 2010-11-28 02:15:45 EST 
In AbstractCDOView there is this line: 

	OM.LOG.warn("Legacy object has been registered multiple times: " + object);
	
Why can this happen? Is it bad? How bad? Do we need to bother the user? Or should we better use the TRACER?
Comment 8 Eike Stepper CLA 2010-11-28 02:16:47 EST 
Created attachment 183992 [details]
Fix v2
Comment 9 Martin Fluegge CLA 2010-11-28 06:38:35 EST 
Oh Lord, so many questions ;)

>Martin, the test (without the fix patch) does *not* fail here...

Yes. That is correct. Sorry that I made the patches a bit confusing. The error occurs if an object is created twice. This does not always lead to the behavior Alex described but both problems have the same root (see below). That's why I introduced the new exception call to detect the problem. Unfortunately the fix and the exception is in the same patch. If you remove the changes from the LegacyWrapper the exception will occur.


>OM.LOG.warn("Legacy object has been registered multiple times: " + object);
>Why can this happen? 

I think while an object is created in the AbstractView (701) the whole legacy mechanism starts. This can lead to other calls of the method when the references are resolved. Which can lead to a multiple registration of the same object.

>Is it bad?

It is not nice. But with the new check now problem. It is only bad having two objects for the same element.

>Do we need to bother the user?

We could remove this, because the new check detected the real problem.

>Or should we better use the TRACER?

Good idea. This leaves the message if this could be the root for another problem.
Comment 10 Martin Fluegge CLA 2010-11-28 12:54:47 EST 
Commited to HEAD.
Comment 11 Eike Stepper CLA 2011-06-23 03:40:06 EDT 
Available in R20110608-1407