feat(gax): add ResumableUploadClient and supporting classes - #14072
feat(gax): add ResumableUploadClient and supporting classes#14072whowes wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a suite of low-level classes and interfaces to support resumable uploads in the com.google.api.gax.resumableupload package, including request/response models, session metadata, progress listeners, and a client interface, along with corresponding unit tests. The review feedback focuses on improving robustness and safety by adding input validation (such as ensuring non-negative offsets and valid total lengths) in the factory methods of the request, response, and status models, as well as defensively copying query parameters in StartUploadRequest to guarantee immutability.
8d9f27f to
24d585f
Compare
24d585f to
1ee141c
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a suite of low-level classes and interfaces to support resumable uploads, including request/response models for starting uploads, transmitting chunks, querying status, and tracking progress, along with associated unit tests. The review feedback suggests enhancing data integrity by adding stricter validation checks to enforce protocol invariants in ChunkUploadRequest and ResumableUploadStatus (such as verifying that offsets and uploaded bytes do not exceed the total payload length), and updating the test suite to cover these new validation scenarios.
| public static ChunkUploadRequest create( | ||
| String uploadUrl, ByteString payload, long offset, long totalLength, boolean isFinal) { | ||
| Preconditions.checkArgument(offset >= 0, "offset must be non-negative"); | ||
| Preconditions.checkArgument(totalLength >= -1, "totalLength must be >= -1"); | ||
| return new AutoValue_ChunkUploadRequest(uploadUrl, payload, offset, totalLength, isFinal); | ||
| } |
There was a problem hiding this comment.
To prevent invalid states, we should enforce that the chunk offset and payload size do not exceed the total length when the total length is known. Additionally, if this is the final chunk, the end offset must exactly equal the total length. Please ensure these validation steps are not redundant with checks already performed by upstream callers.
public static ChunkUploadRequest create(
String uploadUrl, ByteString payload, long offset, long totalLength, boolean isFinal) {
Preconditions.checkArgument(offset >= 0, "offset must be non-negative");
Preconditions.checkArgument(totalLength >= -1, "totalLength must be >= -1");
if (totalLength != -1) {
Preconditions.checkArgument(
offset + payload.size() <= totalLength,
"offset + payload size (%s) cannot exceed totalLength (%s)",
offset + payload.size(),
totalLength);
if (isFinal) {
Preconditions.checkArgument(
offset + payload.size() == totalLength,
"final chunk end offset (%s) must equal totalLength (%s)",
offset + payload.size(),
totalLength);
}
}
return new AutoValue_ChunkUploadRequest(uploadUrl, payload, offset, totalLength, isFinal);
}References
- When implementing property parsing or validation logic, ensure that null checks and validation steps are not redundant with checks already performed by upstream callers or preceding logic in the same method.
There was a problem hiding this comment.
I think this is more appropriately handled and tested on the caller level (the state machine)
| public static ResumableUploadStatus create( | ||
| String uploadUrl, long bytesUploaded, long totalBytes, State state) { | ||
| Preconditions.checkArgument(bytesUploaded >= 0, "bytesUploaded must be non-negative"); | ||
| Preconditions.checkArgument(totalBytes >= -1, "totalBytes must be >= -1"); | ||
| return new AutoValue_ResumableUploadStatus(uploadUrl, bytesUploaded, totalBytes, state); | ||
| } |
There was a problem hiding this comment.
To ensure data integrity, we should validate that the number of bytes uploaded does not exceed the total bytes when the total bytes are known. Furthermore, when the upload state is finalized, the bytes uploaded must equal the total bytes. Please ensure these validation steps are not redundant with checks already performed by upstream callers.
public static ResumableUploadStatus create(
String uploadUrl, long bytesUploaded, long totalBytes, State state) {
Preconditions.checkArgument(bytesUploaded >= 0, "bytesUploaded must be non-negative");
Preconditions.checkArgument(totalBytes >= -1, "totalBytes must be >= -1");
if (totalBytes != -1) {
Preconditions.checkArgument(
bytesUploaded <= totalBytes,
"bytesUploaded (%s) cannot exceed totalBytes (%s)",
bytesUploaded,
totalBytes);
if (state == State.FINALIZED) {
Preconditions.checkArgument(
bytesUploaded == totalBytes,
"bytesUploaded (%s) must equal totalBytes (%s) when finalized",
bytesUploaded,
totalBytes);
}
}
return new AutoValue_ResumableUploadStatus(uploadUrl, bytesUploaded, totalBytes, state);
}References
- When implementing property parsing or validation logic, ensure that null checks and validation steps are not redundant with checks already performed by upstream callers or preceding logic in the same method.
| void chunkUploadRequest_enforcesProtocolInvariants() { | ||
| // Valid: offset >= 0, totalLength == -1 (streaming/unknown) or >= 0 (known) | ||
| ChunkUploadRequest streamingRequest = | ||
| ChunkUploadRequest.create(UPLOAD_URL, ByteString.copyFromUtf8("data"), 0, -1, false); | ||
| assertThat(streamingRequest.getTotalLength()).isEqualTo(-1); | ||
|
|
||
| // Invalid: negative offset | ||
| assertThrows( | ||
| IllegalArgumentException.class, | ||
| () -> ChunkUploadRequest.create(UPLOAD_URL, ByteString.EMPTY, -1, 100, false)); | ||
|
|
||
| // Invalid: totalLength < -1 | ||
| assertThrows( | ||
| IllegalArgumentException.class, | ||
| () -> ChunkUploadRequest.create(UPLOAD_URL, ByteString.EMPTY, 0, -2, false)); | ||
| } |
There was a problem hiding this comment.
Update the test to verify the new chunk upload request invariants (exceeding total length and final chunk offset matching total length). Before adding these new test cases, please verify the existing test suite to ensure these scenarios are not already covered.
void chunkUploadRequest_enforcesProtocolInvariants() {
// Valid: offset >= 0, totalLength == -1 (streaming/unknown) or >= 0 (known)
ChunkUploadRequest streamingRequest =
ChunkUploadRequest.create(UPLOAD_URL, ByteString.copyFromUtf8("data"), 0, -1, false);
assertThat(streamingRequest.getTotalLength()).isEqualTo(-1);
// Invalid: negative offset
assertThrows(
IllegalArgumentException.class,
() -> ChunkUploadRequest.create(UPLOAD_URL, ByteString.EMPTY, -1, 100, false));
// Invalid: totalLength < -1
assertThrows(
IllegalArgumentException.class,
() -> ChunkUploadRequest.create(UPLOAD_URL, ByteString.EMPTY, 0, -2, false));
// Invalid: offset + payload size exceeds totalLength
assertThrows(
IllegalArgumentException.class,
() -> ChunkUploadRequest.create(UPLOAD_URL, ByteString.copyFromUtf8("data"), 90, 92, false));
// Invalid: final chunk end offset does not equal totalLength
assertThrows(
IllegalArgumentException.class,
() -> ChunkUploadRequest.create(UPLOAD_URL, ByteString.copyFromUtf8("data"), 0, 10, true));
}References
- Before adding new test cases for input validation (such as non-numeric values or unrecognized properties), verify the existing test suite to ensure these scenarios are not already covered.
| @Test | ||
| void progressListener_dispatchesStatusSnapshot() { | ||
| AtomicReference<ResumableUploadStatus> capturedStatus = new AtomicReference<>(); | ||
| ResumableUploadProgressListener listener = capturedStatus::set; | ||
|
|
||
| ResumableUploadStatus status = | ||
| ResumableUploadStatus.create( | ||
| UPLOAD_URL, 1024L, 2048L, ResumableUploadStatus.State.UPLOADING); | ||
| listener.onProgress(status); | ||
|
|
||
| assertThat(capturedStatus.get()).isEqualTo(status); | ||
| } |
There was a problem hiding this comment.
Add a test case to verify the new resumable upload status invariants (bytes uploaded exceeding total bytes, and finalized state matching total bytes). Before adding these new test cases, please verify the existing test suite to ensure these scenarios are not already covered.
@Test
void progressListener_dispatchesStatusSnapshot() {
AtomicReference<ResumableUploadStatus> capturedStatus = new AtomicReference<>();
ResumableUploadProgressListener listener = capturedStatus::set;
ResumableUploadStatus status =
ResumableUploadStatus.create(
UPLOAD_URL, 1024L, 2048L, ResumableUploadStatus.State.UPLOADING);
listener.onProgress(status);
assertThat(capturedStatus.get()).isEqualTo(status);
}
@Test
void resumableUploadStatus_enforcesInvariants() {
// Invalid: bytesUploaded exceeds totalBytes
assertThrows(
IllegalArgumentException.class,
() -> ResumableUploadStatus.create(UPLOAD_URL, 101L, 100L, ResumableUploadStatus.State.UPLOADING));
// Invalid: finalized state but bytesUploaded does not equal totalBytes
assertThrows(
IllegalArgumentException.class,
() -> ResumableUploadStatus.create(UPLOAD_URL, 50L, 100L, ResumableUploadStatus.State.FINALIZED));
}References
- Before adding new test cases for input validation (such as non-numeric values or unrecognized properties), verify the existing test suite to ensure these scenarios are not already covered.
|
|
No description provided.