Skip to content

Commit bccb075

Browse files
fabianfettLukasa
fabianfett
and
Lukasa
authored
Refactor Request Validation (#391)
- Refactored code from `TaskHandler` into new method `HTTPClient.Request.createRequestHead` that creates an `HTTPRequestHead` and matching `RequestFramingMetadata` - Added required property `requestFramingMetadata` to `HTTPExecutingRequest` - Added property `requestFramingMetadata` to `RequestBag` Co-authored-by: Cory Benfield <lukasa@apple.com>
1 parent a9aee26 commit bccb075

File tree

8 files changed

+160
-72
lines changed

8 files changed

+160
-72
lines changed

‎Sources/AsyncHTTPClient/ConnectionPool/HTTPExecutingRequest.swift‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -189,10 +189,18 @@ protocol HTTPRequestExecutor {
189189
protocol HTTPExecutingRequest: AnyObject {
190190
/// The request's head.
191191
///
192-
/// Based on the content of the request head the task executor will call `startRequestBodyStream`
193-
/// after `requestHeadSent` was called.
192+
/// The HTTP request head, that shall be sent. The HTTPRequestExecutor **will not** run any validation
193+
/// check on the request head. All necessary metadata about the request head the executor expects in
194+
/// the ``requestFramingMetadata``.
194195
var requestHead: HTTPRequestHead { get }
195196

197+
/// The request's framing metadata.
198+
///
199+
/// The request framing metadata that is derived from the ``requestHead``. Based on the content of the
200+
/// request framing metadata the executor will call ``startRequestBodyStream`` after
201+
/// ``requestHeadSent``.
202+
var requestFramingMetadata: RequestFramingMetadata { get }
203+
196204
/// The maximal `TimeAmount` that is allowed to pass between `channelRead`s from the Channel.
197205
var idleReadTimeout: TimeAmount? { get }
198206

‎Sources/AsyncHTTPClient/ConnectionPool/RequestFramingMetadata.swift‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,8 @@
1212
//
1313
//===----------------------------------------------------------------------===//
1414

15-
struct RequestFramingMetadata {
16-
enum Body {
15+
struct RequestFramingMetadata: Hashable {
16+
enum Body: Hashable {
1717
case none
1818
case stream
1919
case fixedSize(Int)

‎Sources/AsyncHTTPClient/HTTPHandler.swift‎

Lines changed: 31 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,28 @@ extension HTTPClient {
293293
public var port: Int {
294294
return self.url.port ?? (self.useTLS ? 443 : 80)
295295
}
296+
297+
func createRequestHead() throws -> (HTTPRequestHead, RequestFramingMetadata) {
298+
var head = HTTPRequestHead(
299+
version: .http1_1,
300+
method: self.method,
301+
uri: self.uri,
302+
headers: self.headers
303+
)
304+
305+
if !head.headers.contains(name: "host") {
306+
let port = self.port
307+
var host = self.host
308+
if !(port == 80 && self.scheme == "http"), !(port == 443 && self.scheme == "https") {
309+
host += ":\(port)"
310+
}
311+
head.headers.add(name: "host", value: host)
312+
}
313+
314+
let metadata = try head.headers.validate(method: self.method, body: self.body)
315+
316+
return (head, metadata)
317+
}
296318
}
297319

298320
/// Represent HTTP response.
@@ -877,46 +899,29 @@ extension TaskHandler: ChannelDuplexHandler {
877899
typealias OutboundOut = HTTPClientRequestPart
878900

879901
func write(context: ChannelHandlerContext, data: NIOAny, promise: EventLoopPromise<Void>?) {
880-
self.state = .sendingBodyWaitingResponseHead
881-
882902
let request = self.unwrapOutboundIn(data)
903+
self.state = .sendingBodyWaitingResponseHead
883904

884-
var head = HTTPRequestHead(version: HTTPVersion(major: 1, minor: 1),
885-
method: request.method,
886-
uri: request.uri)
887-
var headers = request.headers
888-
889-
if !request.headers.contains(name: "host") {
890-
let port = request.port
891-
var host = request.host
892-
if !(port == 80 && request.scheme == "http"), !(port == 443 && request.scheme == "https") {
893-
host += ":\(port)"
894-
}
895-
headers.add(name: "host", value: host)
896-
}
905+
let head: HTTPRequestHead
906+
let metadata: RequestFramingMetadata
897907

898908
do {
899-
try headers.validate(method: request.method, body: request.body)
909+
(head, metadata) = try request.createRequestHead()
900910
} catch {
901911
self.errorCaught(context: context, error: error)
902912
promise?.fail(error)
903913
return
904914
}
905915

906-
head.headers = headers
907-
908-
if head.headers[canonicalForm: "connection"].map({ $0.lowercased() }).contains("close") {
909-
self.closing = true
910-
}
911916
// This assert can go away when (if ever!) the above `if` correctly handles other HTTP versions. For example
912917
// in HTTP/1.0, we need to treat the absence of a 'connection: keep-alive' as a close too.
913-
assert(head.version == HTTPVersion(major: 1, minor: 1),
918+
assert(head.version == .http1_1,
914919
"Sending a request in HTTP version \(head.version) which is unsupported by the above `if`")
915920

916-
let contentLengths = head.headers[canonicalForm: "content-length"]
917-
assert(contentLengths.count <= 1)
918-
919-
self.expectedBodyLength = contentLengths.first.flatMap { Int($0) }
921+
if case .fixedSize(let length) = metadata.body {
922+
self.expectedBodyLength = length
923+
}
924+
self.closing = metadata.connectionClose
920925

921926
context.write(wrapOutboundOut(.head(head))).map {
922927
self.callOutToDelegateFireAndForget(value: head, self.delegate.didSendRequestHead)

‎Sources/AsyncHTTPClient/RequestBag.swift‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ final class RequestBag<Delegate: HTTPClientResponseDelegate> {
4141
let idleReadTimeout: TimeAmount?
4242

4343
let requestHead: HTTPRequestHead
44+
let requestFramingMetadata: RequestFramingMetadata
4445

4546
let eventLoopPreference: HTTPClient.EventLoopPreference
4647

@@ -50,7 +51,7 @@ final class RequestBag<Delegate: HTTPClientResponseDelegate> {
5051
redirectHandler: RedirectHandler<Delegate.Response>?,
5152
connectionDeadline: NIODeadline,
5253
idleReadTimeout: TimeAmount?,
53-
delegate: Delegate) {
54+
delegate: Delegate) throws {
5455
self.eventLoopPreference = eventLoopPreference
5556
self.task = task
5657
self.state = .init(redirectHandler: redirectHandler)
@@ -59,12 +60,9 @@ final class RequestBag<Delegate: HTTPClientResponseDelegate> {
5960
self.idleReadTimeout = idleReadTimeout
6061
self.delegate = delegate
6162

62-
self.requestHead = HTTPRequestHead(
63-
version: .http1_1,
64-
method: request.method,
65-
uri: request.uri,
66-
headers: request.headers
67-
)
63+
let (head, metadata) = try request.createRequestHead()
64+
self.requestHead = head
65+
self.requestFramingMetadata = metadata
6866

6967
// TODO: comment in once we switch to using the Request bag in AHC
7068
// self.task.taskDelegate = self

‎Sources/AsyncHTTPClient/RequestValidation.swift‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,13 @@ import NIO
1616
import NIOHTTP1
1717

1818
extension HTTPHeaders {
19-
mutating func validate(method: HTTPMethod, body: HTTPClient.Body?) throws {
19+
mutating func validate(method: HTTPMethod, body: HTTPClient.Body?) throws -> RequestFramingMetadata {
20+
var metadata = RequestFramingMetadata(connectionClose: false, body: .none)
21+
22+
if self[canonicalForm: "connection"].lazy.map({ $0.lowercased() }).contains("close") {
23+
metadata.connectionClose = true
24+
}
25+
2026
// validate transfer encoding and content length (https://tools.ietf.org/html/rfc7230#section-3.3.1)
2127
if self.contains(name: "Transfer-Encoding"), self.contains(name: "Content-Length") {
2228
throw HTTPClientError.incompatibleHeaders
@@ -43,13 +49,13 @@ extension HTTPHeaders {
4349
// A user agent SHOULD NOT send a Content-Length header field when the request
4450
// message does not contain a payload body and the method semantics do not
4551
// anticipate such a body.
46-
return
52+
return metadata
4753
default:
4854
// A user agent SHOULD send a Content-Length in a request message when
4955
// no Transfer-Encoding is sent and the request method defines a meaning
5056
// for an enclosed payload body.
5157
self.add(name: "Content-Length", value: "0")
52-
return
58+
return metadata
5359
}
5460
}
5561

@@ -85,14 +91,18 @@ extension HTTPHeaders {
8591
// add headers if required
8692
if let enc = transferEncoding {
8793
self.add(name: "Transfer-Encoding", value: enc)
94+
metadata.body = .stream
8895
} else if let length = contentLength {
8996
// A sender MUST NOT send a Content-Length header field in any message
9097
// that contains a Transfer-Encoding header field.
9198
self.add(name: "Content-Length", value: String(length))
99+
metadata.body = .fixedSize(length)
92100
}
101+
102+
return metadata
93103
}
94104

95-
func validateFieldNames() throws {
105+
private func validateFieldNames() throws {
96106
let invalidFieldNames = self.compactMap { (name, _) -> String? in
97107
let satisfy = name.utf8.allSatisfy { (char) -> Bool in
98108
switch char {

‎Tests/AsyncHTTPClientTests/RequestBagTests.swift‎

Lines changed: 26 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -53,17 +53,20 @@ final class RequestBagTests: XCTestCase {
5353
var maybeRequest: HTTPClient.Request?
5454
XCTAssertNoThrow(maybeRequest = try HTTPClient.Request(url: "https://swift.org", method: .POST, body: requestBody))
5555
guard let request = maybeRequest else { return XCTFail("Expected to have a request") }
56-
5756
let delegate = UploadCountingDelegate(eventLoop: embeddedEventLoop)
58-
let bag = RequestBag(
57+
58+
var maybeRequestBag: RequestBag<UploadCountingDelegate>?
59+
XCTAssertNoThrow(maybeRequestBag = try RequestBag(
5960
request: request,
6061
eventLoopPreference: .delegate(on: embeddedEventLoop),
6162
task: .init(eventLoop: embeddedEventLoop, logger: logger),
6263
redirectHandler: nil,
6364
connectionDeadline: .now() + .seconds(30),
6465
idleReadTimeout: nil,
6566
delegate: delegate
66-
)
67+
))
68+
guard let bag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag.") }
69+
6770
XCTAssert(bag.task.eventLoop === embeddedEventLoop)
6871

6972
let executor = MockRequestExecutor(pauseRequestBodyPartStreamAfterASingleWrite: true)
@@ -161,15 +164,17 @@ final class RequestBagTests: XCTestCase {
161164
guard let request = maybeRequest else { return XCTFail("Expected to have a request") }
162165

163166
let delegate = UploadCountingDelegate(eventLoop: embeddedEventLoop)
164-
let bag = RequestBag(
167+
var maybeRequestBag: RequestBag<UploadCountingDelegate>?
168+
XCTAssertNoThrow(maybeRequestBag = try RequestBag(
165169
request: request,
166170
eventLoopPreference: .delegate(on: embeddedEventLoop),
167171
task: .init(eventLoop: embeddedEventLoop, logger: logger),
168172
redirectHandler: nil,
169173
connectionDeadline: .now() + .seconds(30),
170174
idleReadTimeout: nil,
171175
delegate: delegate
172-
)
176+
))
177+
guard let bag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag.") }
173178
XCTAssert(bag.task.eventLoop === embeddedEventLoop)
174179

175180
let executor = MockRequestExecutor()
@@ -202,15 +207,17 @@ final class RequestBagTests: XCTestCase {
202207
guard let request = maybeRequest else { return XCTFail("Expected to have a request") }
203208

204209
let delegate = UploadCountingDelegate(eventLoop: embeddedEventLoop)
205-
let bag = RequestBag(
210+
var maybeRequestBag: RequestBag<UploadCountingDelegate>?
211+
XCTAssertNoThrow(maybeRequestBag = try RequestBag(
206212
request: request,
207213
eventLoopPreference: .delegate(on: embeddedEventLoop),
208214
task: .init(eventLoop: embeddedEventLoop, logger: logger),
209215
redirectHandler: nil,
210216
connectionDeadline: .now() + .seconds(30),
211217
idleReadTimeout: nil,
212218
delegate: delegate
213-
)
219+
))
220+
guard let bag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag.") }
214221
XCTAssert(bag.eventLoop === embeddedEventLoop)
215222

216223
let executor = MockRequestExecutor()
@@ -233,15 +240,17 @@ final class RequestBagTests: XCTestCase {
233240
guard let request = maybeRequest else { return XCTFail("Expected to have a request") }
234241

235242
let delegate = UploadCountingDelegate(eventLoop: embeddedEventLoop)
236-
let bag = RequestBag(
243+
var maybeRequestBag: RequestBag<UploadCountingDelegate>?
244+
XCTAssertNoThrow(maybeRequestBag = try RequestBag(
237245
request: request,
238246
eventLoopPreference: .delegate(on: embeddedEventLoop),
239247
task: .init(eventLoop: embeddedEventLoop, logger: logger),
240248
redirectHandler: nil,
241249
connectionDeadline: .now() + .seconds(30),
242250
idleReadTimeout: nil,
243251
delegate: delegate
244-
)
252+
))
253+
guard let bag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag.") }
245254
XCTAssert(bag.eventLoop === embeddedEventLoop)
246255

247256
let executor = MockRequestExecutor()
@@ -273,15 +282,17 @@ final class RequestBagTests: XCTestCase {
273282
guard let request = maybeRequest else { return XCTFail("Expected to have a request") }
274283

275284
let delegate = UploadCountingDelegate(eventLoop: embeddedEventLoop)
276-
let bag = RequestBag(
285+
var maybeRequestBag: RequestBag<UploadCountingDelegate>?
286+
XCTAssertNoThrow(maybeRequestBag = try RequestBag(
277287
request: request,
278288
eventLoopPreference: .delegate(on: embeddedEventLoop),
279289
task: .init(eventLoop: embeddedEventLoop, logger: logger),
280290
redirectHandler: nil,
281291
connectionDeadline: .now() + .seconds(30),
282292
idleReadTimeout: nil,
283293
delegate: delegate
284-
)
294+
))
295+
guard let bag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag.") }
285296

286297
let queuer = MockTaskQueuer()
287298
bag.requestWasQueued(queuer)
@@ -328,15 +339,17 @@ final class RequestBagTests: XCTestCase {
328339
guard let request = maybeRequest else { return XCTFail("Expected to have a request") }
329340

330341
let delegate = UploadCountingDelegate(eventLoop: embeddedEventLoop)
331-
let bag = RequestBag(
342+
var maybeRequestBag: RequestBag<UploadCountingDelegate>?
343+
XCTAssertNoThrow(maybeRequestBag = try RequestBag(
332344
request: request,
333345
eventLoopPreference: .delegate(on: embeddedEventLoop),
334346
task: .init(eventLoop: embeddedEventLoop, logger: logger),
335347
redirectHandler: nil,
336348
connectionDeadline: .now() + .seconds(30),
337349
idleReadTimeout: nil,
338350
delegate: delegate
339-
)
351+
))
352+
guard let bag = maybeRequestBag else { return XCTFail("Expected to be able to create a request bag.") }
340353

341354
let executor = MockRequestExecutor()
342355
bag.willExecuteRequest(executor)

‎Tests/AsyncHTTPClientTests/RequestValidationTests+XCTest.swift‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ extension RequestValidationTests {
3232
("testGET_HEAD_DELETE_CONNECTRequestCanHaveBody", testGET_HEAD_DELETE_CONNECTRequestCanHaveBody),
3333
("testInvalidHeaderFieldNames", testInvalidHeaderFieldNames),
3434
("testValidHeaderFieldNames", testValidHeaderFieldNames),
35+
("testMetadataDetectConnectionClose", testMetadataDetectConnectionClose),
36+
("testMetadataDefaultIsConnectionCloseIsFalse", testMetadataDefaultIsConnectionCloseIsFalse),
3537
("testNoHeadersNoBody", testNoHeadersNoBody),
3638
("testNoHeadersHasBody", testNoHeadersHasBody),
3739
("testContentLengthHeaderNoBody", testContentLengthHeaderNoBody),

0 commit comments

Comments
 (0)