From f82886ebad69316af45016d91db624e4b4d9bc9a Mon Sep 17 00:00:00 2001 From: Pushkar N Kulkarni Date: Thu, 13 Dec 2018 18:00:50 +0000 Subject: [PATCH] Implement a very lazy translation of headers Add HeaderContainer tests in dual mode --- Sources/KituraNet/ClientResponse.swift | 4 +- .../KituraNet/HTTP/HTTPServerRequest.swift | 2 +- .../KituraNet/HTTP/HTTPServerResponse.swift | 2 +- Sources/KituraNet/HTTP/HeadersContainer.swift | 133 +++++++++++------- Tests/KituraNetTests/HTTPResponseTests.swift | 16 +-- Tests/KituraNetTests/MiscellaneousTests.swift | 89 +++++++++++- 6 files changed, 186 insertions(+), 60 deletions(-) diff --git a/Sources/KituraNet/ClientResponse.swift b/Sources/KituraNet/ClientResponse.swift index 7a54479f..ef26adff 100644 --- a/Sources/KituraNet/ClientResponse.swift +++ b/Sources/KituraNet/ClientResponse.swift @@ -45,11 +45,11 @@ public class ClientResponse { guard let httpHeaders = httpHeaders else { return HeadersContainer() } - return HeadersContainer.create(from: httpHeaders) + return HeadersContainer(with: httpHeaders) } set { - httpHeaders = newValue.httpHeaders() + httpHeaders = newValue.nioHeaders } } /// The HTTP Status code, as an Int, sent in the response by the remote server. diff --git a/Sources/KituraNet/HTTP/HTTPServerRequest.swift b/Sources/KituraNet/HTTP/HTTPServerRequest.swift index 58d7d4f0..6f0e4a66 100644 --- a/Sources/KituraNet/HTTP/HTTPServerRequest.swift +++ b/Sources/KituraNet/HTTP/HTTPServerRequest.swift @@ -115,7 +115,7 @@ public class HTTPServerRequest: ServerRequest { } init(ctx: ChannelHandlerContext, requestHead: HTTPRequestHead, enableSSL: Bool) { - self.headers = HeadersContainer.create(from: requestHead.headers) + self.headers = HeadersContainer(with: requestHead.headers) self.method = requestHead.method.string() self.httpVersionMajor = requestHead.version.major self.httpVersionMinor = requestHead.version.minor diff --git a/Sources/KituraNet/HTTP/HTTPServerResponse.swift b/Sources/KituraNet/HTTP/HTTPServerResponse.swift index 3b4c257a..df42e65f 100644 --- a/Sources/KituraNet/HTTP/HTTPServerResponse.swift +++ b/Sources/KituraNet/HTTP/HTTPServerResponse.swift @@ -183,7 +183,7 @@ public class HTTPServerResponse: ServerResponse { /// Send response to the client private func sendResponse(channel: Channel, handler: HTTPRequestHandler, status: HTTPResponseStatus, withBody: Bool = true) throws { - let response = HTTPResponseHead(version: httpVersion, status: status, headers: headers.httpHeaders()) + let response = HTTPResponseHead(version: httpVersion, status: status, headers: headers.nioHeaders) channel.write(handler.wrapOutboundOut(.head(response)), promise: nil) if withBody && buffer.readableBytes > 0 { channel.write(handler.wrapOutboundOut(.body(.byteBuffer(buffer))), promise: nil) diff --git a/Sources/KituraNet/HTTP/HeadersContainer.swift b/Sources/KituraNet/HTTP/HeadersContainer.swift index 88ddd2f4..9dd2a4d2 100644 --- a/Sources/KituraNet/HTTP/HeadersContainer.swift +++ b/Sources/KituraNet/HTTP/HeadersContainer.swift @@ -26,9 +26,25 @@ public class HeadersContainer { /// The header storage internal var headers: [String: (key: String, value: [String])] = [:] + /// An alternate backing store of type `NIOHTTP1.HTTPHeader` used to avoid translations between HeadersContainer and HTTPHeaders + var nioHeaders: HTTPHeaders = HTTPHeaders() + /// Create an instance of `HeadersContainer` public init() {} + /// A special initializer for HTTPServerRequest, to make the latter better performant + init(with nioHeaders: HTTPHeaders) { + self.nioHeaders = nioHeaders + } + + private enum _Mode { + case nio // Headers are simply backed up by `NIOHTTP1.HTTPHeaders` + case dual // Headers are backed up by a dictionary as well, we switch to this mode while using HeadersContainer as Collection + } + + // The default mode is nio + private var mode: _Mode = .nio + /// Access the value of a HTTP header using subscript syntax. /// /// - Parameter key: The HTTP header key @@ -42,9 +58,16 @@ public class HeadersContainer { set(newValue) { if let newValue = newValue { - set(key, value: newValue) - } else { - remove(key) + nioHeaders.replace(name: key, values: newValue) + if mode == .dual { + set(key, value: newValue) + } + } + else { + nioHeaders.remove(name: key) + if mode == .dual { + remove(key) + } } } } @@ -54,36 +77,49 @@ public class HeadersContainer { /// - Parameter key: The HTTP header key /// - Parameter value: An array of strings to add as values of the HTTP header public func append(_ key: String, value: [String]) { - let lowerCaseKey = key.lowercased() - let entry = headers[lowerCaseKey] - - switch lowerCaseKey { + var entry = nioHeaders[key] + + switch(lowerCaseKey) { case "set-cookie": - if entry != nil { - headers[lowerCaseKey]?.value += value + if entry.count > 0 { + entry += value + nioHeaders.replace(name: key, values: entry) + if mode == .dual { + headers[lowerCaseKey]?.value += value + } } else { - set(key, lowerCaseKey: lowerCaseKey, value: value) + nioHeaders.add(name: key, values: value) + if mode == .dual { + set(key, lowerCaseKey: lowerCaseKey, value: value) + } } case "content-type", "content-length", "user-agent", "referer", "host", "authorization", "proxy-authorization", "if-modified-since", "if-unmodified-since", "from", "location", "max-forwards", "retry-after", "etag", "last-modified", "server", "age", "expires": - if entry != nil { + if entry.count > 0 { Log.warning("Duplicate header \(key) discarded") break } fallthrough default: - guard let oldValue = entry?.value.first else { - set(key, lowerCaseKey: lowerCaseKey, value: value) - return + if nioHeaders[key].count == 0 { + nioHeaders.add(name: key, values: value) + if mode == .dual { + set(key, lowerCaseKey: lowerCaseKey, value: value) + } + } else { + let oldValue = nioHeaders[key].first! + let newValue = oldValue + ", " + value.joined(separator: ", ") + nioHeaders.replaceOrAdd(name: key, value: newValue) + if mode == .dual { + headers[lowerCaseKey]?.value[0] = newValue + } } - let newValue = oldValue + ", " + value.joined(separator: ", ") - headers[lowerCaseKey]?.value[0] = newValue } } @@ -96,12 +132,17 @@ public class HeadersContainer { } private func get(_ key: String) -> [String]? { - return headers[key.lowercased()]?.value + let values = nioHeaders[key] + // `HTTPHeaders.subscript` returns a [] if no header is found, but `HeadersContainer` is expected to return a nil + return values.count > 0 ? values : nil } /// Remove all of the headers public func removeAll() { - headers.removeAll(keepingCapacity: true) + nioHeaders = HTTPHeaders() + if mode == .dual { + headers.removeAll(keepingCapacity: true) + } } private func set(_ key: String, value: [String]) { @@ -115,6 +156,14 @@ public class HeadersContainer { private func remove(_ key: String) { headers.removeValue(forKey: key.lowercased()) } + + func checkAndSwitchToDualMode() { + guard mode == .nio else { return } + mode = .dual + for header in nioHeaders { + headers[header.name.lowercased()] = (header.name, nioHeaders[header.name]) + } + } } extension HTTPHeaders { @@ -124,22 +173,30 @@ extension HTTPHeaders { } } - mutating func replaceOrAdd(name: String, values: [String]) { - values.forEach { - replaceOrAdd(name: name, value: $0) + mutating func replace(name: String, values: [String]) { + self.replaceOrAdd(name: name, value: values[0]) + for value in values.suffix(from: 1) { + self.add(name: name, value: value) } } } /// Conformance to the `Collection` protocol +/// As soon as either of these properties or methods are invoked, we need to switch to the `dual` mode of operation extension HeadersContainer: Collection { public typealias Index = DictionaryIndex /// The starting index of the `HeadersContainer` collection - public var startIndex: Index { return headers.startIndex } + public var startIndex:Index { + checkAndSwitchToDualMode() + return headers.startIndex + } /// The ending index of the `HeadersContainer` collection - public var endIndex: Index { return headers.endIndex } + public var endIndex:Index { + checkAndSwitchToDualMode() + return headers.endIndex + } /// Get a (key value) tuple from the `HeadersContainer` collection at the specified position. /// @@ -148,7 +205,10 @@ extension HeadersContainer: Collection { /// /// - Returns: A (key, value) tuple. public subscript(position: Index) -> (key: String, value: [String]) { + get { + checkAndSwitchToDualMode() return headers[position].value + } } /// Get the next Index in the `HeadersContainer` collection after the one specified. @@ -156,29 +216,8 @@ extension HeadersContainer: Collection { /// - Parameter after: The Index whose successor is to be returned. /// /// - Returns: The Index in the `HeadersContainer` collection after the one specified. - public func index(after index: Index) -> Index { - return headers.index(after: index) - } -} - -/// Kitura uses HeadersContainer and NIOHTTP1 expects HTTPHeader - bridging methods -extension HeadersContainer { - /// HTTPHeaders to HeadersContainer - static func create(from httpHeaders: HTTPHeaders) -> HeadersContainer { - let headerContainer = HeadersContainer() - for header in httpHeaders { - headerContainer.append(header.name, value: header.value) - } - return headerContainer - } - - func httpHeaders() -> HTTPHeaders { - var httpHeaders = HTTPHeaders() - for (_, keyValues) in headers { - for value in keyValues.1 { - httpHeaders.add(name: keyValues.0, value: value) - } - } - return httpHeaders + public func index(after i: Index) -> Index { + checkAndSwitchToDualMode() + return headers.index(after: i) } } diff --git a/Tests/KituraNetTests/HTTPResponseTests.swift b/Tests/KituraNetTests/HTTPResponseTests.swift index 972371f2..c04d4635 100644 --- a/Tests/KituraNetTests/HTTPResponseTests.swift +++ b/Tests/KituraNetTests/HTTPResponseTests.swift @@ -64,24 +64,24 @@ class HTTPResponseTests: KituraNetTest { func testHeadersContainerHTTPHeaders() { let headers = HeadersContainer() headers["Content-Type"] = ["image/png, text/plain"] - XCTAssertEqual(headers.httpHeaders()["Content-Type"], headers["Content-Type"]!) + XCTAssertEqual(headers.nioHeaders["Content-Type"], headers["Content-Type"]!) headers["Content-Type"] = ["text/html"] - XCTAssertEqual(headers.httpHeaders()["Content-Type"], headers["Content-Type"]!) + XCTAssertEqual(headers.nioHeaders["Content-Type"], headers["Content-Type"]!) headers["Content-Type"] = nil - XCTAssertFalse(headers.httpHeaders().contains(name: "Content-Type")) + XCTAssertFalse(headers.nioHeaders.contains(name: "Content-Type")) headers["Set-Cookie"] = ["ID=123BAS; Path=/; Secure; HttpOnly"] headers.append("Set-Cookie", value: ["ID=KI9H12; Path=/; Secure; HttpOnly"]) - XCTAssertEqual(headers["Set-Cookie"]!, headers.httpHeaders()["Set-Cookie"]) + XCTAssertEqual(headers["Set-Cookie"]!, headers.nioHeaders["Set-Cookie"]) headers["Content-Type"] = ["text/html"] headers.append("Content-Type", value: "text/json") - XCTAssertEqual(headers.httpHeaders()["Content-Type"], headers["Content-Type"]!) + XCTAssertEqual(headers.nioHeaders["Content-Type"], headers["Content-Type"]!) headers["foo"] = ["bar0"] headers.append("foo", value: "bar1") - XCTAssertEqual(headers.httpHeaders()["foo"], headers["foo"]!) + XCTAssertEqual(headers.nioHeaders["foo"], headers["foo"]!) headers.append("foo", value: ["bar2", "bar3"]) - XCTAssertEqual(headers.httpHeaders()["foo"], headers["foo"]!) + XCTAssertEqual(headers.nioHeaders["foo"], headers["foo"]!) headers.removeAll() - XCTAssertFalse(headers.httpHeaders().contains(name: "foo")) + XCTAssertFalse(headers.nioHeaders.contains(name: "foo")) } func testMultipleWritesToResponse() { diff --git a/Tests/KituraNetTests/MiscellaneousTests.swift b/Tests/KituraNetTests/MiscellaneousTests.swift index 375e0ec0..37b6e362 100644 --- a/Tests/KituraNetTests/MiscellaneousTests.swift +++ b/Tests/KituraNetTests/MiscellaneousTests.swift @@ -25,7 +25,8 @@ class MiscellaneousTests: KituraNetTest { static var allTests: [(String, (MiscellaneousTests) -> () throws -> Void)] { return [ ("testEscape", testEscape), - ("testHeadersContainers", testHeadersContainers) + ("testHeadersContainers", testHeadersContainers), + ("testHeadersContainerDualMode", testHeadersContainerDualMode), ] } @@ -63,4 +64,90 @@ class MiscellaneousTests: KituraNetTest { XCTAssert(foundContentType, "Didn't find the Content-Type header") XCTAssert(foundSetCookie, "Didn't find the Set-Cookie header") } + + func testHeadersContainerDualMode() { + let headers = HeadersContainer() + headers.append("Foo", value: "Bar") + headers.append("Foo", value: "Baz") + let numValues = headers["Foo"]?.count ?? 0 + XCTAssertEqual(numValues, 1, "Foo didn't have one value, as expected. It had \(numValues) values") + + let numValues1 = headers[headers.startIndex].value.count + XCTAssertEqual(numValues1, 1, "Foo didn't have one value, as expected. It had \(numValues1) values") + + // Special case: cookies + let cookieHeaders = HeadersContainer() + cookieHeaders.append("Set-Cookie", value: "os=Linux") + cookieHeaders.append("Set-Cookie", value: "browser=Safari") + let numValues2 = cookieHeaders["Set-Cookie"]?.count ?? 0 + XCTAssertEqual(numValues2, 2, "Set-Cookie didn't have two values, as expected. It had \(numValues2) values") + + let numValues3 = cookieHeaders[cookieHeaders.startIndex].value.count + XCTAssertEqual(numValues3, 2, "Set-Cookie didn't have two values, as expected. It had \(numValues3) values") + + // Special case: Content-Type + let contentHeaders = HeadersContainer() + contentHeaders.append("Content-Type", value: "application/json") + contentHeaders.append("Content-Type", value: "application/xml") + let numValues4 = contentHeaders["Content-Type"]?.count ?? 0 + XCTAssertEqual(numValues4, 1, "Content-Type didn't have one value, as expected. It had \(numValues4) values") + XCTAssertEqual(contentHeaders["Content-Type"]?.first!, "application/json") + + let numValues5 = contentHeaders[contentHeaders.startIndex].value.count + XCTAssertEqual(numValues5, 1, "Content-Type didn't have one value, as expected. It had \(numValues5) values") + + // Append arrays + headers.append("Foo", value: ["Cons", "Damn"]) + let numValues6 = headers["Foo"]?.count ?? 0 + XCTAssertEqual(numValues6, 1, "Foo didn't have one value, as expected. It had \(numValues6) values") + + cookieHeaders.append("Set-Cookie", value: ["abx=xyz", "def=fgh"]) + let numValues7 = cookieHeaders["Set-Cookie"]?.count ?? 0 + XCTAssertEqual(numValues7, 4, "Set-Cookie didn't have four values, as expected. It had \(numValues7) values") + + // Append arrays to a new container + let headers1 = HeadersContainer() + headers1.append("Foo", value: "Bar") + headers1.append("Foo", value: ["Cons", "Damn"]) + let numValues8 = headers1[headers1.startIndex].value.count + XCTAssertEqual(numValues8, 1, "Foo didn't have one value, as expected. It had \(numValues8) values") + + let cookieHeaders1 = HeadersContainer() + cookieHeaders1.append("Set-Cookie", value: "os=Linux") + cookieHeaders1.append("Set-Cookie", value: ["browser=Safari", "temp=xyz"]) + let numValues9 = cookieHeaders1[cookieHeaders1.startIndex].value.count + XCTAssertEqual(numValues9, 3, "Set-Cookie didn't have three values, as expected. It had \(numValues9) values") + + // Set using subscript function before and after mode switch + let headers2 = HeadersContainer() + headers2.append("Foo", value: "Bar") + headers2["Foo"] = ["Baz"] + XCTAssertNotNil(headers2["Foo"]) + XCTAssertEqual(headers2["Foo"]!.count, 1) + XCTAssertEqual(headers2["Foo"]!.first!, "Baz") + + let numValues10 = headers2[headers2.startIndex].value.count + XCTAssertEqual(numValues10, 1) + headers2["Foo"] = ["Bar", "Baz"] + XCTAssertEqual(headers2["Foo"]!.count, 2) + XCTAssertEqual(headers2["Foo"]!.first!, "Bar") + + let cookieHeaders2 = HeadersContainer() + cookieHeaders2.append("Set-Cookie", value: "os=Linux") + cookieHeaders2["Set-Cookie"] = ["browser=Safari", "temp=xyz"] + let numValues11 = cookieHeaders2[cookieHeaders2.startIndex].value.count + XCTAssertEqual(numValues11, 2, "Set-Cookie didn't have two values, as expected. It had \(numValues11) values") + + // removeAll + headers2.removeAll() + cookieHeaders2.removeAll() + + for _ in headers2 { + XCTFail("Headers2 must be empty!") + } + + for _ in cookieHeaders2 { + XCTFail("cookieHeaders2 must be empty!") + } + } }