Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

debug_handler: Check IntoResponseParts / IntoResponse for return tuple element types #2173

Open
jplatte opened this issue Aug 14, 2023 · 10 comments
Labels
A-axum-macros C-enhancement Category: A PR with an enhancement E-medium Call for participation: Experience needed to fix: Medium / intermediate

Comments

@jplatte
Copy link
Member

jplatte commented Aug 14, 2023

In Jon Gjengset's Decrusting the axum crate video, he tried using #[debug_handler] to get a more useful error message for (StatusCode, Json<_>, AppendHeaders<_>) not implementing IntoResponse as part of a handler, but that didn't work. I think we can make it work.

@jplatte jplatte added C-enhancement Category: A PR with an enhancement E-medium Call for participation: Experience needed to fix: Medium / intermediate A-axum-macros labels Aug 14, 2023
@SpeedReach
Copy link
Contributor

Is anyone working on this? I'd like to implement this if not.

@davidpdrsn
Copy link
Member

I don’t think so. You’re welcome to submit a PR!

@SpeedReach
Copy link
Contributor

SpeedReach commented Sep 1, 2023

Thanks for the opportunity!
I improved the error message from

error[E0277]: the trait bound `(StatusCode, Json<&str>, AppendHeaders<[(HeaderName, &str); 1]>): IntoResponse` is not satisfied
 --> tests/debug_handler/fail/wrong_return_tuple_type.rs:5:23
  |
5 |   async fn handler() -> (
  |  _______________________^
6 | |     axum::http::StatusCode,
7 | |     axum::Json<&'static str>,
8 | |     axum::response::AppendHeaders<[( axum::http::HeaderName,&'static str); 1]>,
9 | | ) {
  | |_^ the trait `IntoResponse` is not implemented for `(StatusCode, Json<&str>, AppendHeaders<[(HeaderName, &str); 1]>)`

to

error[E0277]: the trait bound `Json<&'static str>: IntoResponseParts` is not satisfied
 --> tests/debug_handler/fail/wrong_return_tuple_type.rs:7:5
  |
7 |     axum::Json<&'static str>,
  |     ^^^^^^^^^^^^^^^^^^^^^^^^ the trait `IntoResponseParts` is not implemented for `Json<&'static str>`

It checks the
first element => StatusCode || Parts || IntoResponseParts
last element => IntoResponse
other elements => IntoResponseParts
and also the max numbers of IntoResponseParts(16)

I'll start to add tests if you guys think this is a good approach.

@jplatte
Copy link
Member Author

jplatte commented Sep 1, 2023

That sounds good, but I think for the extractors we also have special checks for Json and other body extractors of known names where we then print a custom error message if they're not last. Those kinds of messages would be helpful here as well, I think.

@davidpdrsn
Copy link
Member

Yes I agree that would be good but not essential and a PR without is also cool. We can always add that later.

SpeedReach added a commit to SpeedReach/axum that referenced this issue Sep 1, 2023
@SpeedReach
Copy link
Contributor

SpeedReach commented Sep 1, 2023

I was having the same thought when writing it, but I'm not sure what types should always be placed at last.So I added a //todo
where the logic should be written.

@SpeedReach
Copy link
Contributor

Added special checks for known types that implements IntoResponse but not IntoResponseParts, but it only checks for Json and String now , the list can be extended.
Also opened a pr , let me know if there's change needed!

@SpeedReach
Copy link
Contributor

Sorry this is my first time doing a pull request. Should I request a specific reviewer, or just leave it like that?

@davidpdrsn
Copy link
Member

Nope you don’t need to do anything. I’ll take a look this week :)

@SpeedReach
Copy link
Contributor

Got it, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-axum-macros C-enhancement Category: A PR with an enhancement E-medium Call for participation: Experience needed to fix: Medium / intermediate
Projects
None yet
Development

No branches or pull requests

3 participants