Fix IntoResponse for tuples overriding error response codes

[davidpdrsn]

Fixes #2287

I think 3rd option is the way to go. If we went with option 2 then things like

(StatusCode::SEE_OTHER, [("location", "...")], StatusCode::INTERNAL_SERVER_ERROR).into_response()

would have to skip the IntoResponseParts in the middle as well as StatusCode::SEE_OTHER, otherwise we'd get a 500 with a location header.

However we probably shouldn't skip the IntoResponseParts if the first element isn't a status code

(Extension(error_that_happened), StatusCode::INTERNAL_SERVER_ERROR).into_response()

That feels somewhat inconsistent and combined with the fact that it might break middleware that "catch errors" and use tuples to construct new responses I think adding a specific extension to signal "IntoResponse failed" is nicer.

So this PR does just that. It adds IntoResponseFailed which if present in response extensions makes other response transformations not run. Thus the 500 status code of

(StatusCode::CREATED, Json::<NotJsonCompatible>(...))

is maintained.

TODO

• Audit our IntoResponse and IntoResponseParts impls and add IntoResponseFailed where necessary.
• Make sure you can still write middleware that catch and transforms errors.
• Provide OverrideAllStatusCodes(StatusCode) newtype to opt-out and always set the status code.
• Changelog

[jplatte]

Looks decent, but I'm wondering if we should ship this in 0.7.x. It seems pretty likely that some people are relying on IntoResponseParts from a tuple running even if the final tuple element produces an error (e.g. headers used for distributed tracing).

Also, re. OverrideAllStatusCodes, could ForceStatusCode be a better name? It's a bit shorter.

[davidpdrsn]

Looks decent, but I'm wondering if we should ship this in 0.7.x. It seems pretty likely that some people are relying on IntoResponseParts from a tuple running even if the final tuple element produces an error (e.g. headers used for distributed tracing).

Yeah we probably should wait for 0.8.

Also, re. OverrideAllStatusCodes, could ForceStatusCode be a better name? It's a bit shorter.

👍

[jplatte]

Do you think we should ship 0.8 soon / start planning for what should be part of it? The async-trait removal is another big breaking change, plus potentially requiring Sync for inner services (or something along those lines, I have some other ideas in that area). Those changes seem large enough to warrant another breaking release, though there might be other things we should wait for (e.g. another matchit release since the routing syntax is changing).

[davidpdrsn]

Yeah I think we should ship sooner rather than later. We'll probably get some hype from being the first major framework to adopt AFIT 😅 although coordinating with matchit would be good.

[yanns]

@jplatte I've fixed the merge conflicts from main and fixed the compilation errors.
Do you think it makes sense to spend time on this? Is it the right direction?