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 as a code owner Jul 12, 2021
@google-cla google-cla bot added the cla: yes 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
@klya klya force-pushed the BufferedResponseContent branch from 2a9187a to db4f35d Compare Jul 12, 2021
Copy link
Collaborator

@elharo elharo left a comment

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
Collaborator

@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

@klya klya Jul 12, 2021

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
Collaborator

@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.

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

Copy link
Contributor Author

@klya klya Jul 12, 2021

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 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 approved these changes Jul 12, 2021
@elharo elharo merged commit 33acb86 into googleapis:master Jul 12, 2021
14 checks passed
@klya klya deleted the BufferedResponseContent branch Jul 12, 2021
gcf-merge-on-green bot pushed a commit that referenced this issue 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants