Alamofire Support

[RLovelett]

tl;dr Alamofire makes extensive use of the NSURLSessionDelegate protocol. Vinyl did not support this.

What's changing

• Expand URLSessionDataTask to better implement the properties and methods described in the base class NSURLSessionTask
• Trigger the appropriate delegate calls in URLSessionDataTask as described by the documentation in NSURLSessionDataTask
• Accept an Optional<NSURLSessionDelegate> as an argument to Turntable initializers
• Expand the test suite to cover the new delegate pattern
• Rebase and squash the commits to make it a little bit more coherent

Example Usage

class CredentialTests: XCTestCase {

    var turntable: Turntable!

    var manager: Alamofire.Manager!

    override func setUp() {
        super.setUp()
        // Put setup code here. This method is called before the invocation of each test method in the class.

        let types: [RequestMatcherType] = [.Body, .Query]
        let matching: MatchingStrategy = .RequestAttributes(types: types, playTracksUniquely: true)
        let config = TurntableConfiguration(matchingStrategy:  matching)
        let delegate = Alamofire.Manager.SessionDelegate()
        self.turntable = Turntable(vinylName: "credentials", turntableConfiguration: config, delegate: delegate)
        self.manager = Alamofire.Manager(session: turntable, delegate: delegate)

        self.continueAfterFailure = false
        XCTAssertNotNil(turntable)
        XCTAssertNotNil(manager)
        self.continueAfterFailure = true
    }

    func testFailureToProvidePostBody() {
        let expectation = self.expectationWithDescription(#function)

        manager?
            .request(.POST, "https://json.schedulesdirect.org/20141201/token")
            .responseString { response in
                XCTAssertEqual(response.result.value, "{\"response\":\"Username expected but not provided.\",\"code\":403}")
                XCTAssertEqual(response.response?.statusCode, 403)
                expectation.fulfill()
        }

        self.waitForExpectationsWithTimeout(5.0) { (error) in
            if let error = error {
                XCTFail("Expectation failed with error: \(error)")
            }
        }
    }

}

[RLovelett]

@RuiAAPeres I am making a pull-request, not because it is done, but because I wanted you to see the direction that I was headed with this (you may want to add your work-in-progress label).

Hopefully, I'll have all this done before the end of next week. I'm planning to expand the test coverage of a library I'm writing. Which should exercise this code and push its limits. Once I'm done with that then I expect this will be ready for inclusion upstream. Put another way: I'm dog-fooding this still and I'm not 100% sure its right but it at least indicates the direction it is heading.

[Sephiroth87]

I was wondering if we actually need to implement the delegate, instead of simply returning self instead of nil as the delegate from the Turntable, and simply pass it to satisfy the requirement.

Is there an actual use case for it?

[RLovelett]

@Sephiroth87 I'm not sure I follow. I think you are asking if Vinyl's Turntable should implement one or more of NSURLSessionDelegate, NSURLSessionTaskDelegate, NSURLSessionDataDelegate, NSURLSessionDownloadDelegate and then return self from var delegate: NSURLSessionDelegate? and if there is an actual use case for it.

To which my short answer would be: there might be a use case for it but this is most certainly not it.

The single best reason is that the delegate is supposed to be listening for a network response. Why would Turntable (or any class within Vinyl) need to listen for a network response? Turntable is the network response (or at least a mock network response).

Furthermore the goal here is to create a Alamofire.Manager instance with a mocked NSURLSession instance. To do this we have to provide a NSURLSession (e.g., Turntable instance) and a Alamofire.Manager.SessionDelegate instance to the Alamofire.Manager initializer. In order to do that Turntable or some other type would have to inherit Alamofire.Manager.SessionDelegate which of course is a terrible idea. Which leads me back to the point I made above: why would Vinyl want to listen for responses from itself? Only Alamofire would care to do that in this use case.

Which is why a library like Alamofire absolutely needs to conform to all those delegate protocols (and does in Alamofire.Manager.SessionDelegate). So it can listen for a network response (albeit mocked by Vinyl) and then provide appropriate calls on its API from those responses.

Not to dodge the question entirely though. The place where I could see Vinyl start to need to implement those protocols is when it provides the capability to record a network response. In that case it could then be a delegate and accept a delegate. In this capacity it might "pass-through" or curry its delegate responses (after it does its own processing) to the actual delegate that is being tested. Just one thought.

I hope that answers the question.

[Sephiroth87]

I guess my mistake was assuming Alamofire.Manager.SessionDelegate was simply a NSURLSessionDelegate, which is not the case.
My point was simply to keep the code simple and just satisfy the guard delegate === session.delegate else { return nil } check, without actually caring about the delegate calls...

In this case clearly it doesn't make sense to have inherit from it.

Just keep in mind then that Vinyl will need to record network responses "soon" (#12), but I don't know how it will actually handle it.

[RLovelett]

At this point I'm ready to say it is a stable starting point for Vinyl working with Alamofire. Welcome feedback and review.

[tomtaylor]

I've just had a go with this - worked well for me. Thanks a lot! Would be great to get this merged ASAP.

[RuiAAPeres]

@tomtaylor this needs to be evaluated. I am unsure about this approach.

[tomtaylor]

@RuiAAPeres can you provide any suggestions to @RLovelett? At the moment we'd love to use Vinyl (or DVR), but this is the only working integration with Alamofire.

[RLovelett]

@tomtaylor I've been using these changes for a few weeks now. Considering it has relatively few API differences with the master of the repo they should be mostly drop-in replacements for one another. Therefore, I feel like this is stable enough that it can be used as a fork in the meantime.

If you check out the master branch on my fork I have integrated this PR, #62 and #63. I plan to maintain that fork as long as necessary so that there is a working implementation that supports Alamofire.

I hope that helps you have some confidence with moving forward using Vinyl with Alamofire.

[RuiAAPeres]

I don't really agree with this:

1. We use URLSessionTaskType specifically to abstract the kind of Task we are working with.
2. You replaced it with a URLSessionUploadTask, which might not even make sense, depending on the operating we are doing.

[RLovelett]

I can add it back if you'd like. Though I don't think it'll buy you the flexibility you seem to think it will.

The only type that will conform to this going forward is URLSessionUploadTask. So even though it uses generic syntax there is nothing generic about its usage (e.g., protocols for protocols sake).

What is more going forward URLSessionDataTask and URLSessionUploadTask, the only two types that ever conformed to URLSessionTask, are unlikely to be able to conform to the protocol as written. So if they should ever need to conform to some protocol in the future you'd be either adding a new protocol or re-writing the existing one (e.g., the same place you are at now by just removing it in the first place).

In fact, after thinking through this I think the more prudent solution is to move the current implementation of playVinyl to it's only call-site and remove playVinyl entirely.

e.g.,

diff --git a/Vinyl/Turntable.swift b/Vinyl/Turntable.swift
index dce17b7..167884e 100644
--- a/Vinyl/Turntable.swift
+++ b/Vinyl/Turntable.swift
@@ -56,21 +56,6 @@ public final class Turntable: NSURLSession {

     // MARK: - Private methods

-    private func playVinyl(request request: NSURLRequest, fromData bodyData: NSData? = nil, completionHandler: RequestCompletionHandler) throws -> URLSessionUploadTask {
-
-        guard let player = player else {
-            fatalError("Did you forget to load the Vinyl? <U+1F3B6>")
-        }
-
-        let completion = try player.playTrack(forRequest: transformRequest(request, bodyData: bodyData))
-
-        return URLSessionUploadTask {
-            self.operationQueue.addOperationWithBlock {
-                completionHandler(completion.data, completion.response, completion.error)
-            }
-        }
-    }
-
     private func transformRequest(request: NSURLRequest, bodyData: NSData? = nil) -> NSURLRequest {
         guard let bodyData = bodyData else {
             return request
@@ -113,9 +98,18 @@ extension Turntable {
     }

     public override func uploadTaskWithRequest(request: NSURLRequest, fromData bodyData: NSData?, completionHandler: (NSData?, NSURLResponse?, NSError?) -> Void) -> NSURLSessionUploadTask {
+
+        guard let player = player else {
+            fatalError("Did you forget to load the Vinyl? <U+1F3B6>")
+        }

         do {
-            return try playVinyl(request: request, fromData: bodyData, completionHandler: completionHandler) as URLSessionUploadTask
+            let completion = try player.playTrack(forRequest: transformRequest(request, bodyData: bodyData))
+            return URLSessionUploadTask {
+                self.operationQueue.addOperationWithBlock {
+                    completionHandler(completion.data, completion.response, completion.error)
+                }
+            }
         }
         catch Error.TrackNotFound {
             errorHandler.handleTrackNotFound(request, playTracksUniquely: turntableConfiguration.playTracksUniquely)

[RuiAAPeres]

@RLovelett I left a couple of comments in your PR. cc @Sephiroth87

[RuiAAPeres]

ping @RLovelett @Sephiroth87

[Sephiroth87]

sessionDelegate instead of _delegate?

[Sephiroth87]

I'm not sure I agree with moving all the logic inside of the task, it's job should be just to handle receiving/sending data, but it shouldn't handle anything session related, definitely not its delegate directly.

Plus, to add delegate support to upload tasks would require duplicating all this logic again, since URLSessionUploadTask doesn't (and can't) subclass URLSessionDataTask