-
Notifications
You must be signed in to change notification settings - Fork 448
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: remove extraneous content encoding #650
Conversation
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
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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. |
IdtokenProvider implementation for UserCredentials with unit tests for idtoken
🤖 I have created a release \*beep\* \*boop\* --- ## [0.27.0](https://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://github.com/googleapis/google-auth-library-java/issues/650)) ([5a8f467](https://github.com/googleapis/google-auth-library-java/commit/5a8f4676630854c53aa708a9c8b960770067f858)) * add impersonation credentials to ADC ([googleapis#613](https://github.com/googleapis/google-auth-library-java/issues/613)) ([b9823f7](https://github.com/googleapis/google-auth-library-java/commit/b9823f70d7f3f7461b7de40bee06f5e7ba0e797c)) * Adding functional tests for Service Account ([googleapis#685](https://github.com/googleapis/google-auth-library-java/issues/685)) ([dfe118c](https://github.com/googleapis/google-auth-library-java/commit/dfe118c261aadf137a3cf47a7acb9892c7a6db4d)) * allow scopes for self signed jwt ([googleapis#689](https://github.com/googleapis/google-auth-library-java/issues/689)) ([f4980c7](https://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).
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
ByteArrayOutputStream
implementation 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
ByteArrayContent
whichcontains the buffer we wrote to when calculating the size.
We implemented a new
CachingByteArrayOutputStream
so that we can accessthe byte buffer directly rather than copying into a new byte array (for
memory performance)