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
Conversation
…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.
cc @jshier and @kcharwood for review. |
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 } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in e48a78e.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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")
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 9296d43.
@cnoon Review completion, looks awesome! Just some minor comments and then it's good to land. |
All comments addressed @jshier...ready to merge this guy? |
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)) |
There was a problem hiding this comment.
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?
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:
task
optional on aRequest
URLStringConvertible
protocol to throwURLRequestConvertible
protocol to throwRequestAdapter
protocol to throwSessionManager
request creation methods to handle all error propagationThe core change here was making the
task
optional. By not requiring aURLSessionTask
to actually be created and added to the underlyingURLSession
, we freed ourselves from the burden of trying to figure out what to do instead...which was always CRASH, or possibly even worse, send theURLRequest
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 theRequest
, but we would still have to send off the originalURLRequest
(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 thetask
, then starts up the delegate queue whenresume
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:
URLStringConvertible
section of migration guideURLRequestConvertible
section to migration guideRequestAdapter
section of migration guideSummary
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. 🎉🎉🎉