Bugzilla – Full Text Bug Listing
| Summary: | EcoreUtil#equals() returning wrong result for unset/proxified object | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Stephan Eberle <stephaneberle9> | ||||||
| Component: | Core | Assignee: | Ed Merks <Ed.Merks> | ||||||
| Status: | CLOSED FIXED | QA Contact: | |||||||
| Severity: | major | ||||||||
| Priority: | P3 | ||||||||
| Version: | unspecified | ||||||||
| Target Milestone: | --- | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
Hmmm. Should two objects that are otherwise not equal really be considered equal? The testing resolves proxies, so we must be dealing here with unresolvable proxies. In what scenarios/context does this represent a major problem? (In reply to comment #1) > The testing resolves proxies, so we must be dealing here with > unresolvable proxies. Yes, in fact. The proxies are unresolvable. > In what scenarios/context does this represent a major > problem? In our case, we called EcoreUtil#equals(EObject, EObject) with two root objects and none of them being a proxy. But then, while walking down the object trees below we hit on a reference which points to a resolvable/resolved and completely uninitialized target EObject on one side and to an unresolvable proxy on the other side. We would say that the two object trees should be considered as inequal in such a case, but the result returned by EcoreUtil#equals(EObject, EObject) is anyway true. The logic you've shown in the patch will make any two proxies be equal regardless of their proxy URI's value, their class, or any other features set on them. That can't possibly be a good thing. It seems sensible to *add* logic to test that they have equal proxy URIs but I'm not sure any other logic should be bypassed. (In reply to comment #3) > The logic you've shown in the patch will make any two proxies be equal > regardless of their proxy URI's value, their class, or any other features set > on them. That can't possibly be a good thing. It seems sensible to *add* logic > to test that they have equal proxy URIs but I'm not sure any other logic should > be bypassed. That's definitely true. I'll try to improve the patch and submit a new one. Created attachment 175637 [details]
Improved patch
Improved proposed patch in 2 regards:
1) Moved newly introduced proxy-related equality check behind EClass equality check
=> previously compared object check, identical object check, and EClass equality check now also done for proxies
2) Enhanced newly introduced proxy equality check by adding an evaluation of the proxy URIs
=> proxies with different proxy URIs no longer considered equal
However, I don't think that it is necessary to check the features if the compared objects are proxies. Even if they are still set the information they hold is obsolete. The only purpose of the proxy is to get resolved, i.e., to become replaced by another EObject instance of the same EClass which is referenced by the proxy URI. I therefore would say that the EClass and the proxy URI are the only relevant information of a proxy and all other information, if present, is to be ignored.
Yes, I wondered about that too. One could imagine a proxy from an unloaded resource being fully populated with feature values while a proxy from a freshly loaded resource that's not yet resolved might have no features populated. One might want to consider those equal because one would expect them to resolve to the same object in the end.... I suppose it's even possible that one has proxies of different types with the same proxy URI that would resolve to the same object. So maybe comparing classes isn't necessary. Perhaps just adding this before the classes are compared would be good.
if (eObject1.eIsProxy() && eObject2.eIsProxy())
{
if (((InternalEObject)eObject1).eProxyURI().equals(((InternalEObject)eObject2).eProxyURI()))
{
put(eObject1, eObject2);
put(eObject2, eObject1);
return true;
}
}
I.e., if both are proxies, we compare their URIs. If those are equal, we match them and return. In all other cases, we proceed just as always.
What do you think?
(In reply to comment #7) > I suppose it's even possible that one has proxies of different types with the > same proxy URI that would resolve to the same object. So maybe comparing > classes isn't necessary. That is correct! This may happen, for instance, when on one side the proxy has been created from the reference type, and on the other side from a previously referenced object instance whose type was a sub type of the reference type. So OK for not comparing classes of proxies. > I.e., if both are proxies, we compare their URIs. If those are equal, we match > them and return. In all other cases, we proceed just as always. > > What do you think? This doesn't solve my initial problem which was that I had two object trees where on one side the proxies were resolved and on the other side not. I therefore think that we cannot do without explicitly checking if one is a proxy and the other one not (in both directions) an returning false if this is the case. At the bottom line, it seems to me that keeping all proxy-related checks from attached improved patch and just moving them before the class comparison check would be close to want we need here. So we want this logic before comparing the classes?
// If eObject1 is a proxy...
//
if (eObject1.eIsProxy())
{
// Then the other object must be a proxy with the same URI.
//
if (((InternalEObject)eObject1).eProxyURI().equals(((InternalEObject)eObject2).eProxyURI()))
{
put(eObject1, eObject2);
put(eObject2, eObject1);
return true;
}
else
{
return false;
}
}
The fix is committed to CVS for 2.7. Thanks! The fix is available in 3.7RC3 and later. |
Created attachment 174595 [details] Proposed patch Steps to reproduce: * Invoke EcoreUtil#equals(EObject, EObject) with ** an instance of an arbitrary EClass where all features are unset (i.e., an instance which has just been created but no setter has been called yet) ** with another instance of the same EClass that is a proxy Expected result: false Actual result: true