Fix: remove extraneous content encoding #650
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
googlebot
added
the
cla: yes
chingor13
added
the
kokoro:force-run
kokoro-team
removed
the
kokoro:force-run
In order to calculate the contentLength, we must write the encode the data first. The encoded data is written to a buffer using a ByteArrayOutputStream implementation and we use that to figure out how many bytes of data is to be sent. Previously, this data was thrown away and the content was re-encoded when it was actually time to send the data. Instead, we now replace the content with a ByteArrayContent which contains the buffer we wrote to when calculating the size. We implemented a new CachingByteArrayOutputStream so that we can access the byte buffer directly rather than copying into a new byte array (for memory performance)
We really only care that the contents was encode and is correctly marked as such.
* Fix javadoc param name * Group serializeHeaders() overloads
* Remove deprecated google-http-client-jackson artifact. Jackson 1.x has been unsupported for a long time. Users should be using Jackson 2.x. the google-http-client-jackson artifact was deprecated in 1.28.0. * Fix assembly references to jackson
* Add Base64Test case for some base64 decoding edge cases * Preserve decoding behavior for null decodeBase64(null) * Handle encoding with null inputs
sduskis
reviewed
May 29, 2019
| @@ -932,7 +932,14 @@ public HttpResponse execute() throws IOException { | |||
| } else { | |||
| contentEncoding = encoding.getName(); | |||
| streamingContent = new HttpEncodingStreamingContent(streamingContent, encoding); | |||
| contentLength = contentRetrySupported ? IOUtils.computeLength(streamingContent) : -1; | |||
| if (contentRetrySupported) { | |||
| CachingByteArrayOutputStream outputStream = new CachingByteArrayOutputStream(); | |||
There was a problem hiding this comment.
IOUtils checks the size via streaming, and doesn't actually store the data in memory. The same thing for sending the data. A large object will currently never fully have to be in memory.
This change may cause an unexpected memory spike for users with large objects
|
Since this bug was filed on the GAE standard product, memory management is critical as we do not have big mems on instance classes machines... |
|
As is, this implementation won't be good enough |
chingor13
added
the
do not merge
In order to calculate the contentLength, we must write the encode the data first. The encoded data is written to a buffer using a ByteArrayOutputStream implementation and we use that to figure out how many bytes of data is to be sent. Previously, this data was thrown away and the content was re-encoded when it was actually time to send the data. Instead, we now replace the content with a ByteArrayContent which contains the buffer we wrote to when calculating the size. We implemented a new CachingByteArrayOutputStream so that we can access the byte buffer directly rather than copying into a new byte array (for memory performance)
We really only care that the contents was encode and is correctly marked as such.
…p-java-client into fix-double-encoding
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the ℹ️ Googlers: Go here for more info. |
googlebot
added
cla: no
clundin25
pushed a commit
to clundin25/google-http-java-client
that referenced
this pull request
Aug 11, 2022
IdtokenProvider implementation for UserCredentials with unit tests for idtoken
clundin25
pushed a commit
to clundin25/google-http-java-client
that referenced
this pull request
Aug 11, 2022
🤖 I have created a release \*beep\* \*boop\* --- ## [0.27.0](https://www.github.com/googleapis/google-auth-library-java/compare/v0.26.0...v0.27.0) (2021-07-14) ### Features * add Id token support for UserCredentials ([googleapis#650](https://www.github.com/googleapis/google-auth-library-java/issues/650)) ([5a8f467](https://www.github.com/googleapis/google-auth-library-java/commit/5a8f4676630854c53aa708a9c8b960770067f858)) * add impersonation credentials to ADC ([googleapis#613](https://www.github.com/googleapis/google-auth-library-java/issues/613)) ([b9823f7](https://www.github.com/googleapis/google-auth-library-java/commit/b9823f70d7f3f7461b7de40bee06f5e7ba0e797c)) * Adding functional tests for Service Account ([googleapis#685](https://www.github.com/googleapis/google-auth-library-java/issues/685)) ([dfe118c](https://www.github.com/googleapis/google-auth-library-java/commit/dfe118c261aadf137a3cf47a7acb9892c7a6db4d)) * allow scopes for self signed jwt ([googleapis#689](https://www.github.com/googleapis/google-auth-library-java/issues/689)) ([f4980c7](https://www.github.com/googleapis/google-auth-library-java/commit/f4980c77566bbd5ef4c532acb199d7d484dbcd01)) --- 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #648
In order to calculate the content length, we must write the encoded the
data first. The encoded data is written to a buffer using a
ByteArrayOutputStreamimplementation and we use that to figure out howmany bytes of data is to be sent.
Previously, this data was thrown away and the content was re-encoded
when it was actually time to send the data.
Instead, we now replace the content with a
ByteArrayContentwhichcontains the buffer we wrote to when calculating the size.
We implemented a new
CachingByteArrayOutputStreamso that we can accessthe byte buffer directly rather than copying into a new byte array (for
memory performance)