[ http://issues.apache.org/jira/browse/JCR-320?page=all ]
Jukka Zitting updated JCR-320:
------------------------------
Version: 1.0.1
Piyush, did you get around to creating a patch for this? If not, I'll just take
your code from the above comments and apply it.
> 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, 1.0, 1.0.1
> Reporter: Piyush Purang
> Assignee: Jukka Zitting
> Priority: Minor
> Fix For: 1.1
>
> 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 value...)
> 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:
http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
http://www.atlassian.com/software/jira
