Add `try_deserialize` deserialization path error tracking #632

allevo · 2025-01-09T12:01:29Z

Hi!
I'm using config-rs, but I'm having some difficulties finding where my JSON file lacks fields.
I have a nested configuration struct like the one below:

#[derive(Debug, Deserialize)]
struct InnerSettings {
    #[allow(dead_code)]
    value: u32,
    #[allow(dead_code)]
    value2: u32,
}

#[derive(Debug, Deserialize)]
struct Settings {
    #[allow(dead_code)]
    inner: InnerSettings,
}

NB: my use case involves more complex structures

So, if you try to deserialize a wrong JSON, the error will contains the JSON path:

let c = Config::builder()
    .add_source(File::from_str(
        r#"
{
"inner": { "value": 42 }
}
    "#,
        FileFormat::Json,
    ))
    .build()
    .unwrap();

let without_path = c.clone().try_deserialize::<Settings>();
assert_data_eq!(
    without_path.unwrap_err().to_string(),
    str!["inner: missing field `value2`"] // prepended with JSON path
);

This PR:

adds serde_path_to_error
add a JSON path if not empty in front of the error.
adds a test
add method documentation

tests/testsuite/file_json_error_path.rs

Cargo.toml

tests/testsuite/file_json_error_path.rs

epage · 2025-01-09T15:14:41Z

tests/testsuite/file_json_error_path.rs

+        Err(err) => {
+            let path = err.path().to_string();
+            assert_eq!(path, "inner.value");
+
+            let s = format!("{err}");
+            assert_eq!(
+                s,
+                "inner.value: invalid type: unit value, expected a string for key `inner.value`"
+            );
+        }


The contributing guide asks for PRs to made so that a test gets added in a commit before the feature work. It should be passing. The feature would would then appropriately update the test. The goal is to be able to show the behavior change that the feature allows.

In most cases, only the assertion changes. So long as this relies on API changes, the initial test commit would then be using the existing APIs and the feature commit would switch it to the new APIs.

Ops. Sorry, my bad.
Anyway, how do you suggest to proceed?
I can create a first commit a test that uses try_deserialize where the assertion fails (it miss the path). I can put a #[ignore] as well.
After, I can commit the implementation, changing also the test to point to try_deserialize_with_error_path.

Let me know, and sorry again.

Please see https://github.com/rust-cli/config-rs/blob/main/CONTRIBUTING.md#pull-requests

We include clap-rs/clap#5520 as an example (I need to find a better one ...)

The test in the first commit should pass and should not be using #[ignore]. The intent behind this is to show how the behavior changed with the feature work and #[ignore] masks that.

I think I write the commit in the right way. Let me know.

epage · 2025-01-09T15:55:11Z

And feel free to squash commits that are just for review feedback

epage · 2025-01-09T16:40:25Z

tests/testsuite/errors.rs

@@ -349,6 +349,6 @@ fn test_json_error_with_path() {
    let without_path = c.clone().try_deserialize::<Settings>();
    assert_data_eq!(
        without_path.unwrap_err().to_string(),
-        str!["missing field `value2`"]
+        str!["inner: missing field `value2`"]


Now the question is how to format the message.

I feel like this doesn't make clear what inner is.

One idea: "failed reading field inner: missing field value2"

In looking at fn get, I saw that we have some level of path tracking. In those cases, we append the path with for path foo.bar. Should we be consistent one way or the other?

Uhm... probably the current implementation and the implementation of this PR overlap a little bit providing, in some cases, a double information of the error path.

I think we could remove the current implementation of path tracking in favor the one proposed in this PR.
In fact, the current implementation misses some cases. Instead the PR one is more accurate.

I let you decide how to structure the error message (I don't care too much 😅)

Let me know

I tested locally other cases:

#[derive(Debug, Deserialize)] struct InnerSettings { #[allow(dead_code)] value: u32, #[allow(dead_code)] value2: u32, } #[derive(Debug, Deserialize)] struct Settings { #[allow(dead_code)] inner: InnerSettings, } let c = Config::builder() .add_source(File::from_str( r#" { "inner": { "value": 42 } } "#, FileFormat::Json5, )) .build() .unwrap(); let with_path = c.clone().try_deserialize::<Settings>(); assert_data_eq!( with_path.unwrap_err().to_string(), str!["missing field `value2` in path `inner`"] ); let c = Config::builder() .add_source(File::from_str( r#" { "inner": { "value": "abc" } } "#, FileFormat::Json5, )) .build() .unwrap(); let with_path = c.clone().try_deserialize::<Settings>(); assert_data_eq!( with_path.unwrap_err().to_string(), str!["invalid type: string \"abc\", expected an integer for key `inner.value`"] );

So:

if the key is missing: "missing field value2 in path inner"

if the value isn't "castable": "invalid type: string "abc", expected an integer for key inner.value"

it seems to be consistent.

Let me know.

That we already have the path for the other error is suspicious and implies we already have path tracking we can take advantage of.

This is why I wrote this comment #632 (comment).
Currently, the missing field is not supported because the implementation only track the cast issues and not the deserialization error (which is hard to implement without external library).

Sorry, I misunderstood what you were referring to. We have separate path tracking in get on top of whatever else happens and I thought you were referring to that.

I have a branch with the path tracking without an external library. I'm trying to add origin tracking as well but might give up for now.

epage · 2025-01-09T16:53:37Z

src/config.rs

@@ -142,7 +143,20 @@ impl Config {

    /// Attempt to deserialize the entire configuration into the requested type.
    pub fn try_deserialize<'de, T: Deserialize<'de>>(self) -> Result<T> {
-        T::deserialize(self)
+        let mut track = Track::new();
+        match T::deserialize(Deserializer::new(self, &mut track)) {


Should we also make this change to fn get?

(sorry, just now noticed that this was a whole-document case separate from the path look up case)

coveralls · 2025-01-09T18:39:34Z

Pull Request Test Coverage Report for Build 12705981021

Details

9 of 10 (90.0%) changed or added relevant lines in 1 file are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.1%) to 64.927%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/config.rs	9	10	90.0%

Files with Coverage Reduction	New Missed Lines	%
src/format.rs	1	40.0%

Totals
Change from base Build 12695788504:	0.1%
Covered Lines:	933
Relevant Lines:	1437

💛 - Coveralls

tests/testsuite/errors.rs

epage · 2025-01-10T14:56:42Z

tests/testsuite/errors.rs

@@ -341,14 +341,14 @@ fn test_json_error_with_path() {
    "inner": { "value": 42 }
 }
        "#,
-            FileFormat::Json,
+            FileFormat::Json5,


This commit changes it back to json5. Merge conflict problem?