Bugzilla – Full Text Bug Listing
| Summary: | ArtifactDescriptor.equal() breaks the general contract. | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Equinox | Reporter: | Thomas Hallgren <thomas> | ||||||||
| Component: | p2 | Assignee: | Andrew Niefer <aniefer> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | pascal | ||||||||
| Version: | 3.6 | ||||||||||
| Target Milestone: | 3.6 M5 | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
Created attachment 154641 [details]
Patch implementing #2
Please wait for Andrew's review. The issue of identity of ArtifactDescriptor has been at the core of several long discussions (see bug 249035, 254652). If I remember correctly, the capsule summary was that artifact descriptors are repository specific objects. You can pass them in as a mechanism to describe what you are inserting into the repo, but the repo will take a copy of this object and tweak it to its needs. IArtifactRepo#contains(AD) will only return true if the AD passed in has been obtained by a query on this repo. (In reply to comment #2) > Please wait for Andrew's review. of course. > IArtifactRepo#contains(AD) will only return true if the AD passed in has been > obtained by a query on this repo. The behavior of that method is not changed by the patch. artifactDescriptors.contains(AD); The 'artifactDescriptors' in this case is a HashSet. The HashSet uses the keys of an internal HashMap and thus calls containsKey(AD). That method in turn, compares uses AD to find the element by hashCode and then compares AD to that element using the equals() method of AD. Hence, the most generic compare method is used. That said, I think the semantic behind the contains() method is flawed. The hashCode was computed for the element in the collection, and thus, took the ARTIFACT_REFERENCE into consideration which means that the actual result of the contains() call may be random. It might well be that two similar artifacts, one with the ARTIFACT_REFERENCE defined, and one without, will have a hashCode that results in an index that appoints the same slot in the hash table. When that happens, the artifact will be found. If they don't appoint the same hash slot, the artifact is not found, even though the equals method returns true. That would be a fun bug to track... There are three solutions to that problem: 1. Use identity compare instead of contains() 2. Remove the special equals() and hashCode() implementation from SimpleArtifactDescriptor 3. Convert the argument to the contains method to a SimpleArtifactDescriptor if it's not already. My preference would be #2 (which would deprecate my patch) but I'm not sure if that would have any other implications. Thinking while you are waiting for Andrew, it could be useful to write a simple test case demonstrating the issue you are encountering. Created attachment 154681 [details] alternative It looks like not everything we talked about made it into the code. I believe the attached patch still has a problem with transitivity A : SimpleArtifactDescriptor A_1.0.0 REF=1 B : ArtifactDescriptor A_1.0.0 C : SimpleArtifactDescriptor A_1.0.0 REF=2 Here REF just represents some internal detail of the repo that distinguishes B & C. We have A == B (and B==A) , B == C, but A != C I forget all the details of the last conversation, but I think we thought that we needed A != B. See the attached patch. > If I check if desc is equal to any of the descriptors in the repository, that > check yields true. If I check if any of the elements returned by the query is > equal to desc, that yields false. We would then have both of these returning false. For IArtifactRepository.contains, we thought about renaming or getting rid of this. We decided to keep it as a convenience method, such that contains would return true if a query with the given descriptor would contain results. This is a lot looser than equals. I think the fix there is #3, convert the argument to the internal SimpleArtifactDescriptor, or actually delegate to query. Converting to the internal descriptor class should just be an optimization of the running the query. The reference stuff is used by dropins and update site support. Created attachment 154684 [details]
updated alternative
updated patch, the equals can't go by itself, the contains needs to be done at the same time.
patch is released to the api branch |
I'm running some experiments with queries and I found some odd behavior. I have an ArtifactDescriptor created as: ArtifactDescriptor desc = new ArtifactDescriptor(new ArtifactKey("osgi.bundle", "a", Version.create("2.0.0"))); I add this descriptor to a SimpleArtifactRepository. I then query that repository and check if the result contains the added descriptor. This check can have two outcomes depending on how I check. If I check if desc is equal to any of the descriptors in the repository, that check yields true. If I check if any of the elements returned by the query is equal to desc, that yields false. This is because the general contract of the equals method is broken. A == B but B != A. ArtifactDescriptor == SimpleArtifactDescriptor is OK. SimpleArtifactDescriptor == ArtifactDescriptor is always false. A decision must be made to resolve this. We have three options: 1. If the classes differ, the result is always false. 2. If the classes differ, we always use the more generic implementation of equals. 3. If the classes differ, we always use the most specialized implementation of equals and treat the missing field as null. My personal preference is #2 since it doesn't require that the generic implementation have knowledge of it's subclasses. The fix must make hashCode() return the same value for objects that are considered equal. Does anyone have an opinion on this or can I go ahead and fix it?