Skip to content
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

api: make iOS Headers and HeadersBuilder case-insensitive #2383

Merged
merged 39 commits into from
Jun 28, 2022
Merged
Show file tree
Hide file tree
Changes from 35 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
8ffc556
use insensitive key
Augustyniak Jun 23, 2022
c3c9015
Make HeadersBuilder case insensitive
Augustyniak Jun 23, 2022
8137392
remove unnecessary call
Augustyniak Jun 23, 2022
15f722b
Update code
Augustyniak Jun 23, 2022
3576622
Fix formatting
Augustyniak Jun 23, 2022
f7538f2
update version history
Augustyniak Jun 23, 2022
516a1fc
Lint fixes
Augustyniak Jun 23, 2022
2ff4fb6
erge branch 'main' into make-headers-builder-case-insensitive
Augustyniak Jun 23, 2022
42897f2
add doc
Augustyniak Jun 23, 2022
0e328a0
fix casing
Augustyniak Jun 23, 2022
8430f4f
fix typo
Augustyniak Jun 23, 2022
86ce3fc
Improve version history
Augustyniak Jun 23, 2022
37c6e5c
Make Headers case-insensitive too
Augustyniak Jun 24, 2022
35c0536
lint fixes
Augustyniak Jun 24, 2022
1fd9938
swiftlint fix
Augustyniak Jun 24, 2022
6389322
doc string fix
Augustyniak Jun 25, 2022
3ac922c
remove method
Augustyniak Jun 25, 2022
6d6caf2
small tweaks to docs and description property
Augustyniak Jun 28, 2022
cc40134
simplify code
Augustyniak Jun 28, 2022
eea7eff
Merge branch 'main' into make-headers-builder-case-insensitive
Augustyniak Jun 28, 2022
eac2d9d
Doc updates
Augustyniak Jun 28, 2022
302db00
get rid of todo
Augustyniak Jun 28, 2022
9069668
use case-insensitive comparison
Augustyniak Jun 28, 2022
15cb362
fix case insensitive comparison and add tests
Augustyniak Jun 28, 2022
ca4441a
lint fixes
Augustyniak Jun 28, 2022
fe79cb3
cr comments
Augustyniak Jun 28, 2022
a80ef6a
Update library/swift/HeadersBuilder.swift
Augustyniak Jun 28, 2022
1a4d915
Update library/swift/HeadersContainer.swift
Augustyniak Jun 28, 2022
2a5085a
Update library/swift/HeadersContainer.swift
Augustyniak Jun 28, 2022
b9d1dd9
Update library/swift/HeadersBuilder.swift
Augustyniak Jun 28, 2022
75a586d
update version history
Augustyniak Jun 28, 2022
7295362
change used method
Augustyniak Jun 28, 2022
723c76f
update example app
Augustyniak Jun 28, 2022
079a01a
update example app
Augustyniak Jun 28, 2022
62885af
update example app
Augustyniak Jun 28, 2022
702bc24
update example apps
Augustyniak Jun 28, 2022
d3bd1ea
update example apps
Augustyniak Jun 28, 2022
d45fa9f
update example apps
Augustyniak Jun 28, 2022
42b38bf
update example apps
Augustyniak Jun 28, 2022
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
1 change: 1 addition & 0 deletions docs/root/intro/version_history.rst
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ Bugfixes:
- Android: update Kotlin standard libraries to 1.6.21 (:issue:`#2256 <2256>`)
- fix bug where finalStreamIntel was not consistently set on cancel (:issue:`#2285 <2285>`)
- iOS: fix termination crash in ProvisionalDispatcher (:issue:`#2059 <2059>`)
- iOS: make headers lookup in ``HeadersBuilder`` and ``Headers`` case-insensitive. Rename ``allHeaders`` method to ``caseSensitiveHeaders``. (:issue:`#2383 <2383>`)
- iOS: use correct DNS resolver when using C++ config builder (:issue: `#2378 <2378 >`)

Features:
Expand Down
10 changes: 3 additions & 7 deletions examples/swift/async_await/ContentView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,8 @@ private extension StreamClient {
let stream = self
.newStreamPrototype()
.setOnResponseHeaders { headers, _, _ in
let allHeaders = headers.allHeaders()

if allHeaders[":status"]?.first == "200",
// TODO(jpsim): Expose an API that enforces case-insensitive lookups
let contentLengthValue = allHeaders["Content-Length"] ??
allHeaders["content-length"],
if headers.value(forName: ":status")?.first == "200",
let contentLengthValue = headers.value(forName: "content-length"),
let firstContentLength = contentLengthValue.first,
let contentLengthInt = Int64(firstContentLength)
{
Expand All @@ -87,7 +83,7 @@ private extension StreamClient {
return
}

let headerMessage = allHeaders
let headerMessage = headers.caseSensitiveHeaders
.map { "\($0.key): \($0.value.joined(separator: ", "))" }
.joined(separator: "\n")

Expand Down
2 changes: 1 addition & 1 deletion examples/swift/hello_world/ViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ final class ViewController: UITableViewController {
let statusCode = headers.httpStatus.map(String.init) ?? "nil"
let message = "received headers with status \(statusCode)"

let headerMessage = headers.allHeaders()
let headerMessage = headers.caseSensitiveHeaders()
.filter { kFilteredHeaders.contains($0.key) }
.map { "\($0.key): \($0.value.joined(separator: ", "))" }
.joined(separator: "\n")
Expand Down
1 change: 1 addition & 0 deletions library/swift/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ swift_library(
"FinalStreamIntel.swift",
"Headers.swift",
"HeadersBuilder.swift",
"HeadersContainer.swift",
"KeyValueStore.swift",
"LogLevel.swift",
"NetworkMonitoringMode.swift",
Expand Down
41 changes: 30 additions & 11 deletions library/swift/Headers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -4,45 +4,64 @@ import Foundation
/// To instantiate new instances, see `{Request|Response}HeadersBuilder`.
@objcMembers
public class Headers: NSObject {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we merge the Headers type with the HeadersContainer type? I don't see any reason to add that additional layer, but maybe I'm missing a reason why it's necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One of my previous solutions was to merge Headers and HeadersContainer but I decided against it as:

  • I wanted HeadersContainers to be a struct - not possible with Headers as it has to inherit from NSObject. There are some negative of using structs tho - more copying.
  • HeadersBuilder depends on a lot of stuff that's currently in HeadersContainer. If we merge Headers and HeadersContainer then HeadersBuilder would either have to have duplicated logic between Headers and HeadersBuilder or make HeadersBuilder depend on Headers. I decided that having a separate HeadersContainer class was a better approach to ensure that all of the case-insensitive lookup logic lives in one place and is shared between Headers and HeaderBuilder.

let headers: [String: [String]]
let container: HeadersContainer

/// Get the value for the provided header name.
///
/// - note: The lookup for a header name is a case-insensitive operation.
///
Augustyniak marked this conversation as resolved.
Show resolved Hide resolved
/// - parameter name: Header name for which to get the current value.
///
/// - returns: The current headers specified for the provided name.
public func value(forName name: String) -> [String]? {
return self.headers[name]
return self.container.value(forName: name)
}

/// Accessor for all underlying headers as a map.
/// Accessor for all underlying case-sensitive headers. When possible,
/// use case-insensitive accessors instead.
///
/// - warning: It's discouraged to use this dictionary for equality
/// key-based lookups as this may lead to issues with headers
/// that do not follow expected casing i.e., "Content-Length"
/// instead of "content-length".
///
/// - returns: The underlying headers.
public func allHeaders() -> [String: [String]] {
return self.headers
/// - returns: The underlying case-sensitive headers.
public func caseSensitiveHeaders() -> [String: [String]] {
return self.container.allHeaders()
}

/// Internal initializer used by builders.
///
/// - parameter headers: Headers to set.
required init(headers: [String: [String]]) {
self.headers = headers
/// - parameter container: Headers to set.
required init(container: HeadersContainer) {
self.container = container
super.init()
}

/// Inialize the receiver with a given headers map.
///
/// - parameter headers: The headers map to use.
convenience init(headers: [String: [String]]) {
self.init(container: HeadersContainer(headers: headers))
}

override convenience init() {
self.init(headers: [:])
}
}

// MARK: - Equatable

extension Headers {
public override func isEqual(_ object: Any?) -> Bool {
return (object as? Self)?.headers == self.headers
return (object as? Self)?.container == self.container
}
}

// MARK: - CustomStringConvertible

extension Headers {
public override var description: String {
return "\(type(of: self)) \(self.headers.description)"
return "\(type(of: self)) \(self.caseSensitiveHeaders())"
}
}
41 changes: 29 additions & 12 deletions library/swift/HeadersBuilder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,19 @@ import Foundation
private let kRestrictedPrefixes = [":", "x-envoy-mobile"]

private func isRestrictedHeader(name: String) -> Bool {
return name == "host" || kRestrictedPrefixes.contains { name.hasPrefix($0) }
let isHostHeader = name.caseInsensitiveCompare("host") == .orderedSame
lazy var hasRestrictedPrefix = kRestrictedPrefixes
.contains { name.range(of: $0, options: [.caseInsensitive, .anchored]) != nil }
return isHostHeader || hasRestrictedPrefix
}

/// Base builder class used to construct `Headers` instances.
/// It preserves the original casing of headers and enforces
/// a case-insensitive lookup and setting of headers.
/// See `{Request|Response}HeadersBuilder` for usage.
@objcMembers
public class HeadersBuilder: NSObject {
private(set) var headers: [String: [String]]
private(set) var container: HeadersContainer

/// Append a value to the header name.
///
Expand All @@ -24,7 +29,7 @@ public class HeadersBuilder: NSObject {
return self
}

self.headers[name, default: []].append(value)
self.container.add(name: name, value: value)
return self
}

Expand All @@ -40,7 +45,7 @@ public class HeadersBuilder: NSObject {
return self
}

self.headers[name] = value
self.container.set(name: name, value: value)
return self
}

Expand All @@ -55,7 +60,7 @@ public class HeadersBuilder: NSObject {
return self
}

self.headers[name] = nil
self.container.set(name: name, value: nil)
return self
}

Expand All @@ -69,22 +74,34 @@ public class HeadersBuilder: NSObject {
/// - returns: This builder.
@discardableResult
func internalSet(name: String, value: [String]) -> Self {
self.headers[name] = value
self.container.set(name: name, value: value)
return self
}

func allHeaders() -> [String: [String]] {
return self.container.allHeaders()
}

// Only explicitly implemented to work around a swiftinterface issue in Swift 5.1. This can be
// removed once envoy is only built with Swift 5.2+
public override init() {
self.headers = [:]
self.container = HeadersContainer()
super.init()
}

/// Initialize a new builder. Subclasses should provide their own public convenience initializers.
// Initialize a new builder using the provided headers container.
///
/// - parameter headers: The headers with which to start.
required init(headers: [String: [String]]) {
self.headers = headers
/// - parameter container: The headers container to initialize the receiver with.
init(container: HeadersContainer) {
self.container = container
super.init()
}

// Initialize a new builder. Subclasses should provide their own public convenience initializers.
//
// - parameter headers: The headers with which to start.
init(headers: [String: [String]]) {
self.container = HeadersContainer(headers: headers)
super.init()
}
}
Expand All @@ -93,6 +110,6 @@ public class HeadersBuilder: NSObject {

extension HeadersBuilder {
public override func isEqual(_ object: Any?) -> Bool {
return (object as? Self)?.headers == self.headers
return (object as? Self)?.container == self.container
}
}
110 changes: 110 additions & 0 deletions library/swift/HeadersContainer.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
/// The container which manages the underlying headers map.
/// It maintains the original casing of passed header names.
/// It treats headers names as case-insensitive for the purpose
/// of header lookups and header name conflict resolutions.
struct HeadersContainer: Equatable {
private var headers: [String: Header]

/// Represents a headers name together with all of its values.
/// It preserves the original casing of the header name.
struct Header: Equatable {
private(set) var name: String
private(set) var value: [String]

init(name: String, value: [String] = []) {
self.name = name
self.value = value
}

mutating func addValue(_ value: [String]) {
self.value.append(contentsOf: value)
}

mutating func addValue(_ value: String) {
self.value.append(value)
}
}

/// Initialize a new instance of the receiver using the provided headers map.
///
/// - parameter headers: The headers map.
init(headers: [String: [String]]) {
var underlyingHeaders = [String: Header]()
for (name, value) in headers {
let lowercasedName = name.lowercased()
/// Dictionaries in Swift are unordered collections. Process headers with names
/// that are the same when lowercased in an alphabetical order to avoid a situation
/// in which the result of the initialization is non-derministic i.e., we want
/// "[A: ["1"]", "a: ["2"]]" headers to be always converted to ["A": ["1", "2"]] and
/// never to "a": ["2", "1"].
///
/// If a given header name already exists in the processed headers map, check
/// if the currently processed header name is before the existing header name as
/// determined by an alphabetical order.
guard let existingHeader = underlyingHeaders[lowercasedName] else {
underlyingHeaders[lowercasedName] = Header(name: name, value: value)
continue
}

if existingHeader.name > name {
underlyingHeaders[lowercasedName] =
Header(name: name, value: value + existingHeader.value)
} else {
underlyingHeaders[lowercasedName]?.addValue(value)
}
}
self.headers = underlyingHeaders
}

/// Initialize an empty headers container.
init() {
self.headers = [:]
}

/// Add a value to a header with a given name.
///
/// - parameter name: The name of the header. For the purpose of headers lookup
/// and header name conflict resolution, the name of the header
/// is considered to be case-insensitive.
/// - parameter value: The value to add.
mutating func add(name: String, value: String) {
self.headers[name.lowercased(), default: Header(name: name)].addValue(value)
}

/// Set the value of a given header.
///
/// - parameter name: The name of the header.
/// - parameter value: The value to set the header value to.
mutating func set(name: String, value: [String]?) {
guard let value = value else {
self.headers[name.lowercased()] = nil
return
}
self.headers[name.lowercased()] = Header(name: name, value: value)
}

/// Get the value for the provided header name.
///
/// - parameter name: The case-insensitive header name for which to
/// get the current value.
///
/// - returns: The value associated with a given header.
func value(forName name: String) -> [String]? {
return self.headers[name.lowercased()]?.value
}

/// Return all underlying headers.
///
/// - returns: The underlying headers.
func allHeaders() -> [String: [String]] {
return Dictionary(uniqueKeysWithValues: self.headers.map { _, value in
return (value.name, value.value)
})
}
}

extension HeadersContainer: CustomStringConvertible {
var description: String {
return self.headers.description
}
}
2 changes: 1 addition & 1 deletion library/swift/RequestHeaders.swift
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,6 @@ public class RequestHeaders: Headers {
///
/// - returns: The new builder.
public func toRequestHeadersBuilder() -> RequestHeadersBuilder {
return RequestHeadersBuilder(headers: self.headers)
return RequestHeadersBuilder(container: self.container)
}
}
2 changes: 1 addition & 1 deletion library/swift/RequestHeadersBuilder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,6 @@ public final class RequestHeadersBuilder: HeadersBuilder {
///
/// - returns: New instance of request headers.
public func build() -> RequestHeaders {
return RequestHeaders(headers: self.headers)
return RequestHeaders(container: self.container)
}
}
2 changes: 1 addition & 1 deletion library/swift/RequestTrailers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@ public final class RequestTrailers: Trailers {
///
/// - returns: The new builder.
public func toRequestTrailersBuilder() -> RequestTrailersBuilder {
return RequestTrailersBuilder(headers: self.headers)
return RequestTrailersBuilder(container: self.container)
}
}
4 changes: 2 additions & 2 deletions library/swift/RequestTrailersBuilder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@ import Foundation
public final class RequestTrailersBuilder: HeadersBuilder {
/// Initialize a new instance of the builder.
public override convenience init() {
self.init(headers: [:])
self.init(container: HeadersContainer(headers: [:]))
}

/// Build the request trailers using the current builder.
///
/// - returns: New instance of request trailers.
public func build() -> RequestTrailers {
return RequestTrailers(headers: self.headers)
return RequestTrailers(container: self.container)
}
}
2 changes: 1 addition & 1 deletion library/swift/ResponseHeaders.swift
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,6 @@ public final class ResponseHeaders: Headers {
///
/// - returns: The new builder.
public func toResponseHeadersBuilder() -> ResponseHeadersBuilder {
return ResponseHeadersBuilder(headers: self.headers)
return ResponseHeadersBuilder(container: self.container)
}
}
4 changes: 2 additions & 2 deletions library/swift/ResponseHeadersBuilder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import Foundation
public final class ResponseHeadersBuilder: HeadersBuilder {
/// Initialize a new instance of the builder.
public override convenience init() {
self.init(headers: [:])
self.init(container: HeadersContainer(headers: [:]))
}

/// Add an HTTP status to the response headers.
Expand All @@ -23,6 +23,6 @@ public final class ResponseHeadersBuilder: HeadersBuilder {
///
/// - returns: New instance of response headers.
public func build() -> ResponseHeaders {
return ResponseHeaders(headers: self.headers)
return ResponseHeaders(container: self.container)
}
}
Loading