-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Show the errored path on JsonDataError
#1371
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think it looks good.
Worth nothing that this changes the inner error of a JsonRejection
from serde_json::Error
to serde_path_to_error::Error<serde_json::Error>
. Thats not considered a breaking change but I think we should mention it in the changelog regardless.
axum/src/json.rs
Outdated
|
||
assert_eq!(res.status(), StatusCode::UNPROCESSABLE_ENTITY); | ||
let body_text = res.text().await; | ||
assert!(body_text.contains("b[0]"), "body: {:?}", body_text); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wanna change this to match on the whole text, instead of using contains
? Then its easier to see what the final result actually is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amended!
I wonder whether this has significant performance implications for parsing. Maybe it should only be part of axum-extra (for now) if that is the case? |
Good point. I ran the benchmark:
This branch is faster, which is odd so might just be noise and no loss of performance. |
That's the |
Yes |
16b6672
to
4d2efa1
Compare
@davidpdrsn I amended the commit. Could you take a look again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Just a couple of minor things.
Previously, it was difficult to find out the path in the deep JSON tree at which a deserialization error occurred. This patch makes an error message to contain the errored path. In order to find out the path, I added serde_path_to_error, a new optional dependency. Co-authored-by: Lee Dogeon <dev.moreal@gmail.com>
4d2efa1
to
8fe7cf5
Compare
@davidpdrsn Fixed the changelogs! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Motivation
Previously, it was difficult to find out the path in the deep JSON tree at which a deserialization error occurred.
Solution
This patch makes an error message to contain the errored path. In order to find out the path, I added serde_path_to_error, a new optional dependency.