Uploads should be read only once even when logging #4125
Merged
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
…eter to not log the request's body
✅ Deploy Preview for apollo-android-docs canceled.
|
| * This would be the case when reading the source of data consumes it, for instance when uploading files. | ||
| */ | ||
| val isOneShot: Boolean | ||
| get() = false |
There was a problem hiding this comment.
I don't like this too much. That one more API that's useful to a small portion of the usecases but accounts for 25% of the interface. It also introduce default methods, which is something I'm never sure how backward compatible it is.
Instead, I'd much rather fail early in DefaultUpload if the body has been transmitted already.
| * be fully loaded into memory, which may cause OutOfMemoryErrors. | ||
| */ | ||
| class LoggingInterceptor( | ||
| private val logRequestBody: Boolean, |
There was a problem hiding this comment.
Make this a level, like in OkHttp?
BoD
commented
May 20, 2022
| @@ -51,6 +52,10 @@ actual class DefaultHttpEngine constructor( | |||
|
|
|||
| override fun contentLength() = body.contentLength | |||
|
|
|||
| // This prevents OkHttp from reading the body several times (e.g. when using its logging interceptor) | |||
| // which could consume files when using Uploads | |||
| override fun isOneShot() = body is UploadsHttpBody | |||
apollo-api/src/commonMain/kotlin/com/apollographql/apollo3/api/DefaultUpload.kt
Outdated
Show resolved
Hide resolved
martinbonnin
approved these changes
May 23, 2022
…/DefaultUpload.kt Co-authored-by: Martin Bonnin <[email protected]>
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.
In this PR, 2 commits:
isOneShotfield toHttpBody, and use it with with the OkHttp engine. This makes using OkHttp's log interceptor not break UploadsPretty sure 2. is a breaking change, but I still wanted to see where it takes us. (I can think of a hack to still pass the option to OkHttp without adding the field to the interface)
See also #4054