Addressed comments

Kitura · Aug 6, 2018 · 127c07c · 127c07c
1 parent c9b1adc
commit 127c07c
Show file tree

Hide file tree

Showing 3 changed files with 69 additions and 43 deletions.
diff --git a/Sources/Kitura/CodableRouter+TypeSafeMiddleware.swift b/Sources/Kitura/CodableRouter+TypeSafeMiddleware.swift
@@ -273,7 +273,7 @@ extension Router {
         _ route: String,
         handler: @escaping (T, Id, @escaping CodableResultClosure<O>) -> Void
     ) {
-        if parameterIsPresent(in: route) {
+        if !pathSyntaxIsValid(route, identifierExpected: true) {
             return
         }
         registerGetRoute(route: route, id: true, outputtype: O.self)
@@ -315,7 +315,7 @@ extension Router {
         _ route: String,
         handler: @escaping (T1, T2, Id, @escaping CodableResultClosure<O>) -> Void
     ) {
-        if parameterIsPresent(in: route) {
+        if !pathSyntaxIsValid(route, identifierExpected: true) {
             return
         }
         registerGetRoute(route: route, id: true, outputtype: O.self)
@@ -357,7 +357,7 @@ extension Router {
         _ route: String,
         handler: @escaping (T1, T2, T3, Id, @escaping CodableResultClosure<O>) -> Void
     ) {
-        if parameterIsPresent(in: route) {
+        if !pathSyntaxIsValid(route, identifierExpected: true) {
             return
         }
         registerGetRoute(route: route, id: true, outputtype: O.self)
@@ -904,7 +904,7 @@ extension Router {
         _ route: String,
         handler: @escaping (T, Id, @escaping ResultClosure) -> Void
     ) {
-        if parameterIsPresent(in: route) {
+        if !pathSyntaxIsValid(route, identifierExpected: true) {
             return
         }
         registerDeleteRoute(route: route, id: true)
@@ -944,7 +944,7 @@ extension Router {
         _ route: String,
         handler: @escaping (T1, T2, Id, @escaping ResultClosure) -> Void
     ) {
-        if parameterIsPresent(in: route) {
+        if !pathSyntaxIsValid(route, identifierExpected: true) {
             return
         }
         registerDeleteRoute(route: route, id: true)
@@ -984,7 +984,7 @@ extension Router {
         _ route: String,
         handler: @escaping (T1, T2, T3, Id, @escaping ResultClosure) -> Void
     ) {
-        if parameterIsPresent(in: route) {
+        if !pathSyntaxIsValid(route, identifierExpected: true) {
             return
         }
         registerDeleteRoute(route: route, id: true)
@@ -1412,7 +1412,7 @@ extension Router {
         _ route: String,
         handler: @escaping (T, Id, I, @escaping CodableResultClosure<O>) -> Void
     ) {
-        if parameterIsPresent(in: route) {
+        if !pathSyntaxIsValid(route, identifierExpected: true) {
             return
         }
         registerPutRoute(route: route, id: true, inputtype: I.self, outputtype: O.self)
@@ -1454,7 +1454,7 @@ extension Router {
         _ route: String,
         handler: @escaping (T1, T2, Id, I, @escaping CodableResultClosure<O>) -> Void
     ) {
-        if parameterIsPresent(in: route) {
+        if !pathSyntaxIsValid(route, identifierExpected: true) {
             return
         }
         registerPutRoute(route: route, id: true, inputtype: I.self, outputtype: O.self)
@@ -1496,7 +1496,7 @@ extension Router {
         _ route: String,
         handler: @escaping (T1, T2, T3, Id, I, @escaping CodableResultClosure<O>) -> Void
     ) {
-        if parameterIsPresent(in: route) {
+        if !pathSyntaxIsValid(route, identifierExpected: true) {
             return
         }
         registerPutRoute(route: route, id: true, inputtype: I.self, outputtype: O.self)
@@ -1548,7 +1548,7 @@ extension Router {
         _ route: String,
         handler: @escaping (T, Id, I, @escaping CodableResultClosure<O>) -> Void
     ) {
-        if parameterIsPresent(in: route) {
+        if !pathSyntaxIsValid(route, identifierExpected: true) {
             return
         }
         registerPatchRoute(route: route, id: true, inputtype: I.self, outputtype: O.self)
@@ -1596,7 +1596,7 @@ extension Router {
         _ route: String,
         handler: @escaping (T1, T2, Id, I, @escaping CodableResultClosure<O>) -> Void
     ) {
-        if parameterIsPresent(in: route) {
+        if !pathSyntaxIsValid(route, identifierExpected: true) {
             return
         }
         registerPatchRoute(route: route, id: true, inputtype: I.self, outputtype: O.self)
@@ -1644,7 +1644,7 @@ extension Router {
         _ route: String,
         handler: @escaping (T1, T2, T3, Id, I, @escaping CodableResultClosure<O>) -> Void
     ) {
-        if parameterIsPresent(in: route) {
+        if !pathSyntaxIsValid(route, identifierExpected: true) {
             return
         }
         registerPatchRoute(route: route, id: true, inputtype: I.self, outputtype: O.self)

diff --git a/Sources/Kitura/CodableRouter.swift b/Sources/Kitura/CodableRouter.swift
@@ -383,7 +383,7 @@ extension Router {
 
     // PUT with Identifier
     fileprivate func putSafely<Id: Identifier, I: Codable, O: Codable>(_ route: String, handler: @escaping IdentifierCodableClosure<Id, I, O>) {
-        if parameterIsPresent(in: route) {
+        if !pathSyntaxIsValid(route, identifierExpected: true) {
             return
         }
         registerPutRoute(route: route, id: true, inputtype: I.self, outputtype: O.self)
@@ -401,7 +401,7 @@ extension Router {
 
     // PATCH
     fileprivate func patchSafely<Id: Identifier, I: Codable, O: Codable>(_ route: String, handler: @escaping IdentifierCodableClosure<Id, I, O>) {
-        if parameterIsPresent(in: route) {
+        if !pathSyntaxIsValid(route, identifierExpected: true) {
             return
         }
         registerPatchRoute(route: route, id: true, inputtype: I.self, outputtype: O.self)
@@ -523,7 +523,7 @@ extension Router {
     }
     // GET single identified element
     fileprivate func getSafely<Id: Identifier, O: Codable>(_ route: String, handler: @escaping IdentifierSimpleCodableClosure<Id, O>) {
-        if parameterIsPresent(in: route) {
+        if !pathSyntaxIsValid(route, identifierExpected: true) {
             return
         }
         registerGetRoute(route: route, id: true, outputtype: O.self)
@@ -539,7 +539,7 @@ extension Router {
 
     // DELETE
     fileprivate func deleteSafely(_ route: String, handler: @escaping NonCodableClosure) {
-        if !isValidParameter(of: route, isTolerable: false) {
+        if !pathSyntaxIsValid(route, identifierExpected: false) {
             return
         }
         registerDeleteRoute(route: route, id: false)
@@ -551,7 +551,7 @@ extension Router {
 
     // DELETE single element
     fileprivate func deleteSafely<Id: Identifier>(_ route: String, handler: @escaping IdentifierNonCodableClosure<Id>) {
-        if !isValidParameter(of: route, isTolerable: true) {
+        if !pathSyntaxIsValid(route, identifierExpected: true) {
             return
         }
         registerDeleteRoute(route: route, id: true)
@@ -565,25 +565,6 @@ extension Router {
         }
     }
 
-    internal func isValidParameter(of route: String, isTolerable: Bool) -> Bool {
-        if route.contains(":") {
-            if !isTolerable {
-                Log.error("Erroneous path '\(route)', parameter is not allowed. Codable routes without identifier do not allow parameters.")
-                return false
-            } else {
-                let paramaterString = route.split(separator: ":", maxSplits: 1, omittingEmptySubsequences: false)
-                let parameter = paramaterString.count > 0 ? paramaterString[1] : ""
-                if (parameter.isEmpty || parameter.elementsEqual("id")) {
-                    return true
-                } else {
-                    Log.error("Erroneous path '\(route)', parameter ':\(parameter)' is not allowed. Codable routes allow only id as parameter.")
-                    return false
-                }
-            }
-        }
-        return true
-    }
-
     // DELETE w/Query Parameters
     fileprivate func deleteSafely<Q: QueryParams>(_ route: String, handler: @escaping (Q, @escaping ResultClosure) -> Void) {
         registerDeleteRoute(route: route, id: false)
@@ -622,14 +603,30 @@ extension Router {
         }
     }
 
-    internal func parameterIsPresent(in route: String) -> Bool {
-        if route.contains(":") {
-            let paramaterString = route.split(separator: ":", maxSplits: 1, omittingEmptySubsequences: false)
-            let parameter = paramaterString.count > 0 ? paramaterString[1] : ""
-            Log.error("Erroneous path '\(route)', parameter ':\(parameter)' is not allowed. Codable routes do not allow parameters.")
+    /// Precondition: Path is known to contain a : character
+    func pathHasSingleParamIdAsSuffix(_ path: String) -> Bool {
+        let paramaterString = path.split(separator: ":", maxSplits: 1, omittingEmptySubsequences: false)
+        let parameter = paramaterString.count > 0 ? paramaterString[1] : ""
+        return parameter.isEmpty || parameter.elementsEqual("id")
+    }
+
+    func pathSyntaxIsValid(_ path: String, identifierExpected: Bool) -> Bool {
+        let identifierSupplied = path.contains(":")
+        if !identifierExpected && identifierSupplied {
+            Log.error("Erroneous path '\(path)' is not allowed. Codable routes do not allow parameters.")
+            return false
+        }
+        if identifierExpected && !identifierSupplied {
+            Log.warning("Identifier expected for the '\(path)'. Id parameter will be automatically appeneded by default to the path .")
             return true
         }
-        return false
+        let validIdentifierSyntaxUsed = identifierSupplied && pathHasSingleParamIdAsSuffix(path)
+        if identifierExpected && !validIdentifierSyntaxUsed {
+            Log.error("Erroneous path '\(path)'  is not allowed. Codable routes allow only id as parameter.")
+            return false
+        }
+
+        return true
     }
 
     internal func join(path base: String, with component: String) -> String {

diff --git a/Tests/KituraTests/TestCodableRouter.swift b/Tests/KituraTests/TestCodableRouter.swift
@@ -43,7 +43,9 @@ class TestCodableRouter: KituraTest {
             ("testCodableDeleteQueryParameters", testCodableDeleteQueryParameters),
             ("testCodablePostSuccessStatuses", testCodablePostSuccessStatuses),
             ("testNoDataCustomStatus", testNoDataCustomStatus),
-            ("testNoDataDefaultStatus", testNoDataDefaultStatus)
+            ("testNoDataDefaultStatus", testNoDataDefaultStatus),
+            ("testInvalidIdentifierSupplied", testInvalidIdentifierSupplied),
+            ("testIdentifierNotExpected", testIdentifierNotExpected),
         ]
     }
 
@@ -886,4 +888,31 @@ class TestCodableRouter: KituraTest {
 
             .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/:id 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()
+    }
 }