Allow non-array Codables in get without id

[tunniclm]

Description

I have changed the old array-restricted get function to instead allow any Codable in the response callback. This means changing the CodableArrayClosure for the new SimpleCodableClosure which I plan to add to KituraContracts in PR Kitura/KituraContracts#4.

This closure type is looser than CodableArrayClosure. CodableArrayClosure is castable to SimpleCodableClosure so the change to the function is not breaking. We no longer need CodableArrayClosure but removing it from KituraContracts would be a breaking change, so I have left it there for now.

Motivation and Context

Currently it is not possible to register a Codable get handler that doesn't want an id but does respond with a single Codable object (see #1172).

For example, you cannot currently do the following:

app.router.get("/health") { (respondWith: (Health.Status?, RequestError?) -> Void in
    ...
}

This was simply an oversight, and we do allow both of these:

app.router.get("/health") { (id: Int, respondWith: (Health.Status?, RequestError?) -> Void in
    ...
}

and

app.router.get("/users") { (respondWith: ([User]?, RequestError?) -> Void in
    ...
}

In fact, that last example could have been satisfied automatically if we had implemented the single Codable object.

How Has This Been Tested?

Passes existing Kitura tests and I have added a new test so that both the [Codable] and the new single Codable work.

Also tested against an updated KituraKit in development mode (using swift package edit).

Checklist:

• I have submitted a CLA form
• If applicable, I have updated the documentation accordingly.
• If applicable, I have added tests to cover my changes.

[rolivieri]

A few points; let's first take a look at the following code:

func func3(param: String) { }
func func4<A: CustomStringConvertible>(param: A) { 
    //print("size: \(param.count)") // this line won't compile as expected
}
func func5<A: CustomStringConvertible>(param: [A]) { print("size: \(param.count)") }
let a: [String] = ["h1", "h2", "h3"]
//func3(param: a)   // this won't compile, as expected (we are passing an array)
func4(param: a)     // this compiles, which I found it odd... I was expecting this to not compile
func5(param: a)     // this compiles as expected

Let's now also look at the following code:

// aliases
public typealias CodableResultClosure<O: Codable> = (O?, Error?) -> Void
public typealias CodableArrayResultClosure<O: Codable> = ([O]?, Error?) -> Void

// sample usage
func func1<O: Codable>(param1: String, closure: @escaping CodableResultClosure<O>) { }


func func2<O: Codable>(param1: String, closure: @escaping CodableArrayResultClosure<O>) { }

let closureA: (User?, Error?) -> Void = { (user, error) -> Void in
    // the code below does not compile, as expected
    // if let users = users {
    //     print(users.count)
    // }
}

let closureB: ([User]?, Error?) -> Void = { (users, error) -> Void in
    // the code below compiles without having to cast to an array (as expected)
    if let users = users {
        print(users.count)
    }
}

func1(param1: "a string", closure: closureA)
func2(param1: "a string", closure: closureB)
//func2(param1: "a string", closure: closureA)  // this does not not compile, as expected
func1(param1: "a string", closure: closureB)    //this compiles (as the above example), which I find odd.

I am concerned that given the changes in this PR (i.e. not using CodableArrayClosure), we will be making things in our API looser, which I'd say goes somewhat against what we are trying to do (type-safety). Looking at the samples above, I like that the compiler yells at me if I try to pass an instance that is not an array to a closure that expects an array. I also like that within the closure that gets an array as a parameter, I don't need to cast the parameter to an array in order to determine whether or not I have access to the count property. I am thinking we should not lose any of this safety.

[tunniclm]

It will make the API looser, but that is what is needed to support the use case unless we have a different function defined. If we have a different function it needs to have some explicit label or name difference to be worthwhile, otherwise it won't provide any extra type-safety. We'd need to think of a good name for the function/parameter label that isn't too confusing as well.

[rolivieri]

Just implemented a quick prototype as well:

1. Just like you did, I added the following typealias:

public typealias SimpleCodableClosure<O: Codable> = (@escaping CodableResultClosure<O>) -> Void

I also kept these aliases in place:

public typealias CodableArrayClosure<O: Codable> = (@escaping CodableArrayResultClosure<O>) -> Void
public typealias CodableArrayResultClosure<O: Codable> = ([O]?, RequestError?) -> Void

But I also kept the methods in the API that leverage CodableArrayClosure intact:

public func get<O: Codable>(_ route: String, handler: @escaping CodableArrayClosure<O>) {
        getSafely(route, handler: handler)
    }

I then added this new API method (and corresponding getSafely method):

public func get<O: Codable>(_ route: String, handler: @escaping SimpleCodableClosure<O>) {
        getSafely(route, handler: handler)
    }

Then in my application code, I can now do this (which was the main goal for this change):

router.get(“/users”) { (respondWith: ([User]?, RequestError?) -> Void) in
	…
    respondWith(userStore.map({ $0.value }), nil)	
}

router.get("/health") { (respondWith: (Status?, RequestError?) -> Void) in
…
    respondWith(status, nil)	
}

The benefit I see in this is that we are clearly spelling out in our API that there is a handler for processing an array of codable objects, while there is another one that for processing a single entity. I understand that we could get this same behavior by using only the SimpleCodableClosure… but it is not evident to the developer as it is if we have the two typealiases in place.

This would allow us, in the framework code, to explicitly invoke array properties/methods on the instance that is provided to the respondWith() closure without having to cast it to an array in order to determine if it is an array. I know we are not doing this at the moment… but if at some point in the future we need to perform any operations on the object returned from the app to the framework via the respondWith() closure, we will then have the infrastructure in place if we keep both methods using separate handlers, CodableArrayClosure and SimpleCodableClosure. I am thinking the signature of the closure is more than enough to state which one is for a single codable and which one is for an array of codables.

[tunniclm]

@rolivieri Sure, I tried this also but my thought process was that it provided no extra type-safety and was more code. However, I have no problem doing that if it supports future changes and you think it is more clear in the API. I did already check that it will call the correct overload if you do this, so I'm happy with that also.

[rolivieri]

@tunniclm Sounds good, yes, I also tried it in my prototype and, as you pointed out, it does call the correct overload. Yes, I’d prefer to take this approach of keeping both methods using separate handlers, CodableArrayClosure and SimpleCodableClosure. This way we have the infrastructure in place for type-safety in our own code (meaning inside the framework) and we can take advantage of it if needed at some point in the future (plus it makes the API clearer from my perspective).

[tunniclm]

@rolivieri -- this is ready for a quick review. I have left the commits separate so you can see what has changed in case that is helpful. This should be squashed to one commit when merged.

[codecov-io]

Codecov Report

Merging #1176 into master will decrease coverage by 0.15%.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##           master    #1176      +/-   ##
==========================================
- Coverage   87.16%   87.01%   -0.16%     
==========================================
  Files          38       38              
  Lines        2229     2249      +20     
==========================================
+ Hits         1943     1957      +14     
- Misses        286      292       +6

Flag	Coverage Δ
#Kitura	87.01% <72.72%> (-0.16%)	⬇️

Impacted Files	Coverage Δ
Sources/Kitura/CodableRouter.swift	62.36% <72.72%> (+0.57%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57a46cc...8939abf. Read the comment docs.