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

Codable parameter inconsistency changes #1310

Merged
merged 14 commits into from Oct 17, 2018

Conversation

Projects
None yet
4 participants
@saiHemak
Copy link
Contributor

saiHemak commented Jul 25, 2018

Description

Codable routing inconsistent with parameters

Motivation and Context

Currently in Kitura you could register the following route:

router.delete("/users/:id", handler: deleteAllHandler)

func deleteAllHandler(completion: (RequestError?) -> Void) {
        //Some logic
        completion(nil)
}

And Kitura is happy with this.
However if you do the following:

router.delete("/users/:id", handler: deleteSingleHandler)

func deleteSingleHandler(id: String, completion: (RequestError?) -> Void) {
        //Some logic
        completion(nil)
    }

An error is logged:

[ERROR] [CodableRouter.swift:426 parameterIsPresent(in:)] Erroneous path '/users/:id', parameter ':id' is not allowed. Codable routes do not allow parameters.
This is inconsistent, we allow (but ignore) the /:id parameter in the first case but error on the second case.

There's also a few issues with this:

In the first case, if someone makes a request to users/3 it could allow this request to delete all users.

When using any of the Codable routes with an identifier, the code checks to see if the endpoint has any parameters, for example /:some-Id and then errors even if the parameter provided is actually /:id. Which is actually the correct parameter, and is the one Kitura appends for you.

How Has This Been Tested?

Ran Kitura test cases

Checklist:

  • If applicable, I have updated the documentation accordingly.
  • If applicable, I have added tests to cover my changes.
@saiHemak

This comment has been minimized.

Copy link
Contributor

saiHemak commented Jul 25, 2018

@ddunn2 can you please review

@ianpartridge ianpartridge requested review from ianpartridge , djones6 and ddunn2 and removed request for ianpartridge Jul 25, 2018

@ddunn2
Copy link
Contributor

ddunn2 left a comment

We need to also need to add this new isValidParameter check to other relevant methods.

A final point that Dave Jones suggested, is to Log whenever the :id is appended for the user, so in your case when a method isTolerable is true and the route does not contain a ":".

Finally we need tests for this :)
I think a good way to test this would be to check if the endpoint you've defined is reachable.
We have an example of this test case here:

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()
    }

With your changes that test should fail when you've added the check to the get method.
We could have other tests that check an invalid parameter in the route path such as :myId as well.

@@ -539,6 +539,9 @@ extension Router {

// DELETE
fileprivate func deleteSafely(_ route: String, handler: @escaping NonCodableClosure) {
if !isValidParameter(of: route, isTolerable: false) {

This comment has been minimized.

@ddunn2

ddunn2 Jul 25, 2018

Contributor

I like how you've combined the two checks into a single function!
I'm not sure about isTolerable though, could we change that to something else?

I can't think of anything off of the top of my head though.

This comment has been minimized.

@saiHemak

saiHemak Jul 25, 2018

Contributor

how about isAllowed ?

This comment has been minimized.

@djones6

djones6 Jul 26, 2018

Member

My thoughts on naming:

  1. isValidParameter(of:) - The naming doesn't sit well with me. I read the code and ask myself "which parameter are we talking about here?". What this is telling you is whether it is valid for the route string to contain parameters - in fact, one specific parameter: :id which must appear at the end of the line. So how about pathSyntaxIsValid?
  2. isTolerable - The purpose of this parameter is to indicate whether the route handler expects an Identifier, ie. whether we'd expect to find (or add) an :id suffix. So why not name it what it is? identifierExpected

With the two changes above, you'd have something like:

if !pathSyntaxIsValid(route, identifierExpected: false) {
    return
}
} else {
let paramaterString = route.split(separator: ":", maxSplits: 1, omittingEmptySubsequences: false)
let parameter = paramaterString.count > 0 ? paramaterString[1] : ""
if (parameter.isEmpty || parameter.elementsEqual("id")) {

This comment has been minimized.

@djones6

djones6 Jul 26, 2018

Member

This seems like a really long-winded way to express route.hasSuffix(":id") ... but I suppose it does also check that the route doesn't also contain arbitrary path parameters.

I think the whole thing could be clearer in its intent expressed (something like) this:

/// Precondition: Path is known to contain a : character
func pathHasSingleParamIdAsSuffix(_ path: String) -> Bool {
  let paramaterString = route.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 parameterSupplied = path.contains(":")
  let validIdentifierSyntaxUsed = parameterSupplied && pathHasSingleParamIdAsSuffix(path)
  if parameterSupplied && !validIdentifierSyntaxUsed {
    // Log error: arbitrary params not allowed
    return false
  }
  if identifierExpected && !parameterSupplied {
    // Warn in this case?
    return true
  }
  if !identifierExpected && identifierSupplied {
    // Log error: id supplied but route doesn't consume one
    return false
  }
  return true
}

note, I didn't test this 😄

This comment has been minimized.

@saiHemak

saiHemak Jul 27, 2018

Contributor

Thanks @djones6 ..Will address your comments

@@ -539,6 +539,9 @@ extension Router {

// DELETE
fileprivate func deleteSafely(_ route: String, handler: @escaping NonCodableClosure) {
if !isValidParameter(of: route, isTolerable: false) {

This comment has been minimized.

@djones6

djones6 Jul 26, 2018

Member

My thoughts on naming:

  1. isValidParameter(of:) - The naming doesn't sit well with me. I read the code and ask myself "which parameter are we talking about here?". What this is telling you is whether it is valid for the route string to contain parameters - in fact, one specific parameter: :id which must appear at the end of the line. So how about pathSyntaxIsValid?
  2. isTolerable - The purpose of this parameter is to indicate whether the route handler expects an Identifier, ie. whether we'd expect to find (or add) an :id suffix. So why not name it what it is? identifierExpected

With the two changes above, you'd have something like:

if !pathSyntaxIsValid(route, identifierExpected: false) {
    return
}
}

func pathSyntaxIsValid(_ path: String, identifierExpected: Bool) -> Bool {
let identifierSupplied = path.contains(":")

This comment has been minimized.

@saiHemak

saiHemak Aug 6, 2018

Contributor

@djones6 I have re-ordered the checks based on the identifierExpected flag as it is vain to do the split operation if the identifier itself is not expected for the route

@saiHemak

This comment has been minimized.

Copy link
Contributor

saiHemak commented Aug 6, 2018

@ddunn2 and @djones6 Thanks for your review. I have addressed your comments .. Please review

@djones6
Copy link
Member

djones6 left a comment

A few wording changes

return false
}
if identifierExpected && !identifierSupplied {
Log.warning("Identifier expected for the '\(path)'. Id parameter will be automatically appeneded by default to the path .")

This comment has been minimized.

@djones6

djones6 Aug 7, 2018

Member

Identifier expected for '\(path)'. (remove 'the')
appeneded -> appended
We don't really need "by default". The last statement could simply be: :id will be automatically appended to the path.

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.")

This comment has been minimized.

@djones6

djones6 Aug 7, 2018

Member

double space after '\(path)'
Also I'd suggest: Codable routes allow only id as parameter. -> Codable routes support a trailing id parameter only.

Log.error("Erroneous path '\(path)' is not allowed. Codable routes allow only id as parameter.")
return false
}

This comment has been minimized.

@djones6

djones6 Aug 7, 2018

Member

unnecessary new line

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.")

This comment has been minimized.

@djones6

djones6 Aug 7, 2018

Member

The path is not necessarily erroneous, just that we don't support arbitrary params (yet). I might word it as: Path '\(path)' is not allowed: Codable routes do not allow path parameters.

@saiHemak saiHemak force-pushed the saiHemak:codable-routing branch from 127c07c to e64428b Aug 8, 2018

@saiHemak

This comment has been minimized.

Copy link
Contributor

saiHemak commented Aug 8, 2018

@djones6 Thanks for the review.Modified error messages accordingly.

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 {

This comment has been minimized.

@ianpartridge

ianpartridge Aug 10, 2018

Member

As this function is only used from pathSyntaxIsValid() can we make it a nested function?

This comment has been minimized.

@saiHemak

saiHemak Aug 13, 2018

Contributor

I agree ..however, I would feel that it may readability. How about marking it private and restricting its access ?

This comment has been minimized.

@ianpartridge

ianpartridge Aug 15, 2018

Member

I don't see how it would affect readability. This is what nested functions are for - small helpers used by another function.

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)

This comment has been minimized.

@ianpartridge

ianpartridge Aug 10, 2018

Member

paramaterString -> parameterString


func pathSyntaxIsValid(_ path: String, identifierExpected: Bool) -> Bool {
let identifierSupplied = path.contains(":")
if !identifierExpected && identifierSupplied {

This comment has been minimized.

@ianpartridge

ianpartridge Aug 10, 2018

Member

Would this be clearer if we did

switch (identifierExpected, identifierSupplied) {
    ...
}

and pattern-matched?

This comment has been minimized.

@saiHemak

saiHemak Aug 13, 2018

Contributor

@ianpartridge The above code would look like this if we change it to switch-case

switch(identifierExpected, identifierSupplied) {
        case(false, true) :
            Log.error("Path '\(path)' is not allowed: Codable routes do not allow path parameters.")
            return false
        case (true, false):
            Log.warning("Identifier expected for '\(path)'. :id will be automatically appended to the path.")
            return true
        case (true, true):
            guard pathHasSingleParamIdAsSuffix(path) else {
            Log.error("Erroneous path '\(path)' is not allowed. Codable routes support a trailing id parameter only.")
            return false
            }
            return true
        case (false, false):
            return true
        }

However, for two reasons I was thinking not to go for it is , there is no logical reason behind couplingidentifierExpected and identifierSupplied as a tuple. Also, I believe the readability is not better than the legacy if-else approach. Please let me know which approach we need to take ..
@djones6 please let us know your opinion too ..

This comment has been minimized.

@djones6

djones6 Aug 15, 2018

Member

Actually I like the switch statement, I find it slightly clearer.

This comment has been minimized.

@ianpartridge

ianpartridge Aug 15, 2018

Member

Agreed, please make this change. Put the common (false, false) fast-path as the first case.

@pushkarnk pushkarnk changed the title Codabale parameter inconsistency changes Codable parameter inconsistency changes Aug 13, 2018

@saiHemak saiHemak force-pushed the saiHemak:codable-routing branch 3 times, most recently from 47813d4 to 3a8e6e0 Aug 16, 2018

func pathSyntaxIsValid(_ path: String, identifierExpected: Bool) -> Bool {
let identifierSupplied = path.contains(":")
switch(identifierExpected, identifierSupplied) {
case(false, true) :

This comment has been minimized.

@ianpartridge

ianpartridge Aug 16, 2018

Member

Space after case please.

This comment has been minimized.

@ianpartridge

ianpartridge Aug 16, 2018

Member

No space before :.

Log.error("Erroneous path '\(route)', parameter ':\(parameter)' is not allowed. Codable routes do not allow parameters.")
func pathSyntaxIsValid(_ path: String, identifierExpected: Bool) -> Bool {
let identifierSupplied = path.contains(":")
switch(identifierExpected, identifierSupplied) {

This comment has been minimized.

@ianpartridge

ianpartridge Aug 16, 2018

Member

Space after switch please.

func pathHasSingleParamIdAsSuffix(_ path: String) -> Bool {
let parameterString = path.split(separator: ":", maxSplits: 1, omittingEmptySubsequences: false)
let parameter = parameterString.count > 0 ? parameterString[1] : ""
return parameter.isEmpty || parameter.elementsEqual("id")

This comment has been minimized.

@ianpartridge

ianpartridge Aug 16, 2018

Member

Why elementsEqual? Can't we just use ==?

}

guard pathHasSingleParamIdAsSuffix(path) else {
Log.error("Erroneous path '\(path)' is not allowed. Codable routes support a trailing id parameter only.")

This comment has been minimized.

@ianpartridge

ianpartridge Aug 16, 2018

Member

Indentation.

@ianpartridge

This comment has been minimized.

Copy link
Member

ianpartridge commented Aug 16, 2018

Thanks @saiHemak. Please just push another commit to your branch, don't force-push. It's easier to review if each review cycle is a new commit. We squash when we merge.

@saiHemak saiHemak force-pushed the saiHemak:codable-routing branch from af3cc57 to 7f04180 Aug 17, 2018

//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")))

This comment has been minimized.

@ianpartridge

ianpartridge Aug 21, 2018

Member

Indentation.


guard pathHasSingleParamIdAsSuffix(path) else {
Log.error("Erroneous path '\(path)' is not allowed. Codable routes support a trailing id parameter only.")
return false

This comment has been minimized.

@ianpartridge

ianpartridge Aug 21, 2018

Member

Indentation.

func pathHasSingleParamIdAsSuffix(_ path: String) -> Bool {
let parameterString = path.split(separator: ":", maxSplits: 1, omittingEmptySubsequences: false)
let parameter = parameterString.count > 0 ? parameterString[1] : ""
return parameter.isEmpty || parameter == "id"

This comment has been minimized.

@ianpartridge

ianpartridge Aug 21, 2018

Member

What happens if the path is /hello/:, i.e. it ends with a colon? If I am reading the code correctly the function will return true which doesn't seem right? Please add a test for a route that ends with :.

@saiHemak

This comment has been minimized.

Copy link
Contributor

saiHemak commented Aug 21, 2018

Had detailed discussion with @ianpartridge and here is the behavior expected behavior when identifier is expected

/status  -> /status/:id
/status/ -> /status/:id
/status/: -> error
/status/:id -> /status/:id
/status/:foo -> error

As per the above understanding return parameter.isEmpty || parameter == "id" should return true only if parameter name is id

@saiHemak saiHemak force-pushed the saiHemak:codable-routing branch 2 times, most recently from e1cf9b9 to 10cb65d Aug 21, 2018

/// Precondition: Path is known to contain a : character
func pathHasSingleParamIdAsSuffix(_ path: String) -> Bool {
let parameterString = path.split(separator: ":", maxSplits: 1, omittingEmptySubsequences: false)
let parameter = parameterString.count > 0 ? parameterString[1] : ""

This comment has been minimized.

@ianpartridge

ianpartridge Aug 24, 2018

Member

If parameterString.count is 1, this will crash.

This comment has been minimized.

@saiHemak

saiHemak Aug 28, 2018

Contributor

case (true, true) will get executed only when the path has : i.e identifierSupplied is true. I believe parameterString.count will be 1 if the input string does not contain : in it .So in this case, it will not crash as this case itself will not execute . testIdentifierNotSupplied test case exercises this scenario.

This comment has been minimized.

@ianpartridge

ianpartridge Aug 28, 2018

Member

I'm talking about this specific function, not the broader code. You do path.split() with maxSplits: 1. That means theoretically it could do 0 splits. In which case parameterString.count would be 1 and we would crash.

Also, I don't understand why we pass omittingEmptySubsequences: false. Why would we want empty subsequences to be created?

This comment has been minimized.

@saiHemak

saiHemak Aug 28, 2018

Contributor

Yeah it would crash if you call this function on a String which does not have the : and hence the comment Precondition: Path is known to contain a : character has been retained. But in this case it will not happen as we would not end up in calling this function if the path does not have a : character and the possibility of ending up in performing 0 splits is not there . Also, as this method's scope is limited to this case one will not end up in calling it without the contains(":") check from outside code. Moreover, parameterIsPresent in the existing code is also doing the same.

This comment has been minimized.

@ianpartridge

ianpartridge Aug 28, 2018

Member

Right, as I noted, it won't crash today because of the circumstances we call the function under. But who knows about the future? We should practice Defensive Programming and not store up problems for the future.

This comment has been minimized.

@saiHemak

saiHemak Aug 28, 2018

Contributor

If omittingEmptySubsequences is set to false an empty subsequence is returned in the result for each consecutive pair of : . Its useful in cases of handling the case like /status/: where empty sub-sequence would be returned and will not end up in crash while doing comparison.
eg : In case of omittingEmptySubsequences: false

let path = "/status/:"
let parameterString = path.split(separator: ":", maxSplits: 1, omittingEmptySubsequences: false)
print(parameterString.count)  ------------>  would be 2 
let parameter = parameterString.count > 0 ? parameterString[1] : ""
print(parameter) -----------> would be ""

In case of omittingEmptySubsequences: true , an empty sub-sequence would not be returned.

let path = "/status/:"
let parameterString = path.split(separator: ":", maxSplits: 1, omittingEmptySubsequences: true)
print(parameterString.count) ------------> would be 1
let parameter = parameterString.count > 0 ? parameterString[1] : "" // -----------> CRASH 
print(parameter)

This comment has been minimized.

@saiHemak

saiHemak Aug 28, 2018

Contributor

In that case if route.contains(":") { redundant check needs to be added in the method ..Please let me know if you want me to do that !?

@saiHemak saiHemak force-pushed the saiHemak:codable-routing branch from 10cb65d to 794e053 Aug 31, 2018

@djones6

This comment has been minimized.

Copy link
Member

djones6 commented Sep 12, 2018

@ddunn2 Are you happy with the changes for this PR now? Could you re-review?

@ddunn2

ddunn2 approved these changes Sep 24, 2018

@ddunn2 ddunn2 force-pushed the saiHemak:codable-routing branch from 22b498c to 8a76eb1 Sep 24, 2018

ddunn2 and others added some commits Sep 25, 2018

return true
case (true, true):
/// Precondition: Path is known to contain a : character
func pathHasSingleParamIdAsSuffix(_ path: String) -> Bool {

This comment has been minimized.

@djones6

djones6 Sep 25, 2018

Member

It seems weird to be declaring a function within a case statement... if this function has no value outside the block, couldn't the logic just be inlined?

ddunn2 added some commits Oct 9, 2018

@djones6
Copy link
Member

djones6 left a comment

This regresses the fix in #1335 - probably just a mistake when resolving merge conflicts - see my comments below.

(N.b. This is a great example of tests added for a fix/feature avoiding regression!)

if !pathSyntaxIsValid(route, identifierExpected: false) {
return
}
registerGetRoute(route: route, outputType: O.self)

This comment has been minimized.

@djones6

djones6 Oct 10, 2018

Member

You've lost the outputIsArray: true (which is why a test is failing)

if !pathSyntaxIsValid(route, identifierExpected: false) {
return
}
registerGetRoute(route: route, id: Id.self, outputType: O.self)

This comment has been minimized.

@djones6

djones6 Oct 10, 2018

Member

Same here

ddunn2 and others added some commits Oct 10, 2018

@@ -438,9 +444,11 @@ extension Router {
}

// Get array of (Id, Codable) tuples
fileprivate func getSafely<Id: Identifier, O: Codable>(_ route: String, handler: @escaping IdentifierCodableArrayClosure<Id, O>) {
// FIXME: The ID is returned in a tuple, it is not an input parameter
// https://github.com/IBM-Swift/Kitura/issues/1336

This comment has been minimized.

@djones6

djones6 Oct 17, 2018

Member

This comment block has vanished

ddunn2 added some commits Oct 17, 2018

@djones6 djones6 merged commit 9f780a7 into IBM-Swift:master Oct 17, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment