diff --git a/Sources/Kitura/CodableRouter+TypeSafeMiddleware.swift b/Sources/Kitura/CodableRouter+TypeSafeMiddleware.swift index 41ad6b4e6..f61fb7180 100644 --- a/Sources/Kitura/CodableRouter+TypeSafeMiddleware.swift +++ b/Sources/Kitura/CodableRouter+TypeSafeMiddleware.swift @@ -277,7 +277,7 @@ extension Router { return } registerGetRoute(route: route, id: Id.self, outputType: O.self) - get(join(path: route, with: ":id")) { request, response, next in + get(appendId(path: route)) { request, response, next in Log.verbose("Received GET (singular with identifier and middleware) type-safe request") self.handleMiddleware(T.self, request: request, response: response) { typeSafeMiddleware in guard let typeSafeMiddleware = typeSafeMiddleware else { @@ -318,7 +318,7 @@ extension Router { return } registerGetRoute(route: route, id: Id.self, outputType: O.self) - get(join(path: route, with: ":id")) { request, response, next in + get(appendId(path: route)) { request, response, next in Log.verbose("Received GET (singular with identifier and middleware) type-safe request") self.handleMiddleware(T1.self, T2.self, request: request, response: response) { typeSafeMiddleware1, typeSafeMiddleware2 in guard let typeSafeMiddleware1 = typeSafeMiddleware1, let typeSafeMiddleware2 = typeSafeMiddleware2 else { @@ -359,7 +359,7 @@ extension Router { return } registerGetRoute(route: route, id: Id.self, outputType: O.self) - get(join(path: route, with: ":id")) { request, response, next in + get(appendId(path: route)) { request, response, next in Log.verbose("Received GET (singular with identifier and middleware) type-safe request") self.handleMiddleware(T1.self, T2.self, T3.self, request: request, response: response) { typeSafeMiddleware1, typeSafeMiddleware2, typeSafeMiddleware3 in guard let typeSafeMiddleware1 = typeSafeMiddleware1, let typeSafeMiddleware2 = typeSafeMiddleware2, let typeSafeMiddleware3 = typeSafeMiddleware3 else { @@ -1069,7 +1069,7 @@ extension Router { return } registerDeleteRoute(route: route, id: Id.self) - delete(join(path: route, with: ":id")) { request, response, next in + delete(appendId(path: route)) { request, response, next in Log.verbose("Received DELETE (singular with middleware) type-safe request") self.handleMiddleware(T.self, request: request, response: response) { typeSafeMiddleware in guard let typeSafeMiddleware = typeSafeMiddleware else { @@ -1108,7 +1108,7 @@ extension Router { return } registerDeleteRoute(route: route, id: Id.self) - delete(join(path: route, with: ":id")) { request, response, next in + delete(appendId(path: route)) { request, response, next in Log.verbose("Received DELETE (singular with middleware) type-safe request") self.handleMiddleware(T1.self, T2.self, request: request, response: response) { typeSafeMiddleware1, typeSafeMiddleware2 in guard let typeSafeMiddleware1 = typeSafeMiddleware1, let typeSafeMiddleware2 = typeSafeMiddleware2 else { @@ -1147,7 +1147,7 @@ extension Router { return } registerDeleteRoute(route: route, id: Id.self) - delete(join(path: route, with: ":id")) { request, response, next in + delete(appendId(path: route)) { request, response, next in Log.verbose("Received DELETE (singular with middleware) type-safe request") self.handleMiddleware(T1.self, T2.self, T3.self, request: request, response: response) { typeSafeMiddleware1, typeSafeMiddleware2, typeSafeMiddleware3 in guard let typeSafeMiddleware1 = typeSafeMiddleware1, let typeSafeMiddleware2 = typeSafeMiddleware2, let typeSafeMiddleware3 = typeSafeMiddleware3 else { @@ -1738,7 +1738,7 @@ extension Router { return } registerPutRoute(route: route, id: Id.self, inputType: I.self, outputType: O.self) - put(join(path: route, with: ":id")) { request, response, next in + put(appendId(path: route)) { request, response, next in Log.verbose("Received PUT type-safe request") self.handleMiddleware(T.self, request: request, response: response) { typeSafeMiddleware in guard let typeSafeMiddleware = typeSafeMiddleware else { @@ -1779,7 +1779,7 @@ extension Router { return } registerPutRoute(route: route, id: Id.self, inputType: I.self, outputType: O.self) - put(join(path: route, with: ":id")) { request, response, next in + put(appendId(path: route)) { request, response, next in Log.verbose("Received PUT type-safe request") self.handleMiddleware(T1.self, T2.self, request: request, response: response) { typeSafeMiddleware1, typeSafeMiddleware2 in guard let typeSafeMiddleware1 = typeSafeMiddleware1, let typeSafeMiddleware2 = typeSafeMiddleware2 else { @@ -1820,7 +1820,7 @@ extension Router { return } registerPutRoute(route: route, id: Id.self, inputType: I.self, outputType: O.self) - put(join(path: route, with: ":id")) { request, response, next in + put(appendId(path: route)) { request, response, next in Log.verbose("Received PUT type-safe request") self.handleMiddleware(T1.self, T2.self, T3.self, request: request, response: response) { typeSafeMiddleware1, typeSafeMiddleware2, typeSafeMiddleware3 in guard let typeSafeMiddleware1 = typeSafeMiddleware1, let typeSafeMiddleware2 = typeSafeMiddleware2, let typeSafeMiddleware3 = typeSafeMiddleware3 else { @@ -1872,7 +1872,7 @@ extension Router { return } registerPatchRoute(route: route, id: Id.self, inputType: I.self, outputType: O.self) - patch(join(path: route, with: ":id")) { request, response, next in + patch(appendId(path: route)) { request, response, next in Log.verbose("Received PATCH type-safe request") self.handleMiddleware(T.self, request: request, response: response) { typeSafeMiddleware in guard let typeSafeMiddleware = typeSafeMiddleware else { @@ -1919,7 +1919,7 @@ extension Router { return } registerPatchRoute(route: route, id: Id.self, inputType: I.self, outputType: O.self) - patch(join(path: route, with: ":id")) { request, response, next in + patch(appendId(path: route)) { request, response, next in Log.verbose("Received PATCH type-safe request") self.handleMiddleware(T1.self, T2.self, request: request, response: response) { typeSafeMiddleware1, typeSafeMiddleware2 in guard let typeSafeMiddleware1 = typeSafeMiddleware1, let typeSafeMiddleware2 = typeSafeMiddleware2 else { @@ -1965,7 +1965,7 @@ extension Router { return } registerPatchRoute(route: route, id: Id.self, inputType: I.self, outputType: O.self) - patch(join(path: route, with: ":id")) { request, response, next in + patch(appendId(path: route)) { request, response, next in Log.verbose("Received PATCH type-safe request") self.handleMiddleware(T1.self, T2.self, T3.self, request: request, response: response) { typeSafeMiddleware1, typeSafeMiddleware2, typeSafeMiddleware3 in guard let typeSafeMiddleware1 = typeSafeMiddleware1, let typeSafeMiddleware2 = typeSafeMiddleware2, let typeSafeMiddleware3 = typeSafeMiddleware3 else { diff --git a/Sources/Kitura/CodableRouter.swift b/Sources/Kitura/CodableRouter.swift index 795470036..365967d5c 100644 --- a/Sources/Kitura/CodableRouter.swift +++ b/Sources/Kitura/CodableRouter.swift @@ -387,7 +387,7 @@ extension Router { return } registerPutRoute(route: route, id: Id.self, inputType: I.self, outputType: O.self) - put(join(path: route, with: ":id")) { request, response, next in + put(appendId(path: route)) { request, response, next in Log.verbose("Received PUT type-safe request") guard let identifier = CodableHelpers.parseIdOrSetResponseStatus(Id.self, from: request, response: response), let codableInput = CodableHelpers.readCodableOrSetResponseStatus(I.self, from: request, response: response) @@ -405,7 +405,7 @@ extension Router { return } registerPatchRoute(route: route, id: Id.self, inputType: I.self, outputType: O.self) - patch(join(path: route, with: ":id")) { request, response, next in + patch(appendId(path: route)) { request, response, next in Log.verbose("Received PATCH type-safe request") guard let identifier = CodableHelpers.parseIdOrSetResponseStatus(Id.self, from: request, response: response), let codableInput = CodableHelpers.readCodableOrSetResponseStatus(I.self, from: request, response: response) @@ -536,7 +536,7 @@ extension Router { return } registerGetRoute(route: route, id: Id.self, outputType: O.self) - get(join(path: route, with: ":id")) { request, response, next in + get(appendId(path: route)) { request, response, next in Log.verbose("Received GET (singular with identifier) type-safe request") guard let identifier = CodableHelpers.parseIdOrSetResponseStatus(Id.self, from: request, response: response) else { next() @@ -564,7 +564,7 @@ extension Router { return } registerDeleteRoute(route: route, id: Id.self) - delete(join(path: route, with: ":id")) { request, response, next in + delete(appendId(path: route)) { request, response, next in Log.verbose("Received DELETE (singular) type-safe request") guard let identifier = CodableHelpers.parseIdOrSetResponseStatus(Id.self, from: request, response: response) else { next() @@ -641,7 +641,18 @@ extension Router { } } - internal func join(path base: String, with component: String) -> String { + /// Append the `:id` parameter to a path that does not already contain a parameter. + /// If the path already contains a parameter, it will be returned unmodified. + internal func appendId(path base: String) -> String { + let identifierSupplied = base.contains(":") + guard !identifierSupplied else { + return base + } + return joinPath(base, with: ":id") + } + + /// Join two path components together such that they are separated by a single `/`. + private func joinPath(_ base: String, with component: String) -> String { let strippedBase = base.hasSuffix("/") ? String(base.dropLast()) : base let strippedComponent = component.hasPrefix("/") ? String(component.dropFirst()) : component return "\(strippedBase)/\(strippedComponent)" diff --git a/Tests/KituraTests/TestCodablePathParams.swift b/Tests/KituraTests/TestCodablePathParams.swift new file mode 100644 index 000000000..23325dcde --- /dev/null +++ b/Tests/KituraTests/TestCodablePathParams.swift @@ -0,0 +1,212 @@ +/** + * Copyright IBM Corporation 2017 + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + **/ + +import XCTest +import Foundation +import KituraContracts + +@testable import Kitura + +final class TestCodablePathParams: KituraTest, KituraTestSuite { + static var allTests: [(String, (TestCodablePathParams) -> () throws -> Void)] { + return [ + ("testJoinPath", testJoinPath), + ("testRouteWithTrailingSlash", testRouteWithTrailingSlash), + ("testInvalidRouteParameters", testInvalidRouteParameters), + ("testIdentifierNotExpected", testIdentifierNotExpected), + ("testPartialIdentifierSupplied", testPartialIdentifierSupplied), + ("testIdentifierNotSupplied", testIdentifierNotSupplied), + ("testIdentifierSupplied", testIdentifierSupplied), + ] + } + + // Need to initialise to avoid compiler error + var router = Router() + + // Reset for each test + override func setUp() { + super.setUp() // Initialize logging + router = Router() + } + + struct Fruit: Codable, Equatable { + let name: String + let id: Int + + static func == (lhs: Fruit, rhs: Fruit) -> Bool { + return lhs.name == rhs.name && lhs.id == rhs.id + } + } + + func testJoinPath() { + let router = Router() + // Implicit append of :id + XCTAssertEqual(router.appendId(path: "a"), "a/:id") + XCTAssertEqual(router.appendId(path: "a/"), "a/:id") + // User already specified :id + XCTAssertEqual(router.appendId(path: "a/:id"), "a/:id") + // User specified a different identifier name (not supported) + XCTAssertEqual(router.appendId(path: "a/:foo"), "a/:foo") + } + + // Test adding a trailing slash to your route when it has an implicit id parameter + func testRouteWithTrailingSlash() { + router.get("/fruit/") { (id: Int, respondWith: (Fruit?, RequestError?) -> Void) in + respondWith(Fruit(name: "apple", id: id), nil) + } + router.put("/fruit/") { (id: Int, fruit: Fruit, respondWith: (Fruit?, RequestError?) -> Void) in + respondWith(Fruit(name: fruit.name, id: id), nil) + } + router.patch("/fruit/") { (id: Int, fruit: Fruit, respondWith: (Fruit?, RequestError?) -> Void) in + respondWith(Fruit(name: fruit.name, id: id), nil) + } + router.delete("/fruit/") { (id: Int, respondWith: (RequestError?) -> Void) in + XCTAssertEqual(id, 1) + respondWith(nil) + } + let apple = Fruit(name: "apple", id: 1) + let banana = Fruit(name: "banana", id: 2) + + buildServerTest(router, timeout: 30) + .request("get", path: "/fruit/1").hasStatus(.OK).hasContentType(withPrefix: "application/json").hasData(apple) + .request("put", path: "/fruit/2", data: banana).hasStatus(.OK).hasContentType(withPrefix: "application/json").hasData(banana) + .request("patch", path: "/fruit/2", data: banana).hasStatus(.OK).hasContentType(withPrefix: "application/json").hasData(banana) + .request("delete", path: "/fruit/1").hasStatus(.noContent).hasNoData() + .run() + } + + // Test that routes containing a path parameter that is not `:id` are + // correctly rejected. + func testInvalidRouteParameters() { + // These routes should fail to be registered. + router.get("/fruit/:myid") { (id: Int, respondWith: (Fruit?, RequestError?) -> Void) in + XCTFail("GET on /fruit/:myid that should not happen") + let result = Fruit(name: "banana", id: 1) + respondWith(result, nil) + } + router.delete("/fruit/:myid") { (id: Int, respondWith: (RequestError?) -> Void) in + XCTFail("DELETE on /fruit/:myid that should not happen") + respondWith(.badRequest) + } + + buildServerTest(router, timeout: 30) + .request("get", path: "/fruit/1") + .hasStatus(.notFound) + .hasData() + + .request("delete", path: "/fruit/1") + .hasStatus(.notFound) + .hasData() + + .run() + } + + // Test that routes with trailing :id for a DELETE (plural) or GET (plural) + // handler are correctly rejected. + func testIdentifierNotExpected() { + // These routes should fail to be registered. + router.delete("/fruit/:id") { (respondWith: (RequestError?) -> Void) in + XCTFail("DELETE (plural) on /fruit/:id that should not happen") + respondWith(.badRequest) + } + router.get("/fruit/:id") { (respondWith: ([Fruit]?, RequestError?) -> Void) in + XCTFail("GET (plural) on /fruit/:id that should not happen") + } + + buildServerTest(router, timeout: 30) + .request("delete", path: "/fruit/1") + .hasStatus(.notFound) + .hasData() + + .request("get", path: "/fruit/1") + .hasStatus(.notFound) + .hasData() + + .run() + } + + // Test that a route with a partial identifier is rejected. + func testPartialIdentifierSupplied() { + // This route should fail to be registered. + router.delete("/status/:") { (id: Int, respondWith: (RequestError?) -> Void) in + XCTFail("DELETE on /status/: that should not happen") + respondWith(.badRequest) + } + + buildServerTest(router, timeout: 30) + .request("delete", path: "/status/1") + .hasStatus(.notFound) + .hasData() + .run() + } + + // A trailing :id parameter is allowed, but if not supplied will be added + // implicitly (with a warning to signal to the user that this has occurred). + func testIdentifierNotSupplied() { + router.delete("/fruit/") { (id: Int, respondWith: (RequestError?) -> Void) in + print("DELETE on /fruit/ (implicit :id)") + XCTAssertEqual(id, 1) + respondWith(nil) + } + router.get("/fruit") { (id: Int, respondWith: (Fruit?, RequestError?) -> Void) in + print("GET on /fruit (implicit :id)") + respondWith(Fruit(name: "banana", id: id), nil) + } + + let banana = Fruit(name: "banana", id: 10) + + buildServerTest(router, timeout: 30) + .request("delete", path: "/fruit/1") + .hasStatus(.noContent) + .hasNoData() + + .request("get", path: "/fruit/10") + .hasStatus(.OK) + .hasData(banana) + + .run() + } + + // Test added to address fix for https://github.com/IBM-Swift/Kitura/issues/1473 + // Tests that a GET (singular) or DELETE (singular) with explicit :id parameter + // is successful. + // A trailing :id parameter is allowed, and replaces the :id parameter that + // would otherwise be added implicitly. + func testIdentifierSupplied() { + router.get("/fruit/:id") { (id: Int, respondWith: (Fruit?, RequestError?) -> Void) in + print("GET on /fruit/:id") + respondWith(Fruit(name: "banana", id: id), nil) + } + router.delete("/fruit/:id") { (id: Int, respondWith: (RequestError?) -> Void) in + print("DELETE on /fruit/:id") + XCTAssertEqual(id, 1) + respondWith(nil) + } + let banana = Fruit(name: "banana", id: 20) + + buildServerTest(router, timeout: 30) + .request("get", path: "/fruit/20") + .hasStatus(.OK) + .hasData(banana) + + .request("delete", path: "/fruit/1") + .hasStatus(.noContent) + .hasNoData() + + .run() + } + +} diff --git a/Tests/KituraTests/TestCodableRouter.swift b/Tests/KituraTests/TestCodableRouter.swift index d9c262d21..bfd193b84 100644 --- a/Tests/KituraTests/TestCodableRouter.swift +++ b/Tests/KituraTests/TestCodableRouter.swift @@ -33,10 +33,7 @@ final class TestCodableRouter: KituraTest, KituraTestSuite { ("testBasicDeleteSingle", testBasicDeleteSingle), ("testBasicPut", testBasicPut), ("testBasicPatch", testBasicPatch), - ("testJoinPath", testJoinPath), - ("testRouteWithTrailingSlash", testRouteWithTrailingSlash), ("testErrorOverridesBody", testErrorOverridesBody), // Slow compile on 5.1 - ("testRouteParameters", testRouteParameters), ("testCodableRoutesWithBodyParsingFail", testCodableRoutesWithBodyParsingFail), ("testCodableGetSingleQueryParameters", testCodableGetSingleQueryParameters), ("testCodableGetArrayQueryParameters", testCodableGetArrayQueryParameters), @@ -44,10 +41,6 @@ final class TestCodableRouter: KituraTest, KituraTestSuite { ("testCodablePostSuccessStatuses", testCodablePostSuccessStatuses), ("testNoDataCustomStatus", testNoDataCustomStatus), ("testNoDataDefaultStatus", testNoDataDefaultStatus), - ("testInvalidIdentifierSupplied", testInvalidIdentifierSupplied), - ("testIdentifierNotExpected", testIdentifierNotExpected), - ("testPartialIdentifierSupplied", testPartialIdentifierSupplied), - ("testIdentifierNotSupplied", testIdentifierNotSupplied), ] } @@ -602,30 +595,6 @@ final class TestCodableRouter: KituraTest, KituraTestSuite { .run() } - func testJoinPath() { - let router = Router() - XCTAssertEqual(router.join(path: "a", with: "b"), "a/b") - XCTAssertEqual(router.join(path: "a/", with: "/b"), "a/b") - XCTAssertEqual(router.join(path: "a", with: "/b"), "a/b") - XCTAssertEqual(router.join(path: "a/", with: "b"), "a/b") - } - - // Test adding a trailing slash to your route when it has an implicit id parameter - func testRouteWithTrailingSlash() { - let status = Status("Slashes work as expected") - router.get("/status/") { (id: Int, respondWith: (Status?, RequestError?) -> Void) in respondWith(status, nil) } - router.put("/status/") { (id: Int, status: Status, respondWith: (Status?, RequestError?) -> Void) in respondWith(status, nil) } - router.patch("/status/") { (id: Int, status: Status, respondWith: (Status?, RequestError?) -> Void) in respondWith(status, nil) } - router.delete("/status/") { (id: Int, respondWith: (RequestError?) -> Void) in respondWith(nil) } - - buildServerTest(router, timeout: 30) - .request("get", path: "/status/1").hasStatus(.OK).hasContentType(withPrefix: "application/json").hasData(status) - .request("put", path: "/status/1", data: status).hasStatus(.OK).hasContentType(withPrefix: "application/json").hasData(status) - .request("patch", path: "/status/1", data: status).hasStatus(.OK).hasContentType(withPrefix: "application/json").hasData(status) - .request("delete", path: "/status/1").hasStatus(.noContent).hasNoData() - .run() - } - func testErrorOverridesBody() { #if !swift(>=5.1) let status = Status("This should not be sent") @@ -684,21 +653,6 @@ final class TestCodableRouter: KituraTest, KituraTestSuite { #endif } - func testRouteParameters() { - //Add this erroneous route which should not be hit by the test, should log an error but we can't test the log so we check for a 404 not found. - let status = Status("Should not be seen") - router.get("/status/:id") { (id: Int, respondWith: (Status?, RequestError?) -> Void) in - print("GET on /status/:id that should not happen") - respondWith(status, nil) - } - - buildServerTest(router, timeout: 30) - .request("get", path: "/status/1") - .hasStatus(.notFound) - .hasData() - .run() - } - // Test that we get an internalServerError when using BodyParser with a Codable route func testCodableRoutesWithBodyParsingFail() { // Add a BodyParser that covers everything @@ -902,59 +856,4 @@ final class TestCodableRouter: KituraTest, KituraTestSuite { .run() } - func testInvalidIdentifierSupplied() { - //Add this erroneous route with invalid identifier, should log an error but we can't test the log so we check for a 404 not found. - router.delete("/status/:myid") { (id: Int, respondWith: (RequestError?) -> Void) in - print("DELETE on /status/:myid that should not happen") - respondWith(RequestError(.badRequest, body: Conflict(on: "id"))) - } - - buildServerTest(router, timeout: 30) - .request("delete", path: "/status/1") - .hasStatus(.notFound) - .hasData() - .run() - } - - func testIdentifierNotExpected() { - //Add this erroneous route which should not be hit by the test, should log an error but we can't test the log so we check for a 404 not found. - router.delete("/users/:id") { (respondWith: (RequestError?) -> Void) in - print("DELETE on /users") - respondWith(RequestError(.badRequest, body: Conflict(on: "id"))) - } - - buildServerTest(router, timeout: 30) - .request("delete", path: "/users/1") - .hasStatus(.notFound) - .hasData() - .run() - } - - func testPartialIdentifierSupplied() { - //Add this route with partial identifier. should log an error but we can't test the log so we check for a 404 not found. - router.delete("/status/:") { (id: Int, respondWith: (RequestError?) -> Void) in - print("DELETE on /status/: that should not happen") - respondWith(RequestError(.badRequest, body: Conflict(on: "id"))) - } - - buildServerTest(router, timeout: 30) - .request("delete", path: "/status/1") - .hasStatus(.notFound) - .hasData() - .run() - } - - func testIdentifierNotSupplied() { - router.delete("/status/") { (id: Int, respondWith: (RequestError?) -> Void) in - print("DELETE on /status") - respondWith(nil) - } - - buildServerTest(router, timeout: 30) - .request("delete", path: "/status/1") - .hasStatus(.noContent) - .hasNoData() - .run() - } - } diff --git a/Tests/KituraTests/TestLinuxSafeguard.swift b/Tests/KituraTests/TestLinuxSafeguard.swift index 11d402289..96e8be8ab 100644 --- a/Tests/KituraTests/TestLinuxSafeguard.swift +++ b/Tests/KituraTests/TestLinuxSafeguard.swift @@ -31,6 +31,7 @@ verifyCount(MiscellaneousTests.self) verifyCount(TestBridgingHTTPStatusCode.self) verifyCount(TestBridgingRequestError.self) + verifyCount(TestCodablePathParams.self) verifyCount(TestCodableRouter.self) verifyCount(TestContentType.self) verifyCount(TestCookies.self) diff --git a/Tests/LinuxMain.swift b/Tests/LinuxMain.swift index e1a59a78d..7d08255a4 100644 --- a/Tests/LinuxMain.swift +++ b/Tests/LinuxMain.swift @@ -65,5 +65,6 @@ XCTMain([ testCase(TestSwaggerGeneration.allTests.shuffled()), testCase(TestMediaType.allTests.shuffled()), testCase(TestCustomCoders.allTests.shuffled()), + testCase(TestCodablePathParams.allTests.shuffled()), // testCase(TestCRUDTypeRouter.allTests.shuffled()), ].shuffled())