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

tunniclm · 2018-01-29T18:57:43Z

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 KituraContracts, the other is for Kitura (Kitura/Kitura#1214).

In this 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.

This PR also adds a few small tests for the new initializer and properties.

Jazzy API doc has been added for the new code.

In the Kitura 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.

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.

codecov-io · 2018-01-29T19:08:30Z

Codecov Report

❗ No coverage uploaded for pull request base (master@4195ce3). Click here to learn what that means.
The diff coverage is 81.81%.

@@            Coverage Diff            @@
##             master      #12   +/-   ##
=========================================
  Coverage          ?   65.35%           
=========================================
  Files             ?        6           
  Lines             ?      482           
  Branches          ?        0           
=========================================
  Hits              ?      315           
  Misses            ?      167           
  Partials          ?        0

Flag	Coverage Δ
#KituraContracts	`65.35% <81.81%> (?)`

Impacted Files	Coverage Δ
Sources/KituraContracts/BodyFormat.swift	`66.66% <66.66%> (ø)`
Sources/KituraContracts/Contracts.swift	`42.73% <85.18%> (ø)`

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 4195ce3...71c8674. Read the comment docs.

djones6 · 2018-02-01T08:50:58Z

Sources/KituraContracts/Contracts.swift

+     - Note: The code accessing this property is expected to know the
+             concrete type of this object and downcast, handling any
+             failures appropriately.
+     */


Where the client receives an error response and a RequestError is generated, how does the data off the wire get decoded into a Codable? The type is not known, so initially you have some Data. You may be able to add a function to which the user supplies the concrete type of Codable they expect to be able to decode from the error body (in lieu of making the RequestError generic).

tunniclm · 2018-02-02T15:20:41Z

Sources/KituraContracts/Contracts.swift

+    }
+
+    /// Creates an error respresenting the given base error, with a custom
+    /// response body given as Data and a BodyFormat


I need to add - throws to the doc here

djones6 · 2018-02-07T10:03:59Z

@tunniclm is this ready to merge?

tunniclm · 2018-02-07T15:16:27Z

There's a few more requirements that have arisen from testing with KituraKit and talking with @EnriqueL8 / @seabaylea. I'd like to work on that a bit to work out the required changes.

ianpartridge · 2018-02-15T16:49:51Z

Sources/KituraContracts/BodyFormat.swift

+        return lhs.type == rhs.type
+    }
+
+    public static let json = BodyFormat("application/json")


Do we have a separate "registry" of MIME types somewhere that we could reuse?

I added the type here as a convenience for a client (eg KituraKit) trying to tally the BodyFormat with a Content-Type header from a server response.

I did think about how to integrate it with a general approach for MIME/Content-types, but I don't think we have a really coherent story on that in Kitura (something I think we should fix). So I concluded that I shouldn't rathole on it in this feature/PR.

ianpartridge · 2018-02-15T17:08:34Z

Sources/KituraContracts/BodyFormat.swift

+ * limitations under the License.
+ **/
+
+import Foundation


I don't think this file needs to import Foundation.

IIRC Foundation is required for the Data type

ianpartridge · 2018-02-15T17:09:49Z

Sources/KituraContracts/Contracts.swift

        self.reason = reason
    }

+    /// Creates an error respresenting the given base error, with a custom


Typo - respresenting.

ianpartridge · 2018-02-15T17:16:17Z

Sources/KituraContracts/Contracts.swift

+              is not supported
+     */
+    public func encodeBody(_ format: BodyFormat) throws -> Data? {
+        guard case .codable? = body else { return nil }


I'm confused. Doesn't this return nil if we're not .codable? Don't we want to return nil if we are .codable?

As discussed in person, this function is for encoding the Codable body into Data, so the "source" of the body must be Codable rather than Data.
I will change the doc to "Returns the codable error body..." in the hope this clarifies the behaviour.

Side-note: I did consider a different API where the encode/decode was done transparently (ie you could just access the body as Codable or Data and it would work regardless of how it was initialized, but it didn't turn out well so I went for this less magic API instead).

ianpartridge · 2018-02-15T17:16:41Z

Sources/KituraContracts/Contracts.swift

+     - throws a `DecodingError` if decoding fails
+     */
+    public func decodeBody<Body: Codable>(_ type: Body.Type) throws -> Body? {
+        guard case let .data(bodyData, format)? = body else { return nil }


Same comment as before, isn't this the wrong way round?

Similar response to the above, I will update the doc to "Returns the Data error body as..."

ianpartridge · 2018-02-15T17:28:06Z

Sources/KituraContracts/Contracts.swift


+    /// Representation of the error body
+    /// May be a type-erased Codable object or a Data (in a particular format)
+    public enum ErrorBody {


Did you consider using the struct approach again, just in case we ever want a third option?

I thought about it, but I'm not sure it's necessary here. Unlike the BodyFormat case, the function seems fairly tightly defined and fitted to these two cases.

tunniclm added 7 commits January 29, 2018 17:59

Add support for custom Codable response body to RequestError

14c1457

add Codable body property, renamed bodyData

c528304

make body a private(set) var defaulting to nil

d3cb586

Rename and extract BodyFormat

399469d

Improve Jazzy doc

bf7d0f6

Add doc to init, add BodyFormat.swift file

16778a2

Improve Jazzy doc

2801ef5

tunniclm added 2 commits January 29, 2018 19:09

Improve Jazzy doc

df119f2

Improve Jazzy doc

0791f20

tunniclm mentioned this pull request Jan 29, 2018

Add RequestError body codable Kitura/Kitura#1214

Merged

2 tasks

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

djones6 suggested changes Feb 1, 2018

View reviewed changes

tunniclm added 2 commits February 2, 2018 14:59

Refactor RequestError to support client decoding

ab6f835

Refactor tests, add new RequestError tests

2869e26

tunniclm commented Feb 2, 2018

View reviewed changes

tunniclm force-pushed the add_requesterror_body branch from 854ef08 to 9947ddb Compare February 2, 2018 15:30

Fix doc error

60dc142

tunniclm force-pushed the add_requesterror_body branch from 9947ddb to 60dc142 Compare February 2, 2018 15:32

djones6 approved these changes Feb 2, 2018

View reviewed changes

Expose body property, non-throwing decode

a661001

ianpartridge reviewed Feb 15, 2018

View reviewed changes

tunniclm added 3 commits February 16, 2018 13:29

Fix typo

9a506c4

Add throws to doc comment

abda533

Add clarification to doc comments

71c8674

ianpartridge approved these changes Feb 16, 2018

View reviewed changes

ianpartridge merged commit 6419b27 into master Feb 21, 2018

ianpartridge deleted the add_requesterror_body branch February 21, 2018 13:34

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!

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

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djones6 commented Feb 7, 2018

Uh oh!

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

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 29, 2018 •

edited

Loading