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 - Robust Validation #1461

Merged
merged 4 commits into from
Sep 6, 2016
Merged

Feature - Robust Validation #1461

merged 4 commits into from
Sep 6, 2016

Conversation

cnoon
Copy link
Member

@cnoon cnoon commented Sep 5, 2016

This PR splits up validation between Request subclasses and adds Data? or URL? parameters to the Validation closure.

Motivation

Previously, the Validation closure only exposed the optional request and response parameters making it difficult to customize validations in inline closures that were not custom extensions in the Request class. You didn’t have access to the actual response data. This was limiting in certain cases where you want to create a custom validation error that includes the error message buried in the server data. The only way you could do this previously was to write an extension on Request.

Another limitation was that the Validation closure did not expose the destinationURL for a DownloadRequest incase you needed to inspect the data in some special scenario.

One final reason to change up the design was that all validate methods could be chained on any Request. This doesn't make sense for StreamRequest types.

Solution

Modifying the validation system to accomplish all the goals stated above meant rethinking the majority of the design and splitting it up between subclasses.

Data Request

The Validation closure exposed on the DataRequest class is now as follows:

extension DataRequest {
    public typealias Validation = (URLRequest?, HTTPURLResponse, Data?) -> ValidationResult
}

By exposing the Data? property directly in the closure, you no longer have to write an extension on Request to access it. Now you can do something like this:

let urlString = "https://httpbin.org/get"

// When
Alamofire.request("https://httpbin.org/get", withMethod: .get)
    .validate { request, response, data in
        guard data != nil else { return .failure(ValidationError.missingData) }
        return .success
    }
    .response { response in
        debugPrint(response)
    }

This will be helpful in those cases where you would like to create a custom error that includes the error message from the server that's buried in the data.

Download Request

The Validation closure exposed on the DownloadRequest class is now as follows:

extension DownloadRequest {
    public typealias Validation = (URLRequest?, HTTPURLResponse, URL?) -> ValidationResult
}

The URL? parameter allows you to access the response data downloaded from the server and stored in the destinationURL. This allows you to inspect the data inside the file if you've determined you need to in order to create a custom error.

let fileURL = URL(fileURLWithPath: FileManager.documentsDirectory + "/test_response.json")

Alamofire.download(urlString, to: { _, _ in fileURL }, withMethod: .get)
    .validate { request, response, fileURL in
        guard let fileURL = fileURL else { return .failure(ValidationError.missingFile) }

        do {
            let _ = try Data(contentsOf: fileURL)
            return .success
        } catch {
            return .failure(ValidationError.fileReadFailed)
        }
    }
    .response { response in
        debugPrint(response)
    }

DRY by Design

Since the default validation methods supported in AF are the same between data and download requests, I decided to DRY things up by pulling the validation logic out into a fileprivate set of extensions on Request. This is very similar to how the response serialization logic was DRY'd up as well.

Session Manager

I also snuck in commit c46bc01 into this PR that updates the SessionManager to leverage the new TaskConvertible conformance to generate the URLSessionTask instances. This cleans up a bunch of duplicated logic that I should have caught in the earlier PRs. This commit doesn't change any functionality, just DRYs things up a bit.


Summary

These changes allow inline validation closures to be much more powerful, eliminates chaining validation APIs on StreamRequest instances, and exposes the destinationURL directly to the inline closure. We'll want to update the Validation section of the README to include examples of best practices here.

We need to make sure people understand they shouldn't parse the response data unless they know they're in a failure case where they want to dig the error message out.

@cnoon
Copy link
Member Author

cnoon commented Sep 5, 2016

cc @jshier and @kcharwood for review.

@cnoon cnoon force-pushed the feature/response-serializers-per-subclass branch from 6350757 to f02a579 Compare September 6, 2016 04:03
@cnoon cnoon force-pushed the feature/robust-validation branch from c46bc01 to 652536c Compare September 6, 2016 04:03
@@ -34,9 +39,117 @@ extension Request {
case failure(Error)
}

/// A closure used to validate a request that takes a URL request and URL response, and returns whether the
fileprivate struct MIMEType {
Copy link
Contributor

Choose a reason for hiding this comment

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

I've always wondered whether we want to expose this publicly, as it seems useful for users to be able to parse MIME types themselves, similar to URL encoding methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd vote for keeping it hidden until someone actually requests it. I haven't thought this myself yet.

@jshier
Copy link
Contributor

jshier commented Sep 6, 2016

@cnoon Review complete. Great changes! Just some minor comments.

@cnoon cnoon force-pushed the feature/response-serializers-per-subclass branch from f02a579 to 66712de Compare September 6, 2016 05:41
@cnoon cnoon force-pushed the feature/robust-validation branch from 652536c to f8b1c4b Compare September 6, 2016 05:45
@cnoon cnoon changed the base branch from feature/response-serializers-per-subclass to swift3 September 6, 2016 05:54
…ubclasses.

Previously, the Validation closure only exposed the optional request and response parameters making it difficult to customize validations in inline closures that were not custom extensions in the `Request` class. You didn’t have access to the actual response data. This was limiting in certain cases where you want to create a custom validation error that includes the error message buried in the server data. The only way you could do this previously was to write an extension on `Request`. Now you can do it in an inline closure right at the callsight.

Another limitation was that the `Validation` closure did not expose the `destinationURL` for a `DownloadRequest` incase you needed to inspect the data in some special scenario. There is now a custom `Validation` closure on `DownloadRequest` that exposes the `destinationURL` directly. This allows you to inspect the file right in an inline closure without having to extension `DownloadRequest` directly.
@cnoon cnoon force-pushed the feature/robust-validation branch from f8b1c4b to cb44f1f Compare September 6, 2016 05:54
@cnoon cnoon force-pushed the feature/robust-validation branch from a942a7a to a01540b Compare September 6, 2016 06:00
@cnoon
Copy link
Member Author

cnoon commented Sep 6, 2016

Thanks for the review @jshier! All the comments have been addressed, I'm going to go ahead and move this one through.

@cnoon cnoon merged commit cb2f77f into swift3 Sep 6, 2016
@cnoon cnoon deleted the feature/robust-validation branch September 6, 2016 06:02
@A1iAshoor
Copy link

Thanks so much, we need this so bad in swift 2.2 otherwise we'll be forced to switch to swift 3 and da heck we're gonna get by doing this for big project

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.

5 participants