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
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.
9199f0c to
d65e70e
Compare
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.
There was a problem hiding this comment.
I reverted the change to the public interface after offline discussion. I still prefer to have the special-case behavior for binary content outside of the HttpRequestRunnable and in the associated formatter implementation instead, so I defined a separate package-private interface for formatters that handle binary to implement. LMK what you think about this approach.
| GenericUrl url = new Genericurl(/sitelet?url=https%3A%2F%2Fgithub.com%2Fgoogleapis%2Fgoogle-cloud-java%2Fpull%2FnormalizedEndpoint%2520%2B%2520requestFormatter.getPath%28request)); | ||
| 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.
0722994 to
3a870a0
Compare
…tpRequestRunnable
3a870a0 to
7de9fde
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the HttpContentRequestFormatter interface to support requests with arbitrary HttpContent payloads (such as raw bytes or streams) instead of serialized JSON strings. It also updates HttpRequestRunnable to handle these non-JSON payloads and support absolute URLs directly from the request formatter. A high-severity issue was identified where the new HttpContentRequestFormatter interface is package-private, which prevents it from being implemented by external classes or generated client stubs; it should be declared public.
| * streams) rather than serialized JSON strings. | ||
| */ | ||
| @NullMarked | ||
| interface HttpContentRequestFormatter<MessageFormatT> extends HttpRequestFormatter<MessageFormatT> { |
There was a problem hiding this comment.
The HttpContentRequestFormatter interface is currently package-private. Since HttpRequestFormatter is a public interface implemented by generated client stubs and other classes outside of the com.google.api.gax.httpjson package, this interface must be declared public so that it can be implemented by external classes (e.g., for resumable uploads in other packages).
| interface HttpContentRequestFormatter<MessageFormatT> extends HttpRequestFormatter<MessageFormatT> { | |
| public interface HttpContentRequestFormatter<MessageFormatT> extends HttpRequestFormatter<MessageFormatT> { |
There was a problem hiding this comment.
This is intentionally package private, as implementing classes are intended to live only in this package.
|
|
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.