Type safe middleware by Andrew-Lees11 · Pull Request #1274 · Kitura/Kitura

Andrew-Lees11 · 2018-05-22T11:04:48Z

Adds the new TypeSafeMiddleware protocol and routes for up to 3 TypeSafeMiddleware in Codable routing.

Description

TypeSafeMiddleware is a protocol for middleware where they return an instance of self instead of altering the request and response. This allows them to be used in Codable routing.

The routes have been added in a new file for Codable routing allowing a user to have up to three type safe middleware on a route by declaring them in there route signature.

Motivation and Context

type safe middleware means application gains compile-time safety and developers benefit from enhanced separation of concerns between the middleware(s) and route handlers.

It also allows middleware such as Auth and sessions them to be used in Codable routes.

How Has This Been Tested?

Has been tested manually using Sessions as a typed middleware.
Automated tests still need to be added.
Appropriate Docs also need to be added describing the new routes.

…ura into typeSafeMiddleware

codecov-io · 2018-05-22T11:15:57Z

Codecov Report

Merging #1274 into master will decrease coverage by 6.34%.
The diff coverage is 49.45%.

@@            Coverage Diff             @@
##           master    #1274      +/-   ##
==========================================
- Coverage   88.97%   82.62%   -6.35%     
==========================================
  Files          39       40       +1     
  Lines        2412     2872     +460     
==========================================
+ Hits         2146     2373     +227     
- Misses        266      499     +233

Flag	Coverage Δ
#Kitura	`82.62% <49.45%> (-6.35%)`	⬇️

Impacted Files	Coverage Δ
Sources/Kitura/CodableRouter.swift	`80.37% <100%> (ø)`	⬆️
...rces/Kitura/CodableRouter+TypeSafeMiddleware.swift	`49.34% <49.34%> (ø)`

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 5252028...ab2b3b4. Read the comment docs.

shmuelk · 2018-05-22T11:16:28Z

Why is one limited to three middleware? Why isn't the list of middleware an array or a variadic parameter?

ianpartridge

A few initial comments.

ianpartridge · 2018-05-22T11:20:23Z

Sources/Kitura/CodableRouter+TypedMiddleware.swift

@@ -0,0 +1,1020 @@
+/*


Can this file be called CodableRouter+TypeSafeMiddleware.swift?

ianpartridge · 2018-05-22T11:21:04Z

Sources/Kitura/CodableRouter+TypedMiddleware.swift

+extension Router {
+
+    // Used by PUT and PATCH with identifier
+    public typealias MiddlewareIdentifierCodableClosure<T: TypeSafeMiddleware, Id: Identifier, I: Codable, O: Codable> = (T, Id, I, @escaping CodableResultClosure<O>) -> Void


Is there a reason why these typealiases are inside extension Router {}? I would have thought they should be top-level.

ianpartridge · 2018-05-22T11:26:54Z

Sources/Kitura/CodableRouter+TypedMiddleware.swift

+                next()
+                return
+            }
+            self.handleMiddleware(T1.self, T2.self, request: request, response: response) { TypeSafeMiddleware1, TypeSafeMiddleware2 in


Please name variables with lower-case names, e.g. typeSafeMiddleware1 not TypeSafeMiddleware1 to distinguish them from type names. This comment applies across the whole PR.

ianpartridge · 2018-05-22T11:30:15Z

@shmuelk I think the feature we really want is variadic generics but they aren't here yet unfortunately. Andrew and Dave did explore using an array, I'll let them comment on why they decided against that approach.

into typeSafeMiddleware

ianpartridge · 2018-05-23T11:54:30Z

Sources/Kitura/RouterMiddleware.swift

 * limitations under the License.
 */

+import KituraContracts


I think we can revert all the changes to this file now?

ianpartridge · 2018-05-23T11:54:40Z

Sources/Kitura/TypeSafeMiddleware.swift

@@ -0,0 +1,39 @@
+/*
+ * Copyright IBM Corporation 2016


ianpartridge · 2018-05-23T11:55:18Z

Sources/Kitura/TypeSafeMiddleware.swift

+    ///                         attempt to process the request, respectively.
+    static func handle(request: RouterRequest, response: RouterResponse, completion: @escaping (Self?, RequestError?) -> Void) -> Void
+
+    /// Decribe the type-safe middleware


Describe not Decribe. Also, please give an example of the kind of description that would be appropriate.

ianpartridge · 2018-05-23T11:56:19Z

Sources/Kitura/CodableRouter+TypeSafeMiddleware.swift

@@ -0,0 +1,890 @@
+/*
+ * Copyright IBM Corporation 2017


ianpartridge · 2018-05-23T14:42:44Z

Sources/Kitura/CodableRouter+TypeSafeMiddleware.swift

    ) {
        T.handle(request: request, response: response) { (typeSafeMiddleware: T?, error: RequestError?) in
            guard let typeSafeMiddleware = typeSafeMiddleware else {
+                print("failed typeSafeMiddleware. Error: \(String(describing: error))")


ianpartridge · 2018-05-24T10:10:57Z

Sources/Kitura/TypeSafeMiddleware.swift

+        scheme: basic
+     ```
+     */
+    static func describe() -> String


My only question is whether we should take this out for now.

helenmasters · 2018-05-24T13:15:13Z

Sources/Kitura/TypeSafeMiddleware.swift

+/// Defines the protocol which all Kitura type-safe middleware must implement.
+///
+/// TypeSafeMiddleware are classes or structs that process an incoming request
+/// and on success create an instance of self, which is passed to the users route handler.


users route handler -> user's route handler

helenmasters · 2018-05-24T13:17:25Z

Sources/Kitura/CodableRouter+TypeSafeMiddleware.swift

+
+extension Router {
+
+    // MARK: Codable Routing With Type-Safe Middleware


At least the With should be lowercase in the // MARK here as it looks strange with a capital 'W' underneath the previous MARK'd title of Parameter handling (which has a lowercase 'h').

helenmasters · 2018-05-24T13:22:57Z

Sources/Kitura/CodableRouter+TypeSafeMiddleware.swift

+     ```
+     - Parameter route: A String specifying the URL path that will invoke the handler.
+     - Parameter handler: A closure that receives a TypeSafeMiddleware instance and a QueryParams instance, and returns an array of Codable objects or a RequestError.
+     */    public func get<T: TypeSafeMiddleware, Q: QueryParams, O: Codable>(


This */ is being displayed in the API doc. public func... needs to move onto the next line to fix this.

helenmasters · 2018-05-24T13:27:00Z

Sources/Kitura/CodableRouter+TypeSafeMiddleware.swift

+     The handler contains the developer's logic, which determines the server's response.
+     ### Usage Example: ###
+     In this example, MySession is a struct that conforms to the TypeSafeMiddleware protocol.
+     session is an instance of MySession, that contains [Int: User] dictionary.


You need to add the word 'an' before [Int: User] dictionary. Same on line 978.

helenmasters · 2018-05-24T13:28:38Z

Sources/Kitura/CodableRouter+TypeSafeMiddleware.swift

+                             id: Int, respondWith: (User?, RequestError?) -> Void) in
+     ```
+     - Parameter route: A String specifying the URL path that will invoke the handler.
+     - Parameter handler: A closure that receives a TypeSafeMiddleware instance a Codable object and an identifier, and returns a Codable object or a RequestError.


Needs a comma after the word instance. The same is true in the usage example below.... I haven't checked others.

helenmasters · 2018-05-24T13:45:26Z

Sources/Kitura/CodableRouter+TypeSafeMiddleware.swift

+     The handler contains the developer's logic, which determines the server's response.
+     ### Usage Example: ###
+     In this example, MySession is a struct that conforms to the TypeSafeMiddleware protocol.
+     session is an instance of MySession, that contains optional Users.


Anywhere where we are starting a sentence with a variable name which is lowercase it looks poor in the jazzy API doc. Try the following instead:

In this example, MySession is a struct that conforms to the TypeSafeMiddleware protocol and specifies an optional User, where User conforms to Codable.

Andrew-Lees11 · 2018-05-25T16:03:59Z

Tests for all routes and documentation has now been added.
Two and Three middleware are marked as no doc so they don't show up in jazzy for clarity.

ianpartridge

Awesome work guys.

djones6 · 2018-05-29T14:27:55Z

@shmuelk To answer your earlier question on the 'up to 3' TypeSafeMiddleware parameters: Ian is correct, variadic generics is what we'd need to implement this in a nice way.

The reason we can't use an array is that the array would lose the type information of the specific instance types the user is expecting: the router would only know that the user's handler was expecting an array of [TypeSafeMiddleware]. We'd have to pass that type information separately.

Here is an example of how we could do it, but it's pretty hairy (and not very type-safe):

func get(_ route: String, middlewareTypes array: [MyProto.Type], handler: ([MyProto]) -> Void) {
    var result: [MyProto] = []
    for elem in array {
        var constructedType = elem.init()
        result.append(constructedType)
    }
    handler(result)
}

get("/foo", middlewareTypes: [ImplA.self, ImplB.self]) {
    (impls: [MyProto]) in
    guard let implA = impls[0] as? ImplA, let implB = impls[1] as? ImplB else {
        fatalError("Unexpected types passed")
    }
}

The biggest problem with this approach is requiring the user to restore the lost type information as a series of attempted downcasts, exactly matching the types in the array they passed to the router.

Andrew-Lees11 and others added 12 commits April 23, 2018 13:09

change package for private repo

880e5c9

update package

f864ec7

Add TypedMiddleware and GET

a6817d8

Rename user's handler

6031887

Start adding TypedMiddleware to router.all

5f9603e

Dead end

e19520e

Add POST with TypedMiddleware

16af301

Document completion param

4605c79

added Typed Middleware Routes

db86b89

Merge branch 'typeSafeMiddleware' of github.ibm.com:Andrew-Lees11/Kit…

896ae47

…ura into typeSafeMiddleware

renamed to TypeSafeMiddleware

b65bfed

revert to public contracts

b4fb5ef

Andrew-Lees11 requested a review from ianpartridge May 22, 2018 11:04

Merge branch 'master' into typeSafeMiddleware

6976b31

ianpartridge requested changes May 22, 2018

View reviewed changes

Andrew-Lees11 added 2 commits May 22, 2018 12:51

renames to CodableRouter+TypeSafeMiddleware

3575eec

Merge branch 'typeSafeMiddleware' of https://github.com/IBM-Swift/Kitura

4239a12

into typeSafeMiddleware

Andrew-Lees11 mentioned this pull request May 22, 2018

added TypeSafeMiddleware aliases Kitura/KituraContracts#23

Closed

Andrew-Lees11 added 4 commits May 22, 2018 17:34

safely functions inside codable routes

eb2483e

removed typeAliases

1603221

added middleware array route

ef48433

refactor Type-safe routes

9105006

ianpartridge reviewed May 23, 2018

View reviewed changes

Sources/Kitura/TypeSafeMiddleware.swift Outdated

@@ -0,0 +1,39 @@

/*

* Copyright IBM Corporation 2016

Copy link
Copy Markdown

Collaborator

ianpartridge May 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2018

ianpartridge reviewed May 23, 2018

View reviewed changes

added single TypeSafeMiddleware doc

910d5f1

removed print statement

ab72e94

ianpartridge reviewed May 23, 2018

View reviewed changes

added describe doc

3eaee41

ianpartridge reviewed May 24, 2018

View reviewed changes

Andrew-Lees11 and others added 3 commits May 24, 2018 12:25

added full jazzy docs

e71600b

removed describe from middleware

1baa984

Merge branch 'master' into typeSafeMiddleware

11d856b

helenmasters reviewed May 24, 2018

View reviewed changes

djones6 and others added 7 commits May 24, 2018 15:18

Add POST tests for TypeSafeMiddleware

3bdedb4

added TypeSafeMiddleware protocol docs

4fc180e

removed KituraContracts import

7d4aecc

Implemented Helens docs review

f263cf0

rephrase jazzy docs

373509f

added some TSmiddleware tests

83c5656

added remaining TSMiddleware tests

ab2b3b4

djones6 mentioned this pull request May 29, 2018

feat: Type-safe Sessions using TypeSafeMiddleware Kitura/Kitura-Session#48

Merged

ianpartridge approved these changes May 29, 2018

View reviewed changes

ianpartridge merged commit 179356f into master May 29, 2018

ianpartridge deleted the typeSafeMiddleware branch May 29, 2018 12:45


		extension Router {

		// MARK: Codable Routing With Type-Safe Middleware

Conversation

Andrew-Lees11 commented May 22, 2018

Description

Motivation and Context

How Has This Been Tested?

Uh oh!

codecov-io commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

shmuelk commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ianpartridge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianpartridge commented May 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

helenmasters May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

helenmasters May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

helenmasters May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andrew-Lees11 commented May 25, 2018

Uh oh!

ianpartridge left a comment

Choose a reason for hiding this comment

Uh oh!

djones6 commented May 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented May 22, 2018 •

edited

Loading

shmuelk commented May 22, 2018 •

edited

Loading

helenmasters May 24, 2018 •

edited

Loading

helenmasters May 24, 2018 •

edited

Loading

helenmasters May 24, 2018 •

edited

Loading