address PR feedback

smithy-lang · May 15, 2024 · f38c9f2 · f38c9f2
1 parent e0b8ca0
commit f38c9f2
Show file tree

Hide file tree

Showing 10 changed files with 86 additions and 61 deletions.
diff --git a/Sources/ClientRuntime/Message/RequestMessage.swift b/Sources/ClientRuntime/Message/RequestMessage.swift
@@ -17,6 +17,9 @@ public protocol RequestMessage {
     /// The body of the request.
     var body: ByteStream { get }
 
+    // The uri of the request
+    var destination: URI { get }
+
     /// - Returns: A new builder for this request message, with all properties copied.
     func toBuilder() -> RequestBuilderType
 }
diff --git a/Sources/ClientRuntime/Networking/Endpoint.swift b/Sources/ClientRuntime/Networking/Endpoint.swift
@@ -9,7 +9,7 @@ public struct Endpoint: Hashable {
     public let uri: URI
     public let headers: Headers
     public var protocolType: ProtocolType? { uri.scheme }
-    public var queryItems: [SDKURLQueryItem] { uri.query }
+    public var queryItems: [SDKURLQueryItem] { uri.queryItems }
     public var path: String { uri.path }
     public var host: String { uri.host }
     public var port: Int16 { uri.port }
@@ -55,9 +55,8 @@ public struct Endpoint: Hashable {
                 queryItems: [SDKURLQueryItem]? = nil,
                 headers: Headers = Headers(),
                 protocolType: ProtocolType? = .https) {
-
         let uri = URIBuilder()
-            .withScheme(protocolType)
+            .withScheme(protocolType ?? .https)
             .withPath(path)
             .withHost(host)
             .withPort(port)

diff --git a/Sources/ClientRuntime/Networking/Http/Middlewares/ContentTypeMiddleware.swift b/Sources/ClientRuntime/Networking/Http/Middlewares/ContentTypeMiddleware.swift
@@ -42,6 +42,6 @@ extension ContentTypeMiddleware: HttpInterceptor {
     ) async throws {
         let builder = context.getRequest().toBuilder()
         addHeaders(builder: builder)
-        context.updateRequest(updated: try builder.build())
+        context.updateRequest(updated: builder.build())
     }
 }
diff --git a/Sources/ClientRuntime/Networking/Http/Middlewares/FlexibleChecksumsRequestMiddleware.swift b/Sources/ClientRuntime/Networking/Http/Middlewares/FlexibleChecksumsRequestMiddleware.swift
@@ -109,6 +109,6 @@ extension FlexibleChecksumsRequestMiddleware: HttpInterceptor {
     ) async throws {
         let builder = context.getRequest().toBuilder()
         try await addHeaders(builder: builder, attributes: context.getAttributes())
-        context.updateRequest(updated: try builder.build())
+        context.updateRequest(updated: builder.build())
     }
 }
diff --git a/Sources/ClientRuntime/Networking/Http/SdkHttpRequest.swift b/Sources/ClientRuntime/Networking/Http/SdkHttpRequest.swift
@@ -22,7 +22,7 @@ public final class SdkHttpRequest: RequestMessage {
     public let method: HttpMethodType
     public var host: String { destination.host }
     public var path: String { destination.path }
-    public var queryItems: [SDKURLQueryItem]? { destination.query }
+    public var queryItems: [SDKURLQueryItem]? { destination.queryItems }
     public var trailingHeaders: Headers = Headers()
     public var endpoint: Endpoint {
         return Endpoint(uri: self.destination, headers: self.headers)
@@ -54,7 +54,7 @@ public final class SdkHttpRequest: RequestMessage {
             .withHost(self.destination.host)
             .withPort(self.destination.port)
             .withProtocol(self.destination.scheme)
-            .withQueryItems(self.destination.query)
+            .withQueryItems(self.destination.queryItems)
         return builder
     }
 

diff --git a/Sources/ClientRuntime/Networking/Http/URI.swift b/Sources/ClientRuntime/Networking/Http/URI.swift
@@ -5,23 +5,23 @@
 
 import Foundation
 
+/// Universal Resource Identifier component used to construct target location for a Request.
 public struct URI: Hashable {
     public let scheme: Scheme
     public let path: String
     public let host: String
     public let port: Int16
-    public let query: [SDKURLQueryItem]
+    public let queryItems: [SDKURLQueryItem]
     public let username: String?
     public let password: String?
     public var url: URL? {
         self.toBuilder().getUrl()
     }
     public var queryString: String? {
-        if self.query.isEmpty {
+        if self.queryItems.isEmpty {
             return nil
         }
-
-        return self.query.map { queryItem in
+        return self.queryItems.map { queryItem in
             return [queryItem.name, queryItem.value].compactMap { $0 }.joined(separator: "=")
         }.joined(separator: "&")
     }
@@ -30,14 +30,14 @@ public struct URI: Hashable {
                      path: String,
                      host: String,
                      port: Int16,
-                     query: [SDKURLQueryItem],
+                     queryItems: [SDKURLQueryItem],
                      username: String? = nil,
                      password: String? = nil) {
         self.scheme = scheme
         self.path = path
         self.host = host
         self.port = port
-        self.query = query
+        self.queryItems = queryItems
         self.username = username
         self.password = password
     }
@@ -48,49 +48,48 @@ public struct URI: Hashable {
            .withPath(self.path)
            .withHost(self.host)
            .withPort(self.port)
-           .withQueryItems(self.query)
+           .withQueryItems(self.queryItems)
+           .withUsername(self.username)
+           .withPassword(self.password)
     }
 }
 
-public class URIBuilder {
+/// A builder class for URI
+/// The builder performs validation to conform with RFC 3986
+public final class URIBuilder {
     var urlComponents: URLComponents
-    var scheme: Scheme = Scheme.https
-    var path: String = "/"
-    var host: String = ""
-    var port: Int16 = Int16(Scheme.https.port)
-    var query: [SDKURLQueryItem] = []
-    var username: String?
-    var password: String?
-
-    required public init() {
+    var port: Int16
+    var path: String
+
+    public init() {
+        self.port = Int16(Scheme.https.port)
+        self.path = "/"
         self.urlComponents = URLComponents()
-        self.urlComponents.scheme = self.scheme.rawValue
+        self.urlComponents.scheme = Scheme.https.rawValue
         self.urlComponents.path = self.path
-        self.urlComponents.host = self.host
+        self.urlComponents.host = ""
     }
 
     @discardableResult
-    public func withScheme(_ value: Scheme?) -> URIBuilder {
-        self.scheme = value ?? Scheme.https
-        self.urlComponents.scheme = self.scheme.rawValue
+    public func withScheme(_ value: Scheme) -> URIBuilder {
+        self.urlComponents.scheme = value.rawValue
         return self
     }
 
     @discardableResult
     public func withPath(_ value: String) -> URIBuilder {
-        self.path = value
-        if self.path.contains("%") {
-            self.urlComponents.percentEncodedPath = self.path
+        if value.isPercentEncoded {
+            self.urlComponents.percentEncodedPath = value
         } else {
-            self.urlComponents.path = self.path
+            self.urlComponents.path = value
         }
+        self.path = value
         return self
     }
 
     @discardableResult
     public func withHost(_ value: String) -> URIBuilder {
-        self.host = value
-        self.urlComponents.host = self.host
+        self.urlComponents.host = value
         return self
     }
 
@@ -101,48 +100,72 @@ public class URIBuilder {
     }
 
     @discardableResult
-    public func withQueryItems(_ value: [SDKURLQueryItem]) -> URIBuilder {
-        self.query.append(contentsOf: value)
-        if !self.query.isEmpty {
-            self.urlComponents.percentEncodedQuery = self.query.map { queryItem in
-                            return [queryItem.name, queryItem.value].compactMap { $0 }.joined(separator: "=")
-                        }.joined(separator: "&")
+    public func withQueryItems(_ items: [SDKURLQueryItem]) -> URIBuilder {
+        guard !items.isEmpty else {
+            return self
+        }
+        self.urlComponents.queryItems = items.map { item in
+            return URLQueryItem(name: item.name, value: item.value)
         }
         return self
     }
 
     @discardableResult
-    public func withQueryItem(_ value: SDKURLQueryItem) -> URIBuilder {
-        withQueryItems([value])
+    public func appendQueryItems(_ items: [SDKURLQueryItem]) -> URIBuilder {
+        guard !items.isEmpty else {
+            return self
+        }
+        var queryItems = self.urlComponents.queryItems ?? []
+        queryItems += items.map { item in
+            return URLQueryItem(name: item.name, value: item.value)
+        }
+        self.urlComponents.queryItems = queryItems
+        return self
     }
 
     @discardableResult
-    public func withUsername(_ value: String) -> URIBuilder {
-        self.username = value
-        self.urlComponents.user = self.username
+    public func appendQueryItem(_ item: SDKURLQueryItem) -> URIBuilder {
+        var queryItems = self.urlComponents.queryItems ?? []
+        queryItems.append(URLQueryItem(name: item.name, value: item.value))
+        self.urlComponents.queryItems = queryItems
         return self
     }
 
     @discardableResult
-    public func withPassword(_ value: String) -> URIBuilder {
-        self.password = value
-        self.urlComponents.password = self.password
+    public func withUsername(_ value: String?) -> URIBuilder {
+        self.urlComponents.user = value
+        return self
+    }
+
+    @discardableResult
+    public func withPassword(_ value: String?) -> URIBuilder {
+        self.urlComponents.password = value
         return self
     }
 
     public func build() -> URI {
-        return URI(scheme: self.scheme,
+        return URI(scheme: Scheme(rawValue: self.urlComponents.scheme!)!,
                    path: self.path,
-                   host: self.host,
+                   host: self.urlComponents.host!,
                    port: self.port,
-                   query: self.query,
-                   username: self.username,
-                   password: self.password)
+                   queryItems: self.urlComponents.queryItems?.map { item in
+                       return SDKURLQueryItem(name: item.name, value: item.value)
+                   } ?? [],
+                   username: self.urlComponents.user,
+                   password: self.urlComponents.password)
     }
 
     // We still have to keep 'url' as an optional, since we're
     // dealing with dynamic components that could be invalid.
     fileprivate func getUrl() -> URL? {
-        return self.path.isEmpty && self.host.isEmpty ? nil : self.urlComponents.url
+        let isInvalidHost = self.urlComponents.host?.isEmpty ?? false
+        return isInvalidHost && self.urlComponents.path.isEmpty ? nil : self.urlComponents.url
+    }
+}
+
+extension String {
+    var isPercentEncoded: Bool {
+        let decoded = self.removingPercentEncoding
+        return decoded != nil && decoded != self
     }
 }
diff --git a/Sources/SmithyTestUtil/RequestTestUtil/ExpectedSdkHttpRequest.swift b/Sources/SmithyTestUtil/RequestTestUtil/ExpectedSdkHttpRequest.swift
@@ -12,7 +12,7 @@ public struct ExpectedSdkHttpRequest {
     public var headers: Headers?
     public var forbiddenHeaders: [String]?
     public var requiredHeaders: [String]?
-    public var queryItems: [SDKURLQueryItem] { endpoint.uri.query }
+    public var queryItems: [SDKURLQueryItem] { endpoint.uri.queryItems }
     public let forbiddenQueryItems: [SDKURLQueryItem]?
     public let requiredQueryItems: [SDKURLQueryItem]?
     public let endpoint: Endpoint

diff --git a/Tests/ClientRuntimeTests/NetworkingTests/EndpointTests.swift b/Tests/ClientRuntimeTests/NetworkingTests/EndpointTests.swift
@@ -19,7 +19,7 @@ class EndpointTests: XCTestCase {
             SDKURLQueryItem(name: "ghi", value: "jkl"),
             SDKURLQueryItem(name: "mno", value: "pqr")
         ]
-        XCTAssertEqual(endpoint.uri.query, expectedQueryItems)
+        XCTAssertEqual(endpoint.uri.queryItems, expectedQueryItems)
     }
 
     func test_hashableAndEquatable_hashesMatchWhenURLQueryItemsAreEqual() throws {

diff --git a/Tests/ClientRuntimeTests/NetworkingTests/Http/SdkRequestBuilderTests.swift b/Tests/ClientRuntimeTests/NetworkingTests/Http/SdkRequestBuilderTests.swift
@@ -16,9 +16,9 @@ class SdkRequestBuilderTests: XCTestCase {
         let crtRequest = try HTTPRequest()
         crtRequest.path = pathToMatch
 
-        let updatedRequest = try SdkHttpRequestBuilder().update(from: crtRequest, originalRequest: originalRequest).build()
+        let updatedRequest = SdkHttpRequestBuilder().update(from: crtRequest, originalRequest: originalRequest).build()
         let updatedPath = [updatedRequest.destination.path, updatedRequest.destination.queryString].compactMap { $0 }.joined(separator: "?")
         XCTAssertEqual(pathToMatch, updatedPath)
-        XCTAssertEqual(url, updatedRequest.destination.url?.absoluteString)
+        XCTAssertEqual(url, updatedRequest.destination.url?.absoluteString.removingPercentEncoding)
     }
 }
diff --git a/Tests/SmithyTestUtilTests/RequestTestUtilTests/HttpRequestTestBaseTests.swift b/Tests/SmithyTestUtilTests/RequestTestUtilTests/HttpRequestTestBaseTests.swift
@@ -196,7 +196,7 @@ class HttpRequestTestBaseTests: HttpRequestTestBase {
             let forbiddenQueryParams = ["ForbiddenQuery"]
             for forbiddenQueryParam in forbiddenQueryParams {
                 XCTAssertFalse(
-                    self.queryItemExists(forbiddenQueryParam, in: actual.destination.query),
+                    self.queryItemExists(forbiddenQueryParam, in: actual.destination.queryItems),
                     "Forbidden Query:\(forbiddenQueryParam) exists in query items"
                 )
             }
@@ -208,7 +208,7 @@ class HttpRequestTestBaseTests: HttpRequestTestBase {
 
             let requiredQueryParams = ["RequiredQuery"]
             for requiredQueryParam in requiredQueryParams {
-                XCTAssertTrue(self.queryItemExists(requiredQueryParam, in: actual.destination.query),
+                XCTAssertTrue(self.queryItemExists(requiredQueryParam, in: actual.destination.queryItems),
                               "Required Query:\(requiredQueryParam) does not exist in query items")
             }