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

Show the errored path on JsonDataError #1371

Merged
merged 1 commit into from Sep 13, 2022

Conversation

dahlia
Copy link
Contributor

@dahlia dahlia commented Sep 12, 2022

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.

Copy link
Member

@davidpdrsn davidpdrsn left a 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);
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amended!

@jplatte
Copy link
Member

jplatte commented Sep 12, 2022

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?

@davidpdrsn
Copy link
Member

davidpdrsn commented Sep 12, 2022

Good point. I ran the benchmark:

main

  Beginning round 1...
  Benchmarking 10 connections @ http://0.0.0.0:49942 for 10 second(s)
    Latencies:
      Avg      Stdev    Min      Max
      0.07ms   0.02ms   0.02ms   2.63ms
    Requests:
      Total: 1490865 Req/Sec: 149077.94
    Transfer:
      Total: 106.63 MB Transfer Rate: 10.66 MB/Sec

json-error-path branch

  Benchmarking 10 connections @ http://0.0.0.0:49918 for 10 second(s)
    Latencies:
      Avg      Stdev    Min      Max
      0.07ms   0.02ms   0.02ms   0.67ms
    Requests:
      Total: 1517780 Req/Sec: 151771.58
    Transfer:
      Total: 108.56 MB Transfer Rate: 10.86 MB/Sec

This branch is faster, which is odd so might just be noise and no loss of performance.

@jplatte
Copy link
Member

jplatte commented Sep 12, 2022

That's the receive-json bench? Seems good if there's no (measureable) slowdown.

@davidpdrsn
Copy link
Member

That's the receive-json bench?

Yes

@dahlia
Copy link
Contributor Author

dahlia commented Sep 13, 2022

@davidpdrsn I amended the commit. Could you take a look again?

Copy link
Member

@davidpdrsn davidpdrsn left a 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.

axum/CHANGELOG.md Outdated Show resolved Hide resolved
axum/CHANGELOG.md Outdated Show resolved Hide resolved
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>
@dahlia
Copy link
Contributor Author

dahlia commented Sep 13, 2022

@davidpdrsn Fixed the changelogs!

Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@davidpdrsn davidpdrsn merged commit 7476dd0 into tokio-rs:main Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants