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

Unable to deserialize unsigned integers #357

Closed
thrykol opened this issue Jul 18, 2022 · 17 comments
Closed

Unable to deserialize unsigned integers #357

thrykol opened this issue Jul 18, 2022 · 17 comments

Comments

@thrykol
Copy link

thrykol commented Jul 18, 2022

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

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

@thrykol
Copy link
Author

thrykol commented Jul 25, 2022

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

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

@matthiasbeyer
Copy link
Member

I mean in the above code.

@thrykol
Copy link
Author

thrykol commented Jul 25, 2022

I mean in the above code.

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

@matthiasbeyer
Copy link
Member

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

matthiasbeyer commented Jul 26, 2022

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

thrykol commented Jul 26, 2022

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

matthiasbeyer commented Jul 26, 2022

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

thrykol commented Aug 1, 2022

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

Yes.

@thrykol
Copy link
Author

thrykol commented Aug 2, 2022

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

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

thrykol commented Aug 2, 2022

2d74d06 is the first commit which works for me.

@matthiasbeyer
Copy link
Member

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

thrykol commented Aug 2, 2022

That branch works.

@matthiasbeyer
Copy link
Member

Awesome. 0.13.2 incoming shortly.

@matthiasbeyer
Copy link
Member

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

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

2 participants