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

repro for #417 #418

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

repro for #417 #418

wants to merge 3 commits into from

Conversation

Halkcyon
Copy link

No description provided.

@matthiasbeyer
Copy link
Member

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
Copy link
Author

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

@matthiasbeyer
Copy link
Member

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
Copy link
Author

@matthiasbeyer Done. Surprising result, the new test passes

Copy link
Member

@matthiasbeyer matthiasbeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes itself look good.

CI fails because the files in tests/Settings are not updated to match the expected Settings structure!

The commit linter also fails. Please:

  • Rewrite the commits in imperative mood ("Add stuff" instead of "Adding stuff"). Don't hesitate to include details in the message body!
  • Ensure the length of the subject line does not exceed 50 characters
  • Make sure to sign off your commits

In general: https://cbea.ms/git-commit/ 😉


Or ping me if you don't care anymore, then I will take over this PR! 😉


assert_eq!(s.ip_key, "a string value");
}

#[test]
fn test_file() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails because the files in tests/Settings are not updated for the new key ("192.168.1.1")!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is updated. It fails due to #417

fn test_keys_with_periods_deserialize() {
let c = make();

let s: Settings = c.try_deserialize().unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test fails here because the tests/Settings files are not updated.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I guess I was not fully awake when reviewing this. Sorry about that.

@matthiasbeyer
Copy link
Member

So if I understand correctly:

  • Plain yaml-rust succeeds
  • config-rs (which uses yaml-rust) fails

Okay, so we have to investigate why that is.

@polarathene
Copy link
Collaborator

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",
                ),
            },
        },
    ),
}

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

Successfully merging this pull request may close these issues.

4 participants