Bugzilla – Full Text Bug Listing
| Summary: | CDOConflictResolver2 receives spurious delta in case of remote removal + local add | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Caspar D. <caspar_d> | ||||||||||
| Component: | cdo.core | Assignee: | Caspar D. <caspar_d> | ||||||||||
| Status: | CLOSED FIXED | QA Contact: | Eike Stepper <stepper> | ||||||||||
| Severity: | normal | ||||||||||||
| Priority: | P3 | CC: | pascal.lehmann, saulius.tvarijonas | ||||||||||
| Version: | 4.0 | Flags: | stepper:
review+
stepper: review+ |
||||||||||
| Target Milestone: | --- | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Whiteboard: | |||||||||||||
| Attachments: |
|
||||||||||||
Created attachment 188498 [details]
Testcase (as patch)
When refreshing the session manually the delta is computed using the remote and local revision. However if you have already changed something locally the delta will not be correct as the ancestor should have been used to compute the delta. Yeah I realized that, thanks. We could fetch the 'clean' revision from the transaction instead. That would prevent local changes from being included. But that clean revision is not necessarily the same as the remote 'ancestor' -- especially if passiveUpdates=false, it's likely that it's not the ancestor... Created attachment 188506 [details]
Patch (including testcase)
Simple patch that allows a transaction to insert the clean, loaded revision into the viewedRevisions collection, instead of the revision actually backing the object. Perhaps viewedRevisions should be renamed 'loadedRevisions'. Created attachment 188647 [details]
Patch v2 - ready to be committed
I've moved the super call to the end of getViewedRevision() so that it is not unnecessarily (?) executed before cleanRevisions.get().
Do you think the string conversion here is needed?
assertEquals(removedID.toString(), removedValue.toString());
(In reply to comment #6) > I've moved the super call to the end of getViewedRevision() so that it is not > unnecessarily (?) executed before cleanRevisions.get(). I'm wondering if we should check first whether getViewedRevision returns null from the readNoLoad call. > Do you think the string conversion here is needed? > assertEquals(removedID.toString(), removedValue.toString()); No idea. Depends on the equals() of a particular ID implementation I suppose. The String comparison seemed like the safest way. Created attachment 188653 [details]
Patch v3
(In reply to comment #7) > (In reply to comment #6) > > I've moved the super call to the end of getViewedRevision() so that it is not > > unnecessarily (?) executed before cleanRevisions.get(). > > I'm wondering if we should check first whether getViewedRevision returns > null from the readNoLoad call. What would that change? What problem do you foresee otherwise? > > Do you think the string conversion here is needed? > > assertEquals(removedID.toString(), removedValue.toString()); > > No idea. Depends on the equals() of a particular ID implementation > I suppose. The String comparison seemed like the safest way. Not too important, but the toString() contract does not explicitely require the result to be unique. But all CDOIDs have an equals() method. Committed to trunk, rev. 7043 Available in R20110608-1407 |
Say object x has an isMany feature x.zz = {a,b,c}. In session1 with passiveUpdates=false and a resolver configured which implements CDOConflictResolver2, we load this object and add child 'd', so that x.zz = {a,b,c,d}. In another session, we load x also, remove 'a', and commit. Then we call refresh in session 1. The resolver in session 1 receives 1 CDORevDelta (correct), containing 1 CDOFeatDelta (correct), which is a CDOListFeatDelta containing 2 (incorrect) CDORemoveFeatDeltas. The superfluous CDORemoveFeatDelta seems to pertain to 'd'. Btw, I'm aware that we have vague plans to make CDOConflictResolver2 obsolete, but as long as those plans aren't executed (which I'm not sure is a good idea anyway), this bug stands...