Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Use BufferedInputStream to inspect HttpResponse error #1411

Merged
merged 2 commits into from Jul 12, 2021

Conversation

klya
Copy link
Contributor

@klya klya commented Jul 12, 2021

Fixes #1410

@klya klya requested a review from a team as a code owner July 12, 2021 10:29
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 12, 2021
@klya klya changed the title Use BufferedInputStream to inspect HttpResponse error Fix: Use BufferedInputStream to inspect HttpResponse error Jul 12, 2021
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is the best solution for the underlying issue of reading error responses. The whole mark/reset thing is a bit of a kludge. The error response should be more directly accessible in the API.

However this is a small, safe fix that provides a workaround until we can improve the API for this important case.

@@ -371,6 +372,11 @@ public InputStream getContent() throws IOException {
new LoggingInputStream(
lowLevelResponseContent, logger, Level.CONFIG, contentLoggingLimit);
}
if (!returnRawInputStream) {
Copy link
Contributor

@elharo elharo Jul 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

marginally cleaner to do an if-else

if (returnRawInputStream) {
  content = lowLevelResponseContent;
} else {
  content = new BufferedInputStream(lowLevelResponseContent);
}

This avoids reassigning a local variable which can be confusing and error prone in the long term.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose this way just because all previous code in this method used this way. lowLevelResponseContent reassigned two times in preceding code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth cleaning this up in a separate PR some time, but for now please don't introduce more reassignments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@klya
Copy link
Contributor Author

klya commented Jul 12, 2021

I'm not sure this is the best solution for the underlying issue of reading error responses. The whole mark/reset thing is a bit of a kludge. The error response should be more directly accessible in the API.

Yep. This way pretty awkward. I see alternatives like this:

  • Fetching content from HttpResponse and passing it to the HttpUnsuccessfulResponseHandler. Nearly impossible due to braking changes in the HttpUnsuccessfulResponseHandler interface.
  • Allowing access to the HttpResponse.content private field. But this is a violation of encapsulation.
  • Make another kind of a handler that can wrap HttpResponse.getContent() or the entire HttpResponse. It looks rather complicated to implement and may cause some side-effects.

Can't find a better solution.

@elharo elharo merged commit 33acb86 into googleapis:master Jul 12, 2021
@klya klya deleted the BufferedResponseContent branch July 12, 2021 17:22
gcf-merge-on-green bot pushed a commit that referenced this pull request Aug 26, 2021
🤖 I have created a release \*beep\* \*boop\*
---
### [1.40.1](https://www.github.com/googleapis/google-http-java-client/compare/v1.40.0...v1.40.1) (2021-08-26)


### Features

* add `gcf-owl-bot[bot]` to `ignoreAuthors` ([#1380](https://www.github.com/googleapis/google-http-java-client/issues/1380)) ([e69275e](https://www.github.com/googleapis/google-http-java-client/commit/e69275ecaa4d85372ebc253dd415a02ba63075be))


### Bug Fixes

* GSON parser now throws IOException on invalid JSON input ([#1355](https://www.github.com/googleapis/google-http-java-client/issues/1355)) ([0a505a7](https://www.github.com/googleapis/google-http-java-client/commit/0a505a7ce012efcce14af94aa130d0eab2ac89b6))
* Add shopt -s nullglob to dependencies script ([#1412](https://www.github.com/googleapis/google-http-java-client/issues/1412)) ([933b0bd](https://www.github.com/googleapis/google-http-java-client/commit/933b0bd386f413bd960f81c706edae81d9dc030a))
* default charset to UTF-8 for text/csv if not specified ([#1423](https://www.github.com/googleapis/google-http-java-client/issues/1423)) ([26f3da4](https://www.github.com/googleapis/google-http-java-client/commit/26f3da4b6426625d0d88afdad525dbf99c65bc8b))
* make depencence on javax.annotation optional ([#1323](https://www.github.com/googleapis/google-http-java-client/issues/1323)) ([#1405](https://www.github.com/googleapis/google-http-java-client/issues/1405)) ([4ccad0e](https://www.github.com/googleapis/google-http-java-client/commit/4ccad0e9f37adaf5adac469e8dec478eb424a410))
* release scripts from issuing overlapping phases ([#1344](https://www.github.com/googleapis/google-http-java-client/issues/1344)) ([539407e](https://www.github.com/googleapis/google-http-java-client/commit/539407ef7133df7f5b1e0f371c673dbc75e79ff2))
* test error responses such as 403 ([#1345](https://www.github.com/googleapis/google-http-java-client/issues/1345)) ([a83c43f](https://www.github.com/googleapis/google-http-java-client/commit/a83c43fa86966ca1be625086a211211e3861f7b1))
* typo ([#1342](https://www.github.com/googleapis/google-http-java-client/issues/1342)) ([2bbc0e4](https://www.github.com/googleapis/google-http-java-client/commit/2bbc0e4b77ab2c9956b0a65af0e927d5052a7752))
* Update dependencies.sh to not break on mac ([933b0bd](https://www.github.com/googleapis/google-http-java-client/commit/933b0bd386f413bd960f81c706edae81d9dc030a))
* Use BufferedInputStream to inspect HttpResponse error ([#1411](https://www.github.com/googleapis/google-http-java-client/issues/1411)) ([33acb86](https://www.github.com/googleapis/google-http-java-client/commit/33acb8621d6e8dc088cf3bd3324a3db25dafb185))


### Documentation

* bom 20.3.0 ([#1368](https://www.github.com/googleapis/google-http-java-client/issues/1368)) ([0d8d2fe](https://www.github.com/googleapis/google-http-java-client/commit/0d8d2fee8750bcaa79f2c8ee106f17b89de81e58))
* libraries-bom 20.1.0 ([#1347](https://www.github.com/googleapis/google-http-java-client/issues/1347)) ([2570889](https://www.github.com/googleapis/google-http-java-client/commit/2570889e95c7c3bf26d5666dc69a7bb09efd7655))
* libraries-bom 20.5.0 ([#1388](https://www.github.com/googleapis/google-http-java-client/issues/1388)) ([38dc3f6](https://www.github.com/googleapis/google-http-java-client/commit/38dc3f64d24868f90bfc9728ace0ce6aaeb2940a))
* libraries-bom 20.9.0 ([#1416](https://www.github.com/googleapis/google-http-java-client/issues/1416)) ([c6aba10](https://www.github.com/googleapis/google-http-java-client/commit/c6aba10ea9a5c5acc9d07317c5b983309b45e2eb))


### Dependencies

* update dependency com.fasterxml.jackson.core:jackson-core to v2.12.3 ([#1340](https://www.github.com/googleapis/google-http-java-client/issues/1340)) ([81e479a](https://www.github.com/googleapis/google-http-java-client/commit/81e479ac59797ad49e503eb2d41ff17c9cb77d7b))
* update dependency com.fasterxml.jackson.core:jackson-core to v2.12.4 ([#1406](https://www.github.com/googleapis/google-http-java-client/issues/1406)) ([fa07715](https://www.github.com/googleapis/google-http-java-client/commit/fa07715f528f74e0ef1c5737c6730c505746a7ad))
* update dependency com.google.code.gson:gson to v2.8.7 ([#1386](https://www.github.com/googleapis/google-http-java-client/issues/1386)) ([550abc1](https://www.github.com/googleapis/google-http-java-client/commit/550abc1e9f3209ec87b20f81c9e0ecdb27aedb7c))
* update dependency com.google.code.gson:gson to v2.8.8 ([#1430](https://www.github.com/googleapis/google-http-java-client/issues/1430)) ([ae4b0db](https://www.github.com/googleapis/google-http-java-client/commit/ae4b0dbbcf2535e660c70dd9ac0ea20d7f040181))
* update dependency com.google.errorprone:error_prone_annotations to v2.7.1 ([#1378](https://www.github.com/googleapis/google-http-java-client/issues/1378)) ([83b1642](https://www.github.com/googleapis/google-http-java-client/commit/83b164245d4e3298c7cee5b10ab7917f6c85e7b1))
* update dependency com.google.errorprone:error_prone_annotations to v2.8.0 ([#1414](https://www.github.com/googleapis/google-http-java-client/issues/1414)) ([1508657](https://www.github.com/googleapis/google-http-java-client/commit/1508657d27b41babb530a914bd2708c567ac08ef))
* update dependency com.google.errorprone:error_prone_annotations to v2.8.1 ([#1420](https://www.github.com/googleapis/google-http-java-client/issues/1420)) ([1f8be1c](https://www.github.com/googleapis/google-http-java-client/commit/1f8be1c222d7f3fd165abe57387d2f8d9e63d82f))
* update dependency com.google.errorprone:error_prone_annotations to v2.9.0 ([#1429](https://www.github.com/googleapis/google-http-java-client/issues/1429)) ([834ade3](https://www.github.com/googleapis/google-http-java-client/commit/834ade362070c9c93f9eb8a08df3308df46d51f2))
* update dependency com.google.protobuf:protobuf-java to v3.16.0 ([#1366](https://www.github.com/googleapis/google-http-java-client/issues/1366)) ([3148f5d](https://www.github.com/googleapis/google-http-java-client/commit/3148f5daab8598957e05849eaec2eab0b634321d))
* update dependency com.google.protobuf:protobuf-java to v3.17.0 ([#1373](https://www.github.com/googleapis/google-http-java-client/issues/1373)) ([d147628](https://www.github.com/googleapis/google-http-java-client/commit/d147628742bbd327a405e87b1645d1d4bf1f7610))
* update dependency com.google.protobuf:protobuf-java to v3.17.1 ([#1384](https://www.github.com/googleapis/google-http-java-client/issues/1384)) ([c22a0e0](https://www.github.com/googleapis/google-http-java-client/commit/c22a0e0e1c1a4a6e8c93b38db519b49eba4e2f14))
* update dependency com.google.protobuf:protobuf-java to v3.17.2 ([#1390](https://www.github.com/googleapis/google-http-java-client/issues/1390)) ([b34349f](https://www.github.com/googleapis/google-http-java-client/commit/b34349f5d303f15b28c69a995763f3842738177c))
* update dependency com.google.protobuf:protobuf-java to v3.17.3 ([#1394](https://www.github.com/googleapis/google-http-java-client/issues/1394)) ([4e3b3c3](https://www.github.com/googleapis/google-http-java-client/commit/4e3b3c3cebeb8439e729a9f99b58e5fc5e13e2cf))

---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make a way to read HttpResponse content in HttpUnsuccessfulResponseHandler
2 participants