feat(gax): allow non-JSON HttpContent and absolute target URLs in HttpRequest Formatter and Runnable - #14085
feat(gax): allow non-JSON HttpContent and absolute target URLs in HttpRequest Formatter and Runnable#14085whowes wants to merge 1 commit into
Conversation
![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.
1215616 to
52795b5
Compare
whowes
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
b84604b to
0554402
Compare
whowes
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors HTTP request body handling by introducing a default getHttpContent method in HttpRequestFormatter and updating HttpRequestRunnable to use it, which simplifies request creation and adds support for non-JSON payloads and absolute URLs. Corresponding unit tests were also added. The review feedback suggests improving absolute URL detection robustness by using case-insensitive regionMatches and optimizing performance by reusing a shared, lazily initialized JsonObjectParser instead of instantiating one for every request.
| 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.
To make the absolute URL detection robust against uppercase schemes (e.g., HTTP:// or HTTPS://) without allocating new string objects, consider using regionMatches with case-insensitivity enabled.
| if (path.startsWith("http://") || path.startsWith("https://")) { | |
| if (path.regionMatches(true, 0, "http://", 0, 7) || path.regionMatches(true, 0, "https://", 0, 8)) { |
| } | ||
|
|
||
| httpRequest.setParser(new JsonObjectParser(jsonFactory)); | ||
| httpRequest.setParser(new JsonObjectParser(GsonFactory.getDefaultInstance())); |
There was a problem hiding this comment.
To avoid allocating a new JsonObjectParser instance on every HTTP request, consider using a lazily initialized instance. Since JsonObjectParser is thread-safe, sharing a single instance is safe and improves performance. Lazy initialization is preferred over eager initialization for resource-intensive objects to avoid unnecessary performance and memory overhead if they are not guaranteed to be used in all execution paths.
| httpRequest.setParser(new JsonObjectParser(GsonFactory.getDefaultInstance())); | |
| httpRequest.setParser(getJsonObjectParser()); |
References
- Prefer lazy initialization over eager initialization for resource-intensive objects (such as CharsetEncoder) if they are not guaranteed to be used in all execution paths, to avoid unnecessary performance and memory overhead.
0554402 to
d2308bd
Compare
…tpRequestRunnable
d2308bd to
9199f0c
Compare
|
|
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.