Community
Participate
Working Groups
Build Identifier: M20100909-0800 If the list contains proxies and number of elements is more than 4 then contains() method returns incorrect result. Reproducible: Always Steps to Reproduce: 1. Run attached test case. 2. 3.
Created attachment 206779 [details] Test case
It's hard to imagine how this model is useful in a meaningful way. You don't resolve proxies automatically for B.x yet that's the end you're serializing and even worse, in your example here, it contains a cross document reference. How are you ever expecting that proxy to be resolved? Of course if you set it as proxy resolving, the Ecore modeling won't validate because it doesn't make sense to serialize only the single valued side of a bidirectional reference... Can you explain how your example represents a meaningful use case?
Lets say A is a Type and B is a Property. {transient} [Type]--type------------------typedProperties--[Property] If I load a resource that contains Type A then I do not want to load all resources that contains Properties which types are set to Type A. If one of resources that contains a Property that type is A loaded then I want that the resource that contains Type A would be loaded. After load of resources I fix such bidirectional references.
> Lets say A is a Type and B is a Property. {transient} > [Type]--type------------------typedProperties--[Property] > If I load a resource that contains Type A then I do not want to load > all resources that contains Properties which types are set to Type A. Loading a resource doesn't cause other resources to load. Only resolving proxies does that. > If one of resources that contains a Property that type is A loaded then > I want that the resource that contains Type A would be loaded. Again, this happens as proxies are resolved not directly as a result of loading. You don't need a bidirectional reference for this either. > After load of resources I fix such bidirectional references. What does "fixing" mean? It doesn't sound like you have proper bidirectional references but rather broken results that need manual fixing. In the end, I fail to see anything here that justifies the approach as being sound. If you're manually going to fix proxies that aren't working properly anyway, then I don't see the need to further complicate the framework to deal with what looks pathological. I'm sure you can work around the current situation so I'll return this report as won't fix unless there is a compelling use case.
>> Lets say A is a Type and B is a Property. > {transient} >> [Type]--type------------------typedProperties--[Property] >> If I load a resource that contains Type A then I do not want to load >> all resources that contains Properties which types are set to Type A. > Loading a resource doesn't cause other resources to load. Only resolving > proxies does that. [Rimvydas]: I know that. >> If one of resources that contains a Property that type is A loaded then >> I want that the resource that contains Type A would be loaded. > Again, this happens as proxies are resolved not directly as a result of > loading. You don't need a bidirectional reference for this either. [Rimvydas]: All references in my model must be bidirectional. This is a requirement. >> After load of resources I fix such bidirectional references. > What does "fixing" mean? It doesn't sound like you have proper bidirectional > references but rather broken results that need manual fixing. In the end, I > fail to see anything here that justifies the approach as being sound. If > you're manually going to fix proxies that aren't working properly anyway, then > I don't see the need to further complicate the framework to deal with what > looks pathological. > > I'm sure you can work around the current situation so I'll return this report > as won't fix unless there is a compelling use case. [Rimvydas]: Fixing means that property.type = a a.typedProperties = {property} One reference is transient so only one end is saved and because EMF does not have support for such case I have to fix this manually. The model is valid and the code is generated using standard generator so maybe this is not usual case but we can not say that it is pathological. Let's go back to the problem. In the test case (see EcoreEListTest.java) index of element b2 is 1. So if we have 5 elements in the list: a.getY().indexOf(b2) -> returns 1 a.getY().contains(b2) -> returns false if we have 4 elements in the list (remove the line a.getY().add(b5); from the test case and the test will pass): a.getY().indexOf(b2) -> returns 1 a.getY().contains(b2) -> returns true This looks to me like simple bug.
I see no value in fixing this issue. It will certainly add cost for all clients, i.e., minimally a proxy check, and in the case of a proxy, then what? A full iteration over the list, including a second pass that resolves proxies? As I understand it, this proxy resolution is something you're hoping to avoid. So what is the value in making the changes? No other client has complained (because they don't have this questionable use case) and yet all clients will pay a cost. As such, I have to ask, what will be the benefit for that cost? It seems to me you can and already must work around the issue of manual proxy resolution, so you're certainly in a position to write that logic so that it takes this issue into account. Another probably more consistent design approach would be to make both ends proxy resolving and make neither end transient and instead specialize XMLSaveImpl.shouldSaveFeature to return false for such cases.
Bug fix will add cost but the code will work correctly. Possible fix: EcoreEList.java line 376 return ((EObject)object).eGet(getInverseEReference()) == owner; should be replaced with: EObject eObject = ((EObject)object); EObject value = (EObject) eObject.eGet(getInverseEReference()); if (value == null) { return false; } if (value.eIsProxy()) { return owner == EcoreUtil.resolve(value, eObject); } else { return value == owner; } Additional cost for users which do not have non resolving references in their models is: if (value == null) if (value.eIsProxy()) Is this a significant cost ? I can use a workaround. Workaround would be to use indexOf(obj) != -1 instead of contains(obj). Thanks for the hint about solving the problem with XMLSaveImpl.shouldSaveFeature but my model also can be saved into DB using CDO. I do not know whether CDO supports similar functionality.
I think the following minimizes the overhead for existing clients. Object opposite = ((EObject)object).eGet(getInverseEReference()); if (opposite == owner) { return true; } else if (opposite == null || !((EObject)opposite).eIsProxy()) { return false; } The true case has no additional overhead. The false case now involves additional null pointer check (exceedingly cheap) and a cast as well as a interface method call, both of which are relatively expensive. I won't consider adding code to resolve the proxy; your model should do that if you want that. If neither case applies, it does the brute force walk of the contents, including resolving all the proxies, which you probably don't want so you won't be happy with this solution. I continue to get the impression that you really don't need this problem addressed in order to make your software work.
Your fix solves the problem. As I said in the last comment I can use workaround with indexOf() because I know that current implementation of contains() works incorrectly.
The change is committed to git for 2.8.
The changes are available in the M6 build.
