mehcode / config-rs

⚙️ Layered configuration system for Rust applications (with strong support for 12-factor applications).
Apache License 2.0
2.43k stars 206 forks source link

repro for #417 #418

Open Halkcyon opened 1 year ago

matthiasbeyer commented 1 year ago

Nice. Could you also add one with yaml-rust, so that we have all three cases?

I will then take these patches and continue based on them... not sure how, but I guess I will think of something.

Halkcyon commented 1 year ago

@matthiasbeyer not sure I understand; the default case is yaml-rust with the second test case being for serde_yaml.

matthiasbeyer commented 1 year ago

Yes, I meant add another case where you use only yaml-rust, without config-rs in between. This way we can verify that we don't mess things up in the config-rs implementation!

Halkcyon commented 1 year ago

@matthiasbeyer Done. Surprising result, the new test passes

matthiasbeyer commented 1 year ago

So if I understand correctly:

Okay, so we have to investigate why that is.

polarathene commented 8 months ago

This is not specific to YAML parsing, but feature of config-rs with map lookup keys using dot notation.


Okay, so we have to investigate why that is.

Failure is due to . not being a permitted raw identifier char (accepting . here will fix this):

https://github.com/mehcode/config-rs/blob/6946069755e4bc75af9b7ca678da66c055a0af16/src/path/parser.rs#L15-L25

Instead . is used to designate a child field (accepting a . above will cause the test for this feature to fail):

https://github.com/mehcode/config-rs/blob/6946069755e4bc75af9b7ca678da66c055a0af16/src/path/parser.rs#L38-L49

That above code is called from this method during config build:

https://github.com/mehcode/config-rs/blob/6946069755e4bc75af9b7ca678da66c055a0af16/src/source.rs#L30-L38

which multiple locations will call via this method:

https://github.com/mehcode/config-rs/blob/6946069755e4bc75af9b7ca678da66c055a0af16/src/source.rs#L20-L27


I'm not too familiar with config-rs, but vaguely recall some feature to access fields with dot notation. I assume it's for the value map as a string key lookup feature. Thus you probably can't have both here unless there was support to make the distinction 🤷‍♂️

I understand the intent was to deserialize with the serde rename feature to the desired field. This won't work AFAIK since the builder is creating this cache prior to deserializing which splits up the key:

# Example input
hello.world: example
192.168.1.1: a string value
Built config before deserializing to target config from user ``` A config: Config { defaults: {}, overrides: {}, sources: [], cache: Value { origin: None, kind: Table( { "hello": Value { origin: None, kind: Table( { "world": Value { origin: Some( "hello.yaml", ), kind: String( "example", ), }, }, ), }, "192": Value { origin: None, kind: Table( { "168": Value { origin: None, kind: Table( { "1": Value { origin: None, kind: Table( { "1": Value { origin: Some( "hello.yaml", ), kind: String( "a string value", ), }, }, ), }, }, ), }, }, ), }, }, ), }, } ```
Config format deserializes correctly before handed to builder as source input ``` Value { origin: Some( "hello.yaml", ), kind: Table( { "hello.world": Value { origin: Some( "hello.yaml", ), kind: String( "example", ), }, "192.168.1.1": Value { origin: Some( "hello.yaml", ), kind: String( "a string value", ), }, }, ), } ```