Swift3: non dictionary body type #6531
There are no files selected for viewing
This file contains 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 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 |
|---|---|---|
|
|
@@ -47,7 +47,7 @@ private struct SynchronizedDictionary<K: Hashable, V> { | |
| private var managerStore = SynchronizedDictionary<String, Alamofire.SessionManager>() | ||
|
|
||
| open class AlamofireRequestBuilder<T>: RequestBuilder<T> { | ||
| required public init(method: String, URLString: String, parameters: Any?, isBody: Bool, headers: [String : String] = [:]) { | ||
| super.init(method: method, URLString: URLString, parameters: parameters, isBody: isBody, headers: headers) | ||
| } | ||
|
|
||
|
|
@@ -76,8 +76,15 @@ open class AlamofireRequestBuilder<T>: RequestBuilder<T> { | |
| May be overridden by a subclass if you want to control the request | ||
| configuration (e.g. to override the cache policy). | ||
| */ | ||
| open func makeRequest(manager: SessionManager, method: HTTPMethod, encoding: ParameterEncoding, headers: [String:String]) throws -> DataRequest { | ||
| let originalRequest = try URLRequest(url: URLString, method: method, headers: headers) | ||
| if let encoding = encoding as? JSONEncoding { | ||
|
There was a problem hiding this comment. I need to do this here since ParameterEncoding protocol only nows how to take |
||
| let encodedRequest = try encoding.encode(originalRequest, withJSONObject: parameters) | ||
| return manager.request(encodedRequest) | ||
| } else { | ||
| let encodedRequest = try encoding.encode(originalRequest, with: parameters as? Parameters) | ||
| return manager.request(encodedRequest) | ||
| } | ||
| } | ||
|
|
||
| override open func execute(_ completion: @escaping (_ response: Response<T>?, _ error: ErrorResponse?) -> Void) { | ||
|
|
@@ -89,12 +96,14 @@ open class AlamofireRequestBuilder<T>: RequestBuilder<T> { | |
| let encoding:ParameterEncoding = isBody ? JSONEncoding() : URLEncoding() | ||
|
|
||
| let xMethod = Alamofire.HTTPMethod(rawValue: method) | ||
|
|
||
| let param = parameters as? Parameters | ||
| let fileKeys = param == nil ? [] : param!.filter { $1 is NSURL } | ||
| .map { $0.0 } | ||
|
|
||
| if fileKeys.count > 0 { | ||
| manager.upload(multipartFormData: { mpForm in | ||
| for (k, v) in param! { | ||
| switch v { | ||
| case let fileURL as URL: | ||
| if let mimeType = self.contentTypeForFormPart(fileURL: fileURL) { | ||
|
|
@@ -127,11 +136,15 @@ open class AlamofireRequestBuilder<T>: RequestBuilder<T> { | |
| } | ||
| }) | ||
| } else { | ||
| do { | ||
| let request = try makeRequest(manager: manager, method: xMethod!, encoding: encoding, headers: headers) | ||
| if let onProgressReady = self.onProgressReady { | ||
| onProgressReady(request.progress) | ||
| } | ||
| processRequest(request: request, managerId, completion) | ||
| } catch { | ||
| completion(nil, ErrorResponse.HttpError(statusCode: 500, data: nil, error: error)) | ||
|
There was a problem hiding this comment. Is 500 the right status code here? There was a problem hiding this comment. I'm not sure about returning a response code if this didn't come from the server. It could be confusing / misleading. There was a problem hiding this comment. I agree and I'm not really sure HttpError actually make since here. Changing how the error system would be a bigger change? Also I choose 500 since that was the closest to what processRequest would have done. I think this is the response you would have gotten in the old code. There was a problem hiding this comment. @jgavris hmm we do send the 500 at the end of processRequest in AlamofireImplementations as well - maybe we should avoid doing that - is there a better type of ErrorResponse to return? There was a problem hiding this comment. I see that...yes we should improve it. Maybe we can add a There was a problem hiding this comment. For other API client (e.g. PHP), we set the status code to 0 in the API exception object/class if the issue happens in the client side (e.g. network issue). Maybe we can follow the same convention in the Swift client. |
||
| } | ||
| } | ||
|
|
||
| } | ||
|
|
||
This file contains 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 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 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 |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| // | ||
| // AnotherFakeAPI.swift | ||
| // | ||
| // Generated by swagger-codegen | ||
| // https://github.com/swagger-api/swagger-codegen | ||
| // | ||
|
|
||
| import Foundation | ||
| import Alamofire | ||
|
|
||
|
|
||
| open class AnotherFakeAPI: APIBase { | ||
| /** | ||
| To test special tags | ||
| - parameter body: (body) client model | ||
| - parameter completion: completion handler to receive the data and the error objects | ||
| */ | ||
| open class func testSpecialTags(body: Client, completion: @escaping ((_ data: Client?, _ error: ErrorResponse?) -> Void)) { | ||
| testSpecialTagsWithRequestBuilder(body: body).execute { (response, error) -> Void in | ||
| completion(response?.body, error) | ||
| } | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| To test special tags | ||
| - PATCH /another-fake/dummy | ||
| - To test special tags | ||
|
|
||
| - examples: [{contentType=application/json, example={ | ||
| "client" : "client" | ||
| }}] | ||
| - parameter body: (body) client model | ||
| - returns: RequestBuilder<Client> | ||
| */ | ||
| open class func testSpecialTagsWithRequestBuilder(body: Client) -> RequestBuilder<Client> { | ||
| let path = "/another-fake/dummy" | ||
| let URLString = PetstoreClientAPI.basePath + path | ||
| let parameters = body.encodeToJSON() | ||
|
|
||
| let url = NSURLComponents(string: URLString) | ||
|
|
||
| let requestBuilder: RequestBuilder<Client>.Type = PetstoreClientAPI.requestBuilderFactory.getBuilder() | ||
|
|
||
| return requestBuilder.init(method: "PATCH", URLString: (url?.string ?? URLString), parameters: parameters, isBody: true) | ||
| } | ||
|
|
||
| } |
This file contains 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 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 |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| // | ||
| // FakeClassnameTags123API.swift | ||
| // | ||
| // Generated by swagger-codegen | ||
| // https://github.com/swagger-api/swagger-codegen | ||
| // | ||
|
|
||
| import Foundation | ||
| import Alamofire | ||
|
|
||
|
|
||
| open class FakeClassnameTags123API: APIBase { | ||
| /** | ||
| To test class name in snake case | ||
| - parameter body: (body) client model | ||
| - parameter completion: completion handler to receive the data and the error objects | ||
| */ | ||
| open class func testClassname(body: Client, completion: @escaping ((_ data: Client?, _ error: ErrorResponse?) -> Void)) { | ||
| testClassnameWithRequestBuilder(body: body).execute { (response, error) -> Void in | ||
| completion(response?.body, error) | ||
| } | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| To test class name in snake case | ||
| - PATCH /fake_classname_test | ||
| - API Key: | ||
| - type: apiKey api_key_query (QUERY) | ||
| - name: api_key_query | ||
| - examples: [{contentType=application/json, example={ | ||
| "client" : "client" | ||
| }}] | ||
| - parameter body: (body) client model | ||
| - returns: RequestBuilder<Client> | ||
| */ | ||
| open class func testClassnameWithRequestBuilder(body: Client) -> RequestBuilder<Client> { | ||
| let path = "/fake_classname_test" | ||
| let URLString = PetstoreClientAPI.basePath + path | ||
| let parameters = body.encodeToJSON() | ||
|
|
||
| let url = NSURLComponents(string: URLString) | ||
|
|
||
| let requestBuilder: RequestBuilder<Client>.Type = PetstoreClientAPI.requestBuilderFactory.getBuilder() | ||
|
|
||
| return requestBuilder.init(method: "PATCH", URLString: (url?.string ?? URLString), parameters: parameters, isBody: true) | ||
| } | ||
|
|
||
| } |
This file contains 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 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 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 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
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.
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.
Added
throwshere... which might break peoples implementation if they are extendingAlamofireRequestBuilder. They would need to add some type oftryin there code if they are callingsuper.makeRequest. I can back this out, but originally the manager.request was handling these errors for us.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.
I think it would be cleaner to keep the original implementation w/o throws and just return an error if the try's below fail just like in manger.request:
return request(originalRequest, failedWith: error)
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.
Thats what I wanted to do originally but that method is private. I could try figuring out whats going on in that function and try to mimic it?
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.
could this logic be pushed into SessionManager.swift? so AlamofireImplementations just calls a helper method in that class which then has access to that function.
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.
SessionManager is part of Alamo if I remember correctly. I don't think that will work.
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.
ah you're correct. maybe just return some type of error instead (not necessary copying Alamo's logic exactly but something similar)
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.
So change the method signature? Right now it expects DataRequest to be returned. I was trying to avoid changing the method signature but if thats ok I can do that.
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.
@jgavris any thoughts as to how to avoid changing the method signature here so we all don't have to try/catch around these blocks?
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.
my only thought would be to make a new request in the catch block w/ some bogus url to force an error... but seems messy.
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.
I though about this for a while and I think I finally have a clean solution. I'll update the pull request in a little bit.