Finished listing status codes which SHOULD NOT contain body

[pksunkara]

• Empty message body in request will give a warning if Content-Length or Transfer-Encoding header in request is set.
• Non-empty message body in response will give a warning if
  • Request method is HEAD
  • Response status code is 1xx
  • Request method is CONNECT and response status code is 2xx

[Almad]

👍

[pksunkara]

Must have used -a mistakenly. Do you want me to remove them? I have some more code coming anyway.

[zdne]

@pksunkara You plan to add on this PR or open a new one for the rest?

I would prefer to split it in two thus remove the part related to status codes. It would be easier to review it.

[pksunkara]

Added the test. I want to open a new one for the rest. Thanks.

[pksunkara]

@zdne I was planning on opening a new pull request for the second part. But since this isn't merged yet, here they are.

[zdne]

@pksunkara

Well done Pavan! I must say I am impressed. Let's throw out the old Content-Type - based checks and possibly address the edged CONNECT case and we are good with HTTP1.1!

[zdne]

Also (some non-programing) work - It would be good at the end to summarize the change and explain the new behavior for users. Either on this PR or on #77

[pksunkara]

Shall I clean this up now?

[zdne]

@pksunkara absolutely! Please proceed when you can. Also feel free to squash some commits where it makes sense.

[pksunkara]

Squashed related commits and cleaned up the code. Added the case for 2xx CONNECT too.

[zdne]

@pksunkara

Awesome! Will check it tomorrow. Thanks!

[zdne]

Careful CONNECT outside of 2xx range (and 1xx) can include a response!

[zdne]

Also best to define used methods similar to headers e.g.

struct HTTPMethods {
...
}

...
const std::string HTTPMethods::HEAD = "HEAD";

or something like this...

[pksunkara]

I am checking for that when generating the warning.

[zdne]

Understood but, lets refrain from retyping constants by defining them and reusing the definitions (rather then typing the same strings over and over).

[pksunkara]

Shall I remove the Content-Type stuff and fix the HTTPMethod strings?

[zdne]

What about something along these lines:

    struct HTTPPayloadTraits {
        bool allowBody;

        HTTPPayloadTraits (): allowBody(true) {}
    };

    struct XStatusCodeTraits : HTTPPayloadTraits {

        XStatusCodeTraits() {}
        XStatusCodeTraits(const HTTPStatusCode& code)
        {
            if (code == 204 || code == 304 || code / 100 == 1) {
                allowBody = false;
            }
        }
    };

    typedef std::string XHTTPMethod;
    struct XMethodTraits : XStatusCodeTraits {

        XMethodTraits() {}
        XMethodTraits(const XHTTPMethod& method) : XStatusCodeTraits()
        {
            // TODO:
        }

        XMethodTraits(const XHTTPMethod& method, const HTTPStatusCode& code) : XStatusCodeTraits(code)
        {
            // TODO:
        }
    };

[zdne]

Ad my previous comment:

Then MethodTraits would be base class for ResponseTraits and RequestTraits ..

Note the X-names are just temporary.

But maybe I am just over-engineering things?

[zdne]

Either way, I will leave the final decision up to you.

[pksunkara]

In my opinion, this looks too much. If we are going to have a function which takes both method and code as arguments, it's enough to have one thing called ActionTraits or something.

I fixed the method name stuff you asked me for.

[pksunkara]

I would like to remove the Content-Type and let's merge this pull request once you review it. We will discuss on the refactoring of Traits in another issue.

[zdne]

Agree, please proceed when you can.

[pksunkara]

Fixed the content-type stuff.

[zdne]

Minor: Lets at least add comment that for Connect the allowBody is false only for 1xx - 2xx range so we (or someone else) won't forget it when refactoring traits (as this probably won't happen anytime soon).

[pksunkara]

Done. 😄