feat: Add Content-Type to custom encoder/decoder by Andrew-Lees11 · Pull Request #1309 · Kitura/Kitura

Andrew-Lees11 · 2018-07-17T14:35:08Z

Description

This PR adds two fields to Router called encoders and decoders.
These contain a String to BodyEncoderGenerator/BodyDecoderGenerator dictionary.

We have also added a Read(as: Codable, decoder: BodyDecoder) function, which decodes the data from the routerRequest into a codable type using the given Decoder.

The Codable router uses the decoders dictionary to generate a decoder for the Content-type header and then use the new Read function to decode the request using that decoder.

The Codable router uses the Accepts header to select the best encoder from the Encoders dictionary and use that encoder to encode it's response.

Travis pass requirements

This requires a the Query Coders to be able to encode/decoder data which is in this (PR](Kitura/KituraContracts#28)

It also requires the addition of the BodyEncoder/BodyDecoder protocol which is currently being worked on this branch

Motivation and Context

This allows users to customize the JSON encoder and decoder in Codable routes and add their own Encoders/Decoders for encoding/decoding different Content-Type headers.

This is a fix for issue #1298
This follows on from the work in pull request 1221 but is generalized for any custom encoder/decoder

How Has This Been Tested?

This change affects all Codable and Type-Safe routes so tests have been added which use a custom JSONEncoder. The test suite has also been updated to allow custom encoders/decoders to be used.

Checklist:

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

djones6 · 2018-08-28T12:49:55Z

Sources/Kitura/Router.swift

+              let bestMediaType = MediaType(bestAccepts),
+              let bestEncoder = encoders[bestMediaType]
+        else {
+            let jsonEncoder = encoders[.json] ?? { return JSONEncoder() }


Rather than hard-coding a fallback MediaType of .json could we allow the user to specify this on the Router alongside the list of encoders?

djones6 · 2018-08-28T12:55:33Z

Sources/Kitura/RouterRequest.swift

    ///
    /// - Parameter request: the server request
-    init(request: ServerRequest) {
+    convenience init(request: ServerRequest) {


This is internal, and it looks like (now that you've changed Router to call the 2-arg initializer) it is unused. I think we could just remove this init altogether.

djones6 · 2018-08-28T13:12:11Z

Sources/Kitura/Router.swift

    public func handle(request: ServerRequest, response: ServerResponse) {
-        let routeReq = RouterRequest(request: request)
+        var decoder: (() -> BodyDecoder)?
+        if let mediaType = MediaType(headers: request.headers) {


We should test this to see how much overhead it adds for requests such as a simple GET that does not contain a content type (or body).

djones6 · 2018-08-28T13:13:42Z

Sources/Kitura/RouterRequest.swift

        var data = Data()
        _ = try serverRequest.read(into: &data)
-        return try JSONDecoder().decode(type, from: data)
+        let decoderInstance = decoder ?? JSONDecoder()


let decoder = self.decoder ?? JSONDecoder()

djones6 · 2018-08-28T13:14:42Z

Sources/Kitura/RouterResponse.swift

    private var lifecycle = Lifecycle()

-    private let encoder = JSONEncoder()
+    let encoder: BodyEncoder


Could this remain private?

djones6 · 2018-08-28T13:25:34Z

Sources/Kitura/RouterResponse.swift

+        }
+        do {
+            headers.setType("json")
+            send(data: try JSONEncoder().encode(json))


It would be confusing to the user if they customize their JSON encoder that we ignore it and use a default one here.

djones6 · 2018-08-28T13:26:19Z

Sources/Kitura/RouterResponse.swift

        }

-        let jsonStr = String(data: try encoder.encode(jsonp), encoding: .utf8)!
+        let jsonStr = String(data: try JSONEncoder().encode(jsonp), encoding: .utf8)!


same comment as above

djones6 · 2018-08-28T13:42:10Z

Sources/Kitura/String+Extensions.swift


        return result
    }
-


can you undo this whitespace-only change?

djones6 · 2018-08-28T13:42:17Z

Sources/Kitura/contentType/MediaType.swift

    /// The hashValue for the MediaTypes. Required for Hashable conformance.
    public let hashValue: Int
 }
-


djones6 · 2018-08-28T13:43:11Z

Tests/KituraTests/TestCodableRouter.swift

+            XCTAssertEqual(inDate, codableDate)
+            respondWith(1, codableDate, nil)
+        }
+        customRouter.put("/customCoder") { (id: Int, inDate: CodableDate, respondWith: (CodableDate?, RequestError?) -> Void) in


After the refactor we probably only need to test GET and POST now

djones6 · 2018-08-29T16:19:55Z

Sources/Kitura/Router.swift

     * ```
     * - Parameter request: The RouterRequest to check
-     * - Returns: A tuple of the highest rated Encoder, or a JSONEncoder() if no encoders match the Accepts header and it's corresponding MediaType.
+     * - Returns: A tuple of the highest rated MediaType and it's corresponding Encoder, or a JSONEncoder() if no encoders match the Accepts header and it's corresponding .


it's -> its (x2)
Also, you lost the word 'MediaType' here somehow.

djones6 · 2018-08-30T15:59:21Z

I regression tested these changes on an Ubuntu 16.04 box.

The original changes regressed performance quite badly (11%) for a simple Hello World which did not require a decoder/encoder. Commit 29d83a2 delays the evaluation of the output encoder until we need one (which resolves this regression).

We found a further regression when the request contains an Accept header, as we went through some fairly complex Accept header processing even in the case where we only have a single encoder available. The fix in 3f6b9e9 avoids processing the Accept header at all when we only have one encoder (which is the default case), and tries to reduce the cost of constructing a MediaType when a Content-Type header is present.

Perf results:

Hello World (no Content-Type / Accept headers)

               | Throughput (req/s)      | CPU (%) | Mem (kb)     | Latency (ms)                   | good
Implementation | Average    | Max        | Average | Avg peak RSS | Average  | 99%      | Max      | iters
---------------|------------|------------|---------|--------------|----------|----------|----------|-------
          Base |    68332.1 |    70271.5 |    97.9 |        20454 |      1.8 |      5.7 |    202.4 |    20
       e1ec693 |    60803.6 |    65512.3 |    98.7 |        22978 |      2.1 |      5.0 |     35.1 |    20
       29d83a2 |    68129.5 |    70385.6 |    97.8 |        24440 |      1.9 |      6.5 |    203.6 |    20

Codable GET (no Content-Type / Accept headers)

               | Throughput (req/s)      | CPU (%) | Mem (kb)     | Latency (ms)                   | good
Implementation | Average    | Max        | Average | Avg peak RSS | Average  | 99%      | Max      | iters
---------------|------------|------------|---------|--------------|----------|----------|----------|-------
          Base |    32700.2 |    33353.0 |    97.9 |        28159 |      3.9 |      4.6 |     28.7 |    20
       e1ec693 |    32046.6 |    32795.3 |    98.5 |        28972 |      3.9 |      5.1 |    201.3 |    20
       29d83a2 |    32258.2 |    32690.4 |    98.6 |        27474 |      3.9 |      4.6 |     26.2 |    20

Codable POST (no Accept header)

               | Throughput (req/s)      | CPU (%) | Mem (kb)     | Latency (ms)                   | good
Implementation | Average    | Max        | Average | Avg peak RSS | Average  | 99%      | Max      | iters
---------------|------------|------------|---------|--------------|----------|----------|----------|-------
          Base |    29489.9 |    29893.9 |    98.4 |        28787 |      4.3 |      6.6 |     30.5 |    20
       e1ec693 |    26609.5 |    27048.2 |    98.0 |        29326 |      4.8 |      6.1 |     32.5 |    20
       29d83a2 |    26635.6 |    27114.8 |    97.8 |        27570 |      4.8 |      6.5 |     30.8 |    20
       3f6b9e9 |    27226.8 |    27655.3 |    97.5 |        22007 |      4.7 |      6.3 |     29.8 |    10

Codable GET (with Accept header)

               | Throughput (req/s)      | CPU (%) | Mem (kb)     | Latency (ms)                   | good
Implementation | Average    | Max        | Average | Avg peak RSS | Average  | 99%      | Max      | iters
---------------|------------|------------|---------|--------------|----------|----------|----------|-------
          Base |    31769.9 |    32195.2 |    98.1 |        28869 |      4.0 |      6.7 |    199.6 |    20
       e1ec693 |    18288.3 |    18688.2 |    99.0 |        29073 |      7.0 |      8.4 |    208.9 |    20
       29d83a2 |    18004.3 |    18400.5 |    99.2 |        28592 |      7.1 |      8.5 |     36.2 |    20
       3f6b9e9 |    31125.3 |    31452.8 |    98.0 |        27566 |      4.1 |      6.9 |     28.1 |    10

Codable POST (with Accept header)

               | Throughput (req/s)      | CPU (%) | Mem (kb)     | Latency (ms)                   | good
Implementation | Average    | Max        | Average | Avg peak RSS | Average  | 99%      | Max      | iters
---------------|------------|------------|---------|--------------|----------|----------|----------|-------
          Base |    28530.1 |    29070.6 |    98.1 |        29092 |      4.4 |      8.8 |     28.7 |    10
       e1ec693 |    16620.7 |    16883.0 |    99.3 |        29714 |      7.7 |      9.1 |     43.3 |    10
       29d83a2 |    16250.5 |    16571.0 |    99.3 |        29056 |      7.8 |      9.3 |     35.9 |    10
       3f6b9e9 |    26858.6 |    27115.1 |    97.7 |        19545 |      4.7 |      8.8 |     26.5 |    10

djones6 · 2018-08-30T16:03:19Z

We are left with a 6-8% regression for a Codable POST (regardless of whether there is an Accept header), and a smaller ~2% regression for a Codable GET.

There may be other opportunities to reduce regression which we haven't explored yet.

Andrew-Lees11 requested review from djones6 and ianpartridge July 17, 2018 14:35

This was referenced Jul 17, 2018

Cannot set JSONDecoder on Codable router #1298

Closed

Feat: Add BodyEncoder and BodyDecoder Kitura/KituraContracts#29

Merged

Andrew-Lees11 added 15 commits August 24, 2018 15:25

add custom encoder and decoder

dab1ab5

remove duplicate method

6fbf1e1

removed encoder when returning only error

708d6c3

added contentType coder generator dictionary

623a780

added urlEncoder

63d3cc4

removed default queryEncoder

e4c0d7a

removed String+Extensions

b8e87db

fixed tests for equatable in 4.0

b5a0567

moved decoder onto request

2ec066f

move encoder inside response

7cda639

remove added lines

8f72655

reverted spaces in typeSafeMiddleware

dc7ea0c

use MediaType Class

8dffacf

updated Jazzy docs

3d5bd9f

split TypeSafe custom coder test function

82b046c

Andrew-Lees11 force-pushed the customCoder branch from a67008d to 82b046c Compare August 24, 2018 14:51

djones6 requested changes Aug 28, 2018

View reviewed changes

Andrew-Lees11 added 2 commits August 28, 2018 17:07

implemented Dave's review changes

e1ec693

delayed select encoder till send encodable

29d83a2

djones6 reviewed Aug 29, 2018

View reviewed changes

djones6 added 2 commits August 30, 2018 13:55

Fix 4.0.3 optional equatable ness

7d56ead

Attempt to reduce perf regression

3f6b9e9

djones6 approved these changes Aug 30, 2018

View reviewed changes

Merge branch 'master' into customCoder

02bc98b

ianpartridge approved these changes Aug 30, 2018

View reviewed changes

ianpartridge merged commit b387896 into master Aug 30, 2018

ianpartridge deleted the customCoder branch August 30, 2018 16:24

djones6 mentioned this pull request Jan 25, 2019

[WIP] Add BSON to CodableRouting benchmark Kitura/Kitura-Benchmarks#21

Open

djones6 mentioned this pull request Mar 4, 2019

SwaggerGenerator does not reflect Date encoding strategy #1416

Closed

Conversation

Andrew-Lees11 commented Jul 17, 2018

Description

Travis pass requirements

Motivation and Context

How Has This Been Tested?

Checklist:

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!

djones6 commented Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Hello World (no Content-Type / Accept headers)

Codable GET (no Content-Type / Accept headers)

Codable POST (no Accept header)

Codable GET (with Accept header)

Codable POST (with Accept header)

Uh oh!

djones6 commented Aug 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

djones6 commented Aug 30, 2018 •

edited

Loading