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

Bug 337587

Summary: Illegal deltas are produced on compare
Product: [Modeling] EMF Reporter: Egidijus Vaisnora <vaisegid>
Component: cdo.coreAssignee: Egidijus Vaisnora <vaisegid>
Status: CLOSED FIXED QA Contact: Eike Stepper <stepper>
Severity: normal    
Priority: P3 CC: caspar_d, cbateman, saulius.tvarijonas, stefan, szbardy
Version: 4.0Flags: vaisegid: review? (stepper)
stepper: review+
Target Milestone: ---   
Hardware: All   
OS: All   
Whiteboard:
Attachments:
Description Flags
test case
none
Patch v1
none
Patch v2
none
Patch v3
none
Patch v4
none
Patch v5
none
Fixed test case none

Description Egidijus Vaisnora CLA 2011-02-18 11:46:45 EST 
There are situations when dirty revision is compared with clean revision in order to get change deltas. One of such places is ComitIntegrityChecker. Whenever we use chunk loading for collection, there is state, when clean revision contains CDOElementProxy, while dirty revision for the same object is filled with loaded CDOID. Comparing such revisions, we get deltas describing CDOElementProxy removing and adding CDOID objects. Hence we get many odd deltas and by itself CDORemoveFeatureDelta contains CDOElementProxy as value, which doesn't look expected value.
Comment 1 Egidijus Vaisnora CLA 2011-02-18 11:51:41 EST 
Created attachment 189296 [details]
test case
Comment 2 Caspar D. CLA 2011-03-24 05:16:44 EDT 
The solution appears straightforward at first... the comparison
logic should refuse to compare elements that are instances of
CDOElementProxy. But the trouble is that the comparison logic
is in emf.cdo.common while CDOElementProxy is in emf.cdo. So
the comparison logic cannot recognize CDOElementProxies, unless...

... we move CDOElementProxy to emf.cdo.common. Which isn't unreasonable
because CDOElementProxy is really only used in CDORevisions, which are
also in emf.cdo.common. But the complication is that CDOElementProxy
depends on CDOSession... which is in emf.cdo and should stay there (I
think). And surely we don't want emf.cdo.common to depend on emf.cdo.

A nice case of entangled dependencies... any ideas anyone?
Comment 3 Caspar D. CLA 2011-03-24 05:56:46 EDT 
Created attachment 191812 [details]
Patch v1

Moved various classes from emf.cdo to emf.cdo.common:
- CDOElementProxy
- CDOElementProxyImpl
- CDOListWithElementProxiesImpl

Added proxy detection in the compare logic.

This doesn't really fix the bug yet, but at least properly
detects the problem.
Comment 4 Caspar D. CLA 2011-03-28 01:47:43 EDT 
Created attachment 191969 [details]
Patch v2
Comment 5 Caspar D. CLA 2011-03-28 03:14:57 EDT 
Created attachment 191976 [details]
Patch v3
Comment 6 Eike Stepper CLA 2011-04-04 03:27:21 EDT 
CDOElementProxy does not seem to exist anymore. Please fix the patch...
Comment 7 Eike Stepper CLA 2011-04-04 03:37:02 EDT 
Perhapüs a crash corrupted my workspace. Trying again...
Comment 8 Eike Stepper CLA 2011-04-04 03:39:06 EDT 
No, the ptahc removes CDOElementProxy but leaves some classes referring to it.
Comment 9 Caspar D. CLA 2011-04-04 23:23:02 EDT 
Now this *is* a problem with SVN. If you move classes (as I
did with the CDOElementProxy* stuff), svn diff doesn't include
their addition in its output.

I'll work around this by doing a delete-and-add (as opposed to
a move) for generating the patch. For the real commit, I'll do 
a move (so that the history of the files won't be lost).

Btw.. it seems this shortcoming of 'svn diff' will be addressed
in Subversion 1.7.
Comment 10 Caspar D. CLA 2011-04-04 23:43:01 EDT 
Created attachment 192516 [details]
Patch v4
Comment 11 Eike Stepper CLA 2011-04-05 00:41:42 EDT 
Created attachment 192519 [details]
Patch v5
Comment 12 Caspar D. CLA 2011-04-05 01:04:48 EDT 
Committed revision 7588.
Comment 13 Eike Stepper CLA 2011-06-23 03:42:10 EDT 
Available in R20110608-1407
Comment 14 Eike Stepper CLA 2011-12-16 07:40:16 EST 
The committed solution does not fix the reported problem. I've disabled org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_337587_Test._testRevisionCompare() meanwhile.
Comment 15 Egidijus Vaisnora CLA 2011-12-16 09:10:21 EST 
Created attachment 208490 [details]
Fixed test case

As per skype conversation with Eike agreed, test do not identifies provided fix. Fixed test case to match fix.
Comment 16 Eike Stepper CLA 2011-12-19 10:16:10 EST 
Thank you!

commit 65d10b1e3f523b1139289c9aa70208ce57779ab4
Comment 17 Eike Stepper CLA 2012-04-05 08:49:37 EDT 
*** Bug 369646 has been marked as a duplicate of this bug. ***
Comment 18 Eike Stepper CLA 2012-09-21 06:51:56 EDT 
Closing.