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

Provide more data in Path deserialization error #574

Merged
merged 14 commits into from
Dec 2, 2021
Merged

Conversation

davidpdrsn
Copy link
Member

@davidpdrsn davidpdrsn commented Nov 28, 2021

Fixes #559

This changes the error returned by Path if deserialization fails, to provide more data about the exact error. This allows users to convert into their own errors. It also makes the default error messages more precise.

Also added a new example showing how to customize the error https://github.com/tokio-rs/axum/blob/path-error/examples/customize-path-rejection/src/main.rs. Its the exact same approach as this but can't hurt having an example for it.

TODO

  • Write more tests
  • Add missing changelog item

@davidpdrsn davidpdrsn added the C-enhancement Category: A PR with an enhancement label Nov 28, 2021
@davidpdrsn davidpdrsn added this to the 0.4 milestone Nov 28, 2021
@davidpdrsn
Copy link
Member Author

@pnehrer what do you think about this solution?

axum/CHANGELOG.md Outdated Show resolved Hide resolved
axum/CHANGELOG.md Outdated Show resolved Hide resolved
axum/src/extract/path/mod.rs Show resolved Hide resolved
axum/src/extract/path/mod.rs Show resolved Hide resolved
@pnehrer
Copy link

pnehrer commented Nov 29, 2021

This looks great! Thank you so much for doing this 👍

@jplatte
Copy link
Member

jplatte commented Nov 29, 2021

This is a little too big for me to fully review. Is there such a thing as a rustdoc diff? 😄

From my glancing over the changes before I found the internal struct WrongNumberOfParameters<G> API a little bit weird (would probably have used a struct literal instead of this kind of type-checked builder pattern myself) but since it's internal I don't think it's sth. to worry about.

The general idea of this seems good, the only other thing I can say is I think you missed documenting the rejection type rename in the changelog.

@davidpdrsn
Copy link
Member Author

I've been thinking that we should have more tests for this since generating the right errors is an important part of the API. So I'm gonna write some tests for the stuff in this PR. That might also make things slightly easier to review because you can more easily see the behavior of things without having to internalize how all the code works.

From my glancing over the changes before I found the internal struct WrongNumberOfParameters<G> API a little bit weird (would probably have used a struct literal instead of this kind of type-checked builder pattern myself) but since it's internal I don't think it's sth. to worry about.

Hehe yeah for sure its over engineered but also kinda nice I think. But yeah as you say its a small internal API that we can always change later if we want.

The general idea of this seems good, the only other thing I can say is I think you missed documenting the rejection type rename in the changelog.

Good catch! I'll fix that later.

@davidpdrsn
Copy link
Member Author

@jplatte I've added some tests now and found a few things that could be improved.

@davidpdrsn davidpdrsn merged commit 3ec680c into main Dec 2, 2021
@davidpdrsn davidpdrsn deleted the path-error branch December 2, 2021 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: A PR with an enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose PathDeserializerError in InvalidPathParam with additional data points
3 participants