feat(gax): add StringHttpResponseParser - #14090
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces StringHttpResponseParser along with its unit tests to parse HTTP response bodies directly into UTF-8 strings. The review feedback highlights a potential resource leak in StringHttpResponseParser.java where the Reader object passed to the parse method is not closed, and suggests wrapping it in a try-with-resources block to ensure exception-safe resource management.
| 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(Reader, TypeRegistry) is not closed after reading, which can lead to resource leaks. Please wrap the httpContent in a try-with-resources block to ensure it is always closed properly and the implementation is exception-safe.
| 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); | |
| } | |
| } |
References
- When managing closeable resources, ensure they are closed in an exception-safe manner to prevent resource leaks.
There was a problem hiding this comment.
Opting to not close here as the caller that passes in the Reader should be responsible for managing its lifecycle
67ced68 to
a2f2378
Compare
1215616 to
52795b5
Compare
52795b5 to
b84604b
Compare
35f7e29 to
d8d0e87
Compare
0554402 to
d2308bd
Compare
d8d0e87 to
12eb9d6
Compare
d2308bd to
9199f0c
Compare
6aeaca3 to
c5e6e3b
Compare
c5e6e3b to
a5e2a65
Compare
|
|





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 response parser to be able to use the existing HTTP stack to talk to the upload service.