feat(gax): add StringHttpResponseParser - #14089
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 a new StringHttpResponseParser class and its corresponding unit tests to parse HTTP response bodies as UTF-8 strings. The review feedback suggests making the parser class and its factory method public to allow usage by external packages and client libraries. Additionally, it is recommended to use try-with-resources when parsing from a Reader to ensure the stream is properly closed and prevent potential resource leaks.
| class StringHttpResponseParser implements HttpResponseParser<String> { | ||
|
|
||
| private static final StringHttpResponseParser INSTANCE = new StringHttpResponseParser(); | ||
|
|
||
| static StringHttpResponseParser create() { | ||
| return INSTANCE; | ||
| } |
There was a problem hiding this comment.
To allow this parser to be used by generated client libraries or other packages (e.g., for resumable uploads in storage or other services), the class and its static factory method create() should be public instead of package-private.
| class StringHttpResponseParser implements HttpResponseParser<String> { | |
| private static final StringHttpResponseParser INSTANCE = new StringHttpResponseParser(); | |
| static StringHttpResponseParser create() { | |
| return INSTANCE; | |
| } | |
| public class StringHttpResponseParser implements HttpResponseParser<String> { | |
| private static final StringHttpResponseParser INSTANCE = new StringHttpResponseParser(); | |
| public static StringHttpResponseParser create() { | |
| return INSTANCE; | |
| } |
| public String parse(Reader httpContent, TypeRegistry registry) { | ||
| try { | ||
| return CharStreams.toString(httpContent); | ||
| } catch (IOException e) { | ||
| throw new RestSerializationException("Failed to read response body as string", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
The Reader passed to parse is not closed. While some callers might close it, it is safer to use a try-with-resources block to ensure the Reader is closed and prevent potential resource leaks, especially since CharStreams.toString does not close the stream.
| public String parse(Reader httpContent, TypeRegistry registry) { | |
| try { | |
| return CharStreams.toString(httpContent); | |
| } catch (IOException e) { | |
| throw new RestSerializationException("Failed to read response body as string", e); | |
| } | |
| } | |
| @Override | |
| public String parse(Reader httpContent, TypeRegistry registry) { | |
| try (Reader reader = httpContent) { | |
| return CharStreams.toString(reader); | |
| } catch (IOException e) { | |
| throw new RestSerializationException("Failed to read response body as string", e); | |
| } | |
| } |
|
|
1 participant
Currently gax-httpjson includes only one HTTP response parser:
ProtoMessageResponseParser, which assumes responses can be parsed into protos.Resumable upload HTTP operations will return non-proto responses so we'll need a more general HTTP parser to be able to use the existing HTTP stack to talk to the upload service.