fix: Multipart delimiter boundary parsing #502
Conversation
Co-authored-by: Calvin Cestari <[email protected]>
✅ Docs Preview ReadyNo new or changed pages found. |
✅ Deploy Preview for eclectic-pie-88a2ba canceled.
|
✅ Deploy Preview for apollo-ios-docc canceled.
|
| self.wait(for: [expectation], timeout: 1) | ||
| } | ||
|
|
||
| func test__multipart__givenChunkContainingBoundaryStringWithoutClosingBoundary_shouldNotSplitChunk() throws { |
There was a problem hiding this comment.
This doesn't appear to actually test the critical path.
I was a bit confused about how this managed to work, but when I put a breakpoint on L581, we're actually getting the callback from urlSession dataTask didCompleteWithError and not urlSession dataTask didReceive. Which means we're bypassing the problematic parsing code.
From what I could see earlier, this behavior is actually time sensitive, making it hard to control which parts of URLSessionClient actually get exercised. Which is why I ended up calling its data handler directly.
There was a problem hiding this comment.
I was a bit confused about how this managed to work, but when I put a breakpoint on L581, we're actually getting the callback from urlSession dataTask didCompleteWithError and not urlSession dataTask didReceive. Which means we're bypassing the problematic parsing code.
This is an implementation detail; there is no guarantee the different callbacks nor the logic within them will remain the same in the future. The test message data is what's important and there are multiple tests to cover different combinations of the test data.
test__parsing__givenSingleChunk_withDashBoundaryInMessageBody_shouldReturnSuccesstest__multipart__givenChunkContainingBoundaryString_shouldNotSplitChunktest__multipart__givenChunkContainingBoundaryStringWithoutClosingBoundary_shouldNotSplitChunk
If you put a breakpoint on both callbacks you'll see that they're both still called. The important detail in this test is that the urlSession(session:dataTask:didReceive:) does not split the chunk incorrectly and also that it doesn't send .failure simply because there is no closing boundary.
There was a problem hiding this comment.
From what I could see earlier, this behavior is actually time sensitive, making it hard to control which parts of URLSessionClient actually get exercised. Which is why I ended up calling its data handler directly.
It's not time sensitive but dependent on the type of response (multipart header) and number of chunks. As data is sent in it's checked for complete chunks so they can be processed immediately; this is the callback that needs to check chunk boundaries. Then once the request completes any left over response data is sent on for further parsing.
There was a problem hiding this comment.
D'oh, you're right. I was skimming and skipped some of the crucial parts. Looks good!
Yes, that's what the tests I mentioned above do. Note that there are no tests for body data that contains the dash-boundary preceded, nor followed, by |
bc5a0cdf fix: Multipart delimiter boundary parsing (#502) git-subtree-dir: apollo-ios git-subtree-split: bc5a0cdfd11388824aace092701cbdd19ac1c575
git-subtree-dir: apollo-ios git-subtree-mainline: 012913f git-subtree-split: bc5a0cdfd11388824aace092701cbdd19ac1c575
Fixes apollographql/apollo-ios#3453.
The multipart message parsing code was not splitting message chunks at the correct boundary. It would only look for the dash boundary (
--graphql) instead of the full delimiter (\r\n--graphql). This meant that if the dash boundary was present in the message body the chunk would be split causing a parsing error.URLSessionClientTestsrestyled and new tests added to ensure correct delimiter parsing.client-router-e2e-testsrepo test services and everything works as expected.