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

api: limit HTTP_METHOD_MAP to HTTP methods #103

Closed
wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Apr 17, 2021

RTSP methods shouldn't mix up with HTTP methods in the method map since
they are supported only with the RTSP protocol. Node.js uses
HTTP_METHOD_MAP to generate http.METHODS array and this breaks tests
in body-parser because it assumes that all of these methods are for
HTTP requests.

cc @nodejs/http

RTSP methods shouldn't mix up with HTTP methods in the method map since
they are supported only with the RTSP protocol. Node.js uses
`HTTP_METHOD_MAP` to generate `http.METHODS` array and this breaks tests
in `body-parser` because it assumes that all of these methods are for
HTTP requests.
@indutny
Copy link
Member Author

indutny commented Apr 17, 2021

Gosh, I realized that I didn't include the diff of the generated header file. Sorry!

diff --git a/tmp/llhttp-old.h b/tmp/llhttp-new.h
index 8f9590d..6f40b67 100644
--- a/tmp/llhttp-old.h
+++ b/tmp/llhttp-new.h
@@ -232,6 +232,12 @@ typedef enum llhttp_method llhttp_method_t;
   XX(32, UNLINK, UNLINK) \
   XX(33, SOURCE, SOURCE) \
   XX(34, PRI, PRI) \
+
+
+#define RTSP_METHOD_MAP(XX) \
+  XX(1, GET, GET) \
+  XX(3, POST, POST) \
+  XX(6, OPTIONS, OPTIONS) \
   XX(35, DESCRIBE, DESCRIBE) \
   XX(36, ANNOUNCE, ANNOUNCE) \
   XX(37, SETUP, SETUP) \
@@ -245,6 +251,54 @@ typedef enum llhttp_method llhttp_method_t;
   XX(45, FLUSH, FLUSH) \
 
 
+#define HTTP_ALL_METHOD_MAP(XX) \
+  XX(0, DELETE, DELETE) \
+  XX(1, GET, GET) \
+  XX(2, HEAD, HEAD) \
+  XX(3, POST, POST) \
+  XX(4, PUT, PUT) \
+  XX(5, CONNECT, CONNECT) \
+  XX(6, OPTIONS, OPTIONS) \
+  XX(7, TRACE, TRACE) \
+  XX(8, COPY, COPY) \
+  XX(9, LOCK, LOCK) \
+  XX(10, MKCOL, MKCOL) \
+  XX(11, MOVE, MOVE) \
+  XX(12, PROPFIND, PROPFIND) \
+  XX(13, PROPPATCH, PROPPATCH) \
+  XX(14, SEARCH, SEARCH) \
+  XX(15, UNLOCK, UNLOCK) \
+  XX(16, BIND, BIND) \
+  XX(17, REBIND, REBIND) \
+  XX(18, UNBIND, UNBIND) \
+  XX(19, ACL, ACL) \
+  XX(20, REPORT, REPORT) \
+  XX(21, MKACTIVITY, MKACTIVITY) \
+  XX(22, CHECKOUT, CHECKOUT) \
+  XX(23, MERGE, MERGE) \
+  XX(24, MSEARCH, M-SEARCH) \
+  XX(25, NOTIFY, NOTIFY) \
+  XX(26, SUBSCRIBE, SUBSCRIBE) \
+  XX(27, UNSUBSCRIBE, UNSUBSCRIBE) \
+  XX(28, PATCH, PATCH) \
+  XX(29, PURGE, PURGE) \
+  XX(30, MKCALENDAR, MKCALENDAR) \
+  XX(31, LINK, LINK) \
+  XX(32, UNLINK, UNLINK) \
+  XX(33, SOURCE, SOURCE) \
+  XX(34, PRI, PRI) \
+  XX(35, DESCRIBE, DESCRIBE) \
+  XX(36, ANNOUNCE, ANNOUNCE) \
+  XX(37, SETUP, SETUP) \
+  XX(38, PLAY, PLAY) \
+  XX(39, PAUSE, PAUSE) \
+  XX(40, TEARDOWN, TEARDOWN) \
+  XX(41, GET_PARAMETER, GET_PARAMETER) \
+  XX(42, SET_PARAMETER, SET_PARAMETER) \
+  XX(43, REDIRECT, REDIRECT) \
+  XX(44, RECORD, RECORD) \
+  XX(45, FLUSH, FLUSH) \
+
 
 #ifdef __cplusplus
 }  /* extern "C" */

@indutny
Copy link
Member Author

indutny commented Apr 17, 2021

So the point is that HTTP_METHOD_MAP is just the old good one without any RTSP stuff in it, and HTTP_ALL_METHOD_MAP is used for translating integer http methods (enum values) to strings.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

indutny added a commit that referenced this pull request Apr 17, 2021
RTSP methods shouldn't mix up with HTTP methods in the method map since
they are supported only with the RTSP protocol. Node.js uses
`HTTP_METHOD_MAP` to generate `http.METHODS` array and this breaks tests
in `body-parser` because it assumes that all of these methods are for
HTTP requests.

PR-URL: #103
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniele Belardi <dwon.dnl@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
@indutny
Copy link
Member Author

indutny commented Apr 17, 2021

Landed in 25c6ea4, thank you!

@indutny indutny closed this Apr 17, 2021
@indutny indutny deleted the feature/separate-http-and-rtsp-methods branch April 17, 2021 17:16
@BethGriggs
Copy link
Member

@indutny, related query - do we also need to treat the PRI method as a special case? Latest CITGM showed body-parser still failing (below), but I am not sure if the PRI method is something that we can reasonably expect test logic to need to be adjusted for?

 1) bodyParser()
        http methods
          should support PRI requests:
      Error: expected 201 "Created", got 400 "Bad Request"

@indutny
Copy link
Member Author

indutny commented Apr 20, 2021

@BethGriggs ouuuch, didn't think about it. Sorry! Here's the PR to fix it: #105 . I'm going to make it a patch release because it is clearly a bug.

@pallas
Copy link
Contributor

pallas commented Apr 20, 2021

How does a caller tell that PRI is the reason the parser stopped? Is it just due to the upgrade return code?

@indutny
Copy link
Member Author

indutny commented Apr 20, 2021

@pallas yep, this is what it is. The error code was introduced solely for this.

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

Successfully merging this pull request may close these issues.

6 participants