implement HttpResponseStatusCode toString & toInt #1180
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1180 +/- ##
==========================================
- Coverage 82.11% 82.10% -0.02%
==========================================
Files 157 157
Lines 20098 20147 +49
Branches 7575 7516 -59
==========================================
+ Hits 16504 16541 +37
+ Misses 3092 3074 -18
- Partials 502 532 +30
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
@seladb please review, I didn't fix the CI, would like to get your agreement on the change first |
|
When I was reviewing your PR, I noticed some of the HTTP status codes are missing (not in your PR, but in library). Probably the list is a bit outdated. For example, 103 Early Hints (RFC 8297 from 2017), 421 Misredirected Request and maybe more. Not checked all of that but you can find the latest list here. I think the list of all status codes should also be updated. |
|
@egecetin already updated the code list |
Packet++/header/HttpLayer.h
Outdated
| /** 302 Found */ | ||
| Http302Found = 302, |
There was a problem hiding this comment.
previously 302 could also mean Moved Temporarily, do we want to keep supporting it?
There was a problem hiding this comment.
@seladb answer about all comments for adding and removing code in the list:
as @egecetin mentioned, those values are outdated, according to https://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml, so I just updated them according to the latest spec (2022)
The question is should we support the old code or not? IMO, we should avoid supporting old specs (but maybe it's okay to keep the current codes), since it takes more effort to maintain and increases the complexity of the code base.
We probably can make this version a breaking change version?
There was a problem hiding this comment.
I think the unofficial ones should also be supported too as @seladb mentioned because some of them widely used. @tigercosmos so I think there is no need removing old codes only adding missing ones and apply changes enough. But the only problem what will we do when there is a conflict for a status code message (if there are some codes have this situation). Since they are unofficial, different apps can use same code for different messages. What do you think about this @seladb? Should all messages be displayed somehow (maybe comma separated etc), display only most popular one or display a generic message like "various messages" or ...?
There was a problem hiding this comment.
The essence of PcapPlusPlus is parsing packets. People use it to parse different kinds of packets, some of which are very old and contain outdated data, such as old HTTP messages. That's why I think we should support outdated/unofficial HTTP data.
It's true that applications can use HTTP status codes for any purpose, even if that purpose doesn't meet the official specifications. However, for most status codes there is only one usage (classic examples are 200 OK, 404 Not Found, 401 Unauthorized, 500 Internal Server Error). If there are cases where there is more than one usage, we can probably say " or other". Please let me know what you think
There was a problem hiding this comment.
I agree we should support any status code.
actually, even for unknown status codes (1xx-5xx, exclusive other else, e.g. 999), I think we should also support it, e.g. 599 My Customized, because users may want to analyze their internal usage packets.
the current behavior for parsing a packet limits the possibility of the user's customized status code.
how about this, we can add a new std::string m_StatusCodeCustomizedDefination in HttpResponseFirstLine, to allow the user's customized code.
- if the code is
200, and it's already in ourHttpResponseFirstLine, and the definition in the packet is the same as our definition, som_StatusCodeCustomizedDefinationis empty - if the code is
599, and the user defines it'sMy Customized, thenm_StatusCodeCustomizedDefinationisMy Customized. - if the code is
302, it'sFoundin the spec, but the user defines it asHello World, thenm_StatusCodeCustomizedDefinationisHello World.
later on, whenever we need to print or return the definition of the status code, we first see if m_StatusCodeCustomizedDefination is defined; otherwise, we return the default definition.
There was a problem hiding this comment.
HttpResponseFirstLine is probably not a great place to put this definition because it has a private constructor and cannot be modified by the user. Besides, it seems that customized codes should be part of HttpResponseStatusCode.
In order to keep this PR simple and focused, I'd suggest supporting the existing status codes and maybe add customized code later. Please let me know what you think
There was a problem hiding this comment.
sure, let's do it in another PR
|
@seladb please take a look |
…lusPlus into 20230812HttpStatusCode
|
@tigercosmos is there anything we can do with this? #1180 (comment) |
|
@tigercosmos thank you so much for your contribution to this project, much appreciated!! 🙏 🙏 If you're interested maybe you can work on: #1180 (comment) |
|
@seladb thank you for taking time reviewing! :) |
fix #1157
HttpResponseStatusCodeenum class, and providedtoString&toIntmethods