Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes parsing of binary data (such as PDFs) that contains CRLF (Carriage Return Line Feed) sequences within multipart form data. The fix addresses issue #102 by modifying the parsing logic to properly reconstruct binary content that spans multiple lines when split by CRLF delimiters.
Key Changes:
- Modified
MultipartFormData.Element.from()to return an optional type and handle binary data with embedded CRLF sequences - Added comprehensive test coverage with a PDF file resource that likely contains CRLF in its binary content
- Updated test helper functions across URLSession and APIKit to include PDF upload testing
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Sources/MultipartFormDataParser/MultipartFormData.swift | Refactored parsing logic to properly handle binary data with CRLF by appending line separators when reconstructing data from split chunks; extracted regex matching to private helper methods |
| Tests/MultipartFormDataParserTests/_Util/TestResource.swift | Added base64-encoded PDF resource and accessor property for testing binary file parsing |
| Tests/MultipartFormDataParserTests/_TestFunctions/TestFunction_URLSession.swift | Added pdf parameter to all URLSession-based upload test functions |
| Tests/MultipartFormDataParserTests/_TestFunctions/TestFunction_APIKit.swift | Added pdf parameter to all APIKit-based upload test functions |
| Tests/MultipartFormDataParserTests/MultipartFormDataParserTests.swift | Updated test cases to include PDF in multipart uploads and verify correct parsing; expanded assertions to check PDF data in sequence test |
Comments suppressed due to low confidence (2)
Tests/MultipartFormDataParserTests/MultipartFormDataParserTests.swift:36
- The PDF data is now included in the test request (line 30), but there's no assertion to verify it was parsed correctly in this test case. While
testSequence()does verify the PDF data, it would be more thorough to add an explicit assertion here as well:
XCTAssertEqual(data.element(forName: "pdf")?.data, pdf)This would ensure that PDF files (which may contain CRLF sequences in their binary data) are correctly parsed and retrieved.
let data = try MultipartFormData.parse(from: request)
XCTAssertEqual(data.element(forName: "genbaNeko")?.data, genbaNeko)
XCTAssertEqual(data.element(forName: "denwaNeko")?.data, denwaNeko)
XCTAssertEqual(data.element(forName: "message")?.string, "Hello world!")
Tests/MultipartFormDataParserTests/MultipartFormDataParserTests.swift:131
- The Alamofire test functions (
uploadWithAlamoFireanduploadWithAlamoFireConcurrency) were not updated to include thepdfparameter, unlike the APIKit and URLSession test functions which were all updated to test PDF uploads. Since this PR fixes parsing of binary data containing CRLF sequences (like PDFs), the Alamofire tests should also be updated to include PDF in the test data to ensure comprehensive test coverage across all upload methods.
func testAlamofire() throws {
let genbaNeko = try XCTUnwrap(genbaNeko)
let denwaNeko = try XCTUnwrap(denwaNeko)
let message = Data("Hello world!".utf8)
let result = try XCTUnwrap(
uploadWithAlamoFire(
genbaNeko: genbaNeko,
denwaNeko: denwaNeko,
message: message
)
)
XCTAssertEqual(result.status, 200)
XCTAssertNil(result.error)
}
func testAlamofireWithConcurrency() async throws {
let genbaNeko = try XCTUnwrap(genbaNeko)
let denwaNeko = try XCTUnwrap(denwaNeko)
let message = Data("Hello world!".utf8)
let result = try await uploadWithAlamoFireConcurrency(genbaNeko: genbaNeko, denwaNeko: denwaNeko, message: message)
XCTAssertEqual(result.status, 200)
XCTAssertNil(result.error)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or 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
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.
Resolves: #102