rust-cli / rust-cli/config-rs

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

Open
#532 1 comment 2 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

C-enhancement M-breaking-change
Dominant language
Rust
Stars
3.2k
Forks
265
Avg merge
2h 42m
Merged PRs (30d)
4

Description

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

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by tracing ConfigError::Message and prepend_key(), especially how serde:🇩🇪:Error failures are wrapped and how key and source context are currently handled. Compare the proposed optional key field with a separate error variant, then verify that the resulting error preserves the original message while identifying the configuration key and source without breaking existing Message users.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.