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.
…tpRequestRunnable
0554402 to
d2308bd
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.