From dev-return-56020-archive-asf-public=cust-asf.ponee.io@thrift.apache.org Fri May 3 20:32:06 2019 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by mx-eu-01.ponee.io (Postfix) with SMTP id 14ECF18064D for ; Fri, 3 May 2019 22:32:05 +0200 (CEST) Received: (qmail 27439 invoked by uid 500); 3 May 2019 20:32:04 -0000 Mailing-List: contact dev-help@thrift.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@thrift.apache.org Delivered-To: mailing list dev@thrift.apache.org Received: (qmail 27204 invoked by uid 99); 3 May 2019 20:32:04 -0000 Received: from mailrelay1-us-west.apache.org (HELO mailrelay1-us-west.apache.org) (209.188.14.139) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 03 May 2019 20:32:04 +0000 Received: from jira-lw-us.apache.org (unknown [207.244.88.139]) by mailrelay1-us-west.apache.org (ASF Mail Server at mailrelay1-us-west.apache.org) with ESMTP id 48D90E2B5F for ; Fri, 3 May 2019 20:32:03 +0000 (UTC) Received: from jira-lw-us.apache.org (localhost [127.0.0.1]) by jira-lw-us.apache.org (ASF Mail Server at jira-lw-us.apache.org) with ESMTP id C1AC72581D for ; Fri, 3 May 2019 20:32:01 +0000 (UTC) Date: Fri, 3 May 2019 20:32:01 +0000 (UTC) From: "Garret Wilson (JIRA)" To: dev@thrift.apache.org Message-ID: In-Reply-To: References: Subject: [jira] [Commented] (THRIFT-4857) Java field hash code implementation inconsistent with equals. MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-JIRA-FingerPrint: 30527f35849b9dde25b450d4833f0394 [ 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}} 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)