Conversation
c150a47 to
755a631
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
755a631 to
aed4f4b
Compare
aed4f4b to
2f4434b
Compare
2f4434b to
445db61
Compare
445db61 to
86a18dd
Compare
429933c to
628d442
Compare
whowes
changed the title
whowes
changed the title
Add nullable getUploadStatus() accessors to ChunkUploadResponse, QueryStatusResponse, and ResumableUploadSession, and plumb the X-Goog-Upload-Status response header through the HTTP/JSON callables. On HTTP 200 chunk responses where X-Goog-Upload-Status is absent, return a ChunkUploadResponse with a null uploadStatus rather than throwing a wire-level exception, allowing higher-level upload coordinators to classify the missing header and trigger protocol recovery. Existing test uploadChunk_missingUploadStatusHeader_throwsInternalException was updated to uploadChunk_missingUploadStatusHeader_returnsNullUploadStatusOnHttp200 to reflect that the missing status header on HTTP 200 is now surfaced via a null status property on ChunkUploadResponse instead of throwing an InternalException at the transport layer. Note: No end-to-end integration test is included because the test server always returns the X-Goog-Upload-Status header on success, making header absence uninjectable end-to-end.
628d442 to
4ee20f2
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request replaces the boolean 'isComplete' flag in 'ChunkUploadResponse' and 'QueryStatusResponse' with a new 'ResumableUploadStatus' enum to represent the upload session status ('ACTIVE', 'FINAL', 'CANCELLED', 'UNKNOWN'). It updates the chunk and query status callables, improves error handling for cancelled or rejected uploads, and updates the corresponding tests. The review feedback suggests simplifying the creation of 'FAILED_PRECONDITION_STATUS_CODE' by using the existing 'HttpJsonStatusCode' class instead of an anonymous class, and combining the high-level context message with the cause's message in 'createServerRejectionException' to preserve debugging details.
| private static final StatusCode FAILED_PRECONDITION_STATUS_CODE = | ||
| new StatusCode() { | ||
| @Override | ||
| public Code getCode() { | ||
| return Code.FAILED_PRECONDITION; | ||
| } | ||
|
|
||
| @Override | ||
| public @Nullable Integer getTransportCode() { | ||
| return null; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Instead of creating a verbose anonymous class implementing StatusCode, you can use the existing HttpJsonStatusCode class which is already available in this package. This simplifies the code and improves maintainability.
private static final StatusCode FAILED_PRECONDITION_STATUS_CODE =
HttpJsonStatusCode.of(StatusCode.Code.FAILED_PRECONDITION);| static Throwable createServerRejectionException( | ||
| int statusCode, ResumableUploadStatus uploadStatus, HttpJsonMetadata trailers) { | ||
| Throwable cause = trailers.getException(); | ||
| String message = "Upload " + uploadStatus + " by server with status code: " + statusCode; | ||
| if (cause != null && cause.getMessage() != null) { | ||
| message = cause.getMessage(); | ||
| } | ||
| return ApiExceptionFactory.createException( | ||
| message, cause, FAILED_PRECONDITION_STATUS_CODE, /* retryable= */ false); | ||
| } |
There was a problem hiding this comment.
When creating the server rejection exception, if a cause with a message is present, the current implementation completely overwrites the high-level context message (which contains the uploadStatus and statusCode). It is better to append or combine the cause's message to preserve both the high-level context and the low-level error details for better debuggability.
| static Throwable createServerRejectionException( | |
| int statusCode, ResumableUploadStatus uploadStatus, HttpJsonMetadata trailers) { | |
| Throwable cause = trailers.getException(); | |
| String message = "Upload " + uploadStatus + " by server with status code: " + statusCode; | |
| if (cause != null && cause.getMessage() != null) { | |
| message = cause.getMessage(); | |
| } | |
| return ApiExceptionFactory.createException( | |
| message, cause, FAILED_PRECONDITION_STATUS_CODE, /* retryable= */ false); | |
| } | |
| static Throwable createServerRejectionException( | |
| int statusCode, ResumableUploadStatus uploadStatus, HttpJsonMetadata trailers) { | |
| Throwable cause = trailers.getException(); | |
| String message = "Upload " + uploadStatus + " by server with status code: " + statusCode; | |
| if (cause != null && cause.getMessage() != null) { | |
| message += ": " + cause.getMessage(); | |
| } | |
| return ApiExceptionFactory.createException( | |
| message, cause, FAILED_PRECONDITION_STATUS_CODE, /* retryable= */ false); | |
| } |
|
|
1 participant
This is needed to meet the requirement that the protocol implementation inspect responses for the presence of this header, responding differently in its absence depending on the command (e.g. treat as transient error for start commands, recoverable error for upload, fatal error for query)