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

Bug 335772

Summary: BasicEStoreEList created in EStoreEcoreEMap should probably call doClear() in unset()
Product: [Modeling] EMF Reporter: Michal Tkacz <Michal.Tkacz>
Component: cdo.coreAssignee: Eike Stepper <stepper>
Status: CLOSED FIXED QA Contact: Eike Stepper <stepper>
Severity: normal    
Priority: P3    
Version: 4.0   
Target Milestone: ---   
Hardware: PC   
OS: Linux   
Whiteboard:
Attachments:
Description Flags
Test case
stepper: iplog+
Patch v1
none
Patch v2
none
Patch v3 none

Description Michal Tkacz CLA 2011-01-29 13:04:34 EST 
Build Identifier: 

I found that when unset() method is called on delegateEList created in EStoreEcoreEMap (nested in CDOObjectImpl.createList()), BasicEMap.this.doClear() is not called which in certain circumstances leads to invalid behaviour (described below). BasicEMap.this.doClear() should probably be called either directly or through one of the methods that call it (possibly clear()).

Observed behaviour:
EcoreEMap.set(), when you pass a map to it, is implemented basically in two steps:
1. ((EStructuralFeature.Setting)delegateEList).unset();
2. putAll(mapValue);

When calling EcoreEMap.set() for the second time, unset() will clear delegateEList, but leave BasicEMap.entryData full of whatever entries were created during first call to EcoreEMap.set(). Thus when calling putAll(), BasicEMap.put() is fooled into thinking that entries added during first call to EcoreEMap.set() are already in the map and doesn't add them to delegateEList. This is hard to spot at first because most methods on EcoreEmap use entryData to return information, but when you try to persist the map as part of an object, the map will not be persisted correctly.

Note that this issue is specific to CDO and doesn't occur in plain EMF. Also while calling EcoreEMap.set() instead of EcoreEMap.put() may seem rare, that's exactly what happens, when you use EMF Databinding (see org.eclipse.emf.databinding.internal.EMFMapProperty.doSetMap()).

Reproducible: Always
Comment 1 Michal Tkacz CLA 2011-01-29 13:09:23 EST 
Created attachment 187910 [details]
Test case

Attached test case tests both replacing existing entries and adding new ones. Two methods test what happens to delegateEList after these operations, the other two test what happens when you try to persist the map and then read it back.
Comment 2 Michal Tkacz CLA 2011-02-05 05:23:41 EST 
Could someone please take a look at it? I'm trying to make maps work well with CDO. I believe that for someone who knows this code it will be easier to find the correct fix than it was for me to identify the problem and prepare the test case. Thanks in advance!
Comment 3 Eike Stepper CLA 2011-02-10 03:14:44 EST 
Created attachment 188656 [details]
Patch v1

Please see how I overwrite unset() now. this makes the first two tests pass.

The last two tests fail with a NPE in the test logic, with or without the fix. Is that a bad test logic or an indication that the fix is not complete?
Comment 4 Eike Stepper CLA 2011-02-10 03:15:37 EST 
Michal, please confirm that:

1) The number of lines that you changed is smaller than 250.
2) You are the only author of these changed lines.
3) You apply the EPL to these changed lines.
Comment 5 Michal Tkacz CLA 2011-02-10 05:45:46 EST 
Created attachment 188672 [details]
Patch v2

> The last two tests fail with a NPE in the test logic, with or without the fix.
> Is that a bad test logic or an indication that the fix is not complete?

It was a bad test logic. I assumed I'll be able to retrieve value from the map, using key that was used when I added it to the map, but that doesn't work because the two keys are different objects and equals() returns false for them. I now enumerate the contents of the map manually and compare cdoId() of the keys. See attached corrected version.
Comment 6 Michal Tkacz CLA 2011-02-10 05:47:04 EST 
> Michal, please confirm that:

> 1) The number of lines that you changed is smaller than 250.
> 2) You are the only author of these changed lines.
> 3) You apply the EPL to these changed lines.

I confirm all three points.
Comment 7 Michal Tkacz CLA 2011-02-10 05:55:29 EST 
Created attachment 188673 [details]
Patch v3

Actually I should probably use CDOUtil.getCDOObject instead of casting to Product1Impl.
Comment 8 Eike Stepper CLA 2011-02-10 07:53:23 EST 
Committed revision 7046:
- trunk/plugins/org.eclipse.emf.cdo
- trunk/plugins/org.eclipse.emf.cdo.tests
Comment 9 Eike Stepper CLA 2011-02-10 07:53:58 EST 
Thank you, Michal ;-)
Comment 10 Eike Stepper CLA 2011-06-23 03:40:58 EDT 
Available in R20110608-1407