Skip to content

Refactor deconstructURL and scheme parsing #504

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

Merged
merged 11 commits into from
Dec 1, 2021
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 3 additions & 42 deletions Sources/AsyncHTTPClient/ConnectionPool.swift
Original file line number Diff line number Diff line change
Expand Up @@ -19,57 +19,18 @@ enum ConnectionPool {
/// used by the `providers` dictionary to allow retrieving and creating
/// connection providers associated to a certain request in constant time.
struct Key: Hashable, CustomStringConvertible {
var scheme: Scheme
var scheme: DeconstructedURL.Scheme
var connectionTarget: ConnectionTarget
private var tlsConfiguration: BestEffortHashableTLSConfiguration?

init(_ request: HTTPClient.Request) {
self.connectionTarget = request.connectionTarget
switch request.scheme {
case "http":
self.scheme = .http
case "https":
self.scheme = .https
case "unix":
self.scheme = .unix
case "http+unix":
self.scheme = .http_unix
case "https+unix":
self.scheme = .https_unix
default:
fatalError("HTTPClient.Request scheme should already be a valid one")
}
self.scheme = request.deconstructedURL.scheme
self.connectionTarget = request.deconstructedURL.connectionTarget
if let tls = request.tlsConfiguration {
self.tlsConfiguration = BestEffortHashableTLSConfiguration(wrapping: tls)
}
}

enum Scheme: Hashable {
case http
case https
case unix
case http_unix
case https_unix

var requiresTLS: Bool {
switch self {
case .https, .https_unix:
return true
default:
return false
}
}
}

/// Returns a key-specific `HTTPClient.Configuration` by overriding the properties of `base`
func config(overriding base: HTTPClient.Configuration) -> HTTPClient.Configuration {
var config = base
if let tlsConfiguration = self.tlsConfiguration {
config.tlsConfiguration = tlsConfiguration.base
}
return config
}

var description: String {
var hasher = Hasher()
self.tlsConfiguration?.hash(into: &hasher)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -185,9 +185,9 @@ extension HTTPConnectionPool.ConnectionFactory {
logger: Logger
) -> EventLoopFuture<NegotiatedProtocol> {
switch self.key.scheme {
case .http, .http_unix, .unix:
case .http, .httpUnix, .unix:
return self.makePlainChannel(deadline: deadline, eventLoop: eventLoop).map { .http1_1($0) }
case .https, .https_unix:
case .https, .httpsUnix:
return self.makeTLSChannel(deadline: deadline, eventLoop: eventLoop, logger: logger).flatMapThrowing {
channel, negotiated in

Expand All @@ -197,7 +197,7 @@ extension HTTPConnectionPool.ConnectionFactory {
}

private func makePlainChannel(deadline: NIODeadline, eventLoop: EventLoop) -> EventLoopFuture<Channel> {
precondition(!self.key.scheme.requiresTLS, "Unexpected scheme")
precondition(!self.key.scheme.useTLS, "Unexpected scheme")
return self.makePlainBootstrap(deadline: deadline, eventLoop: eventLoop).connect(target: self.key.connectionTarget)
}

Expand Down Expand Up @@ -283,7 +283,7 @@ extension HTTPConnectionPool.ConnectionFactory {
logger: Logger
) -> EventLoopFuture<NegotiatedProtocol> {
switch self.key.scheme {
case .unix, .http_unix, .https_unix:
case .unix, .httpUnix, .httpsUnix:
preconditionFailure("Unexpected scheme. Not supported for proxy!")
case .http:
return channel.eventLoop.makeSucceededFuture(.http1_1(channel))
Expand Down Expand Up @@ -356,7 +356,7 @@ extension HTTPConnectionPool.ConnectionFactory {
}

private func makeTLSChannel(deadline: NIODeadline, eventLoop: EventLoop, logger: Logger) -> EventLoopFuture<(Channel, String?)> {
precondition(self.key.scheme.requiresTLS, "Unexpected scheme")
precondition(self.key.scheme.useTLS, "Unexpected scheme")
let bootstrapFuture = self.makeTLSBootstrap(
deadline: deadline,
eventLoop: eventLoop,
Expand Down Expand Up @@ -470,12 +470,12 @@ extension HTTPConnectionPool.ConnectionFactory {
}
}

extension ConnectionPool.Key.Scheme {
extension DeconstructedURL.Scheme {
var isProxyable: Bool {
switch self {
case .http, .https:
return true
case .unix, .http_unix, .https_unix:
case .unix, .httpUnix, .httpsUnix:
return false
}
}
Expand Down
99 changes: 99 additions & 0 deletions Sources/AsyncHTTPClient/DeconstructedURL.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
//===----------------------------------------------------------------------===//
//
// This source file is part of the AsyncHTTPClient open source project
//
// Copyright (c) 2021 Apple Inc. and the AsyncHTTPClient project authors
// Licensed under Apache License v2.0
//
// See LICENSE.txt for license information
// See CONTRIBUTORS.txt for the list of AsyncHTTPClient project authors
//
// SPDX-License-Identifier: Apache-2.0
//
//===----------------------------------------------------------------------===//

import Foundation
Copy link
Member

Choose a reason for hiding this comment

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

NIT: Only import what is actually needed.

Suggested change
import Foundation
import struct Foundation.URL


struct DeconstructedURL {
enum Scheme: String {
case http
case https
case unix
case httpUnix = "http+unix"
case httpsUnix = "https+unix"
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this should be namespaced in DeconstructedURL. Would you mind moving this out?


var scheme: Scheme
var connectionTarget: ConnectionTarget
var uri: String

init(
scheme: DeconstructedURL.Scheme,
connectionTarget: ConnectionTarget,
uri: String
) {
self.scheme = scheme
self.connectionTarget = connectionTarget
self.uri = uri
}
}

extension DeconstructedURL {
init(url: URL) throws {
guard let schemeString = url.scheme else {
throw HTTPClientError.emptyScheme
}
guard let scheme = Scheme(rawValue: schemeString.lowercased()) else {
throw HTTPClientError.unsupportedScheme(schemeString)
}

switch scheme {
case .http, .https:
guard let host = url.host, !host.isEmpty else {
throw HTTPClientError.emptyHost
}
self.init(
scheme: scheme,
connectionTarget: .init(remoteHost: host, port: url.port ?? scheme.defaultPort),
uri: url.uri
)

case .httpUnix, .httpsUnix:
guard let socketPath = url.host, !socketPath.isEmpty else {
throw HTTPClientError.missingSocketPath
}
self.init(
scheme: scheme,
connectionTarget: .unixSocket(path: socketPath),
uri: url.uri
)

case .unix:
let socketPath = url.baseURL?.path ?? url.path
let uri = url.baseURL != nil ? url.uri : "/"
guard !socketPath.isEmpty else {
throw HTTPClientError.missingSocketPath
}
self.init(
scheme: scheme,
connectionTarget: .unixSocket(path: socketPath),
uri: uri
)
}
}
}

extension DeconstructedURL.Scheme {
var useTLS: Bool {
Copy link
Member

Choose a reason for hiding this comment

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

He, she, it das s muss mit ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We will diverge from the public API on HTTPClient.Request but I’m okay with that

switch self {
case .http, .httpUnix, .unix:
return false
case .https, .httpsUnix:
return true
}
}

var defaultPort: Int {
self.useTLS ? 443 : 80
}
}
Loading