Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add Content-Type to custom encoder/decoder #1309

Merged
merged 20 commits into from
Aug 30, 2018
Merged

Conversation

Andrew-Lees11
Copy link
Contributor

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.

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

Choose a reason for hiding this comment

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

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?

@@ -161,12 +164,28 @@ public class RouterRequest {
/// Initializes a `RouterRequest` instance
///
/// - Parameter request: the server request
init(request: ServerRequest) {
convenience init(request: ServerRequest) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

@@ -492,11 +548,16 @@ extension Router : ServerDelegate {
/// - Parameter response: The `ServerResponse` object used to send responses to the
/// HTTP request at the [Kitura-net](http://ibm-swift.github.io/Kitura-net/) API level.
public func handle(request: ServerRequest, response: ServerResponse) {
let routeReq = RouterRequest(request: request)
var decoder: (() -> BodyDecoder)?
if let mediaType = MediaType(headers: request.headers) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

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

Choose a reason for hiding this comment

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

let decoder = self.decoder ?? JSONDecoder()

@@ -85,7 +85,9 @@ public class RouterResponse {

private var lifecycle = Lifecycle()

private let encoder = JSONEncoder()
let encoder: BodyEncoder
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this remain private?

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

Choose a reason for hiding this comment

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

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

@@ -572,7 +591,7 @@ extension RouterResponse {
.replacingOccurrences(of: "\u{2029}", with: "\\u2029")
}

let jsonStr = String(data: try encoder.encode(jsonp), encoding: .utf8)!
let jsonStr = String(data: try JSONEncoder().encode(jsonp), encoding: .utf8)!
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment as above

@@ -53,7 +53,6 @@ extension String {

return result
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

can you undo this whitespace-only change?

@@ -144,4 +144,3 @@ public struct MediaType: CustomStringConvertible, Equatable, Hashable {
/// The hashValue for the MediaTypes. Required for Hashable conformance.
public let hashValue: Int
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

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

Choose a reason for hiding this comment

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

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

*
* ### Usage Example: ###
* ```swift
* let (mediaType, encoder) = selectResponseEncoder(request)
* ```
* - 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 .
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@djones6
Copy link
Collaborator

djones6 commented Aug 30, 2018

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
Copy link
Collaborator

djones6 commented Aug 30, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants