feat(gax): allow non-JSON HttpContent and absolute request URLs in HttpRequestRunnable - #14134
feat(gax): allow non-JSON HttpContent and absolute request URLs in HttpRequestRunnable#14134whowes wants to merge 1 commit into
Conversation
![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 support for non-JSON HTTP content and absolute URLs in HttpRequestRunnable. It adds a default getHttpContent method to HttpRequestFormatter to retrieve the request body as HttpContent, allowing custom content types like binary payloads. Additionally, HttpRequestRunnable is updated to handle absolute paths directly, bypassing endpoint normalization when the path starts with http:// or https://. Corresponding unit tests have been added to verify these changes. There are no review comments, and I have no additional feedback to provide.
whowes
changed the title
whowes
changed the title
9199f0c to
d65e70e
Compare
…tpRequestRunnable
d65e70e to
41307c2
Compare
|
|
| * #getRequestBody(Object)} to JSON, or {@link EmptyContent} if the body is empty. | ||
| */ | ||
| @BetaApi | ||
| default HttpContent getHttpContent(MessageFormatT apiMessage) { |
There was a problem hiding this comment.
Do we expect this method to be used by classes other than HttpRequestRunnable? If not, I would prefer it to be a private helper method in HttpRequestRunnable instead of a public method in this interface.
There was a problem hiding this comment.
The only caller is expected to be HttpRequestRunnable, but the point of this being a method on the formatter interface is for formatters that process binary message bodies (e.g. for chunk uploads) to be able to override this to be not-JSON. See e.g. in the unit test for this functionality.
If this JSON implementation were a private helper in HttpRequestRunnable then IIUC the special case logic for the binary case would have to live there as well (e.g. switching behavior based on instanceof the request). IMO it's cleaner for any special casing required for a particular formatter to live in that formatter. I can play around with alternative ways to express that differently if you feel strongly though.
| GenericUrl url = new GenericUrl(normalizedEndpoint + requestFormatter.getPath(request)); | ||
| String path = requestFormatter.getPath(request); | ||
| GenericUrl url; | ||
| if (path.startsWith("http://") || path.startsWith("https://")) { |
There was a problem hiding this comment.
Is this for the upload URL that is returned from the start request?
There was a problem hiding this comment.
Yes, that's the use case for this.
2 participants
GAX HTTP infrastructure currently assumes that
This PR relaxes those assumptions to allow non-JSON content and arbitrary URLs, which will be needed for resumable upload support.