Improvement/attachments and requests #147
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,5 +38,4 @@ - (IBAction) crashAction: (id) sender { | |
| array[1]; | ||
| } | ||
|
|
||
| @end | ||
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,41 +48,59 @@ extension MultipartRequest { | |
| var multipartRequest = urlRequest | ||
| let boundary = generateBoundaryString() | ||
| multipartRequest.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") | ||
|
|
||
| // temporary file to stream data | ||
| let tempURL = URL(fileURLWithPath: NSTemporaryDirectory()).appendingPathComponent(UUID().uuidString) | ||
|
There was a problem hiding this comment. what happen to the file if the app is restarted while file is being created? There was a problem hiding this comment. file is created in Temp directory managed by OS, if the app is restarted there is no guarantee the file is persisted but :
There was a problem hiding this comment. unlike NSMutableData it's even possible to recover multipart request file! (not sure if necessary but can be a good excuse to consider for |
||
| FileManager.default.createFile(atPath: tempURL.path, contents: nil, attributes: nil) | ||
| let fileHandle = try FileHandle(forWritingTo: tempURL) | ||
|
|
||
| defer { fileHandle.closeFile() } | ||
|
|
||
| // attributes | ||
| for attribute in report.attributes { | ||
| writeToFile(fileHandle, "--\(boundary)\r\n") | ||
| writeToFile(fileHandle, "Content-Disposition: form-data; name=\"\(attribute.key)\"\r\n\r\n") | ||
| writeToFile(fileHandle, "\(attribute.value)\r\n") | ||
|
There was a problem hiding this comment. shouldn't we do one write operation instead of multiple? I know opening writing and closing cost resources and time There was a problem hiding this comment. My intention was multiple small write operations with batched writes to reduce system overhead, i'll look into it. There was a problem hiding this comment. hmm i think it would be faster if we create a string with attributes in memory and then do one write operation. There is no chance we will oom the app via string with attributes so we should be good to go |
||
| } | ||
|
|
||
| // report | ||
| writeToFile(fileHandle, "--\(boundary)\r\n") | ||
| writeToFile(fileHandle, "Content-Disposition: form-data; name=\"upload_file\"; filename=\"upload_file\"\r\n") | ||
| writeToFile(fileHandle, "Content-Type: application/octet-stream\r\n\r\n") | ||
| fileHandle.write(report.reportData) | ||
| writeToFile(fileHandle, "\r\n") | ||
|
|
||
| // attachments | ||
| for attachment in Set(report.attachmentPaths).compactMap(Attachment.init(filePath:)) { | ||
|
There was a problem hiding this comment. can we stream data instead of reading it to memory? There was a problem hiding this comment. we can use a Stream. Benefits:
Caviats:
There was a problem hiding this comment. comment is on original not the updated code |
||
| writeToFile(fileHandle, "--\(boundary)\r\n") | ||
| writeToFile(fileHandle, "Content-Disposition: form-data; name=\"\(attachment.filename)\"; filename=\"\(attachment.filename)\"\r\n") | ||
| writeToFile(fileHandle, "Content-Type: \(attachment.mimeType)\r\n\r\n") | ||
| fileHandle.write(attachment.data) | ||
| writeToFile(fileHandle, "\r\n") | ||
| } | ||
|
|
||
| // Final boundary | ||
| writeToFile(fileHandle, "--\(boundary)--\r\n") | ||
|
|
||
| // Set Content-Length | ||
| let fileSize = try FileManager.default.attributesOfItem(atPath: tempURL.path)[.size] as? Int ?? 0 | ||
| multipartRequest.setValue("\(fileSize)", forHTTPHeaderField: "Content-Length") | ||
|
|
||
| // Attach file stream to HTTP body | ||
| multipartRequest.httpBodyStream = InputStream(url: tempURL) | ||
|
|
||
| return multipartRequest | ||
| } | ||
|
|
||
| private static func generateBoundaryString() -> String { | ||
| return "Boundary-\(NSUUID().uuidString)" | ||
| } | ||
|
|
||
| private static func writeToFile(_ fileHandle: FileHandle, _ string: String) { | ||
| if let data = string.data(using: .utf8) { | ||
| fileHandle.write(data) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private extension NSMutableData { | ||
|
|
||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.