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 route for Url encoded forms #1228

Merged
merged 14 commits into from Mar 20, 2018

Conversation

Projects
None yet
5 participants
@Andrew-Lees11
Contributor

Andrew-Lees11 commented Mar 1, 2018

Description

update the codable router's POST handlers to detect application/x-www-form-urlencoded using the query parameters decoding logic. This allows users to send a html form to a Kitura router which will decode to a codable struct.

How Has This Been Tested?

A new test has been added for this behaviour and this along with all other Kitura tests pass.

This was also tested manually by setting up a html form, a Kitura route and a codable struct. The form successfully sent a post request to the router which was mapped to the codable struct.

Since this is changing an existing route it will probably be worth testing the performance impact.

Checklist:

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

@Andrew-Lees11 Andrew-Lees11 requested review from ianpartridge and djones6 Mar 1, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Mar 1, 2018

Codecov Report

Merging #1228 into master will increase coverage by 0.23%.
The diff coverage is 69.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1228      +/-   ##
==========================================
+ Coverage   89.39%   89.62%   +0.23%     
==========================================
  Files          38       38              
  Lines        2234     2246      +12     
==========================================
+ Hits         1997     2013      +16     
+ Misses        237      233       -4
Flag Coverage Δ
#Kitura 89.62% <69.23%> (+0.23%) ⬆️
Impacted Files Coverage Δ
Sources/Kitura/RouterRequest.swift 90.12% <66.66%> (-1.88%) ⬇️
Sources/Kitura/CodableRouter.swift 80.64% <71.42%> (+1.97%) ⬆️
Sources/Kitura/String+Extensions.swift 90.9% <0%> (+13.63%) ⬆️

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 6690f2b...b5d472e. Read the comment docs.

@djones6

This looks good, however it will have some performance impact. We'd need to run a comparison to determine how much. In particular I'd be concerned with increasing overhead if we were to support more Content-Types in the future.

I've suggested an approach that might help avoid this (by only parsing the Content-Type once per request object).

@@ -614,7 +627,7 @@ public struct CodableHelpers {
* - Returns: An instance of `InputType` representing the decoded body data.
*/
public static func readCodableOrSetResponseStatus<InputType: Codable>(_ inputCodableType: InputType.Type, from request: RouterRequest, response: RouterResponse) -> InputType? {
guard CodableHelpers.isContentTypeJSON(request) else {
guard CodableHelpers.isContentTypeJSON(request) || CodableHelpers.isContentTypeURLEncoded(request) else {

This comment has been minimized.

@djones6

djones6 Mar 6, 2018

Member

This seems okay from a performance perspective: the guard should short-circuit evaluate in the existing case (where the content is JSON) and we will only evaluate the second clause in the case where the data is not JSON. What would this look like if we add additional content types though?

response.status(.internalServerError)
return nil
}
let urlKeyValuePairs = bodyAsString.urlDecodedFieldValuePairs

This comment has been minimized.

@djones6

djones6 Mar 6, 2018

Member

What magic is this?
Oh, I see, it's a String extension that we already defined. 👍

@@ -624,13 +637,41 @@ public struct CodableHelpers {
return nil
}
do {
return try request.read(as: InputType.self)
return CodableHelpers.isContentTypeURLEncoded(request) ? try CodableHelpers.parseURLEncoded(InputType.self, from: request, response: response) : try request.read(as: InputType.self)

This comment has been minimized.

@djones6

djones6 Mar 6, 2018

Member

We're evaluating Content-Type a second time, and each time we call isContentTypeX() we have to perform a Dictionary lookup and a String compare. There will be a performance impact for existing JSON encoding here, because we first have to ask isContentTypeURLEncoded, before executing the existing request.read.
How about we evaluate the Content-Type once, and store the result alongside the request (eg. in an enum)? Then we could write the above something like:

switch request.contentType {
    case urlEncoded: return try CodableHelpers.parseURLEncoded(InputType.self, from: request, response: response)
    case jsonEncoded: return try request.read(as: InputType.self)
}

Checking the value of an enum on each request would be trivial extra overhead.

In addition, the guard above could be simplified to

guard request.contentType != .unsupported else {
/// - Throws: An error if any value throws an error during decoding.
/// - Returns: The instantiated Codable object
public func readURLForm<T: Decodable>(as type: T.Type) throws -> T {
let bodyAsString = try self.readString()

This comment has been minimized.

@ianpartridge

ianpartridge Mar 9, 2018

Member

Maybe it reads better as:

guard let body = try self.readString()
      let urlKeyValuePairs = body.urlDecodedFieldValuePairs else {
    throw ()
}
return try QueryDecoder(dictionary: urlKeyValuePairs).decode(T.self)
/// - Throws: `DecodingError.dataCorrupted` if values requested from the payload are corrupted, or if the given data is not valid JSON.
/// - Throws: An error if any value throws an error during decoding.
/// - Returns: The instantiated Codable object
public func readURLForm<T: Decodable>(as type: T.Type) throws -> T {

This comment has been minimized.

@ianpartridge

ianpartridge Mar 9, 2018

Member

A better name might be readURLEncoded().

@Andrew-Lees11

This comment has been minimized.

Contributor

Andrew-Lees11 commented Mar 9, 2018

@ianpartridge Changes are up and this can be merged once travis passes and you approve.

guard let contentType = request.headers["Content-Type"] else {
return false
}
return (contentType.hasPrefix("application/x-www-form-urlencoded"))

This comment has been minimized.

@ianpartridge

ianpartridge Mar 9, 2018

Member

Remove the outer parentheses.

@@ -42,6 +43,7 @@ import Dispatch
protocol RequestTestBuilder {
func request(_ method: String, path: String) -> AssertionTestBuilder
func request<T: Encodable>(_ method: String, path: String, data: T) -> AssertionTestBuilder
func request(_ method: String, path: String, URLEncodedObject: String) -> AssertionTestBuilder

This comment has been minimized.

@ianpartridge

ianpartridge Mar 9, 2018

Member

Call this URLEncodedString?

@@ -110,6 +122,11 @@ class ServerTestBuilder: RequestTestBuilder, AssertionTestBuilder {
requests.append(Request(test, method, path, data))
return self
}
public func request(_ method: String, path: String, URLEncodedObject: String) -> AssertionTestBuilder {

This comment has been minimized.

@ianpartridge

ianpartridge Mar 9, 2018

Member

url... not URL...

/// - Throws: An error if any value throws an error during decoding.
/// - Returns: The instantiated Codable object
public func readURLEncoded<T: Decodable>(as type: T.Type) throws -> T {
let bodyAsString = try self.readString()

This comment has been minimized.

@ianpartridge

ianpartridge Mar 9, 2018

Member

I think this could just be body not bodyAsString. The use of readString() hints enough that it's a String.

@@ -19,7 +19,7 @@ import Socket
import LoggerAPI
import Foundation
import KituraContracts

This comment has been minimized.

@ianpartridge

ianpartridge Mar 9, 2018

Member

Add a blank line below this.

public func readURLEncoded<T: Decodable>(as type: T.Type) throws -> T {
let bodyAsString = try self.readString()
guard let urlKeyValuePairs = bodyAsString?.urlDecodedFieldValuePairs else {
throw(Error.failedToParseRequestBody(body: bodyAsString ?? "Failed to read body as String"))

This comment has been minimized.

@ianpartridge

ianpartridge Mar 9, 2018

Member

Remove outer parentheses.

.request("post", path: "/urlencoded", urlEncodedString: "id=4&name=David")
.hasStatus(.created)
.hasData(user)

This comment has been minimized.

@ianpartridge

ianpartridge Mar 9, 2018

Member

I feel we could expand the testing a bit here... What about an invalid urlEncodedString for example?

@@ -84,6 +86,16 @@ class ServerTestBuilder: RequestTestBuilder, AssertionTestBuilder {
})
}
}
init(_ test: KituraTest, _ method: String, _ path: String, URLEncodedObject: String) {

This comment has been minimized.

@ianpartridge

ianpartridge Mar 9, 2018

Member

I think this should be _ urlEncodedString: String to match the <T: Encodable> API above?

self.test = test
self.invoker = { callback in
test.performRequest(method, path: path, callback: callback, requestModifier: { request in
request.headers["Content-Type"] = "application/x-www-form-urlencoded; charset=utf-8"

This comment has been minimized.

@ianpartridge

ianpartridge Mar 9, 2018

Member

According to https://stackoverflow.com/questions/16819502/application-x-www-form-urlencoded-and-charset-utf-8 the application/x-www-form-urlencoded MIME type doesn't have a charset parameter, so this should just be request.headers["Content-Type"] = "application/x-www-form-urlencoded" I think?

@Andrew-Lees11

This comment has been minimized.

Contributor

Andrew-Lees11 commented Mar 9, 2018

Have done the test review comments. Have also added tests for failure case and when you send more data than the codable expected.

/// Read the URLEncoded body of the request as a Codable object.
///
/// - Parameter type: Codable object to which the body of the request will be converted.

This comment has been minimized.

@ianpartridge

ianpartridge Mar 9, 2018

Member

This parameter is called as not type (the doc is wrong on the function above too...)

@@ -84,6 +86,16 @@ class ServerTestBuilder: RequestTestBuilder, AssertionTestBuilder {
})
}
}
init(_ test: KituraTest, _ method: String, _ path: String, _ URLEncodedObject: String) {

This comment has been minimized.

@ianpartridge

ianpartridge Mar 9, 2018

Member

URLEncodedObject - > urlEncodedString I think, to be consistent with the request() function.

@@ -84,6 +86,16 @@ class ServerTestBuilder: RequestTestBuilder, AssertionTestBuilder {
})
}
}
init(_ test: KituraTest, _ method: String, _ path: String, _ URLEncodedString: String) {

This comment has been minimized.

@ianpartridge

ianpartridge Mar 9, 2018

Member

Sorry, can this be urlEncodedString not URLEncodedString, otherwise it looks like a reference to a type.

@djones6

This comment has been minimized.

Member

djones6 commented Mar 12, 2018

FWIW, there was no measurable performance penalty for a simple benchmark using Codable routing:

               | Throughput (req/s)      | CPU (%) | Mem (kb)     | Latency (ms)                   | good
Implementation | Average    | Max        | Average | Avg peak RSS | Average  | 99%      | Max      | iters
---------------|------------|------------|---------|--------------|----------|----------|----------|-------
          Base |    34122.9 |    34337.7 |    98.4 |        29125 |      3.7 |      4.2 |     20.1 |     5
        pr1228 |    34158.5 |    34385.6 |    98.2 |        28717 |      3.7 |      4.2 |     21.3 |     5
@ianpartridge

LGTM!

@djones6

This looks good. I'll add a FIXME for the future improvement to querying Request's content type.

guard let contentType = request.headers["Content-Type"] else {
return false
}
return contentType.hasPrefix("application/x-www-form-urlencoded")

This comment has been minimized.

@djones6

djones6 Mar 20, 2018

Member

This is executed twice on the critical path (by read(), and by readCodableOrSetResponseStatus() which itself calls read()). Each execution involves a dictionary lookup and a call to String.hasPrefix(), both of which involve String comparison.

It would be better to cache this state against the Request itself (eg. in an enum), particularly if we end up supporting more content types in the future. For now though, it's a trivial extra cost and we can make a note to fix it in the future.

/// - Throws: Socket.Error if an error occurred while reading from a socket.
/// - Throws: `DecodingError.dataCorrupted` if values requested from the payload are corrupted, or if the given data is not valid JSON.
/// - Throws: An error if any value throws an error during decoding.
/// - Returns: The instantiated Codable object
public func read<T: Decodable>(as type: T.Type) throws -> T {
if CodableHelpers.isContentTypeURLEncoded(self) {

This comment has been minimized.

@djones6

djones6 Mar 20, 2018

Member

If the content type was stored on Request, we could simply switch on it. (see my comment above)

.request("post", path: "/urlencoded", urlEncodedString: "id=4&name=David")
.hasStatus(.created)
.hasContentType(withPrefix: "application/json")
.hasData(user)

This comment has been minimized.

@djones6

djones6 Mar 20, 2018

Member

👍 for nicely concise and expressive tests.

djones6 added some commits Mar 20, 2018

@ianpartridge ianpartridge merged commit a49d741 into master Mar 20, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@ianpartridge ianpartridge deleted the URLEncodedForms branch Mar 20, 2018

@svanimpe

This comment has been minimized.

Contributor

svanimpe commented Apr 17, 2018

Is there a way to use this in server-side web apps (routes that receive a form and render a HTML response)?

@ianpartridge

This comment has been minimized.

Member

ianpartridge commented Apr 17, 2018

I'm afraid the Codable routing handlers are still JSON and REST-centric...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment