-
Notifications
You must be signed in to change notification settings - Fork 484
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: calculate/add MD5 hash for multiple object delete. #581
Conversation
8812c1c
to
d9bcf6b
Compare
All functional tests pass with Amazon AWS S3 |
} else if (data instanceof byte[]) { | ||
sha256Digest.update((byte[]) data, 0, len); | ||
} else { | ||
throw new InternalException("unknown data to calculate SHA256 hash. This should not happend. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unknown data source to calculate sha256 hash. This should not happen, please report this issue at https://github.com/minio/minio-java/issues
- is better i think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
md5Digest.update((byte[]) data, 0, len); | ||
} else { | ||
throw new InternalException("unknown data to calculate SHA256 hash. This should not happend. " | ||
+ "Please report this issue at https://github.com/minio/minio-java/issues"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above..
} else if (data instanceof byte[]) { | ||
md5Digest.update((byte[]) data, 0, len); | ||
} else { | ||
throw new InternalException("unknown data to calculate SHA256 hash. This should not happend. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above..
return BaseEncoding.base64().encode(md5Digest.digest()); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unexpected line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no. you can see double newlines between methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a code review, have not tested yet.
sha256Hash = Digest.sha256Hash((RandomAccessFile) body, length); | ||
} else if (body instanceof byte[]) { | ||
sha256Hash = Digest.sha256Hash((byte[]) body, length); | ||
Object data = body; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might want to consider ternary operator for conciseness
Object data = (body == null) ? new byte[0] : body;
int len = (body == null) ? 0 : length;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this case, data
and len
are reset when data == null
in one if
seems better to me for readability sake.
There is a chance of misread two (body == null)
statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #579