Delete Request not including request body

[prafsoni]

I am seeing that even though I create delete Request with body but the request received by server doesn't have the body. Is this not supported ? if so what is the rationale behind it?

[weissi]

CC @artemredkin

[weissi]

From RFC7231

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

That seems like we do actually comply to the spec because a payload on DELETE has no defined semantics. However, it might be better to pass the body on or to fail the request...

CC @Lukasa

[Lukasa]

"No defined semantics" doesn't mean "cannot have", just means that REST doesn't imply anything about what such a payload means. It's totally fine to have one, it's just up to the server and client to agree on what it means.

The actual bug here is in NIOHTTP1. I captured the TCP stream of such a request in Wireshark, and here's what we sent (I just picked a random body I had lying around in a sample project):

DELETE /delete HTTP/1.1
Content-Type: application/x-www-form-urlencoded; charset=utf-8
Host: httpbin.org
Connection: close

grant_type=urn:ietf:params:oauth:grant-type:jwt-bearer&assertion=ddd

This occurs because NIOHTTP1's sanitizeTransportHeaders is stripping the content-length header that async-http-client set. The switch statement in HTTPMethod.hasRequestBody is overbroad: we should be only forbidding bodies in TRACE. HEAD and DELETE may have bodies, they just have no defined meaning, so they should go in .unlikely. That will fix this issue we see here.

[weissi]

@Lukasa my reading of “no defined semantics” was that server & client can do whatever they want. Therefore I was assuming that not sending it is spec compliant (and so is sending it).
I agree that the spec definitely doesn’t say it must not have a body.

[Lukasa]

Not sending it is spec compliant but not useful, as there are definitely use-cases that require sending it.

[weissi]

Agreed, hence suggesting to either sending it (more useful) or failing it (less useful). I’ll file a NIOHTTP1 bug.

[prafsoni]

So it had to do purely with swift-nio. This is awesome I really appreciate the fix, as there are scenarios/apis where we require body in delete

Assuming this will be fixed in next patch release of swift-nio And using it in my project will fix it for I will have to wait for async-http-client Release?

[weissi]

@prafsoni once nio is released a swift package update will be enough. You could also add a dependency on the to-be-released NIO version into your own package to force the new version even without swift package update

[prafsoni]

@weissi Thanks. My project already has NIO dependency so it’s good to know I can get fix sooner. But I think I can wait till nio is released.

[weissi]

@prafsoni should be fixed with NIO 2.12.0. Mind confirming this works?

[prafsoni]

@weissi it's working :)

[weissi]

Thanks so much, closing