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 - URLRequest Creation Safety #1505

Merged
merged 15 commits into from Sep 11, 2016
Merged

Conversation

cnoon
Copy link
Member

@cnoon cnoon commented Sep 11, 2016

This PR simply put makes Alamofire safe. Invalid URL strings, parameter encoding errors, adaptation errors, etc. are now all allowed to be used and thrown and Alamofire will gracefully handle it and expose the error in the response handlers.

Motivation

A VERY common problem in Alamofire is that users forget to percent escape their URL strings and Alamofire will crash. Up until now, we (the Alamofire team) have taken the stance that this is how Alamofire is designed and your URLs need to conform to RFC 2396. This is certainly not ideal for the community because we all would rather have Alamofire tell us that our URL was invalid rather than having it crash.

Solution

In order to make this safe, there were several things that needed to change including:

  • Making the task optional on a Request
  • Allowing the URLStringConvertible protocol to throw
  • Allowing the URLRequestConvertible protocol to throw
  • Allowing the RequestAdapter protocol to throw
  • Modifying all the SessionManager request creation methods to handle all error propagation

The core change here was making the task optional. By not requiring a URLSessionTask to actually be created and added to the underlying URLSession, we freed ourselves from the burden of trying to figure out what to do instead...which was always CRASH, or possibly even worse, send the URLRequest off even though we know it was wrong.

Once we made the task optional, I started allowing the protocols involved to be able to throw errors as well. We'd already made the change a few days ago to allow ParameterEncoding to throw which was a great change. But previously if it threw, we would record the error on the Request, but we would still have to send off the original URLRequest (missing the parameter encoding) because Alamofire required us to. You would receive the error in the response handler (good), but the request you intended to send to the server is not actually what got sent (very very bad).

Now, all these problem have been alleviated. If any of the steps encounter an error, Alamofire stops the task creation process, builds the Request subclass without the task, then starts up the delegate queue when resume is called. The response handlers will have access to the original error, and no task was ever sent out since we knew we couldn't create the one you wanted.

Remaining Tasks

There are still some documentation updates that need to be added to this PR before we move it through. They include:

  • Update README with all API changes
  • Update URLStringConvertible section of migration guide
  • Add URLRequestConvertible section to migration guide
  • Update RequestAdapter section of migration guide

Summary

Of all the great changes going into Alamofire 4, I think this one is probably the best. Alamofire 3.x is simply not safe if you don't make sure every request going through it is set up properly. Now most of the community has adjusted to this stance, but it's all to simple to miss a single case and have it end up crashing your app in production.

No more with Alamofire 4. Invalid requests simply go through Alamofire and come out on the other side with an error describing exactly what went wrong. 🎉🎉🎉

…stAdapter.

All three protocols now throw in the event that an error occurs. This allows any errors that occur in the URL request creation process to bubble through to the response handlers rather than crash.
@cnoon cnoon added the request label Sep 11, 2016
@cnoon cnoon added this to the 4.0.0 milestone Sep 11, 2016
@cnoon
Copy link
Member Author

cnoon commented Sep 11, 2016

cc @jshier and @kcharwood for review.

@cnoon
Copy link
Member Author

cnoon commented Sep 11, 2016

All open tasks have been completed.

///
/// Methods accepting a `URLStringConvertible` type parameter parse it according to RFCs 1738 and 1808.
///
/// See https://tools.ietf.org/html/rfc2396
/// See https://tools.ietf.org/html/rfc1738
/// See https://tools.ietf.org/html/rfc1808
var urlString: String { get }
var urlString: String? { get }
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's necessary to include the property in the protocol definition anymore. Just the protocol extension is enough to provide the property.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in e48a78e.

}

// MARK: -

/// Types adopting the `URLRequestConvertible` protocol can be used to construct URL requests.
public protocol URLRequestConvertible {
/// The URL request.
var urlRequest: URLRequest { get }
var urlRequest: URLRequest? { get }
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as with URLStringConvertible, I don't think we need the property in the protocol anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in e48a78e.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we decided this property can be removed.

@@ -1158,7 +1151,7 @@ enum Router: URLRequestConvertible {
```

```swift
Alamofire.request(resource: Router.ReadUser("mattt")) // GET /users/mattt
Alamofire.request(Router.ReadUser("mattt")) // GET /users/mattt
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be Alamofire.request(Router.readUser("mattt").

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 9296d43.

@jshier
Copy link
Contributor

jshier commented Sep 11, 2016

@cnoon Review completion, looks awesome! Just some minor comments and then it's good to land.

@cnoon
Copy link
Member Author

cnoon commented Sep 11, 2016

All comments addressed @jshier...ready to merge this guy?

@cnoon cnoon merged commit e8bca7e into master Sep 11, 2016
@cnoon cnoon deleted the feature/urlrequest-creation-safety branch September 11, 2016 06:50
@cnoon cnoon removed the in progress label Sep 11, 2016
let request = StreamRequest(session: session, task: task, originalTask: streamable)
do {
let task = try streamable.task(session: session, adapter: adapter, queue: queue)
let request = StreamRequest(session: session, requestTask: .stream(streamable, task))
Copy link
Contributor

Choose a reason for hiding this comment

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

why not request renamed to stream like in the upload func?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants