CONNECT method should not have request body

[norio-nomura]

CONNECT method should not have request body.

Motivation:

rfc7231 said:

A payload within a CONNECT request message has no defined semantics;
sending a payload body on a CONNECT request might cause some existing
implementations to reject the request.

Modifications:

Change HTTPMethod.CONNECT.hasRequestBody to returns .no.

Result:

HTTPRequestEncoder does not generate chunked body for CONNECT method.

[swift-nio-bot]

Can one of the admins verify this patch?

[swift-nio-bot]

Can one of the admins verify this patch?

[swift-nio-bot]

Can one of the admins verify this patch?

[Lukasa]

Thanks for this patch!

Sadly I wonder if perhaps we may need to fix this implementation a bit more. When RFC 7231 says “a payload has no defined semantics”, that is not the same as saying it may not be there. GET, for example, uses the same wording.

So I think this enum is wrong for a number of methods, and many of them should go into .unlikely instead of .no.

[Lukasa]

It would also be really good to add a regression test for the CONNECT change to validate that it continues to work in future.

[norio-nomura]

So I think this enum is wrong for a number of methods, and many of them should go into .unlikely instead of .no.

Changed to .no.

It would also be really good to add a regression test for the CONNECT change to validate that it continues to work in future.

Is HTTPRequestEncoderTests.testCONNECT() not enough?
If hasRequestBody returns .yes, the request contains transfer-encoding: chunked\r\n\r\n0 without writing HTTPClientRequestPart.body to HTTPRequestEncoder.

[Lukasa]

Cool, this looks good to me! Sorry for the note about testing, that’s what happens when I review on my phone.

[Lukasa]

@swift-nio-bot test this please

[weissi]

thank you!

[weissi]

@swift-nio-bot test this please

[norio-nomura]

Thanks! 🙏

[normanmaurer]

late LGTM