337587 – Illegal deltas are produced on compare

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

Bug 337587 - Illegal deltas are produced on compare

Summary: Illegal deltas are produced on compare

Status:	CLOSED FIXED

Alias:	None

Product:	EMF
Classification:	Modeling
Component:	cdo.core (show other bugs)
Version:	4.0
Hardware:	All All

Importance:	P3 normal (vote)
Target Milestone:	---
Assignee:	Egidijus Vaisnora
QA Contact:	Eike Stepper

URL:
Whiteboard:
Keywords:

Depends on:
Blocks:

Reported:	2011-02-18 11:46 EST by Egidijus Vaisnora
Modified:	2012-09-21 06:51 EDT (History)
CC List:	5 users (show)

See Also:

Flags:	vaisegid: review? (stepper) stepper: review+

Attachments
test case (3.37 KB, patch) 2011-02-18 11:51 EST, Egidijus Vaisnora	no flags	Details \| Diff
Patch v1 (19.79 KB, patch) 2011-03-24 05:56 EDT, Caspar D.	no flags	Details \| Diff
Patch v2 (29.09 KB, patch) 2011-03-28 01:47 EDT, Caspar D.	no flags	Details \| Diff
Patch v3 (32.64 KB, patch) 2011-03-28 03:14 EDT, Caspar D.	no flags	Details \| Diff
Patch v4 (34.62 KB, patch) 2011-04-04 23:43 EDT, Caspar D.	no flags	Details \| Diff
Patch v5 (35.91 KB, patch) 2011-04-05 00:41 EDT, Eike Stepper	no flags	Details \| Diff
Fixed test case (2.37 KB, text/plain) 2011-12-16 09:10 EST, Egidijus Vaisnora	no flags	Details
Show Obsolete (6) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Egidijus Vaisnora

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

2011-02-18 11:51:41 EST

Created attachment 189296 [details]
test case

Comment 2 Caspar D.

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.

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.

2011-03-28 01:47:43 EDT

Created attachment 191969 [details]
Patch v2

Comment 5 Caspar D.

2011-03-28 03:14:57 EDT

Created attachment 191976 [details]
Patch v3

Comment 6 Eike Stepper

2011-04-04 03:27:21 EDT

CDOElementProxy does not seem to exist anymore. Please fix the patch...

Comment 7 Eike Stepper

2011-04-04 03:37:02 EDT

Perhapüs a crash corrupted my workspace. Trying again...

Comment 8 Eike Stepper

2011-04-04 03:39:06 EDT

No, the ptahc removes CDOElementProxy but leaves some classes referring to it.

Comment 9 Caspar D.

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.

2011-04-04 23:43:01 EDT

Created attachment 192516 [details]
Patch v4

Comment 11 Eike Stepper

2011-04-05 00:41:42 EDT

Created attachment 192519 [details]
Patch v5

Comment 12 Caspar D.

2011-04-05 01:04:48 EDT

Committed revision 7588.

Comment 13 Eike Stepper

2011-06-23 03:42:10 EDT

Available in R20110608-1407

Comment 14 Eike Stepper

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

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

2011-12-19 10:16:10 EST

Thank you!

commit 65d10b1e3f523b1139289c9aa70208ce57779ab4

Comment 17 Eike Stepper

2012-04-05 08:49:37 EDT

*** Bug 369646 has been marked as a duplicate of this bug. ***

Comment 18 Eike Stepper

2012-09-21 06:51:56 EDT

Closing.