Add RequestError body codable by tunniclm · Pull Request #1214 · Kitura/Kitura

tunniclm · 2018-01-29T19:21:51Z

Add the capability to provide a Codable object to RequestError that will be encoded and sent by the Router as the response body when passed to the completion handler of a codable route.

Description

This PR is one part of a pair, this one is for Kitura, the other is for KituraContracts (Kitura/KituraContracts#12).

In the KituraContracts PR we add a new generic constructor to RequestError that can specify a "base" request error and a body Codable, for example: RequestError(.unavailable, body: health.status).

This will assign the object to a new stored property body as a (type-erased) Codable. The fact the type is erased allow the RequestError struct to remain non-generic (changing it to be generic would be a breaking change to the API).

However, this means that the real type of the body is no longer know. In order for the Router to encode the body into Data in some format (currently we only support JSON, but in the future there may be other formats) we need to be able to access the real type to pass to the encoder.

To achieve this we also store a closure in a second new stored property bodyData at the same time (from within the generic init where we still have access to the real type of the body codable). This closure accepts an argument describing the format into which you want to encode the body and returns a Data containing the encoded form of the body.

A new struct BodyFormat has been added to enumerate the different supported format. Currently only JSON is supported. This was made as a struct rather than an enum to allow new formats to be added without breaking the API (enums are exhaustive and adding a case would break any switch statements relying on that.)

The bodyData closure performs a switch on the BodyFormat provided and calls the appropriate encoder on the body to convert it to Data. If the BodyFormat is not recognised, then an UnsupportedBodyFormatError (another new type added in this change) will be thrown. Since BodyFormat only has a private initializer, no other code should be able to create them, and provided the generic initializer of RequestError and BodyFormat are kept in sync, then UnsupportedBodyFormatError should never be thrown.

In this PR the Router has been updated so that codable routes will check for the existence of a bodyData in any RequestError returned by a codable route handler, and if it is non-nil then will use it to get a Data to send as the body of the response.

This PR also adds a number of tests, and to prevent excessive code duplication a new test builder API has been added to allow construction of concise tests in a declarative style.

Motivation and Context

This change allows codable route handlers to customise the body of error responses. One example of where this feature is needed is the health route generated by the Swift Server Generator and kitura-cli.

This route should send HTTP 200 OK when the application is healthy and HTTP 503 Service Unavailable when the application is not working. It should also provide a response body that is a JSON object describing the status of the application and each dependent service. This body should be sent both when the application is healthy and when it is not, and is arguably more important in the latter case as it will give information about which service is not available.

Without a custom body for RequestError it is not possible to send a response body if the status code is not 2xx (usually HTTP 200 OK, but it varies depending on the HTTP method, eg: delete uses HTTP 204 No content).

How Has This Been Tested?

New tests have been added, tested with swift test.

Checklist:

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

tunniclm · 2018-01-29T20:05:06Z

Working through rebasing the code on master, there are some conflicts to resolve.

tunniclm · 2018-01-30T17:35:28Z

Conflicts now resolved but this code requires the KituraContracts changes before it will compile.

codecov-io · 2018-01-30T17:48:49Z

Codecov Report

Merging #1214 into master will increase coverage by 0.99%.
The diff coverage is 86.48%.

@@            Coverage Diff             @@
##           master    #1214      +/-   ##
==========================================
+ Coverage   86.94%   87.93%   +0.99%     
==========================================
  Files          38       38              
  Lines        2367     2404      +37     
==========================================
+ Hits         2058     2114      +56     
+ Misses        309      290      -19

Flag	Coverage Δ
#Kitura	`87.93% <86.48%> (+0.99%)`	⬆️

Impacted Files	Coverage Δ
Sources/Kitura/CodableRouter.swift	`74.27% <86.48%> (+8.28%)`	⬆️

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 4fbe57d...96ac814. Read the comment docs.

djones6

The changes look good (in particular, the refactoring of the tests looks great), I made a few minor suggestions.

djones6 · 2018-01-31T14:54:58Z

Tests/KituraTests/TestCodableRouter.swift

-            })
-        }
+            .request("post", path: "/error/users", data: user)
+            .hasStatus(.conflict)


Did you want to assert .hasNoData() here?

This looks correct to me, the "/error/users" route responds with nil.

It wasn't obvious from the diff, but Dave was pointing out that .hasNoData() wasn't being called here. Then I added it in, but his original comment remains and now it reads like he's questioning if the .hasNoData() should be there!

I think we all agree it should be. :)

djones6 · 2018-01-31T14:55:47Z

Tests/KituraTests/TestCodableRouter.swift

-            })
-        }
+            .request("post", path: "/error/users", data: user)
+            .hasStatus(.conflict)


(same comment as above)

djones6 · 2018-01-31T15:05:05Z

Tests/KituraTests/TestCodableRouter.swift

+        }
+        router.post("/bodyerror/users") { (user: User, respondWith: (User?, RequestError?) -> Void) in
+            print("POST on /bodyerror/users for user \(user)")
+            respondWith(user, RequestError(.conflict, body: Conflict(on: "id")))


I think this shouldn't be trying to respond with a user as well as an error. (We should probably have separate tests that check that the error overrides the body, both when the error does / does not have its own body)

djones6 · 2018-01-31T15:52:27Z

Tests/KituraTests/TestCodableRouter.swift

-                    return
-                }
+            .request("get", path: "/error/status")
+            .hasStatus(.serviceUnavailable)


(same comment as above)

tunniclm · 2018-02-01T17:34:01Z

@djones6 I think I have addressed everything in the review on this PR, can you re-review for me? Thanks

djones6 · 2018-02-02T09:03:12Z

LGTM, thanks for adding those extra tests.

djones6 · 2018-02-02T09:07:10Z

Package.swift

        .package(url: "https://github.com/IBM-Swift/Kitura-net.git", .upToNextMinor(from: "2.0.0")),
        .package(url: "https://github.com/IBM-Swift/Kitura-TemplateEngine.git", .upToNextMinor(from: "1.7.0")),
-        .package(url: "https://github.com/IBM-Swift/KituraContracts.git", .upToNextMinor(from: "0.0.17"))
+        .package(url: "https://github.com/IBM-Swift/KituraContracts.git", .branch("add_requesterror_body"))


Remember to revert before this is merged :)

ianpartridge · 2018-02-16T15:10:23Z

Sources/Kitura/CodableRouter.swift

                    response.status(status)
+                    do {
+                        if let bodyData = try err.encodeBody(.json) {
+                                response.headers.setType("json")


Indentation is off here.

ianpartridge · 2018-02-16T15:19:22Z

Tests/KituraTests/TestCodableRouter.swift

            respondWith(Status("GOOD"), nil)
        }
+        router.get("/error/status") { (respondWith: (Status?, RequestError?) -> Void) in
+            print("GET on /status")


Should be print("GET on /error/status")

ianpartridge · 2018-02-16T15:19:38Z

Tests/KituraTests/TestCodableRouter.swift

+            respondWith(nil, .serviceUnavailable)
+        }
+        router.get("/bodyerror/status") { (respondWith: (Status?, RequestError?) -> Void) in
+            print("GET on /status")


print("GET on /bodyerror/status")

ianpartridge · 2018-02-16T15:23:35Z

Tests/KituraTests/TestCodableRouter.swift

-            })
-        }
+            .request("post", path: "/error/users", data: user)
+            .hasStatus(.conflict)


This looks correct to me, the "/error/users" route responds with nil.

ianpartridge · 2018-02-16T15:31:54Z

Tests/KituraTests/TestCodableRouter.swift

+        buildServerTest(router, timeout: 30)
+            .request("get", path: "/status/1")
+            .hasStatus(.notFound)
+            .hasData("Cannot GET /status/1.")


Not sure if testing the precise error message is wise - 404 seems enough.

tunniclm mentioned this pull request Jan 29, 2018

Add RequestError body codable Kitura/KituraContracts#12

Merged

2 tasks

tunniclm added 4 commits January 30, 2018 13:52

Add support for custom response body to RequestError

65d815e

Refactor CodableRouterTests, add tests for RequestError

bf92c80

updates and fixes

21aff38

Fix content type on error body

5a26833

tunniclm force-pushed the add_requesterror_body branch from 2dd2a0b to 5a26833 Compare January 30, 2018 14:07

TEMPORARY: Use feature branch for KituraContracts for CI

1937369

djones6 self-requested a review January 31, 2018 14:35

djones6 requested changes Jan 31, 2018

View reviewed changes

tunniclm added 2 commits February 1, 2018 15:02

Address review comments

bd1dedf

Add tests for error overriding body

2b9b02a

Add more tests for error overriding body

5b613d4

tunniclm force-pushed the add_requesterror_body branch from 4760018 to 5b613d4 Compare February 1, 2018 17:35

djones6 approved these changes Feb 2, 2018

View reviewed changes

djones6 reviewed Feb 2, 2018

View reviewed changes

tunniclm added 2 commits February 2, 2018 15:08

Update for latest changes to KituraContracts feature branch

5defcb2

Update for latest changes to KituraContracts feature branch

5532dc3

ianpartridge requested changes Feb 16, 2018

View reviewed changes

tunniclm added 4 commits February 21, 2018 13:42

Update Package.swift to new KituraContracts

f15fc16

Fix indentation

93a0d95

Fix review comments

bf6b683

Merge remote-tracking branch 'origin/master' into add_requesterror_body

96ac814

ianpartridge approved these changes Feb 21, 2018

View reviewed changes

ianpartridge merged commit 0e1f01e into master Feb 21, 2018

ianpartridge deleted the add_requesterror_body branch February 21, 2018 14:04

djones6 mentioned this pull request Mar 13, 2018

Allow POST success status codes besides 201 (CREATED) #1218

Closed

2 tasks

djones6 mentioned this pull request May 2, 2018

Decode RequestError body Kitura/KituraKit#30

Merged

Conversation

tunniclm commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Checklist:

Uh oh!

tunniclm commented Jan 29, 2018

Uh oh!

tunniclm commented Jan 30, 2018

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

djones6 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!

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!

tunniclm commented Feb 1, 2018

Uh oh!

djones6 commented Feb 2, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tunniclm commented Jan 29, 2018 •

edited

Loading

codecov-io commented Jan 30, 2018 •

edited

Loading