jackrabbit-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Piyush Purang (JIRA)" <j...@apache.org>
Subject [jira] Commented: (JCR-320) BinaryValue equals fails for two objects with two different byte arrays that contain the same bytes.
Date Thu, 16 Feb 2006 08:20:00 GMT
    [ http://issues.apache.org/jira/browse/JCR-320?page=comments#action_12366599 ] 

Piyush Purang commented on JCR-320:

IMO returning 0 as hashCode implies all Values across all types are equal! The value should
atleast be different for different Types.

And it also doesn't really satisfy the javadoc comment of  "Returns zero to satisfy the Object
equals/hashCode contract. This class is mutable and not meant to be used as a hash key." A
HashMap with a value as a key will fail rather silently. With an UnsupportedOperationException
one will know the error of one's way at once.

>From an api design point of view we can dump Value equals and hashcode i.e. let direct
comparisons using equals always return false (i.e. Object's implementation holds true) and
same for hashCode.

And we could introduce a Values class that could introduce overloaded equals methods (Like
Arrays, Collections etc.), we can merge Values and the present ValueHelper class into one

And Jukka I will get down to providing you with a patch and testcases. It might take some
time though... 

> BinaryValue equals fails for two objects with two different byte arrays that contain
the same bytes.
> ----------------------------------------------------------------------------------------------------
>          Key: JCR-320
>          URL: http://issues.apache.org/jira/browse/JCR-320
>      Project: Jackrabbit
>         Type: Bug
>   Components: core
>     Versions: 0.9
>     Reporter: Piyush Purang
>     Priority: Minor

> http://svn.apache.org/repos/asf/incubator/jackrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbit/value/BinaryValue.java
> Here is the present implementation
>     public boolean equals(Object obj) {
>         if (this == obj) {
>             return true;
>         }
>         if (obj instanceof BinaryValue) {
>             BinaryValue other = (BinaryValue) obj;
>             if (text == other.text && stream == other.stream
>                     && streamData == other.streamData) {
>                 return true;
>             }
>             // stream, streamData and text are mutually exclusive,
>             // i.e. only one of them can be non-null
>             if (stream != null) {
>                 return stream.equals(other.stream);
>             } else if (streamData != null) {
>                 return streamData.equals(other.streamData);
>             } else {
>                 return text.equals(other.text);
>             }
>         }
>         return false;
>     }
> Here are the problems with the present implementation
> 1. streamData.equals(other.streamData ) will fail miserably.
> 2. too many return statements! I guess no one ran a checkstyle on it.
> 3. return stream.equals(other.stream); will always be false unless both have been created
with the same InputStream!  
> I wrote some testcases in SetValueBinaryTest
>     public void testBinaryValueEquals() throws Exception {
>         BinaryValue binaryValue1 = null;
>         BinaryValue binaryValue2 = null;
>         // initialize some binary value
>         data = createRandomString(10).getBytes();
>         binaryValue1 = (BinaryValue) superuser.getValueFactory().createValue(new ByteArrayInputStream(data));
>         binaryValue2 = (BinaryValue) superuser.getValueFactory ().createValue(new ByteArrayInputStream(data));
>         //ideallly setup a test harness that tests all the cases as defined by the contract
in Object.equals()
>         assertTrue( binaryValue1.equals(binaryValue2));
>         assertTrue( binaryValue1.equals(binaryValue1));
>         assertFalse( binaryValue1.equals(null));
>     }
>     public void testBinaryValueEquals2() throws Exception {
>         String str = createRandomString(10);
>         BinaryValue binaryValue1 = new BinaryValue(str.getBytes());
>         BinaryValue binaryValue2 = new BinaryValue(str.getBytes());
>         assertTrue( binaryValue1.equals(binaryValue2));
>         assertTrue( binaryValue1.equals(binaryValue1));
>         assertFalse( binaryValue1.equals(null));
>     }
>     public void testBinaryValueEquals3() throws Exception {
>         String str1 = "Some string xyz";
>         String str2 = new StringBuffer().append("Some string xyz").toString();
>         BinaryValue binaryValue1 = new BinaryValue(str1);
>         BinaryValue binaryValue2 = new BinaryValue(str2);
>         assertTrue( binaryValue1.equals(binaryValue2));
>         assertTrue( binaryValue1.equals(binaryValue1));
>         assertFalse( binaryValue1.equals(null));
>     }
> They were written quickly but with the present implementation the first two fail at the
very first assert* statement which for stream I can understand (as we are basically propogating
InputStream's equals contract) but for byte arrays I can't agree with this behaviour unless
it is so intended. It behaves the best when BinaryVlaue wraps a string i.e. testBinaryValueEquals3()
 passes without trouble.
> I propose a new implementation where I am not very convinced with the behaviour when
we have streams being wrapped. If it should fail for the streams then we should change the
documentation for the method. As for making it work right when streams are involved .. well
the streams will have to be read and compared.
> public boolean equals(Object obj) {
>         boolean result = false;
>         if (this == obj) {
>             result = true;
>         } else if (obj instanceof BinaryValue) {
>             BinaryValue other = (BinaryValue) obj;
>             // only one of text, stream and streamData are set at any given point
>             if (text != null) {
>                 result = text.equals(other.text);
>             } else if (stream != null) {
>                 result = stream.equals(other.stream);
>             } else if (streamData != null) {
>                 result = Arrays.equals(streamData, other.streamData);
>             } else {
>                 // all values are null; test that the other object (which could be us)
>                 // also has everything set to null!
>                 result = (other.text == null && other.stream == null
>                         && other.streamData == null);
>             }
>         }
>         return result;
>     }
> This implementation of course fails at the first assert* of the first test method testBinaryValueEquals
(It will pass the other two assert*s).
> And if this new implementation doesn't fit the bill and an alternative isn't needed then
just skip implementing equals.
> One more thought running through my mind is - if text, stream and data are mutually exclusive
why don't we have different classes for each of them? (Try and wrap a stringValue into a binary
> I have also noticed that the hashCodes all return 0's throughtout the package. In the
case that the hashCode can't keep up with the contract of equal I would propose throwing an
UnsupportedOpertaionException. And if not then declare it in the BasValue as it will be inherited
(unless this leads the QA tools to report infringement of the rule that when you define equals
you need to redefine hashCode - checkstyle does that).
> The value package doesn't have any tests for it.. or did I miss them? 

This message is automatically generated by JIRA.
If you think it was sent incorrectly contact one of the administrators:
For more information on JIRA, see:

View raw message