Tracking: Improvements to DefaultBodyLimit

[davidpdrsn]

#1400 will remove ContentLengthLimit in favor of DefaultBodyLimit but there a few things missing on DefaultBodyLimit for it to have feature parity with ContentLengthLimit:

• We should make sure GET, HEAD, and OPTIONS requests without a body are still accepted. This can be done by checking the content-length and if that is not there apply http_body::Limited. Then since GET, HEAD, and OPTIONS doesn't have bodies we should never hit the limit. Will be nice if that works because it removes special casing of those methods. Edit: I think we should drop this. Not really clear where to enforce this. In Router, MethodRouter, handlers, or extractors? Just checking in Router isn't enough since MethodRouter can be used without Router, and we don't wanna duplicate the check everywhere. If its really important correctness-wise then hyper should check it.
• Currently DefaultBodyLimit has private APIs for actually enforcing the limit. This is used by Bytes::from_request. We should add a public API for that that Multipart, and other library developers, can use to get the same limiting the request of axum-core has. Could probably be a new method on RequestExt but since that isn't in axum-core it might require some exposing it there as well. Or maybe we move RequestExt to axum-core and re-export it from axum.
• Clearly document the difference between RequestBodyLimit and DefaultBodyLimit. The difference is that DefaultBodyLimit doesn't change the request body type in Router<ReqBody> (see Middleware that change the request body have poor usability #1110). Instead if applies the checks directly in the extractors themselves. Thus its more opt-in but easier to integrate, so its a trade-off which middleware one should use.

[jplatte]

I will take care of the second todo in the list.

[ttys3]

GET could have body

According moz

Sending body/payload in a GET request may cause some existing implementations to reject the request — while not prohibited by the specification, the semantics are undefined. It is better to just avoid sending payloads in GET requests.

[davidpdrsn]

may cause some existing implementations to reject the request

That means we're allowed to reject the request, which I think we should.

[jplatte]

We should make sure GET, HEAD, and OPTIONS requests without a body are still accepted. This can be done by checking the content-length and if that is not there apply http_body::Limited. Then since GET, HEAD, and OPTIONS doesn't have bodies we should never hit the limit. Will be nice if that works because it removes special casing of those methods.

Where is this special-casing happening?

[davidpdrsn]

@jplatte ContentLengthLimit had checks for that here https://github.com/tokio-rs/axum/blob/0.5.x/axum/src/extract/content_length_limit.rs#L54

[jplatte]

I'm afraid I still don't really understand what has to change, but maybe that's fine and you'll just do the PR to tick the first checkbox 😄

[davidpdrsn]

I think we should just drop the first item. I elaborated a bit in the top comment.

So when #1461 is merged I think we'll be in a good place. I'll just close this for now.