Skip to content

Show the errored path on JsonDataError - #1371

Merged
davidpdrsn merged 1 commit into
tokio-rs:mainfrom
dahlia:json-error-path
Sep 13, 2022
Merged

Show the errored path on JsonDataError#1371
davidpdrsn merged 1 commit into
tokio-rs:mainfrom
dahlia:json-error-path

Conversation

@dahlia

@dahlia dahlia commented Sep 12, 2022

Copy link
Copy Markdown
Contributor

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.

@davidpdrsn davidpdrsn left a comment

Copy link
Copy Markdown
Member

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.

Comment thread 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
Copy Markdown
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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Amended!

@jplatte

jplatte commented Sep 12, 2022

Copy link
Copy Markdown
Contributor

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

davidpdrsn commented Sep 12, 2022

Copy link
Copy Markdown
Member

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

jplatte commented Sep 12, 2022

Copy link
Copy Markdown
Contributor

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

@davidpdrsn

Copy link
Copy Markdown
Member

That's the receive-json bench?

Yes

@dahlia

dahlia commented Sep 13, 2022

Copy link
Copy Markdown
Contributor Author

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

@dahlia
dahlia requested a review from davidpdrsn September 13, 2022 13:38

@davidpdrsn davidpdrsn left a comment

Copy link
Copy Markdown
Member

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.

Comment thread axum/CHANGELOG.md Outdated
Comment thread axum/CHANGELOG.md Outdated
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

dahlia commented Sep 13, 2022

Copy link
Copy Markdown
Contributor Author

@davidpdrsn Fixed the changelogs!

@davidpdrsn davidpdrsn left a comment

Copy link
Copy Markdown
Member

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.

3 participants