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

failed type conversion in .ini file for serde flatten #504

Open
mfirhas opened this issue Dec 13, 2023 · 10 comments
Open

failed type conversion in .ini file for serde flatten #504

mfirhas opened this issue Dec 13, 2023 · 10 comments

Comments

@mfirhas
Copy link

mfirhas commented Dec 13, 2023

I think other formats are all fine, but for .ini/.env, when I flatten the structs into another struct, type conversion failed. I checked the code and found this:

Value::new(uri, ValueKind::String(v.to_owned())),

Value::new(uri, ValueKind::String(v.to_owned())),

All code are string-ed rightaway.

Should it be parsed like this?:

let value = if self.try_parsing {

I test locally and it works and was about to create PR, but confuse with master branch status I faced unexpected error, unlike in v0.13.4.

@matthiasbeyer
Copy link
Collaborator

Hi, thanks for reporting this! A PR would be awesome of course!

, but confuse with master branch status I faced unexpected error,

What do you mean by this?

@mfirhas
Copy link
Author

mfirhas commented Dec 13, 2023

I created my own test code, test it on master branch got error missing field 'field_name', switched to v0.13.4 got no error. The test was for loading configs from .env/.ini file.

@matthiasbeyer
Copy link
Collaborator

The last CI run on master is green though.

@mfirhas
Copy link
Author

mfirhas commented Dec 13, 2023

Here is my test code:

// tests/file_env_test.rs

use config::{
    builder::DefaultState, Config, ConfigBuilder, ConfigError, Environment, File, FileFormat,
};
use serde::Deserialize;
use std::fmt::{Debug, Display};

const ALL_CFG_FILE: &str = "./tests/all.env";

#[derive(Debug, Deserialize)]
struct TestConfig {
    #[serde(alias = "TEST__NUMBER")]
    pub number: i32,
    #[serde(alias = "TEST__STRING")]
    pub string: String,
    #[serde(alias = "TEST__FLOAT")]
    pub float: f32,
    #[serde(alias = "TEST__BOOLEAN")]
    pub boolean: bool,
}

#[test]
fn test_file_config_success() {
    let cfg = Config::builder()
        .add_source(config::File::new(ALL_CFG_FILE, FileFormat::Ini))
        .build()
        .unwrap()
        .try_deserialize::<TestConfig>();
    dbg!(&cfg);
    assert!(cfg.is_ok());
}

ini file
tests/all.env

TEST__NUMBER=123
TEST__STRING=this is string from file
TEST__FLOAT=123.3
TEST__BOOLEAN=true

My original issue is about type incompatibility on nested configs parsing from .ini file.
Above test code is only to test between master and v0.13.4, but found said error(missing field) on master.

@mfirhas
Copy link
Author

mfirhas commented Dec 15, 2023

There're many commits unmerged from v0.13.4 into master, hence different version.

@mfirhas
Copy link
Author

mfirhas commented Dec 15, 2023

This is the my commit id: e30debf

@polarathene
Copy link
Collaborator

All code are string-ed rightaway.

For reference the INI support is being refactored a little here: #470

That doesn't change the logic you draw attention to though. The reason we are calling .to_owned() there is because rust-ini is used and it provides an iterator over &str, there is no type information available. To "import" that data we need to create a copy of the strings it provides.

The other formats often parse the strings themselves and provide a Value enum that can then be mapped into the equivalent for config-rs (which normalizes Value type for internal use).

Should it be parsed like this?:

Yes, that sort of improvement could probably be added to (or after) the referenced refactor PR. It is intended for 0.14 so as a breaking change would be ok AFAIK, @matthiasbeyer ?

It can be a bit problematic depending on the value I think 🤷‍♂️

  • A "0" or "1" could "deserialize" into a bool that can be converted to 0/1 number type of a config struct.
  • But a string that intentionally begins with 0 like a phone number would lose that information, even if the config struct from the user is a string type? I think this is why you'll find other formats that do provide a value type tend to map into an equivalent config-rs value, so rust-ini providing strings and converting to strings still seems appropriate.

Here a string to a float:

config-rs/src/value.rs

Lines 521 to 540 in b3bda2c

pub fn into_float(self) -> Result<f64> {
match self.kind {
ValueKind::Float(value) => Ok(value),
ValueKind::String(ref s) => {
match s.to_lowercase().as_ref() {
"true" | "on" | "yes" => Ok(1.0),
"false" | "off" | "no" => Ok(0.0),
_ => {
s.parse().map_err(|_| {
// Unexpected string
ConfigError::invalid_type(
self.origin.clone(),
Unexpected::Str(s.clone()),
"a floating point",
)
})
}
}
}

and likewise string (or whatever other intermediary value in our container) to a bool type of the user config struct:

config-rs/src/value.rs

Lines 233 to 245 in b3bda2c

pub fn into_bool(self) -> Result<bool> {
match self.kind {
ValueKind::Boolean(value) => Ok(value),
ValueKind::I64(value) => Ok(value != 0),
ValueKind::I128(value) => Ok(value != 0),
ValueKind::U64(value) => Ok(value != 0),
ValueKind::U128(value) => Ok(value != 0),
ValueKind::Float(value) => Ok(value != 0.0),
ValueKind::String(ref value) => {
match value.to_lowercase().as_ref() {
"1" | "true" | "on" | "yes" => Ok(true),
"0" | "false" | "off" | "no" => Ok(false),

I think the issue is more related to that area, than rust-ini and if you attempt to fix it with the ini format you risk introducing regressions and less flexibility 😅

@mfirhas
Copy link
Author

mfirhas commented Dec 15, 2023

Okay, then I'll just need to use that version after release.

@polarathene
Copy link
Collaborator

when I flatten the structs into another struct, type conversion failed

I believe there are other issues related to serde deserializing (the internal value.rs representation, into user struct) within the project?


I'll just need to use that version after release.

I don't know if your concern will be fixed. Addressing INI to parse from &str to a ValueKind may fix your concern, but replace it with a regression that impacts others, which is not really a fix.

The value should already be captured into Value (value.rs) as a String and from there to the desired type. The fix you want should be handled there instead, but as shown above there are some cases where serde or the internal usage of it within config-rs is not compatible with some serde attributes like #[serde(flatten)].

Some formats are also known to be lossy in conversion into the config-rs Value container.

@mfirhas
Copy link
Author

mfirhas commented Dec 15, 2023

For now, I'll just refer to my commit in my fork. After this is fixed/refactored as a whole, will switch back.

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

No branches or pull requests

3 participants