feat(storage): add full object checksum validation for bidi flow#13266
feat(storage): add full object checksum validation for bidi flow#13266Dhriti07 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements cumulative checksum validation for full object reads within the ObjectReadSessionStream. It introduces a CumulativeHasher that is conditionally applied in BaseObjectReadSessionStreamRead and validated in ObjectReadSessionStream upon completion. The changes also include extensive unit tests covering success and failure cases for single and multi-chunk reads. Feedback suggests refining the logic for enabling the CumulativeHasher; currently, it activates for any read starting at offset 0, which could incorrectly include partial reads. It is recommended to use rangeSpec.isAll() to restrict this validation strictly to full object reads.
| this.hasher = | ||
| (rangeSpec.begin() == 0) | ||
| ? new CumulativeHasher(hasher, 0, rangeSpec.maxLength()) | ||
| : hasher; |
There was a problem hiding this comment.
The condition rangeSpec.begin() == 0 is too broad for full object checksum validation. It will trigger for any partial read that starts at the beginning of the object (e.g., reading only the first 100 bytes). This results in unnecessary overhead and potential validation failures if the hasher attempts to compare a partial range's checksum against the full object's checksum. Consider using rangeSpec.isAll() to ensure validation is only enabled for full object reads.
| this.hasher = | |
| (rangeSpec.begin() == 0) | |
| ? new CumulativeHasher(hasher, 0, rangeSpec.maxLength()) | |
| : hasher; | |
| this.hasher = | |
| rangeSpec.isAll() | |
| ? new CumulativeHasher(hasher, 0, rangeSpec.maxLength()) | |
| : hasher; |
| this.hasher = | ||
| (rangeSpec.begin() == 0) | ||
| ? new CumulativeHasher(hasher, 0, rangeSpec.maxLength()) | ||
| : hasher; |
There was a problem hiding this comment.
Similar to the AccumulatingRead constructor, using rangeSpec.begin() == 0 here will enable CumulativeHasher for partial reads starting at offset 0. It is better to use rangeSpec.isAll() to target only full object reads.
| this.hasher = | |
| (rangeSpec.begin() == 0) | |
| ? new CumulativeHasher(hasher, 0, rangeSpec.maxLength()) | |
| : hasher; | |
| this.hasher = | |
| rangeSpec.isAll() | |
| ? new CumulativeHasher(hasher, 0, rangeSpec.maxLength()) | |
| : hasher; |
Adding full object checksum for bidi flow
Refer to: go/full_checksum_java