Bug 337054

Comment 1 Szabolcs Bardy 2011-02-14 08:53:30 EST

Created attachment 188901 [details]
Patch v1

We found that the problem comes from the CDORevisionDeltaImpl.compare(CDORevision originRevision, CDORevision dirtyRevision) method.

In the method all features of the origin and the end revisions are compared with EMF Compare's ListDifferenceAnalyzer. When partial collection loading is used the lists of both the origin and the end revision will contain CDOElementProxy items. When comparing the lists, each CDOElementProxy items will be different, hence each CDOElementProxy from the origin list will appear as REMOVE feature delta, all from the end revision's list as ADD feature delta. 
As feature value comparison is done by the ListDifferenceAnalyzer, the only way we found to avoid adding these elements into the change set is checking the type (value instanceof CDOElementProxy) of the feature value:

In CDORevisionDelataImpl instead of 

		protected void createAddListChange(EList<Object> oldList, EList<ListChange> listChanges, Object value,
                int index)
            {
              CDOFeatureDelta delta = new CDOAddFeatureDeltaImpl(feature, index, value);
              changes.add(delta);
              oldList.add(index, value);
            }
			
			
use

		protected void createAddListChange(EList<Object> oldList, EList<ListChange> listChanges, Object value,
                int index)
            {

              if (!(value instanceof CDOElementProxy))
              {
                CDOFeatureDelta delta = new CDOAddFeatureDeltaImpl(feature, index, value);
                changes.add(delta);
              }
              oldList.add(index, value);
            }
			
(and the same for createRemoveListChange and createMoveListChange)

The only problem is that CDOElementProxy (org.eclipse.emf.cdo plugin) is not visible from CDORevisionDeltaImpl (org.eclipse.emf.cdo.common plugin). I attached a patch which adds org.eclipse.emf.cdo as a dependency to the org.eclipse.emf.cdo.common plugin, however probably this is not the desired way for the fix.

How could this dependency problem be resolved?

Comment 2 Eike Stepper 2011-02-14 09:06:16 EST

Thanks for your analysis!

We can definitely not make the cdo common plugin depend on the cdo client. We could create an empty marker interface in cdo common and let CDOElementProxy extend it.

But I have my doubts that simply ignoring the proxies leads to correct change sets. Have you tested that with your solution?

Comment 3 Szabolcs Bardy 2011-02-14 09:28:10 EST

Hi Eike,

I knew that adding this dependency will not solve the problem, making structural changes needs to be decided by you. I just wanted to attach a solution that solved the problem for us. The origin of the problem is, that the CDOElementProxy items get into the CDOChangeSetData. 
Is there any case when a proxy needs to appear as a change? We were thinking about such situtations, but found that any modifications that is made implies that the object will be loaded, thus when comparing changes no value will appear as a proxy. Is it a right assumption?

Applying these modifications in our tests we got the same result for the change sets with and without partial collection loading.

Comment 4 Eike Stepper 2011-02-14 09:32:23 EST

The proxies do not contain the real values of that list position. But I guess this real value could impact the compare result. To ignore a proxy means to ignore the real value that does exist at that position.

Comment 5 Szabolcs Bardy 2011-02-15 04:21:31 EST

If it is assumed that CDOElementProxy items may get into the change sets, then probably the CDOElementProxy should contain more information about the underlying real value than only the index (like in EMF the URI is available for the proxies), otherwise the same lists will appear always as different. Even if there is a situation when two revisions are compared, where the same lists contain CDOElementProxy items with different real values, there is no real information based on they could be distinguished.

Comment 6 Eike Stepper 2011-02-15 04:33:20 EST

(In reply to comment #5)
> If it is assumed that CDOElementProxy items may get into the change sets, then
> probably the CDOElementProxy should contain more information about the
> underlying real value than only the index

You're putting it upside down :P

The only piece of information that is missing is the ID of the target object because this is not known without actually loading that part of the list. If you would load it (the ID, not the target object!) you wouldn't need a CDOElementProxy at all.

That's why I think the proxies may *not* get into the change sets.

Comment 7 Szabolcs Bardy 2011-02-15 05:49:44 EST

Based on the avaiable information:

Ignoring the CDOElementProxy items may result to incorrect change sets when comparing revisions. Since there is no means to compare CDOElementProxy items when comparing two revisions everything that is a proxy must be resolved. Is it a correct assumption?

Comment 8 Eike Stepper 2011-04-14 02:50:36 EDT

Committed revision 7618

Comment 9 Eike Stepper 2011-04-14 03:19:13 EDT

A CDORevision does not remember its RevisionLoader once it has been loaded. That makes it impossible that a many valued feature within a revision resolves proxy elements automatically. And I doubt that such an automatism would be adequate in all situations.

Can you please explain exactly what methods you're calling so that I can get an impression what context I can use?

Comment 10 Szabolcs Bardy 2011-04-27 07:31:25 EDT

Created attachment 194143 [details]
Test case to reproduce the error

Hi Eike,

sorry for the late response.

I've attached a test case that can be used to reproduce the error. The test case commits some data to the main branch, then creates another branch and tries to merge the branch back to the main. The error occurs only when the repository is shut down once and restarted. The restart is included in the test case. After the restart CDOTransaction.merge(..) is called, where the following exception is thrown:

java.lang.IllegalStateException: List contains proxy elements
	at org.eclipse.emf.cdo.internal.common.revision.delta.CDORevisionDeltaImpl$1.checkNoProxies(CDORevisionDeltaImpl.java:362)
	at org.eclipse.emf.cdo.internal.common.revision.delta.CDORevisionDeltaImpl$1.analyzeLists(CDORevisionDeltaImpl.java:320)
	at org.eclipse.emf.cdo.internal.common.revision.delta.CDORevisionDeltaImpl.compare(CDORevisionDeltaImpl.java:371)
	at org.eclipse.emf.cdo.internal.common.revision.delta.CDORevisionDeltaImpl.<init>(CDORevisionDeltaImpl.java:108)
	at org.eclipse.emf.cdo.spi.common.revision.BaseCDORevision.compare(BaseCDORevision.java:378)
	at org.eclipse.emf.cdo.spi.common.revision.BaseCDORevision.compare(BaseCDORevision.java:1)
	at org.eclipse.emf.cdo.common.revision.CDORevisionUtil.createChangeSetData(CDORevisionUtil.java:231)
	at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl.createChangeSet(CDOTransactionImpl.java:469)
	at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl.merge(CDOTransactionImpl.java:455)
	at org.eclipse.emf.internal.cdo.transaction.CDOTransactionImpl.merge(CDOTransactionImpl.java:401)

Comment 11 Eike Stepper 2011-04-28 04:03:55 EDT

Ok, with your test case I could easily fix the problem ;-)

For the test case, 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 12 Eike Stepper 2011-04-28 04:04:46 EDT

Created attachment 194240 [details]
Patch v2

Comment 13 Eike Stepper 2011-04-28 04:05:16 EDT

Committed revision 7641:
- trunk/plugins/org.eclipse.emf.cdo
- trunk/plugins/org.eclipse.emf.cdo.tests

Comment 14 Szabolcs Bardy 2011-04-28 05:28:27 EDT

(In reply to comment #11)
> Ok, with your test case I could easily fix the problem ;-)
> 
> For the test case, 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 can confirm these statements.

Thanks for the quick fix!

Comment 15 Eike Stepper 2011-05-05 06:52:27 EDT

You're welcome ;-)

Comment 16 Szabolcs Bardy 2011-05-05 07:22:21 EDT

Created attachment 194814 [details]
Another test case to reproduce the error

Hi Eike,

I just wanted to add another test case that reproduces the error, when I saw that you resolved the bug. The test case is attached as a patch for the previously created test class. 

When running the test case with the current code base, the following error is thrown: 
org.eclipse.net4j.util.ImplementationError: SELECT cdo_version, cdo_created, cdo_revised, cdo_resource, cdo_container, cdo_feature, folder, name, contents FROM eresource_CDOResource WHERE cdo_id=? AND cdo_branch=? AND (cdo_created<=? AND (cdo_revised=0 OR cdo_revised>=?)) already in cache
	at org.eclipse.emf.cdo.server.internal.db.SmartPreparedStatementCache$Cache.put(SmartPreparedStatementCache.java:124)
	at org.eclipse.emf.cdo.server.internal.db.SmartPreparedStatementCache.releasePreparedStatement(SmartPreparedStatementCache.java:61)
	at org.eclipse.emf.cdo.server.internal.db.mapping.horizontal.HorizontalBranchingClassMapping.readRevision(HorizontalBranchingClassMapping.java:452)
	at org.eclipse.emf.cdo.server.internal.db.DBStoreAccessor.readRevision(DBStoreAccessor.java:215)
	at org.eclipse.emf.cdo.internal.server.Repository.loadRevisions(Repository.java:441)
	at org.eclipse.emf.cdo.internal.common.revision.CDORevisionManagerImpl.loadRevisions(CDORevisionManagerImpl.java:352)
	at org.eclipse.emf.cdo.tests.util.TestRevisionManager.loadRevisions(TestRevisionManager.java:116)
	at org.eclipse.emf.cdo.internal.common.revision.CDORevisionManagerImpl.getRevisions(CDORevisionManagerImpl.java:263)
	at org.eclipse.emf.cdo.tests.util.TestRevisionManager.getRevisions(TestRevisionManager.java:71)
	at org.eclipse.emf.cdo.internal.common.revision.CDORevisionManagerImpl.getRevision(CDORevisionManagerImpl.java:246)
	at org.eclipse.emf.cdo.internal.common.revision.CDORevisionManagerImpl.getRevision(CDORevisionManagerImpl.java:239)
	at org.eclipse.emf.cdo.internal.server.Repository.getRevisionFromBranch(Repository.java:1253)
	at org.eclipse.emf.cdo.internal.server.Repository.loadMergeData(Repository.java:1231)
	at org.eclipse.emf.cdo.internal.server.Repository.getMergeData(Repository.java:1194)
	at org.eclipse.emf.cdo.server.internal.net4j.protocol.LoadMergeDataIndication.responding(LoadMergeDataIndication.java:113)

I have already bumped into this issue previously. It is coming from the method org.eclipse.emf.cdo.server.internal.db.SmartPreparedStatementCache.Cache.put(CachedPreparedStatement). The first question is, whether it is really an error, when adding a prepared statement to the cache that is already there? Isn't it possible to silently overwrite the previously added one?


If I comment out the line which throws the exception, I receive the exception that is related to this bug: 
java.lang.IllegalStateException: List contains proxy elements
	at org.eclipse.emf.cdo.internal.common.revision.delta.CDORevisionDeltaImpl$1.checkNoProxies(CDORevisionDeltaImpl.java:362)
	at org.eclipse.emf.cdo.internal.common.revision.delta.CDORevisionDeltaImpl$1.analyzeLists(CDORevisionDeltaImpl.java:320)
	at org.eclipse.emf.cdo.internal.common.revision.delta.CDORevisionDeltaImpl.compare(CDORevisionDeltaImpl.java:371)
	at org.eclipse.emf.cdo.internal.common.revision.delta.CDORevisionDeltaImpl.<init>(CDORevisionDeltaImpl.java:108)
	at org.eclipse.emf.cdo.spi.common.revision.BaseCDORevision.compare(BaseCDORevision.java:378)
	at org.eclipse.emf.cdo.spi.common.revision.BaseCDORevision.compare(BaseCDORevision.java:1)
	at org.eclipse.emf.cdo.common.revision.CDORevisionUtil.createChangeSetData(CDORevisionUtil.java:231)
	at org.eclipse.emf.internal.cdo.session.CDOSessionImpl.compareRevisions(CDOSessionImpl.java:1290)
	at org.eclipse.emf.internal.cdo.view.AbstractCDOView.compareRevisions(AbstractCDOView.java:1391)

This could be solved adding the resolveAllElementProxies(revision) call to the cacheRevisions(CDORevisionAvailabilityInfo info) method instead of the createRevisionAvailabilityInfo(CDOBranchPoint branchPoint) in the org.eclipse.emf.internal.cdo.session.CDOSessionImpl class as it was done in your patch.

Comment 17 Eike Stepper 2011-06-23 03:39:52 EDT

Available in R20110608-1407

Comment 18 András Péteri 2012-10-02 17:18:00 EDT

Created attachment 221816 [details]
Revised test case

Szabolcs's revised test case from comment 16

Comment 19 András Péteri 2012-10-02 17:19:33 EDT

Created attachment 221817 [details]
Proposed fix

Comment 20 András Péteri 2012-10-02 17:32:52 EDT

We're seeing the issue from comment 16 in our production environment after applying a fix similar to attachment 221814 [details]. Just as with bug 369646, the server side revision cache returns a partially loaded revision if the comparison targets the base timestamp on the main branch (but not if the base timestamp with the non-main branch is compared - try reordering the assertCanCreateChangeSetDataOnBaseBranch(session, branch) and assertCanCreateChangeSetDataOnBaseTimestamp(session, branch) calls without applying the fix). The revisions in LoadMergeDataIndication are always serialized with the UNCHUNKED flag set, so CDOSessionImpl.resolveAllElementProxies won't help when the results are read on the client's end.

The proposed fix is to fully populate revision lists with Repository.ensureChunks on the server instead.