Improvement/attachments and requests #147
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,5 +38,4 @@ - (IBAction) crashAction: (id) sender { | |
| array[1]; | ||
| } | ||
|
|
||
| @end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,41 +48,85 @@ extension MultipartRequest { | |
| var multipartRequest = urlRequest | ||
| let boundary = generateBoundaryString() | ||
| multipartRequest.setValue("multipart/form-data; boundary=\(boundary)", forHTTPHeaderField: "Content-Type") | ||
|
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 |
||
|
|
||
| // 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 |
||
|
|
||
| do { | ||
| let fileCreated = FileManager.default.createFile(atPath: tempURL.path, contents: nil, attributes: nil) | ||
| if !fileCreated { | ||
| throw HttpError.fileCreationFailed(tempURL) | ||
|
There was a problem hiding this comment. it won't be captured by the L:111 - which means we will return a generic error rather this this one? |
||
| } | ||
|
|
||
| let fileHandle = try FileHandle(forWritingTo: tempURL) | ||
| defer { | ||
| if #available(iOS 13.0, tvOS 13.0, macOS 11.0, *) { | ||
| try? fileHandle.close() | ||
| } else { | ||
| fileHandle.closeFile() | ||
| } | ||
| } | ||
|
|
||
| // attributes | ||
| var attributesString = "" | ||
| for attribute in report.attributes { | ||
| attributesString += "--\(boundary)\r\n" | ||
| attributesString += "Content-Disposition: form-data; name=\"\(attribute.key)\"\r\n\r\n" | ||
| attributesString += "\(attribute.value)\r\n" | ||
| } | ||
| try writeToFile(fileHandle, attributesString) | ||
|
|
||
| // report | ||
| var reportString = "--\(boundary)\r\n" | ||
|
There was a problem hiding this comment. It would be better if we start with report rather than with attributes - if the application stops during submission part, we can still submit a valid report. |
||
| reportString += "Content-Disposition: form-data; name=\"upload_file\"; filename=\"upload_file\"\r\n" | ||
| reportString += "Content-Type: application/octet-stream\r\n\r\n" | ||
| try writeToFile(fileHandle, reportString) | ||
| fileHandle.write(report.reportData) | ||
| try writeToFile(fileHandle, "\r\n") | ||
|
|
||
| // attachments | ||
| for attachmentPath in Set(report.attachmentPaths) { | ||
| guard let attachment = Attachment(filePath: attachmentPath) else { | ||
| BacktraceLogger.error("Failed to create attachment for path: \(attachmentPath)") | ||
| continue | ||
| } | ||
|
|
||
| try writeToFile(fileHandle, "--\(boundary)\r\n") | ||
| try writeToFile(fileHandle, "Content-Disposition: form-data; name=\"\(attachment.filename)\"; filename=\"\(attachment.filename)\"\r\n") | ||
| try writeToFile(fileHandle, "Content-Type: \(attachment.mimeType)\r\n\r\n") | ||
|
There was a problem hiding this comment. I think we should do it with a single write |
||
| fileHandle.write(attachment.data) | ||
|
There was a problem hiding this comment. you're making an attachment copy in the temporary file. WE cannot stream a file directly from the file available somewhere? There was a problem hiding this comment. possible but requires creating an InputStream for each attachments. |
||
| try writeToFile(fileHandle, "\r\n") | ||
| } | ||
|
|
||
| // Final boundary | ||
| try 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) | ||
|
There was a problem hiding this comment. Instead of creating the file and setting it as I'm not proficient in Swift, but I found this: You can use bound streams to achieve that, it seems. There was a problem hiding this comment. we can, this is the the approach I was referencing:
|
||
|
|
||
| } catch { | ||
| BacktraceLogger.error("Error during multipart form creation: \(error.localizedDescription)") | ||
| throw HttpError.multipartFormError(error) | ||
| } | ||
|
|
||
| return multipartRequest | ||
| } | ||
|
|
||
| private static func generateBoundaryString() -> String { | ||
| return "Boundary-\(NSUUID().uuidString)" | ||
| } | ||
|
|
||
| private static func writeToFile(_ fileHandle: FileHandle, _ string: String) throws { | ||
| if let data = string.data(using: .utf8) { | ||
| fileHandle.write(data) | ||
| } else { | ||
| throw HttpError.fileWriteFailed | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private extension NSMutableData { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.