-
Notifications
You must be signed in to change notification settings - Fork 128
Feature/response header in did finish upload delegate #167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| // | ||
| // HTTPURLResponse+Headers.swift | ||
| // | ||
| // | ||
| // Created by Sai Raghu Varma Kallepalli on 11/07/23. | ||
| // | ||
|
|
||
| import Foundation | ||
|
|
||
| extension HTTPURLResponse { | ||
| func extractHeaders() -> [String: String] { | ||
| var headers: [String: String] = [:] | ||
| allHeaderFields.forEach { key, value in | ||
| headers[String(describing: key)] = String(describing: value) | ||
| } | ||
| return headers | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -128,12 +128,12 @@ final class UploadDataTask: NSObject, IdentifiableTask { | |
| } | ||
| } | ||
|
|
||
| func taskCompleted(result: Result<Int, TUSAPIError>, completed: @escaping TaskCompletion) { | ||
| func taskCompleted(result: UploadTaskResult, completed: @escaping TaskCompletion) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the test app, the response headers never come through even when there are response headers passed to this method as part of the upload task result. The headers are read from the upload metadata object but they are never assigned. The One thing to keep in mind here, is that I'd love for @LefterisHaritou to weigh in on this design decision because it's probably heavily use case dependent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For us the usecase is that our api will provide us with the header once task is completed and not during chunk upload. So as you've correctly said, we are interested only on the last set of received headers There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That said, it doesn't seem to return the response headers when the upload finishes |
||
| do { | ||
| let receivedOffset = try result.get() | ||
| let receivedOffset = try result.get().0 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to change to: |
||
| let currentOffset = metaData.uploadedRange?.upperBound ?? 0 | ||
| metaData.uploadedRange = 0..<receivedOffset | ||
|
|
||
| let hasFinishedUploading = receivedOffset == metaData.size | ||
| if hasFinishedUploading { | ||
| try files.encodeAndStore(metaData: metaData) | ||
|
|
@@ -144,23 +144,23 @@ final class UploadDataTask: NSObject, IdentifiableTask { | |
| // assertionFailure("Server returned a new uploaded offset \(offset), but it's lower than what's already uploaded \(metaData.uploadedRange!), according to the metaData. Either the metaData is wrong, or the server is returning a wrong value offset.") | ||
| throw TUSClientError.receivedUnexpectedOffset | ||
| } | ||
|
|
||
| try files.encodeAndStore(metaData: metaData) | ||
|
|
||
| // If the task has been canceled | ||
| // we don't continue to create subsequent UploadDataTasks | ||
| if self.isCanceled { | ||
| throw TUSClientError.taskCancelled | ||
| } | ||
|
|
||
| let nextRange: Range<Int>? | ||
| if let range = range { | ||
| let chunkSize = range.count | ||
| nextRange = receivedOffset..<min((receivedOffset + chunkSize), metaData.size) | ||
| } else { | ||
| nextRange = nil | ||
| } | ||
|
|
||
| let task = try UploadDataTask(api: api, metaData: metaData, files: files, range: nextRange, headerGenerator: headerGenerator) | ||
| task.progressDelegate = progressDelegate | ||
| completed(.success([task])) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,8 +26,8 @@ final class UploadMetadata: Codable { | |
| case customHeaders | ||
| case size | ||
| case errorCount | ||
| case responseHeaders | ||
| case appliedCustomHeaders | ||
|
|
||
| } | ||
|
|
||
| var isFinished: Bool { | ||
|
|
@@ -50,6 +50,8 @@ final class UploadMetadata: Codable { | |
| } | ||
| } | ||
|
|
||
| var responseHeaders: [String: String]? | ||
|
|
||
| private var _remoteDestination: URL? | ||
| var remoteDestination: URL? { | ||
| get { | ||
|
|
@@ -112,10 +114,11 @@ final class UploadMetadata: Codable { | |
| } | ||
| } | ||
|
|
||
| init(id: UUID, filePath: URL, uploadURL: URL, size: Int, customHeaders: [String: String]? = nil, mimeType: String? = nil, context: [String: String]? = nil) { | ||
| init(id: UUID, filePath: URL, uploadURL: URL, responseHeaders: [String: String]? = nil, size: Int, customHeaders: [String: String]? = nil, mimeType: String? = nil, context: [String: String]? = nil) { | ||
| self.id = id | ||
| self._filePath = filePath | ||
| self.uploadURL = uploadURL | ||
| self.responseHeaders = responseHeaders | ||
| self.size = size | ||
| self._customHeaders = customHeaders | ||
| self.mimeType = mimeType | ||
|
|
@@ -138,6 +141,7 @@ final class UploadMetadata: Codable { | |
| _customHeaders = try values.decode([String: String]?.self, forKey: .customHeaders) | ||
| size = try values.decode(Int.self, forKey: .size) | ||
| _errorCount = try values.decode(Int.self, forKey: .errorCount) | ||
| responseHeaders = try values.decode([String: String].self, forKey: .responseHeaders) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be |
||
| _appliedCustomHeaders = try values.decode([String: String]?.self, forKey: .appliedCustomHeaders) | ||
| } | ||
|
|
||
|
|
@@ -154,6 +158,7 @@ final class UploadMetadata: Codable { | |
| try container.encode(_customHeaders, forKey: .customHeaders) | ||
| try container.encode(size, forKey: .size) | ||
| try container.encode(_errorCount, forKey: .errorCount) | ||
| try container.encode(responseHeaders, forKey: .responseHeaders) | ||
| try container.encode(_appliedCustomHeaders, forKey: .appliedCustomHeaders) | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I actually think this would still be breaking since if I conform to the delegate protocol I must implement all methods required by the protocol. This slipped my mind earlier.
Maybe we can provide a default empty implementation in a protocol extension for this method so that existing users of the SDK wouldn't have to do any work after updating?
I do wonder if we could mark a protocol method as deprecated. If we can, we could deprecate the old one and tell clients to implement the new method