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

Bug 350987

Summary: Revision compare does not consider EObject values in references
Product: [Modeling] EMF Reporter: Egidijus Vaisnora <vaisegid>
Component: cdo.coreAssignee: Eike Stepper <stepper>
Status: CLOSED FIXED QA Contact: Eike Stepper <stepper>
Severity: normal    
Priority: P3 CC: Ed.Merks, saulius.tvarijonas
Version: 4.2   
Target Milestone: ---   
Hardware: All   
OS: All   
Whiteboard:
Bug Depends on: 351480    
Bug Blocks:    
Attachments:
Description Flags
test case v1
none
premature fix none

Description Egidijus Vaisnora CLA 2011-07-02 08:53:32 EDT
Revision compare produces wrong deltas, when comparing revision with CDOIDs against revision with EObjects containing the same CDOIDs.
Comment 1 Egidijus Vaisnora CLA 2011-07-02 08:57:10 EDT
Created attachment 199000 [details]
test case v1

Issue was originated from https://bugs.eclipse.org/bugs/show_bug.cgi?id=349958
Comment 2 Egidijus Vaisnora CLA 2011-07-05 09:46:44 EDT
Created attachment 199126 [details]
premature fix

There are two drawbacks: 
- I was needed to copy-paste list compare method from EMF. Need to overload object compare
- legacy object is not supported. All legacy  object support is located in org.eclipse.emf.cdo component, but we need to use it in commons. Cyclic dependency
Comment 3 Egidijus Vaisnora CLA 2011-07-05 09:59:44 EDT
Looks like we hit on obstacles going this direction. I don't feel comfortable by copy and pasting emf compare methods and reworking simple object compare logic... In addition we will need to provide CDOObject and legacy object related code into "commons" component...
Just looks like we are going into wrong direction and perhaps we should re-consider fix provided in https://bugs.eclipse.org/bugs/show_bug.cgi?id=349958. It is simple solution, which is working for current CDO architecture. 
Eike, what do you think?
Comment 4 Eike Stepper CLA 2011-07-05 14:09:34 EDT
Ed, in your ListDifferenceAnalyzer.createListChanges() there's this part of a condition: oldValue.equals(newValue). Do you think it's affordable to refactor this condition into a protected method so that we can override it separately?
Comment 5 Eike Stepper CLA 2011-07-05 14:11:44 EDT
I think it could even be additional:

  if (oldValue == null ? newValue == null : oldValue == newValue || oldValue.equals(newValue) || overrideableEquals(oldValue, newValue))
Comment 6 Eike Stepper CLA 2012-08-14 22:55:55 EDT
Moving all open issues to 4.2. Open bugs can be ported to 4.1 maintenance after they've been fixed in master.
Comment 7 Eike Stepper CLA 2012-11-01 01:44:12 EDT
Now that ListDifferenceAnalyzer.equal(Object, Object) is protected (EMF 2.8) we can override and call:


  private boolean compare(Object originValue, Object dirtyValue)
  {
    originValue = convertEObject(originValue);
    dirtyValue = convertEObject(dirtyValue);

    if (originValue == null)
    {
      return dirtyValue == null;
    }

    if (dirtyValue == null)
    {
      return false;
    }

    return originValue == dirtyValue || originValue.equals(dirtyValue);
  }

  private Object convertEObject(Object value)
  {
    CDOID id = CDOIDUtil.getCDOID(value);
    if (id != null)
    {
      return id;
    }

    return value;
  }
Comment 8 Eike Stepper CLA 2012-11-01 02:12:46 EDT
commit 607c5558dcc1916cb56e4733d4c7f198d551e809
Comment 9 Eike Stepper CLA 2012-11-01 05:08:15 EDT
Bugzilla_350987_Test.testRestoringReferences() fails in legacy scenarios.
Comment 10 Eike Stepper CLA 2012-11-01 05:09:24 EDT
CDOLegacyWrapper.equals() should be moved up to CDOObjectWrapperBase and changed to:

  @Override
  public boolean equals(Object obj)
  {
    return obj == this || obj == instance || //
        obj instanceof CDOObjectWrapperBase && ((CDOObjectWrapperBase)obj).instance == instance;
  }
Comment 11 Eike Stepper CLA 2012-11-01 05:09:54 EDT
commit 5e4be45a27589a157ad508815635db8de61b5497
Comment 12 Eike Stepper CLA 2013-06-27 03:30:58 EDT
Available in R20130613-1157 (4.2)