Skip to content

BREAKING: Re-implement SwiftyRequest on NIO async-http-client#73

Merged
djones6 merged 28 commits intomasterfrom
issue.nio
Sep 27, 2019
Merged

BREAKING: Re-implement SwiftyRequest on NIO async-http-client#73
djones6 merged 28 commits intomasterfrom
issue.nio

Conversation

@djones6
Copy link
Copy Markdown
Contributor

@djones6 djones6 commented Sep 12, 2019

This PR re-implements the SwiftyRequest API on top of the Swift-NIO based async-http-client, and adds client certificate support.

This is based on work done by @Andrew-Lees11 described in #58, including:

  • All JSON code has been removed and replaced with Codable
  • HTTPMethod.swift has been removed and replaced with the NIOHTTP1.HTTPMethod
    • update: HTTPMethod remains, and is now convertible to/from NIOHTTP1.HTTPMethod.
  • headerParameters has changed from a [String:String] to HTTPHeaders
    • update: headerParameters remains [String:String] with mapping under the covers
  • response() now returns Result<HTTPClient.Response, RestError> instead of (Data?, HTTPURLResponse?, Error?)
  • RestResponse now represents an HTTPClient.Response that is generic over the body instead of an HTTPURLResponse with the result of the body.
  • Credentials input has been converted to be an extensible struct.
  • RestError now represents any error that could be returned from a request attempt, including errors relating to malformed URLs or HTTPClientErrors. So strictly speaking, it's not just a REST error, but I'd prefer to retain the naming.
    • RestError now has a convenience function for retrieving the response body data when the error represents an unsuccessful request.
  • RestRequest.init parameter containsSelfSignedCert has been renamed to insecure to more accurately reflect what it means, though I've retained (and deprecated) the current naming.

In addition, I have implemented Client Certificate support for 2-way SSL. This is non-functional in SwiftyRequest 2.x. This initial support provides a convenience API for consuming a certificate (and its private key) from a PEM-formatted file, String, Data or [UInt8]. There is also support for supplying the raw NIOSSLCertificate and NIOSSLPrivateKey which allows complete freedom of the data sources, if required. We could add additional convenience APIs for other popular file formats in the future.

As well as refactoring the existing tests for the new API, I've added a Client Certificate test that downloads a valid certificate from https://badssl.com/download/ and then presents it to https://client.badssl.com. The certificate must be downloaded by the test because it may be reissued as it expires. The password for the certificate's key is hard-coded into the test - I'm assuming that it will not change over time.

Effect of breaking API changes

Kitura/KituraKit#53 demonstrates the extent of changes required to an application to migrate to the new API.

Motivation and Context

Resolves #58 and #57

Andrew-Lees11 and others added 6 commits September 10, 2019 11:29
Remove JSON

Remove commented out lines

Add check for status code

Update streaming and Body for new API

Attach request to response

Move handle invocation to be a closure

Update nio-http name

Update import statements

Remove extra tests from AllTests
Create a MutableRequest that can vend the (immutable) HTTPClient.Request

Move templating and queryparam logic into MutableRequest
@ianpartridge
Copy link
Copy Markdown
Contributor

FYI @artemredkin @weissi

@weissi
Copy link
Copy Markdown

weissi commented Sep 12, 2019

awesome!

@djones6
Copy link
Copy Markdown
Contributor Author

djones6 commented Sep 12, 2019

@ianpartridge @weissi This currently builds against 1.0.0-alpha.3 but I expect it'll break as more breaking changes are tagged. To avoid this we could hard-code a .exact version, or wait until 1.0.0 is ready. What do you think?

I'd be inclined to do the former, as updating to newer releases of the client should be straightforward. However, we do expose the HTTPClient.Request and HTTPClient.Cookie types as part of RestResponse, so those might change (though I'd imagine, not significantly).

@weissi
Copy link
Copy Markdown

weissi commented Sep 12, 2019

@djones6 I think that is sensible. I would like if we tried to slow down the breakages but I can understand if you want to .exact pin it until it reaches 1.0.0

/// request.headerParameters = HTTPHeaders([("Cookie", "v1")])
/// ```
public var headerParameters: [String: String] {
public var headerParameters: HTTPHeaders {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Provide existing API with mapping to HTTPHeaders

completionHandler(dataResponse)
return
}
completionHandler: @escaping (Result<RestResponse<Data>, Error>) -> Void) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Make result return RestError rather than generic Error

@djones6
Copy link
Copy Markdown
Contributor Author

djones6 commented Sep 25, 2019

@ianpartridge Example of changes required to KituraKit to adopt the new API: Kitura/KituraKit@53ea6e2

@weissi
Copy link
Copy Markdown

weissi commented Sep 25, 2019

@djones6 btw, beta 4 landed earlier. It should be API compatible with beta 3, although there are some deprecations (with very straightforward fixes)

@djones6 djones6 added the jazzy-doc When applied to a PR, instructs Package-Builder to regenerate the Jazzy documentation label Sep 27, 2019
@djones6 djones6 merged commit 25a59e0 into master Sep 27, 2019
@djones6 djones6 deleted the issue.nio branch September 27, 2019 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

jazzy-doc When applied to a PR, instructs Package-Builder to regenerate the Jazzy documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reimplement on swift-nio-http-client

5 participants