chore: add RangeValidatingReadableByteChannel #1599
Conversation
Define a ReadableByteChannel that can validate limits relative to the delegate channel provided to it. If an over-read happens a warning message will be logged. If an under-read happens an IOException will be thrown. Add unit tests to validate behavior using copying and direct read operations.
Summary of ChangesHello @BenWhitehead, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness of byte channel operations by introducing a Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a RangeValidatingReadableByteChannel to encapsulate read validation logic, which is a great improvement for modularity. The GoogleCloudStorageClientReadChannel is refactored to use this new component, simplifying its implementation. The new class is well-tested, covering over-read and under-read scenarios. My feedback includes a couple of suggestions to improve code clarity and simplify logic.
| private BlobSourceOption[] generateReadOptions(StorageResourceId blobId) { | ||
| List<BlobSourceOption> blobReadOptions = new ArrayList<>(); | ||
| // enable transparent gzip-decompression | ||
| blobReadOptions.add(BlobSourceOption.shouldReturnRawInputStream(false)); | ||
|
|
||
| if (blobId.getGenerationId() > StorageResourceId.UNKNOWN_GENERATION_ID) { | ||
| blobReadOptions.add(BlobSourceOption.generationMatch(blobId.getGenerationId())); | ||
| } |
There was a problem hiding this comment.
The parameter blobId is of type StorageResourceId, which could be confusing since there is also a com.google.cloud.storage.BlobId class used in this file. To improve clarity and avoid potential confusion, consider renaming it to resourceId to match its type and the variable name used at the call site in the constructor.
| private BlobSourceOption[] generateReadOptions(StorageResourceId blobId) { | |
| List<BlobSourceOption> blobReadOptions = new ArrayList<>(); | |
| // enable transparent gzip-decompression | |
| blobReadOptions.add(BlobSourceOption.shouldReturnRawInputStream(false)); | |
| if (blobId.getGenerationId() > StorageResourceId.UNKNOWN_GENERATION_ID) { | |
| blobReadOptions.add(BlobSourceOption.generationMatch(blobId.getGenerationId())); | |
| } | |
| private BlobSourceOption[] generateReadOptions(StorageResourceId resourceId) { | |
| List<BlobSourceOption> blobReadOptions = new ArrayList<>(); | |
| // enable transparent gzip-decompression | |
| blobReadOptions.add(BlobSourceOption.shouldReturnRawInputStream(false)); | |
| if (resourceId.getGenerationId() > StorageResourceId.UNKNOWN_GENERATION_ID) { | |
| blobReadOptions.add(BlobSourceOption.generationMatch(resourceId.getGenerationId())); | |
| } |
| if (expectedChannelRemaining < dst.remaining()) { | ||
| expectedMaxRead = Math.toIntExact(expectedChannelRemaining); | ||
| } else { | ||
| expectedMaxRead = dst.remaining(); | ||
| } |
There was a problem hiding this comment.
The calculation for expectedMaxRead can be simplified and made more robust by ensuring it's never negative. Using Math.max(0, ...) will handle cases where position > endOffset cleanly, and Math.min can combine the logic into a single line.
expectedMaxRead = (int) Math.min(dst.remaining(), Math.max(0, expectedChannelRemaining));There was a problem hiding this comment.
dst.remaining will always return a value >= 0. This is already taken care of gemini. Also, blindly casing a long to an int can cause overflow, which is why Math.toIntExact is used instead.
| position += read; | ||
| if (read > expectedMaxRead) { | ||
| // over-read | ||
| logger.atWarning().log( |
There was a problem hiding this comment.
I think we should close theRangeValidatingReadableByteChannel in GoogleCloudStorageClientReadChannel.java when overshoot happens to avoid query failures.
| int read = delegate.read(dst); | ||
| if (read > -1) { | ||
| position += read; | ||
| if (read > expectedMaxRead) { |
There was a problem hiding this comment.
In the logs received in the bug, buffer had some remaining space but channel end was crossed. So expectedMaxRead will always be greater than or equal to read and unlikely to log what is happening in the bug. I think we should log over shoot when position > endOffset to find out what is happening in the actual bug.
2 participants
Define a ReadableByteChannel that can validate limits relative to the delegate channel provided to it.
If an over-read happens a warning message will be logged. If an under-read happens an IOException will be thrown.
Add unit tests to validate behavior using copying and direct read operations.