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

Environment variable names are converted to lower case #340

Closed
pictographer opened this issue May 26, 2022 · 10 comments · Fixed by #354
Closed

Environment variable names are converted to lower case #340

pictographer opened this issue May 26, 2022 · 10 comments · Fixed by #354

Comments

@pictographer
Copy link

Environment variable names are converted to lower case internally. This is surprising undocumented behavior. We want to be able to override values from a configuration file with environment variables. It would be natural to use the same spelling (not counting the prefix), but this doesn't work if the configuration file keys are upper case. To make it work, the configuration file keys must be lower case.

config.toml

FOO="You can't override me!"
bar="You can override me just fine."

In the shell

APP_FOO="Why are you ignoring me?" APP_BAR="Yay! New bar." ./target/debug/app

I was using TOML for the config file and bash for the shell, if it matters.

@YounessBird
Copy link
Contributor

YounessBird commented May 26, 2022

Yes this happens because Environment variables are converted to lower case but this is not the case for the rest of the file formats supported by the crate.
For this kind of example to work, all the files in the folder /Format has to convert the key to lower case before it gets inserted into the internal Map of the Value.
I don't think this simple fix have any side effects. I am happy to submit a pull request.

@cw-kajiwara
Copy link

Any update on this issue? I would also want the option not to convert env var into lowercase internally.

@matthiasbeyer
Copy link
Member

@YounessBird yes please submit a PR and we go from there.

@YounessBird
Copy link
Contributor

YounessBird commented Jun 28, 2022

@matthiasbeyer I have wrote a patch to fix this, however, it turns out it's not as straight forward as I first thought.
If we use the lowercase keys approach to fix this issue, then the following code where enums allow for uppercase fields, suddenly won't work for example.

config.toml

   FOO = "I should be overridden" 

main.rs

#[derive(Debug, Deserialize,PartialEq)]
enum TestEnum {
   FOO(String)
}
let cfg= Config::builder()
.add_source(File::new("config.toml", FileFormat::Toml)).build().unwrap();

let foo: TestEnum = cfg.try_deserialize().unwrap();
 // Error 

the code won't work because all the keys inserted into the map are now lowercase, which means in the deserialization phase serde will fail to match the lowercase key foo with the enum field FOO.
but this will work great in the case of a struct since struct fields are usually lowercase.
So I think a better approach to fix this is to use the keys as they were initially set by the user of the crate in the environment variable internals .i.e, remove the lowercase. This will make the crate "case agnostic", then the users of the crate will have the freedom to set their keys as they wish, as well as the responsibility to make those keys match the data structure they want to deserialize into.
I have run some tests with this approach and it seems to work.
Let me know what you think about this, and or if you have any other concerns that I haven't taken into consideration.

@pictographer
Copy link
Author

Leaving the case as is makes sense to me. It's easy enough to override the Rust warnings about identifier case, i.e. #[allow(non_camel_case_types)]

@YounessBird
Copy link
Contributor

YounessBird commented Jun 29, 2022

@pictographer Yes, that could also be easily fixed by matching for uppercase in the deserialization phase for enums. It will be awesome if the same can be done for structs but that will require a manual struct deserialization. Also as a side note I am not sure why the crate limits enums to deserialize for one field only.

@pictographer
Copy link
Author

Are you familiar with the strum crate and the config crate? These two are awesome for unifying environment variables, enumeration constants, and configuration file values.

@matthiasbeyer
Copy link
Member

@pictographer you're referencing the config crate here? You realize that this is the codebase of the config crate? 😆

Leaving the case as is makes sense to me. It's easy enough to override the Rust warnings about identifier case

This.

I am eager to do things the right way in the next generation of this crate (see #321) and not put too much effort into the existing code IF it turns out that the next generation will actually be delivered at some point. I still invite you all, though, to propose fixes or changes to the existing implementation, of course!

@pictographer
Copy link
Author

pictographer commented Jun 29, 2022

@pictographer you're referencing the config crate here? You realize that this is the codebase of the config crate? laughing

Oops! I've gotta laugh. I wish my brain worked better.

@pictographer
Copy link
Author

@matthiasbeyer, thank you for the careful work! Nice to see this merged.

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 a pull request may close this issue.

4 participants