Make () and None return a HTTP 204 response instead of 200

[jbethune]

Motivation

I'm proposing an implementation for #2363: The unit type () and None should generate a HTTP 204 response.

Solution

This PR does the following things:

• Change the implementation of IntoResponse for () to always return a HTTP 204 result
• Add an implementation of IntoResponse for Option<T: IntoResponse> so that HTTP 204 is returned when the Option is None.
• Add unit tests for the above cases in axum/src/response/mod.rs. (Please let me know if they should be somewhere else)
• Adjust some of the existing unit tests to expect HTTP 204 responses. Many of the existing unit tests define routes that produce an empty result and the body of the response is not part of those unit tests.

[jbethune]

There are also tests in the examples folder. This one fails (line 75):

axum/examples/handle-head-request/src/main.rs

Lines 66 to 81 in 801a78a

	#[tokio::test]
	async fn test_implicit_head() {
	let app = app();
	let response = app
	.oneshot(Request::head("/get-head").body(Body::empty()).unwrap())
	.await
	.unwrap();
	assert_eq!(response.status(), StatusCode::OK);
	assert_eq!(response.headers()["x-some-header"], "header from HEAD");
	let body = response.collect().await.unwrap().to_bytes();
	assert!(body.is_empty());
	}
	}

Hm. The return value from the route is

([("x-some-header", "header from HEAD")]).into_response();

Which now produces a HTTP 204 response. I'm not sure yet why that is. I'll keep looking.

edit: I understand now why this response is generated and this is actually intented behaviour. The implementation of the IntoResponse trait is here. I'll adjust the example in a moment.

[jplatte]

I think Option should be discussed separately. I seem to remember that I've wanted None to result in a 404 status at some point.

[jbethune]

I think Option should be discussed separately. I seem to remember that I've wanted None to result in a 404 status at some point.

That's a good point. I can easily remove the Option parts of this PR.

[davidpdrsn]

Yes I agree we should remove the impl for Option. We've previously discussed making None return 404 Not Found but decided against it.

About (), I'm wondering if it could be considered a breaking change. Seeing how many tests we needed to update I think it's likely users will have to update some of their tests as well. It also makes me think if changing it is even worth the churn.

[jbethune]

I removed the implentation for Option<T>.

About (), I'm wondering if it could be considered a breaking change. Seeing how many tests we needed to update I think it's likely users will have to update some of their tests as well. It also makes me think if changing it is even worth the churn.

I think it is fair to say that it is a breaking change. But it is also a pre-1.0 breaking change and this gives you the opportunity to discuss how axum should behave at version 1.0. The way I see it: Using a HTTP 204 header will make it easier for users to write tests that check that there is no response body for a given route. Current tests might break, but they will become more reliable after this breaking change.

If you think that the current behaviour is fine then feel free to close this PR.

[jbethune]

About (), I'm wondering if it could be considered a breaking change. Seeing how many tests we needed to update I think it's likely users will have to update some of their tests as well. It also makes me think if changing it is even worth the churn.

To be fair, the tests that this PR updates are testing the core behaviour of axum. And the test behaviour is typically: I make a request and I expect a valid response. And most tests only care about the HTTP headers and not about the body. And it's very convenient to write a test that just returns () by using a semicolon or {}. This is why there are so many tests that return () in the axum testsuite.

But projects that use axum as a library will have a lot more tests that expect an actual body in the response. The routes that return a () are called for their side-effects. And it is a good idea to be explicit about this by returning a 204 response. This also makes misuse and breakage in an API more visible to clients.

I've taken my time to update all of the tests in this PR. It was a lot of tests and I looked at each one individually to make sure I'm not overlooking something. But most tests in this project are fairly easy to read through and understand. It was really not that much work to fix them. It will probably be less of an issue for downstream projects. Or do you have a particular project in mind that would be heavily affected by this change?

[davidpdrsn]

Or do you have a particular project in mind that would be heavily affected by this change?

I'm not worried that updating the tests will be hard. I think you're right that it'll be easy. My question is more about whether it's worth the churn and whether this change really matters. I'm still not sure about that and not sure how to decide 🤔

[davidpdrsn]

I'm still not sure I think this is a great idea so I think I'll be a bit conservative and close this for now. Thanks for the suggestion though!

[oxalica]

I also encountered this.

explicit about this by returning a 204 response.

If we do not want to change the () implementation, could we have a pub struct NoContent; under axum::response for this purpose? It's more expressive to have async fn delete_user() -> Result<NoContent, MyJsonError> than -> Result<StatusCode/*what code? success or error?*/, MyJsonError>

[jplatte]

I like that idea! Want to send a PR?