fix(#4329): use readFully instead of read in JavaBinarySerializer.readExternal#4348
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Coverage variation | ✅ -7.95% coverage variation |
| Diff coverage | ✅ 100.00% diff coverage |
Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (61177a8) 127719 94366 73.89% Head commit (0eb8448) 159397 (+31678) 105102 (+10736) 65.94% (-7.95%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>
Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#4348) 1 1 100.00% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Code Review
This pull request fixes a partial read corruption issue in JavaBinarySerializer.readExternal by replacing in.read(array) with in.readFully(array) to guarantee the buffer is fully populated. It also introduces a regression test using a custom ThrottledObjectInput helper. The review feedback suggests adding standard index validation and bounds checks to the custom read and readFully methods in the test helper to fully comply with standard Java stream contracts.
| public int read(final byte[] b, final int off, final int len) throws IOException { | ||
| if (len == 0) | ||
| return 0; | ||
| return delegate.read(b, off, 1); | ||
| } |
There was a problem hiding this comment.
The custom read implementation in ThrottledObjectInput does not perform standard bounds checks on off and len. To comply with the ObjectInput.read contract and prevent potential out-of-bounds access or unexpected behavior, standard index validation should be added.
@Override
public int read(final byte[] b, final int off, final int len) throws IOException {
if (off < 0 || len < 0 || len > b.length - off) {
throw new IndexOutOfBoundsException();
}
if (len == 0)
return 0;
return delegate.read(b, off, 1);
}| public void readFully(final byte[] b, final int off, final int len) throws IOException { | ||
| int total = 0; | ||
| while (total < len) { | ||
| final int n = read(b, off + total, len - total); | ||
| if (n < 0) | ||
| throw new EOFException(); | ||
| total += n; | ||
| } | ||
| } |
There was a problem hiding this comment.
The custom readFully implementation in ThrottledObjectInput does not perform standard bounds checks on off and len. To comply with the DataInput.readFully contract, standard index validation should be added.
| public void readFully(final byte[] b, final int off, final int len) throws IOException { | |
| int total = 0; | |
| while (total < len) { | |
| final int n = read(b, off + total, len - total); | |
| if (n < 0) | |
| throw new EOFException(); | |
| total += n; | |
| } | |
| } | |
| @Override | |
| public void readFully(final byte[] b, final int off, final int len) throws IOException { | |
| if (off < 0 || len < 0 || len > b.length - off) { | |
| throw new IndexOutOfBoundsException(); | |
| } | |
| int total = 0; | |
| while (total < len) { | |
| final int n = read(b, off + total, len - total); | |
| if (n < 0) | |
| throw new EOFException(); | |
| total += n; | |
| } | |
| } |
Code ReviewOverviewThis PR fixes a real, contract-level bug in The Fix
Issues1. The docs file should not be part of this PR
2. Multi-line Javadoc blocks violate project style The project guidelines say "Never write multi-paragraph docstrings or multi-line comment blocks - one short line max." Both the test method and the // short-read regression: readFully must loop until the buffer is completely filled
@Test
void readExternalSurvivesShortReadInput() throws Exception { ... }// simulates a short-read ObjectInput - returns at most 1 byte per read(byte[],int,int) call
private static final class ThrottledObjectInput implements ObjectInput { ... }Test QualityThe regression test design is solid:
SummaryThe fix is correct and the test design is good. Two changes needed before merge:
|
Code ReviewOverall assessment: The fix is correct and the test is well-designed. One change is needed before merge. The fix (line 129) - correctReplacing Test - well doneThe
The test confirms failure before the fix and passes after - that is the ideal regression test structure. Issue - please remove the committed docs file
Please drop this file from the PR before merging. Minor observation (no action required)The Verdict: Approve pending removal of |
Code ReviewOverviewThis PR fixes a real correctness bug: The Fix - CorrectThe change at Test - Well DesignedThe regression test in
Minor nit - test assertion: The new test only asserts Minor nit - comments: The multi-line Javadoc blocks are more verbose than the project style typically uses (existing tests in this file have no comments). Not a blocker. Issue - The docs/ file should be removed
Write-side audit (out of scope for this PR, but worth noting)
Summary
|
abe6db8 to
0eb8448
Compare
Code ReviewOverviewThis PR fixes a genuine correctness bug: The fix is correct and well-targeted. Fix (
|
| Area | Assessment |
|---|---|
| Fix correctness | Correct - readFully is the right API |
| Test approach | Sound regression strategy |
| Test style | Multi-line Javadoc comments violate project convention |
| Docs file | Follows existing pattern; minor em-dash usage |
| Security | No concerns |
| Performance | No concerns - readFully is equivalent on normal streams |
Actionable items before merge:
- Trim the multi-line Javadoc blocks on
readExternalSurvivesShortReadInputandThrottledObjectInputto single-line comments or remove them entirely. - Replace em dashes with regular dashes in
docs/4329-javabinaryserializer-readfully.md.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4348 +/- ##
============================================
- Coverage 64.72% 64.25% -0.47%
+ Complexity 446 423 -23
============================================
Files 1647 1647
Lines 127719 127719
Branches 27375 27375
============================================
- Hits 82660 82064 -596
- Misses 33384 34117 +733
+ Partials 11675 11538 -137 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Closes #4329
ObjectInput.read(byte[])is permitted by contract to return fewer bytes than the array length (a short read). On compressed or network-backedObjectInputimplementations this is possible.JavaBinarySerializer.readExternalcalledin.read(array)and ignored the return value, so on a short read the remaining bytes were silently lost, misaligning the stream and corrupting every subsequent property in the record.The fix is a one-character change: replace
in.read(array)within.readFully(array), which loops until the buffer is completely filled or throwsEOFException.Test plan
readExternalSurvivesShortReadInputwraps the deserialization stream in aThrottledObjectInputthat returns at most 1 byte perread(byte[], off, len)call, directly exercising the short-read path.UTFDataFormatExceptionfrom stream misalignment) and PASS after.JavaBinarySerializerTestsuite: 6/6 passed.*Serializer*): 30/30 passed, no regressions.