Add IANA registered HTTP methods

[CTerasa-ep]

The IANA curates a list of HTTP methods in the Hypertext Transfer Protocol (HTTP) Method Registry. See: https://www.iana.org/assignments/http-methods/http-methods.xhtml

These methods are well established for HTTP, although in most cases, especially for REST APIs only a limited subset is valid. However, this does not render incoming messages invalid or even malformed.

The default behavior of pistache is, to respond with a 400 "Bad Request". RFC 9119 states, that this should be perceived as "client error", which is not always the case.

In order to handle non-REST, but "common" HTTP requests in pistache, these need to be registered in the HTTP_METHODS.

This MR adds the missing IANA registered HTTP methods to pistache.

Background

We are currently assessing the use of pistache together with the OpenAPI openapi-generator and the CATS testing and fuzzing tool. The CATS test case NonRestHttpMethodsFuzzer spitted out many warnings. The warnings stated, that non-REST method requests should return the HTTP response status code of 405 Method Not Allowed.

I was not able to handle this in my handler as these requests were filtered out by pistache and generated a 400 Bad Request. To be able to handle non-REST methods we need to add the methods to pistache.

A reasonable source for "common" HTTP request methods is the IANA Registry.

Of course there are many things can be argued about (and even more):

1. Should pistache know and handle the arguably more obscure HTTP requests and if so is the list complete?
2. Should there be a different way to handle unknown HTTP requests
3. Should the pisatche project be modified based on CATS (or other REST APIs tools) that enforce some behavior.
4. Is the 405 Method Not Allowedresponse the correct response for non-REST requests?
5. Does this CATS test make sense for REST APIs?

Questions

1. I currently do no know if this changes the ABI so the version.txt is yet untouched.
2. The naming convention for the methods with dashes (BASELINE-CONTROL and VERSION-CONTROL) is open for discussion.

On the patch

Created by using a shell one-liner to extract IANA methods:

curl https://www.iana.org/assignments/http-methods/methods.csv | awk -F "," '{print $1}' | sed -e '1d' -e '$d' | awk '{print "METHOD(" toupper(substr($1,1,1)) tolower(substr($1,2)) ", \"" $1 "\") \\"}'

After that employ manual filtering and reformatting.

[dennisjenkins75]

Please edit the PR's title to deal with the "commitlint" error. I don't recall the exact syntax, but see recent successfully merged PRs for examples.

As for the abidiff check, I defer to Kip and/or Andreas for their assessment.

Overall, I do not have a problem with this PR and would be in favor of merging it.

[kiplingw]

Thanks @dennisjenkins75 and @CTerasa-ep.

Regarding API / ABI changes, @CTerasa-ep please refer to our documentation on when to bump them here. Based on what I can see each additional usage of METHOD is augmenting the enumerated class Method.

But the question here is whether this actually changes the ABI? My initial guess is no, but I could be wrong. At the very least the API minor version will need to be bumped.

[CTerasa-ep]

So I just pushed to my branch, and was a bit surprised to see that the change was automatically adjusted in the MR. OK, just learning GiHub MR workflow. Took me some clicks to find the previous failing tests again.

The commit https://github.com/pistacheio/pistache/commit/32e8aae1c35669da0c29d88be5789bb89783facb fixes the commit message issue and bumps the minor version to 0.1.0.20230111 as requested by @kiplingw in https://github.com/pistacheio/pistache/pull/1112#issuecomment-1377777076.

I am still unsure about the abidiff test though. https://github.com/pistacheio/pistache/actions/runs/3882965880/jobs/6629637027 states that there were functions added (should be OK) and indirect sub-type changes (without any size changes). That is plausible, as the Methods enum class now has more valid int values but does not change in size. Both changes should be OK for library consumers AKAIK. Correct me if I am wrong.

Perhaps the abidiff check could be tweaked to allow this kind of changes?

It would also be great if the commitlint.config.js of the commitlint check was part of the project instead of being created during the check itself. See https://github.com/pistacheio/pistache/actions/runs/3882965863/jobs/6629636420

[kiplingw]

Thanks @CTerasa-ep. I didn't setup the abidiff test so this might be a good opportunity for @Tachi107 to share his wisdom and insight.

[dennisjenkins75]

I am not 100% sure about the abidiff test results. I agree with the PR author that its probably safe to override / ignore in this case. I am in favor of merging this PR, but think that we should wait for @Tachi107 to respond.

[Tachi107]

@CTerasa-ep thanks for the patch! The abidiff results look pretty innocuous to me, it is just reporting that something changed in the binary, but not necessarily in a backwards incompatible way.

[kiplingw]

Not to push the goal posts, but since he's got to at least bump the minor version, why don't we take this time to tag our first 1.0.0 stable release?

[Tachi107]

I don't know, it's just a number after all. We do already have a tagged commit, and we could just tag this one too.

[kiplingw]

We could tag this one as a 1.0.0 release? I know we had been talking about it months ago and, if I recall, all the outstanding major blockers were resolved.

[dennisjenkins75]

We should not mix this PR and bumping from 0.x to 1.0. We should keep these separate. And I would not go to 1.0 until this version goes out and we do not receive any relevant bug reports for a few weeks. Declaring it "stable" now seems premature?

[kiplingw]

Yeah that's a good point. We can tag a stable release down the road if this doesn't break anything. I'm fine with merging.

[kiplingw]

Sadly it broke in CI on Launchpad. This may not be related, but I've reverted at least temporarily.

==================================== 8/25 ====================================
test:         pistache / http_server_test
start time:   19:50:16
duration:     36.52s
result:       exit status 1
command:      MALLOC_PERTURB_=214 '/<<PKGBUILDDIR>>/obj-x86_64-linux-gnu/tests/run_http_server_test'
----------------------------------- stdout -----------------------------------
Running main() from ./googletest/src/gtest_main.cc
[==========] Running 13 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 13 tests from http_server_test
[ RUN      ] http_server_test.client_disconnection_on_timeout_from_single_threaded_server
[test] [7f86d34382c0] Trying to run server...
[server] [7f86d34382c0] Init Hello handler with 6 seconds delay
[test] [7f86d34382c0] Server address: localhost:38281
[client] [7f86d2c336c0] [0] Reject with reason:
[client] [7f86d34382c0] resolves: 0, rejects: 1, request timeout: 1 seconds, wait: 6 seconds
[       OK ] http_server_test.client_disconnection_on_timeout_from_single_threaded_server (6000 ms)
[ RUN      ] http_server_test.client_multiple_requests_disconnection_on_timeout_from_single_threaded_server
[test] [7f86d34382c0] Trying to run server...
[server] [7f86d34382c0] Init Hello handler with 6 seconds delay
[test] [7f86d34382c0] Server address: localhost:40167
[client] [7f86d34346c0] [0] Reject with reason:
[client] [7f86d34346c0] [1] Reject with reason:
[client] [7f86d34346c0] [2] Reject with reason:
[client] [7f86d34382c0] resolves: 0, rejects: 3, request timeout: 1 seconds, wait: 6 seconds
[       OK ] http_server_test.client_multiple_requests_disconnection_on_timeout_from_single_threaded_server (6000 ms)
[ RUN      ] http_server_test.multiple_client_with_requests_to_multithreaded_server
[test] [7f86d34382c0] Trying to run server...
[server] [7f86d34382c0] Init Hello handler with 0 seconds delay
[test] [7f86d34382c0] Server address: localhost:44363
[client] [7f86d0c2f6c0] [2] Response: OK, body: `Hello, World!`
[client] [7f86d0c2f6c0] [0] Response: OK, body: `Hello, World!`
[client] [7f86d0c2f6c0] [3] Response: OK, body: `Hello, World!`
[client] [7f86d14306c0] [1] Response: OK, body: `Hello, World!`
[client] [7f86d14306c0] [0] Response: OK, body: `Hello, World!`
[client] [7f86d14306c0] [3] Response: OK, body: `Hello, World!`
[client] [7f86d14306c0] [2] Response: OK, body: `Hello, World!`
[client] [7f86d24326c0] resolves: 4, rejects: 0, request timeout: 0 seconds, wait: 6 seconds
[client] [7f86d0c2f6c0] [4] Response: OK, body: `Hello, World!`
[client] [7f86d34346c0] resolves: 4, rejects: 0, request timeout: 0 seconds, wait: 6 seconds
../tests/http_server_test.cc:331: Failure
Expected equality of these values:
  res2
    Which is: 4
  SECOND_CLIENT_REQUEST_SIZE
    Which is: 5
[  FAILED  ] http_server_test.multiple_client_with_requests_to_multithreaded_server (6000 ms)

[Tachi107]

Il giorno mer 11 gen 2023 alle 15:41:36 -08:00:00, Kip @.***> ha scritto:

Sadly it broke in CI on Launchpad. This may not be related, but I've reverted at least temporarily.

Did reverting solve the issue? Maybe recent changes to their CI runners broke our CI, but we noticed only now because of a new commit to master.

[CTerasa-ep]

Not much experience with the Launchpad CI system on my side. From what I see on Launchpad:

• the ubuntu package name was not adjusted accordingly (seems there is no automation around this which might be OK)
• Except AMD64 for Kinetic, all series and architectures built and tested fine with c80a9a9. So, yes, might be an unstable CI test
• I do not see any later builds including the revert commit https://github.com/pistacheio/pistache/commit/9327df845dcbcd36785b7f88bae3c2e3a8f71bef

I have no idea how the Launchpad trigger works (perhaps manually, semi-manually ?)

[kiplingw]

I think it could be unrelated to your PR. I don't know how to "revert" the revert though.

[Tachi107]

I think it could be unrelated to your PR. I don't know how to "revert" the revert though.

git revert 817eb1ec4ec46a35fb9c166ce3f4ecd43416b241

;)

[kiplingw]

Done!