thrift-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Garret Wilson (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (THRIFT-4857) Java field hash code implementation inconsistent with equals.
Date Fri, 03 May 2019 20:32:01 GMT

    [ 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)

Mime
View raw message