Skip to content
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

Feature - Response Serializers per Subclass #1457

Merged
merged 9 commits into from Sep 6, 2016

Conversation

cnoon
Copy link
Member

@cnoon cnoon commented Sep 5, 2016

This PR locks current response serializers down to data and upload requests and creates all new response serializers for download requests.

Motivation

Prior to this PR, response serializers could be applied to any Request subclass. This works great for data and upload requests, but completely falls apart for download and stream requests. Response serializers should only be able to be chained onto Request subclasses that match the actual serializer behavior.

Solution

In order to achieve this, I had to create a few new types:

  • DefaultDataResponse - Encapsulates all data associated with an unserialized data or upload request
  • DataResponse - Encapsulates all data associated with a serialized data or upload request
  • DefaultDownloadResponse - Encapsulates all data associated with an unserialized download request
  • DownloadResponse - Encapsulates all data associated with a serialized download request
  • DataResponseSerializerType - data response serialization protocol
  • DataResponseSerializer - generic data response serializer
  • DownloadResponseSerializerType - download response serialization protocol
  • DownloadResponseSerializer - generic download response serializer

Default Responses

The DefaultDataResponse and DefaultDownloadResponse structs were created to make the unserialized forms of the response APIs consistent with the serialized versions in only returning a single response parameter. The consistency is a nice addition.

Data and Download Response Serializers

It would have been possible to add support to the ResponseSerializer and Response types for download requests, but it would not have been an ideal set of changes. First off, the chainable Request APIs would still be available to the StreamRequest. Also, the Response struct would have picked up destinationURL and resumeData properties which would only be set when serializing a DownloadRequest. Because of these reasons, I thought it best to make a clean split between them and tailor each set of APIs to their respective subclass.

Request Serialization Extensions

When I first built out the variants between data and download response serializers, I realized there was a lot of code duplication when running all the different validation scenarios. The only real difference between them was that the download response serializer first needed to extract the data out of the destinationURL. Once that was done, the rest of the validation was the same between them.

Because of this, I pulled the common logic out into extensions on Request to be shared between both the DataRequest and DownloadRequest extension implementations. Here's an example of the String serialization.

extension Request {
    public static func serializeResponseString(
        encoding: String.Encoding?,
        response: HTTPURLResponse?,
        data: Data?,
        error: Error?)
        -> Result<String>
    {
        guard error == nil else { return .failure(error!) }

        if let response = response, emptyDataStatusCodes.contains(response.statusCode) { return .success("") }

        guard let validData = data else {
            return .failure(AFError.responseSerializationFailed(reason: .inputDataNil))
        }

        var convertedEncoding = encoding

        if let encodingName = response?.textEncodingName as CFString!, convertedEncoding == nil {
            convertedEncoding = String.Encoding(rawValue: CFStringConvertEncodingToNSStringEncoding(
                CFStringConvertIANACharSetNameToEncoding(encodingName))
            )
        }

        let actualEncoding = convertedEncoding ?? String.Encoding.isoLatin1

        if let string = String(data: validData, encoding: actualEncoding) {
            return .success(string)
        } else {
            return .failure(AFError.responseSerializationFailed(reason: .stringSerializationFailed(encoding: actualEncoding)))
        }
    }
}

extension DataRequest {
    public static func stringResponseSerializer(encoding: String.Encoding? = nil) -> DataResponseSerializer<String> {
        return DataResponseSerializer { _, response, data, error in
            return Request.serializeResponseString(encoding: encoding, response: response, data: data, error: error)
        }
    }
}

extension DownloadRequest {
    public static func stringResponseSerializer(encoding: String.Encoding? = nil) -> DownloadResponseSerializer<String> {
        return DownloadResponseSerializer { _, response, fileURL, error in
            guard error == nil else { return .failure(error!) }

            guard let fileURL = fileURL else {
                return .failure(AFError.responseSerializationFailed(reason: .inputFileNil))
            }

            do {
                let data = try Data(contentsOf: fileURL)
                return Request.serializeResponseString(encoding: encoding, response: response, data: data, error: error)
            } catch {
                return .failure(AFError.responseSerializationFailed(reason: .inputFileReadFailed(at: fileURL)))
            }
        }
    }
}

As you can see, almost all the string validation takes place in the shared Request. serializeResponseString method. I used this same pattern across all the response serializers to DRY it up significantly.

Common Usage

Now that we have download request response serializers, you can actually download a json blob to a file and still use a responseJSON serializer to extract the data. No more trying to figure out what the heck the destinationURL actually is!

let fileURL = URL(fileURLWithPath: FileManager.documentsDirectory + "/get.json")
let destination: DownloadRequest.DownloadFileDestination = { _, _ in fileURL }

Alamofire.download("https://httpbin.org/get", to: destination, withMethod: .get).responseJSON { response in
    debugPrint(response)
}

Summary

In summary, these changes should fix up all sorts of confusing problems people have had with response serializers in the past. The don't make creating your own custom serializers any easier, but I don't think they make it any harder either. They now just add the ability to make custom ones for in-memory downloads or on-disk downloads.

@cnoon
Copy link
Member Author

cnoon commented Sep 5, 2016

cc @jshier and @kcharwood for review.

public protocol ResponseSerializerType {
/// The type of serialized object to be created by this `ResponseSerializerType`.
/// The type in which all data response serializers must conform to in order to serialize a response.
public protocol DataResponseSerializerType {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the Swift 3 guidelines call for these sorts of protocols to end in Protocol now, not Type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, will change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 2cb2782.

@cnoon cnoon force-pushed the feature/request-subclasses-and-progress branch from db619cc to 97658eb Compare September 6, 2016 04:01
@jshier
Copy link
Contributor

jshier commented Sep 6, 2016

@cnoon Review complete. Some minor feedback, but it looks great! I'm a bit worried about the growing complexity but I think if it's hidden well we should be fine.

I also couldn't come up with a better name than DefaultDataResponse, which saddens me. If only a generic and non-generic type could have the same name.

@cnoon cnoon force-pushed the feature/response-serializers-per-subclass branch from 6350757 to f02a579 Compare September 6, 2016 04:03
@cnoon
Copy link
Member Author

cnoon commented Sep 6, 2016

Fair enough @jshier on the name...I couldn't come up with a better one either given all the tradeoffs. As for the growing complexity, I think it's all manageable. It was certainly a concern of mine as well.

@cnoon cnoon closed this Sep 6, 2016
@cnoon cnoon removed the in progress label Sep 6, 2016
@cnoon cnoon changed the base branch from feature/request-subclasses-and-progress to swift3 September 6, 2016 05:39
@cnoon cnoon reopened this Sep 6, 2016
…wnload requests.

Previously, all response serializers could be used on any Request which was very confusing for download and stream requests. This commit moves all the previous response serializers into extensions on DataRequest which makes them only accessible to data and upload requests. The same response serializers were then re-created to support download requests as well.
@cnoon cnoon force-pushed the feature/response-serializers-per-subclass branch from f02a579 to 66712de Compare September 6, 2016 05:41
@cnoon
Copy link
Member Author

cnoon commented Sep 6, 2016

All comments addressed. I'm going to move this through. Thanks for the review @jshier! Much appreciated.

@cnoon cnoon merged commit dae62b2 into swift3 Sep 6, 2016
@cnoon cnoon deleted the feature/response-serializers-per-subclass branch September 6, 2016 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants