[
https://issues.apache.org/jira/browse/THRIFT-4857?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16832807#comment-16832807
]
Garret Wilson commented on THRIFT-4857:
---------------------------------------
All right, I just created a [pull
request|https://github.com/apache/thrift/pull/1792]. Let me know if I missed
anything.
The contribution instructions weren't clear whether the language indication
(e.g. {{java}}) in the pull request should be uppercase or lowercase. I saw
both in historical commits. As the official contribution instructions showed
e.g. {{perl}}, I guess that you want to follow the token form used in the
{{thrift --gen}} CLI, so I went with {{java}} rather than {{Java}} in the
commit message.
I have my own opinions about approaches to generating hash codes (doing it
manually is tedious and error-prone), and I could quibble about the approach to
checking for class equivalence. For example, the equality check compares exact
classes, which the author apparently thought would be more efficient, but this
will break if anyone ever subclasses {{TField}}. Either someone should make
{{TField}} a {{final}} class, or use a normal {{instanceof}} in comparison. But
these issues are ancillary to the core bug here, so I tried to make my change
as surgical as possible, especially as this is my first contribution.
Let me know what you think!
> Java field hash code implementation inconsistent with equals.
> -------------------------------------------------------------
>
> Key: THRIFT-4857
> URL: https://issues.apache.org/jira/browse/THRIFT-4857
> Project: Thrift
> Issue Type: Bug
> Components: Java - Library
> Affects Versions: 0.12.0
> Reporter: Garret Wilson
> Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> The {{TField}} hash code implementation is inconsistent with equals, which is
> a breaking bug. If you know what hash codes are and are familiar with the
> Java {{Object}} API, then you already know what I'm talking about. Basically
> Java _requires_ that, if you overriden {{hashCode()}} and {{equals()}}, then
> for any two objects that are equal they _must_ return the same hash code.
> The {{TField}} class API contract isn't clear about what is considered
> equality, but according to the {{TField.equals()}} implementation, fields are
> equal if and only if:
> * Both objects are a {{TField}} (I'm generalizing here; there's another
> subtle bug lurking with class checking, but that's another story).
> * The fields both have the same {{type}} and {{id}}.
> In other words, fields are equal _without regard to name_. And this follows
> the overall Thrift architecture, in which field names are little more than
> window dressing, and the IDs carry the semantics.
> Unfortunately {{TField.hashCode()}} includes the name in the hash code
> calculation! This completely breaks the {{Object}} contract. It makes the
> hash code inconsistent with equality. To put it another way, two fields
> {{foo}} and {{bar}} could have the same type and ID, and {{foo.equals(bar)}}
> would return {{true}}, but they would be given different hash codes!!
> This is completely forbidden, and means that with this bug you cannot use a
> {{TField}} as the key in a map, for example, or even reliably keep a
> {{Set<TField>}} of fields! If you were to store {{foo}} and {{bar}} as keys
> in a map, for example, the different hash codes would put them in different
> buckets, even though they were considered equal, providing inconsistent and
> strange lookup results, depending on the name of the field you used to query
> with.
> This is simply broken as per the Java {{Object}} API contract.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
