Non-deterministic results for overlapping overrides

[neilisaac]

Source::collect returns a config::Map which is a HashMap. HashMap traversals are unordered, so the processing order of a Source is non-deterministic.

This can be an issue if multiple values in the Source affect the same data:

    #[derive(Debug, Serialize, Deserialize)]
    struct TestConfig {
        nested: std::ops::Range<i32>,
    }

impl Source for ExampleSource {
    fn collect(&self) -> Result<config::Map<String, config::Value>, config::ConfigError> {
        Ok([
            ("nested".to_string(), config::Value::new(None, config::ValueKind::String("{\"start\": 1, \"end\": 3}".to_string())),
            ("nested.end".to_string(), config::Value::new(None, config::ValueKind::String("5".to_string()))),
        ].into_iter().collect())
    }
}

This can result in deserializing 1..3 or 1..5 randomly.

[matthiasbeyer]

I agree that this can be an issue.

From what I see the solution for this would be to use an BTreeMap or something similar in the implementation, right?

[neilisaac]

Yes, although for compatibility you may want to keep the trait as is, and sort afterwards. Alternatively a breaking api change to BTreeMap would mean that the implementations need to handle determinism (ex. by sorting env vars for env var source).

[matthiasbeyer]

So config-rs is in 0.x.y-version, so a breaking API wouldn't be that much of a problem, at least semver-wise.

Could you maybe provide a PR with a (failing, of course) testcase for above example? I think we can then go from there... changing the impl from HashMap to BTreeMap should be a rather trivial patch I think...

[haydenflinner]

Testing something non-deterministic sounds like a recipe for a flaky test, maybe unnecessary in this case. If the patch is straightforward I think it makes sense to go straight to that.