[ http://issues.apache.org/jira/browse/JCR-320?page=all ]
Jukka Zitting updated JCR-320:
------------------------------
Version: 1.0
> BinaryValue equals fails for two objects with two
different byte arrays that contain the same bytes.
>
------------------------------------------------------------
----------------------------------------
>
> Key: JCR-320
> URL: http://i
ssues.apache.org/jira/browse/JCR-320
> Project: Jackrabbit
> Type: Bug
> Components: core
> Versions: 0.9, 1.0
> Reporter: Piyush Purang
> Priority: Minor
> Fix For: 1.1
>
> http://svn.apache.org/repos/asf/incubator/ja
ckrabbit/trunk/jackrabbit/src/main/java/org/apache/jackrabbi
t/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.atl
assian.com/software/jira
|