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

ux: Lack of key and origin in ConfigError::Message leads to uncertain sources from serde error messages #532

Open
nmathewson opened this issue Feb 8, 2024 · 1 comment

Comments

@nmathewson
Copy link

Summary

If a configuration error passes through a serde::de::Error implementation, it is wrapped as a ConfigError::Message, and information about what caused the error is not preserved. This confuses the users, because the message does not tell them which option was set incorrectly.

Example

use serde::Deserialize;

#[derive(Deserialize, Clone, Debug)]
pub struct Settings {
    direct: Option<u8>,
    via_serde: Option<Item>,
}

#[derive(Deserialize, Clone, Debug)]
#[serde(try_from="String")]
pub struct Item(u8);

impl TryFrom<String> for Item {
    type Error = std::num::ParseIntError;
    fn try_from(s: String) -> Result<Item, Self::Error> {
	s.parse().map(Item)
    }
}

fn main() {
    let settings = config::Config::builder()
	.add_source(config::File::with_name("demo.toml"))
	.build()
	.unwrap();

    let settings = settings
	.try_deserialize::<Settings>();

    println!("{:?}", settings);
}

If the above code is run with the configuration direct = "foo", then we get a helpful error message:
invalid type: string "foo", expected an integer for key `direct` in demo.toml

But if the above code is run with the configuration via_serde = "foo", then we get the not-so-helpful message:
invalid digit found in string.
Note that it doesn't mention "via_serde" or "demo.toml".

It would be great if instead we go something like:
invalid digit found in string (for key `via_serde` in demo.toml

Possible solutions

My first thought here would be to adjust Message to include an optional key fields, so that prepend_key can set it.

If we don't want to break backward compatibility with code that uses the current Message variant, I would define a new MessageWithKey or InvalidValue error variant, and have prepend_key() convert Message into that variant instead.

Reference

For us this issue is https://gitlab.torproject.org/tpo/core/arti/-/issues/1267

@nmathewson
Copy link
Author

If you like, I am happy to work on any of the solutions here—just let me know which approach you would prefer.

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

No branches or pull requests

1 participant