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 - Destinations Closures, Download Options and Top-Level APIs #1462

Merged
merged 11 commits into from Sep 6, 2016

Conversation

cnoon
Copy link
Member

@cnoon cnoon commented Sep 6, 2016

This PR refactors the download destination system as well as the top-level APIs for constructing requests.

Motivation

Up until now, the DownloadFileDestination closures have been too limiting. They "force" you to move the file from the temporary URL to some other URL. Even if you don't want to move it, you still have to implement a destination closure and return the temp URL.

A bigger issue is that you have to do all your preparation of the file system up front. You cannot modify the file system at the time the destination closure is called. For example, let's say I have an avatar image at $Sandbox/Documents/avatar.jpg and I want to update it. Right now, you have to either delete the avatar.jpg file before making the request or move it to a different location. Otherwise you'll get a file system error when trying to move the file because a file already exists at the destination url. But you don't want to delete it before you make the request because you don't know that the request will actually succeed.

On a different note, we've wanted to remove the with portion of the withMethod external parameter name in the top-level APIs. We've also wanted to figure out how add default values for the method without introducing ambiguity.

Solution

Download File Destination Closure

The first major change in this PR is to make the destination closure optional in all the top-level APIs. You no longer have to move the file if you don't want to. It's now perfectly acceptable to do the following:

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

If you do this, the file will stay in the temporary location. If you want to move it somewhere else, you'll need to handle that on your own.

It's important to note that you should always move the file out of the temporary location if you are going to need access to it at a later time.

Now if you do want to move the file to a different destination, you'll need to use the DownloadFileDestination closure which is a bit different.

public typealias DownloadFileDestination = (
    _ temporaryURL: URL,
    _ response: HTTPURLResponse)
    -> (destinationURL: URL, options: DownloadOptions)

You'll notice that the typealias is the same as it was before with the exception of the DownloadOptions in the return type.

Download Options

The DownloadOptions struct is an OptionSet that comes with a couple cool new features.

  • .createIntermediateDirectories - creates all intermediate directories to the destination url if specified
  • .removePreviousFile - removes a previous file at the destination url if it exists right before moving the temp file to the destination url

These options allow you to customize the file system behavior right before the temp file is moved to the final destination url. Here's an example of what it looks like to use the new destination closure system.

let destination: DownloadRequest.DownloadFileDestination = { _, _ in 
    return (fileURL, [.removePreviousFile, .createIntermediateDirectories]) 
}

Alamofire.download(urlString, to: destination).response { response in
    debugPrint(response)
}

DownloadResponse

The DownloadResponse has been modified to support both the temporaryURL and destinationURL properties. The temporaryURL will always be set if the file was successfully downloaded. The destinationURL will be set if the file was successfully downloaded and the destination closure was set. Make sure to check whether the Result was a success though to know the file was able to be copied to the destinationURL. If the move operation fails, an error is generated, but destinationURL is not nil. I designed it this way so that when an error occurs, you have as much information as possible to debug what happened.

Download Request Validation

The Validation typealias for a DownloadRequest has been modified to support both the temporaryURL and destinationURL.

public typealias Validation = (URLRequest?, HTTPURLResponse, _ temporary: URL?, _ destination: URL?) -> ValidationResult

Top-Level APIs

The goal of the top-level API changes was to set method defaults and get rid of the with portion of the withMethod external parameter name. The results are pretty awesome. You can now do the following:

Alamofire.request("https://httpbin.org/get") // defaults to `.get`
Alamofire.download("https://httpbin.org/get") // defaults to `.get`
Alamofire.upload("https://httpbin.org/post") // defaults to `.post`

Since this is certainly the majority of the use cases, this makes the call sights much more concise. If you do need to specify the method, it's now just method instead of withMethod.

Alamofire.request("https://httpbin.org/delete", method: .delete)

In order to disambiguate the request APIs, we had to add the external resource parameter name to the URLRequestConvertible variant.

Alamofire.request(resource: urlRequest)
Alamofire.download(resource: urlRequest, to: destination)

The reason for this is that URLRequest conforms to both URLStringConvertible and URLRequestConvertible. Rather than going down the route of removing conforms, I decided to just remove the potential of ambiguity altogether. Otherwise users could add their own extensions to types that would re-introduce ambiguity. And as well all know, ambiguous errors suck to debug.

One thing that I think is still open is whether we add the resource name to the upload APIs.

// Current
Alamofire.upload(fileURL, to: urlString)
Alamofire.upload(fileURL, with: urlRequest)

// Possible Improvements
Alamofire.upload(fileURL, to: urlRequest)
Alamofire.upload(fileURL, toResource: urlRequest)

Thoughts here?


Summary

These changes greatly improve the functionality and usefulness of download requests by allowing them to support all the potential use cases. In the event that the DownloadOptions don't offer you enough flexibility, you can alway fall back to moving the file on your own in the response serializer. The top-level API changes are a huge improvement and make the common usage cases very concise.

@cnoon
Copy link
Member Author

cnoon commented Sep 6, 2016

cc @jshier and @kcharwood for review.

@cnoon cnoon force-pushed the feature/destinations-options-and-apis branch from 3125beb to f2ad0db Compare September 6, 2016 04:04
@@ -148,65 +148,74 @@ public func request(_ urlRequest: URLRequestConvertible) -> DataRequest {
/// Creates a `DownloadRequest` using the default `SessionManager` to retrieve the contents of a URL based on the
/// specified `urlString`, `method`, `parameters`, `encoding`, `headers` and save them to the `destination`.
///
/// If `destination` is not specified, the contents will remain in the temporary location determined by the
/// URL session.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be more specific and say by the underlying URLSession?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure! 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.

Addressed in 66fc1a8.

@jshier
Copy link
Contributor

jshier commented Sep 6, 2016

@cnoon Reviews finished, just some minor comments. Looks great!

@cnoon cnoon force-pushed the feature/destinations-options-and-apis branch from f2ad0db to 7fcbff7 Compare September 6, 2016 05:50
@cnoon cnoon force-pushed the feature/destinations-options-and-apis branch from 7fcbff7 to cbbd4d3 Compare September 6, 2016 05:55
@cnoon cnoon changed the base branch from feature/robust-validation to swift3 September 6, 2016 06:01
…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/destinations-options-and-apis branch from cbbd4d3 to 722b701 Compare September 6, 2016 06:02
@cnoon
Copy link
Member Author

cnoon commented Sep 6, 2016

Thanks for the review @jshier...much appreciated! 🍻

All comments have been addressed on this PR. I'm going to go ahead and move it through.

@cnoon cnoon merged commit 78c7b9d into swift3 Sep 6, 2016
@cnoon cnoon deleted the feature/destinations-options-and-apis branch September 6, 2016 06:14
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