electrode-ios / elwebservice Goto Github PK

This will require a major version bump since it is a breaking change.

Handlers added with updateUI() are not called unless a response() or responseJSON() handler is added first.

As an example, this code seems reasonable:

self.status = .Sending
sendRequest()
    .updateUI { _ in
        self.status = .Processing
    }
    .response { data, response in
        // *snip*
    }
    .updateUI { _ in
        self.status = .Idle
    }

But, in practice the first updateUI() handler is not called. This makes sense, because there is no "value" yet, but it's also a little surprising, and it's difficult to debug because the handler is silently skipped.

In the objc interop code for ServiceTasks, there is a type-check before the updateUI block is called:

if let value = value as? AnyObject {
    handler(value)
}

If value is nil, the check fails and the block is not called. I think it should be value as? AnyObject?.

Do we really need ServiceTaskResult? It would be more consistent with Swift's error handling to use throw instead of returning .Failure as well as using an optional to return nil for empty results instead of .Empty.

We could start supporting the use of throw and optional returns in handlers before completely removing ServiceTaskResult which would ease the transition for API consumers.

This would have a minor impact on existing code and migrating to the new API would be very straight-forward.

Thoughts?

Overview

• Deprecate ServiceTaskResult API, remove in next major release.
• Update response handlers to use throw for raising errors instead of ServiceTaskResult.Failure(ErrorType).
• Update response handler to return an optional type of Any?
  • Handlers return nil for no value results instead of ServiceTaskResult.Empty.
  • Handlers directly return the value instead of wrapping it in .Value()

1. Remove .Failure in favor of throw

The older syntax with .Failure:

.response { data, response in
    // filter reseponses that do not respond with status 200

    if let httpResponse = response as? NSHTTPURLResponse
        where httpResponse.statusCode != 200 {
        return .Failure(ResponseError.ExpectedStatus200)
    }

    return .Empty
}

The new syntax with throw:

.response { data, response in
    // filter reseponses that do not respond with status 200

    guard let httpResponse = response as? NSHTTPURLResponse 
      where httpResponse.statusCode == 200 else {
        throw ResponseError.ExpectedStatus200
      }

    return nil
}

2. Remove .Empty in favor of nil

The older syntax with .Empty:

.response { data, response in
    return .Empty
}

The new syntax with nil:

.response { data, response in
    return nil
}

3. Directly return value instead of wrapping it in .Value

The old syntax using .Value:

.responseJSON { json, response in
    // decode JSON as an array of models

    if let models: [Brewer] = JSONDecoder<Brewer>.decode(json)  {
        // return valid model data to updateUI handler
        return .Value(models)
    } else {
        // any value conforming to ErrorType
        return .Failure(JSONDecoderError.FailedToDecodeBrewer)
    }
}

The new syntax using Any? as the return type:

.responseJSON { json, response in
    // decode JSON as an array of models

    guard let models: [Brewer] = JSONDecoder<Brewer>.decode(json) else {
      throw JSONDecoderError.FailedToDecodeBrewer
    }

    return models
}

ServiceTaskResult.Value does not support using struct values because of its AnyObject requirement. ServiceTaskResult will need to be refactored to support struct values.

As part of the work to separate the request API from ServiceTask, a new ServiceRequest struct type will be addded to replace for ServiceTask's requesting building API.

Currently ServiceTask API conflates request building with response processing:

service
  .POST("/brews")
  .setFormParameters(["foo": "bar"])
  .response { data, response in
    // process response
  }
  .resume()

With the new request API, ServiceRequest and ServiceTask values can be created independently for greater flexibility.

// create a request
var request = service.request(.post, "/brews")
request.formParameters = ["foo": "bar"]

// spawn a task instance for sending the request and processing the response
service
  .task(request: request)
  .response { data, response in
    // process response
  }
  .resume()

My Xcode Version is: 7.2(7C68)
My Carthage version is: 0.11.0
And Carthage build output is the following:

2016-01-12 10:19:48.655 xcodebuild[46513:1904034] [MT] PluginLoading: Required plug-in compatibility UUID F41BD31E-2683-44B8-AE7F-5F09E919790E for plug-in at path '/Library/Application Support/Developer/Shared/Xcode/Plug-ins/VVDocumenter-Xcode.xcplugin' not present in DVTPlugInCompatibilityUUIDs
2016-01-12 10:19:48.656 xcodebuild[46513:1904034] [MT] PluginLoading: Required plug-in compatibility UUID F41BD31E-2683-44B8-AE7F-5F09E919790E for plug-in at path '/Library/Application Support/Developer/Shared/Xcode/Plug-ins/Unity4XC.xcplugin' not present in DVTPlugInCompatibilityUUIDs
2016-01-12 10:19:48.656 xcodebuild[46513:1904034] Failed to load plugin at: /Users/mty/Library/Application Support/Developer/Shared/Xcode/Plug-ins/Unity4XC.xcplugin, skipping. Reason for failure: *** -[__NSPlaceholderDictionary initWithObjects:forKeys:count:]: attempt to insert nil object from objects[0]
** BUILD FAILED **

The following build commands failed:
CompileSwift normal arm64 /Users/mty/XcodeProjects/CourseVideo/Carthage/Checkouts/Swallow/THGWebServiceTests/WebServiceTests.swift
CompileSwift normal arm64 /Users/mty/XcodeProjects/CourseVideo/Carthage/Checkouts/Swallow/THGWebServiceTests/RequestTests.swift
CompileSwiftSources normal arm64 com.apple.xcode.tools.swift.compiler
(3 failures)
/Users/mty/XcodeProjects/CourseVideo/Carthage/Checkouts/Swallow/THGWebServiceTests/RequestTests.swift:10:18: error: module 'THGWebService' was not compiled for testing
/Users/mty/XcodeProjects/CourseVideo/Carthage/Checkouts/Swallow/THGWebServiceTests/RequestTests.swift:10:18: error: module 'THGWebService' was not compiled for testing
A shell task failed with exit code 65:
2016-01-12 10:19:48.655 xcodebuild[46513:1904034] [MT] PluginLoading: Required plug-in compatibility UUID F41BD31E-2683-44B8-AE7F-5F09E919790E for plug-in at path '/Library/Application Support/Developer/Shared/Xcode/Plug-ins/VVDocumenter-Xcode.xcplugin' not present in DVTPlugInCompatibilityUUIDs
2016-01-12 10:19:48.656 xcodebuild[46513:1904034] [MT] PluginLoading: Required plug-in compatibility UUID F41BD31E-2683-44B8-AE7F-5F09E919790E for plug-in at path '/Library/Application Support/Developer/Shared/Xcode/Plug-ins/Unity4XC.xcplugin' not present in DVTPlugInCompatibilityUUIDs
2016-01-12 10:19:48.656 xcodebuild[46513:1904034] Failed to load plugin at: /Users/mty/Library/Application Support/Developer/Shared/Xcode/Plug-ins/Unity4XC.xcplugin, skipping. Reason for failure: *** -[__NSPlaceholderDictionary initWithObjects:forKeys:count:]: attempt to insert nil object from objects[0]
** BUILD FAILED **

The following build commands failed:
CompileSwift normal arm64 /Users/mty/XcodeProjects/CourseVideo/Carthage/Checkouts/Swallow/THGWebServiceTests/WebServiceTests.swift
CompileSwift normal arm64 /Users/mty/XcodeProjects/CourseVideo/Carthage/Checkouts/Swallow/THGWebServiceTests/RequestTests.swift
CompileSwiftSources normal arm64 com.apple.xcode.tools.swift.compiler
(3 failures)

The methods updateUI() and updateErrorUI() allow handlers to be added that run on the main thread. This is really nice. The current implementation does not block the task's operation queue while the UI update code runs, which allows other operations to run simultaneously. This is not so nice.

For example, consider this code:

class Brewer {
    var name: String
}

service.GET("/brewer/\(id)")
    .response { data, _ in
        let brewer: Brewer = // decode data…
        return .Value(brewer)
    }
    .updateUI { value in
        let brewer = value as! Brewer
        self.label.text = brewer.name // <-- read
    }
    .transform { value in
        let brewer = value as! Brewer
        brewer.name = "uh oh" // <-- write
    }

This is a contrived example, but the point is that this code is not thread safe. The Brewer instance might be read from the main thread (in the updateUI() handler) while it is simultaneously being mutated from a background thread (in the transform() handler). It seems reasonable to expect all handlers to be called sequentially.

tl;dr

Maybe the ServiceTask request-building methods should be separated from the response-processing methods.

Discussion

The ServiceTask is used both for setting up a request, and for pausing/resuming/processing the response. In practice, it's not very common to want to expose both APIs. It can lead to confusing situations like this:

class BrewClientAPI {
    func fetchAllBrewers() -> ServiceTask {
        return webService
            .GET("/brews")
            .setHeaders(someHeaders)
            .resume()
    }
}

// Elsewhere:

brewClientAPI.fetchAllBrewers()
    .setHeaderValue(specialAuthSecret, forName: SpecialAuthHeader) // <-- Ruh roh!
    .updateUI { /* and so on… */ }

The "special auth header" will not be sent to the server because a snapshot of the request was created when resume() was called. But the calling code has no good indication that the request can no longer be modified.

Here's another one:

class BrewClientAPI {
    func fetchAllBrewers() -> ServiceTask {
        return webService.GET("/brews") // <-- Oops, forgot to call resume()!
    }
}

// Elsewhere:

brewClientAPI.fetchAllBrewers().updateUI { /* and so on… */ }

This service call is never sent and there's no obvious indication why.

Proposed Changes

As part of the breaking changes for version 4, I'd like to propose removing the request-building methods from ServiceTask. Since these methods all just call through to the underlying Request instance, we could just expose the request directly instead. The API changes might look something like this:

extension WebService {
    func GET(path: String) -> Request {
        return Request(method, url: absoluteURLString(path))
    }
}

struct Request {
    func send(session: Session) -> ServiceTask {
        return ServiceTask(request: self, session: session)
    }
}

And here's a usage example:

class BrewClientAPI {
    func fetchAllBrewers() -> ServiceTask {
        return webService
            .GET("/brews")
            .setHeaders(someHeaders)
            .send(webService)
    }
}

// Elsewhere:

brewClientAPI.fetchAllBrewers()
    .setHeaderValue(specialAuthSecret, forName: SpecialAuthHeader) // <-- compiler error, no such method
    .updateUI { /* and so on… */ }

And another:

class BrewClientAPI {
    func fetchAllBrewers() -> ServiceTask {
        return webService.GET("/brews") // <-- compiler error, cannot convert Request to ServiceTask
    }
}

Unanswered questions

I'm not sure how the passthrough delegate should be assigned in this scenario. One thought is that the ServiceTask could check whether its session is a WebService, and actively pull the delegate from it.

I don't love the session parameter to send(), although it's also kind of cool how it allows you to save off a request and send it multiple times (if that's something you're in to).

It seems URLComponents is failing to properly URL-encode the + character in query values. As stated in RFC3986, the + character is a reserved character and therefore should be percent-encoded as "%2B".

let item = URLQueryItem(name: "plus", value: "time+date")
var components = URLComponents(string: "")
components?.queryItems = [item]

// prints "percentEncodedQuery: plus=time+date"
print("percentEncodedQuery: \(components!.percentEncodedQuery!)") 

We should expect URLComponents to handle URL-encoding the parameter value since it encodes it for other reserved characters like =:

let item = URLQueryItem(name: "equals", value: "time=date")
var components = URLComponents(string: "")
components?.queryItems = [item]

// prints "percentEncodedQuery: equals=time%3Ddate"
print("percentEncodedQuery: \(components!.percentEncodedQuery!)")

and &:

let item = URLQueryItem(name: "ampersand", value: "time&date")
var components = URLComponents(string: "")
components?.queryItems = [item]

// prints "percentEncodedQuery: ampersand=time%26date"
print("percentEncodedQuery: \(components!.percentEncodedQuery!)")

SwallowExample.xcodeproj needs to be updated to work with the latest API changes

add aServiceTask response handler named updateUI() that will run on the main queue

Swift 3 support will be introduced in ELWebService 4.0. See the v4 roadmap for more info.

Can you please let me know if this has support for image upload with some parameter .?

The public API that ELWebService exposes does not bridge over to Objective-C which means networking calls cannot be made directly from Objective-C code.

The main issue is that ServiceTaskResult cannot be represented in Obj-C because it is defined as an enumeration with associated values. The associated values are used to encapsulate the data that gets passed through the response handler chain.

An example of using ServiceTaskResult to pass values through response handler chains:

service
    .GET("/brewers")
    .responseJSON { json in
      if let models: [Brewer] = JSONDecoder<Brewer>.decode(json)  {
          // pass encoded value via ServiceTaskResult
          return .Value(models)
      } else {
        // any value conforming to ErrorType
        return .Failure(JSONDecoderError.FailedToDecodeBrewer) 
      }
    }
    .updateUI { value in
        if let brewers = value as? [Brewer] {
            // update some UI with brewer models
        }
    }
    .resume()

add aServiceTask response handler named updateErrorUI() that will run on the main queue

The current implementation does not conveniently support setting URL query parameters separately from request body parameters. This is because of API behavior where request parameters are set in the URL for GET and DELETE requests but are set in the body for all other HTTP methods. There are cases where you need to set the URL query parameters and body data independently from the HTTP method-based behavior.

This behavior was implemented to follow RESTful HTTP client conventions but I think deprecating this behavior and adding separate URL and body parameter methods makes the API more flexible and makes the intent of the code much clearer.

I suggest the following changes:

1. Deprecate setParameters(parameters:encoding:) and setParameterEncoding(encoding) methods. The API would be removed in the next major release.
2. Add methods that enable setting URL query parameters and request body parameters separately.

Add setQueryParameters() method for setting URL query parameters.

This new method would enable your code to set the URL query parameters seperately from the request body data.

service
    .POST("/brewers")
    .setQueryParameters(["included-fields": "all"]) // set URL query params
    .setJSON(["name": "Trashboat Brewing"])         // set JSON body

The resulting HTTP request would look like:

POST /brewers?included-fields=all HTTP/1.1
Content-Type: application/json

{"name": "Trashboat Brewing"}

Add setPercentEncodedBodyParameters() method for setting parameters as percent-encoded data in the request body.

service
    .POST("/brewers")
    .setQueryParameters(["included-fields": "all"])                 // set URL params
    .setPercentEncodedBodyParameters(["name": "Trashboat Brewing"]) // set body params

The resulting HTTP request would look like:

POST /brewers?included-fields=all HTTP/1.1
Content-Length: 55

name=Trashboat%20Brewing

One thing I am seeing in swallow usage is people baking base urls into their service extension.

This can work, but we likely want to share base extensions across services that use the same base extension. Is there anything Swallow should do to encourage this, or is this left to the team utilizing Swallow?

Currently the ServiceTask API uses Any? to pass handler result values but this requires the updateUI handler to conditionally cast the Any? type to the actual type of the expected value which is a bit of a burden. Ideally the handler should be able to infer the result type from the context.

A new API could enable custom handlers like:

extension ServiceTask {
    func responseAs<T where T: ModelDecodable>(handler: ([T]) -> Void) ->  ModelTask<[T]> {
        return responseJSON { (json, response) -> [T]? in
            guard let json = self.jsonObject(json, forKey: "brews"),
                let jsonArray = json as? [AnyObject],
                let decodedArray = ModelDecoder<T>.decodeArray(jsonArray) else {
                throw ServiceTaskDecodeError.FailedToDecodeJSONArray
            }

            return decodedArray

            }.updateUI { (brews: [T]?) in
                guard let brews = brews else { return }
                handler(brews)
            }
        }
}

That could enable your code to make service calls like:

brewClient
    .fetchAllBrews()
    .responseAs { [weak self] brews in
        // self?.brews is of type `[Brew]?` and the type is inferred, no need to cast
        self?.brews = brews
    }
    .error { error in
        print("I AM ERROR = \(error)")
    }
    .resume()

the readme and programming guide both need to be updated to properly document the newer 2.0 API

iOS 10 introduced a new API for gathering performance metrics of HTTP requests, NSURLSessionTaskMetrics. The API will provide more accurate and granular performance metrics than our own instrumentation exposes through ServicePassthroughDelegate.

I would like to add a new metrics API that wraps NSURLSessionTaskMetrics and falls back to our own instrumentation when running on pre iOS 10.

WebService(baseURLString: "https://somehapi.herokuapp.com")
.setHeaders(["beer-client" : "iOS"])
.setParameters(["zip" : "15217"], encoding: .JSON)
.setBody(someData, encoding: .JSON)
.GET("/stores")

set* methods are typed such that only specific types are allowed and "AnyObject" is not.

currently response handlers are running on the main queue by default.