mehcode / config-rs

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

Unable to deserialize unsigned integers #357

Closed thrykol closed 2 years ago

thrykol commented 2 years ago

When implementing config::ValueKind for a custom type, the error invalid type: unsigned integer 64 bit '128', expected an signed 64 bit or less integer for key 'inner.unsigned' is thrown if the custom type contains a unsigned integer. The following is a minimum example:

#[derive(Deserialize, Eq, PartialEq, Debug)]
struct Container<T> {
    inner: T,
}

#[derive(Deserialize, Eq, PartialEq, Debug)]
struct Unsigned {
    unsigned: u16,
}

impl Default for Unsigned {
    fn default() -> Self {
        Self { unsigned: 128 }
    }
}

impl From<Unsigned> for config::ValueKind {
    fn from(unsigned: Unsigned) -> Self {
        let mut properties = HashMap::default();
        properties.insert(
            "unsigned".to_string(),
            config::Value::from(unsigned.unsigned),
        );

        Self::Table(properties)
    }
}

assert_eq!(
    Container {
        inner: Unsigned::default()
    },
    config::Config::builder()
        .set_default("inner", Unsigned::default())
        .unwrap()
        .build()
        .unwrap()
        .try_deserialize::<Container<Unsigned>>()
        .unwrap()
);
matthiasbeyer commented 2 years ago

Hi! Can you tell me where exactly that error gets thrown?

thrykol commented 2 years ago

Hi! Can you tell me where exactly that error gets thrown?

Unfortunately, no, I was unable to trace the origin down.

matthiasbeyer commented 2 years ago

I mean in the above code.

thrykol commented 2 years ago

I mean in the above code.

Sorry... totally misunderstood you. There error is returned from .try_deserialize::<Container<Unsigned>>()

matthiasbeyer commented 2 years ago

I just added a testcase derived from your code in #359. Lets see whether I can reproduce your issue... but so far it looks like I can't!

matthiasbeyer commented 2 years ago

Yep, I cannot reproduce your issue... Maybe I've missed something, feel free to have a look at #359 and review whether it is equal to your code.

thrykol commented 2 years ago

Only difference I can see is the switch from HashMap to IndexMap. Tried making that change locally but it appears that 0.13.1 doesn't yet support creating a Table from a HashMap. Don't think it matters, but my Rust version is rustc 1.62.1 (e092d0b6b 2022-07-16)

matthiasbeyer commented 2 years ago

Only difference I can see is the switch from HashMap to IndexMap

That is because a Table is constructed using an IndexMap instead of a HashMap if the preserve_order feature is enabled.

thrykol commented 2 years ago

Wondering if there might be a difference in the logic when preserve_order is enabled. Have you possibly been able to duplicate with preserve_order = false?

matthiasbeyer commented 2 years ago

Yes.

thrykol commented 2 years ago

After cloning the repository and patching the source into my project with

[patch.crates-io]
config = { path = "../config-rs" }

everything works as expected. So there must be some change from 0.13.1 to HEAD which fixes what I'm seeing. Any chances of getting a new release out today?

matthiasbeyer commented 2 years ago

Can you please do some more testing?

Does 7db2e8b this exact patch maybe fix your issue? If it does, I can release this as 0.13.2.

thrykol commented 2 years ago

2d74d06 is the first commit which works for me.

matthiasbeyer commented 2 years ago

Okay, can you test the release-0.13.x branch on my personal repository? If that's it, I will use that to make the 0.13.2 release.

thrykol commented 2 years ago

That branch works.

matthiasbeyer commented 2 years ago

Awesome. 0.13.2 incoming shortly.

matthiasbeyer commented 2 years ago

0.13.2 was just released. I'm closing this issue for now, feel free to open a new one if need be.