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

[SR-7338] Handle cookies in URLSession #1542

Merged
merged 1 commit into from
May 29, 2018

Conversation

mamabusi
Copy link
Contributor

This PR implements storage of HTTP cookies received as part of the response from the server and setting of appropriate cookies on the URLRequest of the URLSession.

@mamabusi mamabusi changed the title Handle cookies in URLSession [SR-7338] Handle cookies in URLSession Apr 30, 2018
@pushkarnk
Copy link

@swift-ci please test

2 similar comments
@alblue
Copy link
Contributor

alblue commented Apr 30, 2018

@swift-ci please test

@spevans
Copy link
Contributor

spevans commented May 1, 2018

@swift-ci please test

@mamabusi
Copy link
Contributor Author

@ianpartridge Could you please review this PR.

Copy link
Contributor

@ianpartridge ianpartridge left a comment

Choose a reason for hiding this comment

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

A few suggestions.

let r = request.createMutableURLRequest()
_configuration.configure(request: r)
var r = request.createMutableURLRequest()
r = _configuration.configure(request: r)
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 just return _configuration.configure(request: r)?

var request = request
httpAdditionalHeaders?.forEach {
guard request.value(forHTTPHeaderField: $0.0) == nil else { return }
request.setValue($0.1, forHTTPHeaderField: $0.0)
}
request = setCookies(on: request)
Copy link
Contributor

Choose a reason for hiding this comment

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

return setCookies(on: request)?

if httpShouldSetCookies {
//TODO: Ask the cookie storage what cookie to set.
if let cookieStorage = self.httpCookieStorage, let cookies = cookieStorage.cookies(for: request.url!) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the force unwrap, e.g.

if let cookieStorage = self.httpCookieStorage,
   let url = request.url,
   let cookies = cookieStorage.cookies(for: url) {
    ...
}

@mamabusi
Copy link
Contributor Author

mamabusi commented May 29, 2018

@ianpartridge Thanks for the suggestions. I have made the necessary changes and updated the branch.

@ianpartridge
Copy link
Contributor

@swift-ci please test

@ianpartridge ianpartridge merged commit 26e255f into swiftlang:master May 29, 2018
@maksimorlovich
Copy link
Contributor

@spevans @mamabusi @ianpartridge Would it be possible to get this merged into 4.2 branch? I think this is an important piece of functionality that's missing from Swift 4.2 release.

Also, looks like this was partially already merged in via this PR:
#1615

I think it was by accident? The new test cases made it to Swift 4.2 branch, but not the actual code...

@mamabusi
Copy link
Contributor Author

@maksimorlovich Sure, I'll be creating a PR soon for the Swift 4.2 branch.

maksimorlovich pushed a commit to maksimorlovich/swift-corelibs-foundation that referenced this pull request Aug 28, 2018
@maksimorlovich
Copy link
Contributor

@mamabusi , I created a PR to Swift 4.2 branch here: #1676 Please take a look!

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

Successfully merging this pull request may close these issues.

6 participants