Parameter Encoding Tests #2
Conversation
* master: fixed request throwing weird wrapped errors fix error in encoding logic update podspec bump version remove todos, update project settings that were throwing a warning
| case .get: | ||
| guard let url = url, | ||
| var components = URLComponents(url: url, resolvingAgainstBaseURL: true), | ||
| let paramsDict = request.parameters as? [String: Codable] else { |
There was a problem hiding this comment.
I don't like the hidden requirement for the parameters to be a dictionary. Even though that's definitely the most common case, I think ideally you should be able to do something like this:
struct MyParams: Codable {
var a: String
var b: Int
}
struct MyRequest: Request {
typealias Parameters = MyParams
typealias Returning = /*...*/
public var method: HTTPMethod { return .get }
public var path: String {
return "somewhere"
}
public var params: Codable {
return MyParams(a: "foo", b: 2)
}
}
and have the url generate to somewhere?a=foo&b=2, which this approach prevents. We're gonna have to do some runtime checking here (because I don't think we can reasonably support passing an array, anything with more than one level of hierarchy, or anything that is a single value container encodable to the URL parameter encoding), but I think this it too strict.
Implementing that is a bit of a pain though, there's no way to Encoder something straight to a dictionary's so I believe right now we need to encode it to JSON and the decode the JSON to a dictionary, which kinda sucks.
There was a problem hiding this comment.
Got the other changes in and made some progress on this, but I'm running in to some trouble checking for params that are arrays or nested dictionaries and catching them before it tries to encode them.
I've committed what I've got, but it basically comes down to Array != Array. It doesn't seem right, or feasible for that matter, that I would have to check for each different type of Array. Am I missing something obvious here?
There was a problem hiding this comment.
I think the trick is to do the validation after the encode rather than before. Instead of trying to prove that the original types follow the rules, just encode it anyway, and then verify that a) you get a dictionary and b) all of the members are just strings/numbers.
There was a problem hiding this comment.
Lets try this again
simplified some try/catch logic add unknownError type switch requestFailed error type from String to Error moved NetworkAPIError: Equatable into tests, fixed tests
add anyDecodableType to remove restriction on GET params being [String: String] changed passing an empty object for get param encoding from an error to just carrying on simplifyed some try/catch blocks.
| let message = "Decoding error: \(error)" | ||
| debugLogError(message) | ||
| guard let networkError = error as? NetworkAPIError else { | ||
| completion(.failure(.codingError(message))) |
There was a problem hiding this comment.
I think this should also be a wrapped error rather than as string?
| let jsonEncodedParams = try JSONEncoder().encode(request.parameters) | ||
| guard let url = url, | ||
| var components = URLComponents(url: url, resolvingAgainstBaseURL: true), | ||
| let paramsDict = try JSONDecoder().decode(AnyDecodable.self, from: jsonEncodedParams).value as? [String: Any] else { |
There was a problem hiding this comment.
Two other things related to this:
-
As we were talking about this is encode to text then decode loop is probably better than the alternatives (including a large dependancy just to get dictionary encoding), but it is pretty crummy, and should probably a) be commented b) have a bug filed to switch to encode-to-dictionary once the standard library supports it (not sure if that's on tap for the Swift 5 timeframe, I know there were some discussions about it but couldn't find them in a quick swift-evolution search).
-
This might be cleaner, and not need AnyDecodable, if it did the decoding with NSJSONSerialization rather than JSONDecoder.
There was a problem hiding this comment.
Created #3 to address this
…of unpermitted types Add some more tests, refined current tests removed unused debugging prints
…of unpermitted types Add some more tests, refined current tests removed unused debugging prints
…into bwl/testing * 'bwl/testing' of https://github.com/steamclock/networkAPI: Fixed get param encoding to use NSJSONSerialization, better handling of unpermitted types Add some more tests, refined current tests removed unused debugging prints
@nbrooke
Split out the parameter encoding and url qualifying into separate functions and added some unit tests. A couple questions: