Multipart Form Data Encoding Fix #3438
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
cnoon
force-pushed
the
cnoon/multipart-encoding-bugfix
branch
from
0c88d31
to
42e41b9
Compare
cnoon
force-pushed
the
cnoon/multipart-encoding-bugfix
branch
from
42e41b9
to
4dec591
Compare
jshier
requested changes
Apr 17, 2021
| @@ -1058,6 +1063,7 @@ open class Session { | |||
| guard !request.isCancelled else { return } | |||
|
|
|||
| guard let adapter = adapter(for: request) else { | |||
| guard shouldCreateTask() else { return } | |||
There was a problem hiding this comment.
I don't think we should do this work twice. The check below should be enough, so do we want to remove this one?
There was a problem hiding this comment.
It doesn't. This is inside the guard for when we don't have an adapter.
Tests/UploadTests.swift
Outdated
| XCTAssertNil(response?.error) | ||
|
|
||
| switch uploadRequest.uploadable { | ||
| case .data(let data): |
There was a problem hiding this comment.
Our usual style is case let, so make sure to run the formatter and it'll fix it.
cnoon
force-pushed
the
cnoon/multipart-encoding-bugfix
branch
from
ad7978d
to
5f96852
Compare
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.
This PR fixes up a regression introduced by PR #3421 where clients relying on the
URLRequestcreation process to append body parts toMultipartFormDataobjects were no longer doing so. The body parts would no longer be appended since theUploadablecreation was happening before theURLRequestcreation took place.As @jshier and I looked into the issue further, it became apparent that we needed to redesign the logic to instead operate as follows:
URLRequest(this is where I'm appending the body parts)URLRequestwith any appended interceptorsUploadablewhich will encode theMultipartFormDataURLSessionUploadTaskwhich will actually upload the dataThis approach will ensure we not only allow clients to append body parts as part of the
URLRequestConvertibleconformance prior to encoding, but it also allows us to avoid encoding theMultipartFormDataif theURLRequestcreation or adaptation fails. Win win!Issue Link 🔗
No original issue filed.
Goals ⚽
MultipartFormDataas part of theURLRequestcreation processMultipartFormDataupload process to only encode the data if theURLRequestcreation and adaptation process succeedsImplementation Details 🚧
Minor change internally where we're just re-ordering the operations for uploads.
Testing Details 🔍
Added a test which ensures the body parts are appended to the
MultipartFormDataas part of theURLRequestcreation process and also verifies the size of the data being uploaded.