Mailing-List: contact issues-help@commons.apache.org; run by ezmlm
Precedence: bulk
Reply-To: issues@commons.apache.org
Message-ID: <496840743.1225185044796.JavaMail.jira@brutus>
Date: Tue, 28 Oct 2008 02:10:44 -0700 (PDT)
From: "David Jones (JIRA)" <jira@apache.org>
To: issues@commons.apache.org
Subject: [jira] Issue Comment Edited: (LANG-467) EqualsBuilder and
 HashCodeBuilder treat java.math.BigDecimal inconsistantly and break general
 contract of hashCode
In-Reply-To: <1146294641.1225101284518.JavaMail.jira@brutus>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


    [ https://issues.apache.org/jira/browse/LANG-467?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12642958#action_12642958 ] 

davey.jones edited comment on LANG-467 at 10/28/08 2:10 AM:
------------------------------------------------------------

The use of compareTo() in EqualsBuilder() is arguably wrong, however compareTo is referenced from the BigDecimal javadoc as the alternative to equals for cases where the scale is not relevant

In LANG-393 it was decided that EqualsBuidler should use the compareTo() method to compare BigDecimals(), which is a nice convenience for those of use using BigDecimals in conjunction with EqualsBuilder and who want 0 and 0.0 to be considered equal.

However LANG-393 did not put the equivalent fix into the HashCodeBuilder so this has made HashCodeBuilder and EqualsBuilder non-symmetric. It was actually LANG-393 which added a feature to EqualsBuilder and at the same time created this bug in HashCodeBuilder.

So as far as the EqualsBuilder is concerned 10.2 and 10.20 are equal, i.e. the following evaluates to true.
 {code}
new EqualsBuilder().append(new BigDecimal("10.2"), new BigDecimal("10.20")).isEquals();
{code}

However when using these two values with HashCodeBuilder they actually give different hashCodes(), the following evaluates to false.
{code}
new HashCodeBuilder(17, 37).append(new BigDecimal("10.2")).toHashCode() == new HashCodeBuilder(17, 37).append(new BigDecimal("10.20")).toHashCode()
{code}

However the contract of hashCode() method for Object says that if two objects are considered equal using their equals method then they must also generate the same hashCode().

Of course this is true for BigDecimal class itself, even though it is somewhat inconvenient. 

MyPojo class as given in the test case above 
* implements the equals() method as documented by EqualsBuilder
* implements the hashCode() method as documented by HashCodeBuilder

Despite following the documented approach for implementing equals and hashCode the test case *proves* that MyPojo breaks the contract of hashCode(), the following evaluates to true
{code}
myPojo1.equals(myPojo2)
{code}
However myPojo1 and myPojo2 generate different hashCodes(), the following evaluates to false
{code}
myPojo1.hashCode() == myPojo2.hashCode()
{code}


      was (Author: davey.jones):
    The use of compareTo() in EqualsBuilder() is arguably wrong, however compareTo is referenced from the BigDecimal javadoc as the alternative to equals for cases where the scale is not relevant

In LANG-393 it was decided that EqualsBuidler should use the compareTo() method to compare BigDecimals(), which is a nice convenience for those of use using BigDecimals in conjunction with EqualsBuilder and who want 0 and 0.0 to be considered equal.

However LANG-393 did not put the equivalent fix into the HashCodeBuilder so this has made HashCodeBuilder and EqualsBuilder non-symmetric. It was actually LANG-393 which added a feature to EqualsBuilder and at the same time created this bug in HashCodeBuilder.

So as far as the EqualsBuilder is concerned 10.2 and 10.20 are equal, i.e. the following evaluates to true.
 {code}
new EqualsBuilder().append(new BigDecimal("10.2"), new BigDecimal("10.20")).isEquals();
{code}

However when using these two values with HashCodeBuilder they actually give different hashCodes(), the following evaluates to false.
{code}
new HashCodeBuilder(17, 37).append(new BigDecimal("10.2")) == new HashCodeBuilder(17, 37).append(new BigDecimal("10.20"))
{code}

However the contract of hashCode() method for Object says that if two objects are considered equal using their equals method then they must also generate the same hashCode().

Of course this is true for BigDecimal class itself, even though it is somewhat inconvenient. 

MyPojo class as given in the test case above 
* implements the equals() method as documented by EqualsBuilder
* implements the hashCode() method as documented by HashCodeBuilder

Despite following the documented approach for implementing equals and hashCode the test case *proves* that MyPojo breaks the contract of hashCode(), the following evaluates to true
{code}
myPojo1.equals(myPojo2)
{code}
However myPojo1 and myPojo2 generate different hashCodes(), the following evaluates to false
{code}
myPojo1.hashCode() == myPojo2.hashCode()
{code}

  
> EqualsBuilder and HashCodeBuilder treat java.math.BigDecimal inconsistantly and break general contract of hashCode
> ------------------------------------------------------------------------------------------------------------------
>
>                 Key: LANG-467
>                 URL: https://issues.apache.org/jira/browse/LANG-467
>             Project: Commons Lang
>          Issue Type: Bug
>    Affects Versions: 2.4
>            Reporter: David Jones
>            Priority: Minor
>
> A POJO with a BigDecimal field and equals() and hashCode() methods implemented using EqualsBuilder and HashCodeBuilder breaks the general contract of Object.hashCode();
> EqualsBuilder treats BigDecimal specially by comparing it using BigDecimal.compareTo() == 0 rather than BigDecimal.equals()
> Using BigDecimal.compareTo() ignores the scale of the BigDecimal()
> However the append(Object o) method of HashCodeBuilder uses BigDecimal.hashCode() in the case that o is a BigDecimal, which takes the scale into account when generating the hashCode.
> The following test case shows the problem!
> {code:title=TestApacheCommonsLangHashCodeBuilder.java|borderStyle=solid}
> // package declaration and imports not shown
> public class TestApacheCommonsLangHashCodeBuilder extends TestCase {
>     
>     public void testHashCode() {
>         MyPojo myPojo1 = new MyPojo(new String("foo"), new BigDecimal("10.2"));
>         MyPojo myPojo2 = new MyPojo(new String("foo"), new BigDecimal("10.20"));
>         
>         // equals method ignores the scale of the big decimal
>         // so this test passes
>         assertTrue(myPojo1.equals(myPojo2));
>         
>         // however in the case the equals returns true the
>         // hashCode must be the same according to the contract
>         // TEST FAILS AT THIS LINE
>         assertEquals(myPojo1.hashCode(), myPojo2.hashCode());
>     }
>     
>     private class MyPojo {
>         private String name;
>         private BigDecimal value;
>         
>         public MyPojo(String name, BigDecimal value) {
>             this.name = name;
>             this.value = value;
>         }
>         
>         public String getName() {
>             return name;
>         }
>         public BigDecimal getValue() {
>             return value;
>         }
>         /**
>          * equals method implemented using EqualsBuilder 
>          * as documented by apache commons
>          */
>         @Override public boolean equals(Object obj) {
>             if(this == obj) {
>                 return true;
>             }
>             
>             if(!(obj instanceof MyPojo)) {
>                 return false;
>             }
>             
>             MyPojo other = (MyPojo) obj;
>             return new EqualsBuilder()
>                 .append(name, other.getName())
>                 .append(value, other.getValue())
>                 .isEquals();
>         }
>         
>         /**
>          * hashCode method implemented using HashCodeBuilder
>          * as documented by apache commons
>          */
>         @Override public int hashCode() {
>             HashCodeBuilder hcb = new HashCodeBuilder(17, 31);
>             return hcb
>                 .append(name)
>                 .append(value)
>                 .toHashCode();
>         }
>     }
> }
> {code}
> Note that the only reason I haven't provided a patch is because I could not think of one which I thought was reasonable.
> *Option 1*
> Always set the scale to some value and then get the hashCode()
> Example change in HashCodeBuilder.append(Object) add the following:
> {code}
> else if (object instanceof BigDecimal) {
> 	append(((BigDecimal) object).setScale(DEFAULT_BIGDECIMAL_SCALE, RoundingMode.DOWN).hashCode());
> }
> {code}
> However what is a reasonable scale for calculating this hashCode? I cannot see a reasonanble scale to choose, that depends on the circumstance
> *Option 2*
> stripTrailingZeros() before calculating the hashCode()
> Example change in HashCodeBuilder.append(Object) add the following:
> {code}
> else if (object instanceof BigDecimal) {
> 	append(((BigDecimal) object).stripTrailingZeros().hashCode());
> }
> {code}
> The performance of this method under different circumstances is not documented.
> *Option 3*
> Document the problem and propose that the client solves the problem.
> For example change HashCodeBuilder documentation as follows
> {code}
> /*
>  * ...
>  * public class Person {
>  *   String name;
>  *   int age;
>  *   boolean smoker;
>  *   BigDecimal netWorth;
>  *   ...
>  *
>  *   public int hashCode() {
>  *     // you pick a hard-coded, randomly chosen, non-zero, odd number
>  *     // ideally different for each class
>  *     return new HashCodeBuilder(17, 37).
>  *       append(name).
>  *       append(age).
>  *       append(smoker).
>  *       // take special care when using BigDecimal as scale takes 
>  *       // is included in the hashCode calculation breaking hashCode contract
>  *       // choose a scale which is reasonable for hashCode calculation
>  *       append(netWorth == null ? null : netWorth.setScale(2)).
>  *       toHashCode();
>  *   }
>  * }
>  * ...
>  */
> {code}

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.