Implement NSHTTPURLResponse #287

danieleggert · 2016-03-15T17:38:27Z

This implements both the trivial attributes (statusCode and allHeaderFields), and also parses the headers in order to set the following attributes:

public var mimeType: String? { get }
public var expectedContentLength: Int64 { get }
public var textEncodingName: String? { get }
public var suggestedFilename: String? { get }

Added tests for these.

The

public class func localizedStringForStatusCode(statusCode: Int) -> String { NSUnimplemented() }

method on NSHTTPURLResponse is still unimplemented.

danieleggert · 2016-03-15T17:40:05Z

Note: This changes the type of allHeaderFields to [String : String] — I hope that's ok?

phausler · 2016-03-15T17:45:24Z

That actually makes sense since the init method is NSDictionary<NSString *, NSString *> * headerFields.

It seems like that might just be an oversight for the property. I will follow up with that component owner to determine if it is actually the case that it can never be anything but strings.

danieleggert · 2016-03-16T15:08:07Z

@phausler The same goes for NSURLSessionConfiguration’s HTTPAdditionalHeaders.

phausler · 2016-03-18T14:41:43Z

Foundation/NSURLResponse.swift

@@ -167,9 +178,9 @@ public class NSHTTPURLResponse : NSURLResponse {
        @result A dictionary containing all the HTTP header fields of the
        receiver.
    */
-    public var allHeaderFields: [NSObject : AnyObject] { NSUnimplemented() }
+    public let allHeaderFields: [String : String]


This should have a comment on why the differential and identifying it as an "experimental" change; albeit that I agree that this should be the signature, I have not yet heard back from the component owner on a definitive reason on either why it should or should not be marked as string to string.

danieleggert · 2016-03-18T16:08:36Z

Thanks for the feedback. I’ve tried to address you suggesting with ae45629 and 82a818e. I’ll squash everything into a single commit if / once I get a green light.

phausler · 2016-03-21T14:36:32Z

looks pretty good to me.

danieleggert · 2016-03-21T19:17:28Z

Should this be assigned to @salgernon ?

danieleggert · 2016-03-22T09:37:09Z

I rebased against master and squashed the 3 commits into a single commit.

phausler · 2016-03-30T22:39:05Z

The tests seem to have some failures when it is tested against the objective-c version for me:

Test: test_contentLength_notAvailable_2() in TestNSHTTPURLResponse failed on My Mac
Assertions: XCTAssertEqual failed: ("Optional(997)") is not equal to ("Optional(-1)") - 
File: test.swift:174



Test: test_contentLength_notAvailable_3() in TestNSHTTPURLResponse failed on My Mac
Assertions: XCTAssertEqual failed: ("Optional(997)") is not equal to ("Optional(-1)") - 
File: test.swift:179



Test: test_contentLength_notAvailable_4() in TestNSHTTPURLResponse failed on My Mac
Assertions: XCTAssertEqual failed: ("Optional(997)") is not equal to ("Optional(-1)") - 
File: test.swift:184



Test: test_contentLength_notAvailable_5() in TestNSHTTPURLResponse failed on My Mac
Assertions: XCTAssertEqual failed: ("Optional(997)") is not equal to ("Optional(-1)") - 
File: test.swift:189



Test: test_MIMETypeAndCharacterEncoding_3() in TestNSHTTPURLResponse failed on My Mac
Assertions: XCTAssertEqual failed: ("Optional("iso-8859-4")") is not equal to ("Optional("ISO-8859-4")") - 
File: test.swift:265



Test: test_suggestedFilename_4() in TestNSHTTPURLResponse failed on My Mac
Assertions: XCTAssertEqual failed: ("Optional("wrong.ext")") is not equal to ("Optional("fname.ext")") - 
File: test.swift:234



Test: test_suggestedFilename_notAvailable_1() in TestNSHTTPURLResponse failed on My Mac
Assertions: XCTAssertNil failed: "Unknown" - 
File: test.swift:208



Test: test_suggestedFilename_notAvailable_2() in TestNSHTTPURLResponse failed on My Mac
Assertions: XCTAssertNil failed: "Unknown" - 
File: test.swift:213



Test: test_suggestedFilename_onlyTheLastPathComponent_1() in TestNSHTTPURLResponse failed on My Mac
Assertions: XCTAssertEqual failed: ("Optional("_a_b_name")") is not equal to ("Optional("name")") - 
File: test.swift:239



Test: test_suggestedFilename_onlyTheLastPathComponent_2() in TestNSHTTPURLResponse failed on My Mac
Assertions: XCTAssertEqual failed: ("Optional("a_.._b_name")") is not equal to ("Optional("name")") - 
File: test.swift:244

Test: test_suggestedFilename() in TestNSURLResponse failed on My Mac
Assertions: XCTAssertNil failed: "path" - 
File: test.swift:67

danieleggert · 2016-04-04T16:13:27Z

The failure in test_suggestedFilename_4 looks like a bug in Darwin Foundation to me.

danieleggert · 2016-04-04T16:20:25Z

I've addressed the difference to Objective-C Foundation with the 4 above commits.

phausler · 2016-04-04T16:25:24Z

I tend to agree that probably the failure of test_suggestedFilename_4 on Darwin is a bit un-expected; I will follow up with the networking team via radar on this. For now I would probably claim if that is the only differential it is an edge-case enough that it shouldn't hamper this PR.

phausler · 2016-04-04T16:25:34Z

@swift-ci Please test

danieleggert · 2016-04-04T16:54:57Z

I’ll squash the commits into a single commit…

parkera · 2016-04-06T17:50:24Z

@swift-ci please test

parkera · 2016-04-14T19:29:46Z

@swift-ci please test

lilyball · 2016-04-15T05:26:51Z

Foundation/NSURLResponse.swift

+    ///
+    /// - Important: This is an *experimental* change from the
+    /// `[NSObject: AnyObject]` type that Darwin Foundation uses.
+    public let allHeaderFields: [String: String]


This doesn't match the behavior of the real Foundation. Specifically, allHeaderFields uses a custom NSDictionary subclass that's case-insensitive when looking up keys. That case-insensitive behavior is pretty important behavior when dealing with HTTP headers.

It also canonicalizes certain headers, e.g. Content-Length, although it doesn't document which headers (besides Content-Length), and the canonicalization really only affects the case where you're displaying the header name so it's not really particularly important.

Given that Swift dictionaries are a value type and not a class, the only way we're going to get the same behavior while still using a dictionary type is to rely on bridging support and using an NSDictionary subclass, like the real Foundation does.

Also note that this has to handle the case where the same header is provided with different capitalizations to the initializer (in that event, the resulting allHeaderFields dictionary should only contain one of the equivalent keys and its corresponding value, and the conflicts should be discarded).

Good point. Can you open a bug on that and assign it to me? Thanks.

[test] Fix notification handler test when run in serial

phausler reviewed Mar 18, 2016
View reviewed changes

danieleggert force-pushed the httpurlresponse branch from 82a818e to 8bb8ce7 Compare March 22, 2016 09:36

danieleggert force-pushed the httpurlresponse branch from 9ad8740 to 12a1b4e Compare April 4, 2016 16:56

danieleggert mentioned this pull request Apr 6, 2016

Basic NSURLSession implementation #299

Closed

danieleggert force-pushed the httpurlresponse branch from 12a1b4e to e352c67 Compare April 6, 2016 21:57

Implement NSHTTPURLResponse

d55f3fa

danieleggert force-pushed the httpurlresponse branch from e352c67 to d55f3fa Compare April 14, 2016 13:55

parkera merged commit be0a9c8 into apple:master Apr 14, 2016

lilyball reviewed Apr 15, 2016
View reviewed changes

danieleggert deleted the httpurlresponse branch April 15, 2016 09:35

atrick pushed a commit to atrick/swift-corelibs-foundation that referenced this pull request Jan 12, 2021

Merge pull request apple#287 from benlangmuir/test-serial-fix

0ea45c7

[test] Fix notification handler test when run in serial

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement NSHTTPURLResponse #287

Implement NSHTTPURLResponse #287

danieleggert commented Mar 15, 2016

danieleggert commented Mar 15, 2016

phausler commented Mar 15, 2016

danieleggert commented Mar 16, 2016

phausler Mar 18, 2016

danieleggert commented Mar 18, 2016

phausler commented Mar 21, 2016

danieleggert commented Mar 21, 2016

danieleggert commented Mar 22, 2016

phausler commented Mar 30, 2016

danieleggert commented Apr 4, 2016

danieleggert commented Apr 4, 2016

phausler commented Apr 4, 2016

phausler commented Apr 4, 2016

danieleggert commented Apr 4, 2016

parkera commented Apr 6, 2016

parkera commented Apr 14, 2016

lilyball Apr 15, 2016

lilyball Apr 15, 2016

danieleggert Apr 15, 2016

Implement NSHTTPURLResponse #287

Implement NSHTTPURLResponse #287

Conversation

danieleggert commented Mar 15, 2016

danieleggert commented Mar 15, 2016

phausler commented Mar 15, 2016

danieleggert commented Mar 16, 2016

phausler Mar 18, 2016

Choose a reason for hiding this comment

danieleggert commented Mar 18, 2016

phausler commented Mar 21, 2016

danieleggert commented Mar 21, 2016

danieleggert commented Mar 22, 2016

phausler commented Mar 30, 2016

danieleggert commented Apr 4, 2016

danieleggert commented Apr 4, 2016

phausler commented Apr 4, 2016

phausler commented Apr 4, 2016

danieleggert commented Apr 4, 2016

parkera commented Apr 6, 2016

parkera commented Apr 14, 2016

lilyball Apr 15, 2016

Choose a reason for hiding this comment

lilyball Apr 15, 2016

Choose a reason for hiding this comment

danieleggert Apr 15, 2016

Choose a reason for hiding this comment