Unify query and url-encoded-form implementation (#694)

crazytonyli · web-flow · commit 5fba5e8cb65c · 2024-01-24T12:17:54.000+13:00
diff --git a/WordPressKit/HTTPRequestBuilder.swift b/WordPressKit/HTTPRequestBuilder.swift
@@ -1,5 +1,9 @@
 import Foundation
 
+/// A builder type that appends HTTP request data to a URL.
+///
+/// Calling this class's url related functions (the ones that changes path, query, etc) does not modify the
+/// original URL string. The URL will be perserved in the final result that's returned by the `build` function.
 final class HTTPRequestBuilder {
     enum Method: String {
         case get = "GET"
@@ -13,17 +17,20 @@ final class HTTPRequestBuilder {
         }
     }
 
-    private var urlComponents: URLComponents
+    private let original: URLComponents
     private var method: Method = .get
+    private var appendedPath: String = ""
     private var headers: [String: String] = [:]
+    private var defaultQuery: [URLQueryItem] = []
+    private var appendedQuery: [URLQueryItem] = []
     private var bodyBuilder: ((inout URLRequest) throws -> Void)?
     private(set) var multipartForm: [MultipartFormField]?
 
     init(url: URL) {
         assert(url.scheme == "http" || url.scheme == "https")
         assert(url.host != nil)
 
-        urlComponents = URLComponents(url: url, resolvingAgainstBaseURL: true)!
+        original = URLComponents(url: url, resolvingAgainstBaseURL: true)!
     }
 
     func method(_ method: Method) -> Self {
@@ -34,16 +41,7 @@ final class HTTPRequestBuilder {
     func append(path: String) -> Self {
         assert(!path.contains("?") && !path.contains("#"), "Path should not have query or fragment: \(path)")
 
-        var relPath = path
-        if relPath.hasPrefix("/") {
-            _ = relPath.removeFirst()
-        }
-
-        if urlComponents.path.hasSuffix("/") {
-            urlComponents.path = urlComponents.path.appending(relPath)
-        } else {
-            urlComponents.path = urlComponents.path.appending("/").appending(relPath)
-        }
+        appendedPath = Self.join(appendedPath, path)
 
         return self
     }
@@ -53,32 +51,34 @@ final class HTTPRequestBuilder {
         return self
     }
 
+    func query(defaults: [URLQueryItem]) -> Self {
+        defaultQuery = defaults
+        return self
+    }
+
     func query(name: String, value: String?, override: Bool = false) -> Self {
         append(query: [URLQueryItem(name: name, value: value)], override: override)
     }
 
-    func append(query: [URLQueryItem], override: Bool = false) -> Self {
-        var allQuery = urlComponents.queryItems ?? []
+    func query(_ parameters: [String: Any]) -> Self {
+        append(query: parameters.flatten(), override: false)
+    }
 
+    func append(query: [URLQueryItem], override: Bool = false) -> Self {
         if override {
             let newKeys = Set(query.map { $0.name })
-            allQuery.removeAll(where: { newKeys.contains($0.name) })
+            appendedQuery.removeAll(where: { newKeys.contains($0.name) })
         }
 
-        allQuery.append(contentsOf: query)
-
-        urlComponents.queryItems = allQuery
+        appendedQuery.append(contentsOf: query)
 
         return self
     }
 
-    func body(form: [String: String]) -> Self {
+    func body(form: [String: Any]) -> Self {
         headers["Content-Type"] = "application/x-www-form-urlencoded; charset=utf-8"
         bodyBuilder = { req in
-            let content = form.map {
-                    "\(HTTPRequestBuilder.urlEncode($0))=\(HTTPRequestBuilder.urlEncode($1))"
-                }
-                .joined(separator: "&")
+            let content = form.flatten().percentEncoded
             req.httpBody = content.data(using: .utf8)
         }
         return self
@@ -119,7 +119,29 @@ final class HTTPRequestBuilder {
     }
 
     func build(encodeMultipartForm: Bool = false) throws -> URLRequest {
-        guard let url = urlComponents.url else {
+        var components = original
+
+        var newPath = Self.join(components.path, appendedPath)
+        if !newPath.isEmpty, !newPath.hasPrefix("/") {
+            newPath = "/\(newPath)"
+        }
+        components.path = newPath
+
+        // Add default query items if they don't exist in `appendedQuery`.
+        var newQuery = appendedQuery
+        if !defaultQuery.isEmpty {
+            let toBeAdded = defaultQuery.filter { item in
+                !newQuery.contains(where: { $0.name == item.name})
+            }
+            newQuery.append(contentsOf: toBeAdded)
+        }
+
+        // Bypass `URLComponents`'s URL query encoding, use our own implementation instead.
+        if !newQuery.isEmpty {
+            components.percentEncodedQuery = Self.join(components.percentEncodedQuery ?? "", newQuery.percentEncoded, separator: "&")
+        }
+
+        guard let url = components.url else {
             throw URLError(.badURL)
         }
 
@@ -175,10 +197,80 @@ extension HTTPRequestBuilder {
     }
 }
 
-private extension HTTPRequestBuilder {
+extension HTTPRequestBuilder {
     static func urlEncode(_ text: String) -> String {
         let specialCharacters = ":#[]@!$&'()*+,;="
         let allowed = CharacterSet.urlQueryAllowed.subtracting(.init(charactersIn: specialCharacters))
         return text.addingPercentEncoding(withAllowedCharacters: allowed) ?? text
     }
+
+    /// Join a list of strings using a separator only if neighbour items aren't already separated with the given separator.
+    static func join(_ aList: String..., separator: String = "/") -> String {
+        guard !aList.isEmpty else { return "" }
+
+        var list = aList
+        let start = list.removeFirst()
+        return list.reduce(into: start) { result, path in
+            guard !path.isEmpty else { return }
+
+            guard !result.isEmpty else {
+                result = path
+                return
+            }
+
+            switch (result.hasSuffix(separator), path.hasPrefix(separator)) {
+            case (true, true):
+                var prefixRemoved = path
+                prefixRemoved.removePrefix(separator)
+                result.append(prefixRemoved)
+            case (true, false), (false, true):
+                result.append(path)
+            case (false, false):
+                result.append("\(separator)\(path)")
+            }
+        }
+    }
+}
+
+private extension Dictionary where Key == String, Value == Any {
+
+    static func urlEncode(into result: inout [URLQueryItem], name: String, value: Any) {
+        switch value {
+        case let array as [Any]:
+            for value in array {
+                urlEncode(into: &result, name: "\(name)[]", value: value)
+            }
+        case let object as [String: Any]:
+            for (key, value) in object {
+                urlEncode(into: &result, name: "\(name)[\(key)]", value: value)
+            }
+        case let value as Bool:
+            urlEncode(into: &result, name: name, value: value ? "1" : "0")
+        default:
+            result.append(URLQueryItem(name: name, value: "\(value)"))
+        }
+    }
+
+    func flatten() -> [URLQueryItem] {
+        reduce(into: []) { result, entry in
+            Self.urlEncode(into: &result, name: entry.key, value: entry.value)
+        }
+    }
+
+}
+
+extension Array where Element == URLQueryItem {
+
+    var percentEncoded: String {
+        map {
+            let name = HTTPRequestBuilder.urlEncode($0.name)
+            guard let value = $0.value else {
+                return name
+            }
+
+            return "\(name)=\(HTTPRequestBuilder.urlEncode(value))"
+        }
+        .joined(separator: "&")
+    }
+
 }
diff --git a/WordPressKitTests/Utilities/HTTPRequestBuilderTests.swift b/WordPressKitTests/Utilities/HTTPRequestBuilderTests.swift
@@ -5,6 +5,43 @@ import XCTest
 
 class HTTPRequestBuilderTests: XCTestCase {
 
+    static let nestedParameters: [String: Any] =
+        [
+            "number": 1,
+            "nsnumber-true": NSNumber(value: true),
+            "true": true,
+            "false": false,
+            "string": "true",
+            "dict": ["foo": true, "bar": "string"],
+            "nested-dict": [
+                "outer1": [
+                    "inner1": "value1",
+                    "inner2": "value2"
+                ],
+                "outer2": [
+                    "inner1": "value1",
+                    "inner2": "value2"
+                ]
+            ],
+            "array": ["true", 1, false]
+        ]
+    static let nestedParametersEncoded = [
+        "number=1",
+        "nsnumber-true=1",
+        "true=1",
+        "false=0",
+        "string=true",
+        "dict[foo]=1",
+        "dict[bar]=string",
+        "nested-dict[outer1][inner1]=value1",
+        "nested-dict[outer1][inner2]=value2",
+        "nested-dict[outer2][inner1]=value1",
+        "nested-dict[outer2][inner2]=value2",
+        "array[]=true",
+        "array[]=1",
+        "array[]=0",
+    ]
+
     func testURL() throws {
         try XCTAssertEqual(HTTPRequestBuilder(url: URL(string: "https://wordpress.org")!).build().url?.absoluteString, "https://wordpress.org")
         try XCTAssertEqual(HTTPRequestBuilder(url: URL(string: "https://wordpress.com")!).build().url?.absoluteString, "https://wordpress.com")
@@ -140,6 +177,33 @@ class HTTPRequestBuilderTests: XCTestCase {
         )
     }
 
+    @available(iOS 16.0, *)
+    func testSetQueryWithDictionary() throws {
+        let query = try HTTPRequestBuilder(url: URL(string: "https://wordpress.org")!)
+            .query(HTTPRequestBuilderTests.nestedParameters)
+            .build()
+            .url?
+            .query(percentEncoded: false)?
+            .split(separator: "&")
+            .reduce(into: Set()) { $0.insert(String($1)) }
+            ?? []
+
+        XCTAssertEqual(query.count, HTTPRequestBuilderTests.nestedParametersEncoded.count)
+
+        for item in HTTPRequestBuilderTests.nestedParametersEncoded {
+            XCTAssertTrue(query.contains(item), "Missing query item: \(item)")
+        }
+    }
+
+    func testDefaultQuery() throws {
+        let builder = HTTPRequestBuilder(url: URL(string: "https://wordpress.org")!)
+            .query(defaults: [URLQueryItem(name: "locale", value: "en")])
+
+        try XCTAssertEqual(builder.build().url?.query, "locale=en")
+        try XCTAssertEqual(builder.query(name: "locale", value: "zh").build().url?.query, "locale=zh")
+        try XCTAssertEqual(builder.query(name: "foo", value: "bar").build().url?.query, "locale=zh&foo=bar")
+    }
+
     func testJSONBody() throws {
         var request = try HTTPRequestBuilder(url: URL(string: "https://wordpress.org")!)
             .method(.post)
@@ -225,6 +289,41 @@ class HTTPRequestBuilderTests: XCTestCase {
         XCTAssertEqual(form, decodedForm)
     }
 
+    func testJoin() throws {
+        XCTAssertEqual(HTTPRequestBuilder.join("foo", "bar"), "foo/bar")
+        XCTAssertEqual(HTTPRequestBuilder.join("foo/", "bar"), "foo/bar")
+        XCTAssertEqual(HTTPRequestBuilder.join("foo", "/bar"), "foo/bar")
+        XCTAssertEqual(HTTPRequestBuilder.join("foo/", "/bar"), "foo/bar")
+        XCTAssertEqual(HTTPRequestBuilder.join("foo=1", "bar=2", separator: "&"), "foo=1&bar=2")
+        XCTAssertEqual(HTTPRequestBuilder.join("foo=1/", "bar=2", separator: "&"), "foo=1/&bar=2")
+        XCTAssertEqual(HTTPRequestBuilder.join("foo=1/", "&bar=2", separator: "&"), "foo=1/&bar=2")
+
+        XCTAssertEqual(HTTPRequestBuilder.join("", "foo"), "foo")
+        XCTAssertEqual(HTTPRequestBuilder.join("foo", ""), "foo")
+        XCTAssertEqual(HTTPRequestBuilder.join("foo", "/"), "foo/")
+        XCTAssertEqual(HTTPRequestBuilder.join("/", "/foo"), "/foo")
+        XCTAssertEqual(HTTPRequestBuilder.join("", "/foo"), "/foo")
+    }
+
+    func testPreserveOriginalURL() throws {
+        try XCTAssertEqual(
+            HTTPRequestBuilder(url: URL(string: "https://wordpress.org/api?locale=en")!)
+                .query(name: "locale", value: "zh")
+                .build()
+                .url?
+                .query,
+            "locale=en&locale=zh"
+        )
+        try XCTAssertEqual(
+            HTTPRequestBuilder(url: URL(string: "https://wordpress.org/api?locale=en")!)
+                .query(name: "foo", value: "bar")
+                .build()
+                .url?
+                .query,
+            "locale=en&foo=bar"
+        )
+    }
+
     func testMultipartForm() throws {
         XCTAssertNotNil(
             try HTTPRequestBuilder(url: URL(string: "https://wordpress.org")!)
diff --git a/WordPressKitTests/WordPressComRestApiTests.swift b/WordPressKitTests/WordPressComRestApiTests.swift
@@ -60,6 +60,38 @@ class WordPressComRestApiTests: XCTestCase {
         }
     }
 
+    @available(iOS 16.0, *)
+    func testQuery() {
+        var requestURL: URL?
+        stub(condition: isRestAPIRequest()) {
+            requestURL = $0.url
+            return HTTPStubsResponse(error: URLError(URLError.Code.networkConnectionLost))
+        }
+
+        let expect = self.expectation(description: "One callback should be invoked")
+        let api = WordPressComRestApi(oAuthToken: "fakeToken")
+        api.GET(
+            wordPressMediaRoutePath,
+            parameters: HTTPRequestBuilderTests.nestedParameters as [String: AnyObject],
+            success: { _, _ in expect.fulfill() },
+            failure: { (_, _) in expect.fulfill() }
+        )
+        wait(for: [expect], timeout: 0.1)
+
+        let query = requestURL?
+            .query(percentEncoded: false)?
+            .split(separator: "&")
+            .reduce(into: Set()) { $0.insert(String($1)) }
+            ?? []
+        let expected = HTTPRequestBuilderTests.nestedParametersEncoded + ["locale=en"]
+
+        XCTAssertEqual(query.count, expected.count)
+
+        for item in expected {
+            XCTAssertTrue(query.contains(item), "Missing query item: \(item)")
+        }
+    }
+
     func testSuccessfullCall() {
         stub(condition: isRestAPIRequest()) { _ in
             let stubPath = OHPathForFile("WordPressComRestApiMedia.json", type(of: self))
