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

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

Merged
merged 1 commit into from
Mar 9, 2018

Conversation

c1ira
Copy link
Contributor

@c1ira c1ira commented Feb 7, 2018

Allow POST success status codes besides 201 (CREATED)

Description

This change adds HTTPStatusCode extensions for identifying whether a status code represents success. The remainder of the change is PRed to Kitura: Kitura/Kitura#1218

Motivation and Context

When Kitura receives a POST, its success response is 201 (CREATED). If one explicitly specifies any other success response, such as 200 (OK), Kitura treats it like an error.

How Has This Been Tested?

Ran swift test.
Have been using this modification at Capital One for several months.

Checklist:

  • [x ] I have submitted a CLA form
  • If applicable, I have updated the documentation accordingly.
  • If applicable, I have added tests to cover my changes.

…f one explicitly specifies any other success response, such as 200 (OK), Kitura treats it like an error.

This change adds HTTPStatusCode extensions for identifying whether a status code represents success.
@CLAassistant
Copy link

CLAassistant commented Feb 7, 2018

CLA assistant check
All committers have signed the CLA.

@codecov-io
Copy link

Codecov Report

Merging #240 into master will decrease coverage by 0.32%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #240      +/-   ##
==========================================
- Coverage   74.89%   74.56%   -0.33%     
==========================================
  Files          31       31              
  Lines        4162     4164       +2     
==========================================
- Hits         3117     3105      -12     
- Misses       1045     1059      +14
Flag Coverage Δ
#CHTTPParser 57.46% <ø> (-0.07%) ⬇️
#KituraNet 74.56% <0%> (-0.33%) ⬇️
Impacted Files Coverage Δ
Sources/KituraNet/HTTP/HTTP.swift 76.47% <0%> (-10.2%) ⬇️
...ces/KituraNet/Server/ServerLifecycleListener.swift 79.16% <0%> (-4.17%) ⬇️
Sources/KituraNet/IncomingSocketHandler.swift 78.03% <0%> (-3.74%) ⬇️
Sources/KituraNet/HTTP/HTTPServer.swift 79.88% <0%> (-1.12%) ⬇️
Sources/CHTTPParser/http_parser.c 57.27% <0%> (-0.07%) ⬇️

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 d781655...4ca5210. Read the comment docs.

@djones6 djones6 self-requested a review March 9, 2018 12:06
Copy link
Contributor

@djones6 djones6 left a comment

Choose a reason for hiding this comment

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

This looks good to me.


extension HTTPStatusCode {

public static var successRange: Range<HTTPStatusCode> { return .OK ..< .multipleChoices }
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems reasonable to define a range on HTTPStatusCode in this way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, we might consider adding the other ranges while we're at it (1xx informational, 3xx redirection, 4xx clienterror, 5xx servererror)

@djones6
Copy link
Contributor

djones6 commented Mar 9, 2018

@c1ira Thanks for the PR. I'm happy to merge this now in order to unblock work on Kitura/Kitura#1218.

Ideally we'd have some tests to confirm that successRange represents the expected range, but I'm happy for that to be in a subsequent PR.

@djones6 djones6 merged commit 651136d into Kitura:master Mar 9, 2018
@c1ira
Copy link
Contributor Author

c1ira commented Mar 9, 2018

Thanks, and I'll put the test on my to-do list.

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.

5 participants