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

Deserialization fails if set_default is passed an unsigned integer #352

Closed
kesyog opened this issue Jun 28, 2022 · 1 comment · Fixed by #353
Closed

Deserialization fails if set_default is passed an unsigned integer #352

kesyog opened this issue Jun 28, 2022 · 1 comment · Fixed by #353

Comments

@kesyog
Copy link
Contributor

kesyog commented Jun 28, 2022

Small motivating example that demonstrates the issue:

use config::{Config, Environment};
use serde::Deserialize;
use std::error::Error;

// Using any unsigned type here leads to a runtime deserialization error
// Runs fine when using i8, i16, i32, i64
const DEFAULT_BAR: u32 = 30;

fn main() -> Result<(), Box<dyn Error>> {
    #[derive(Deserialize, Debug)]
    struct Foo {
        bar: u32,
    }
    let config: Foo = Config::builder()
        .add_source(Environment::with_prefix("TEST"))
        .set_default("bar", DEFAULT_BAR)?
        .build()?
        .try_deserialize()?;
    dbg!(config.bar);
    Ok(())
}

Output:

Error: invalid type: unsigned integer 64 bit `30`, expected an signed 64 bit or less integer for key `bar`

While looking into potentially fixing it, I noticed that Value::into_uint is defined but never used. Modifying the deserialize_u32 method to use into_uint instead of into_int fixes this issue, but breaks some existing tests. I'm guessing that's because of the next observation:

Value::into_int and Value::into_uint both seem overly strict. For example, calling Value::into_uint() when the kind is a ValueKind::I64 always fails. Shouldn't we try a conversion using the TryInto trait and only fail if the conversion fails?

Draft PR showing what I'm envisioning: https://github.com/mehcode/config-rs/compare/master...kesyog:config-rs:kyogeswaran/deserialize-uints?expand=1

This looks potentially related to #93.

@matthiasbeyer
Copy link
Member

I feel like more and more issues come from the fact that we're deserializing internally and store things in our own data structure.

Thanks for reporting this! 👍

I guess the fix looks alrightish, feel free to open a PR for this and we go from there!

kesyog added a commit to kesyog/config-rs that referenced this issue Jun 28, 2022
* Attempt to convert between integer types using `TryInto`-based
conversions rather than blanket failing for some source and
destination types.
* Use `into_uint` instead of `into_int` in `Value` Deserialize
implementations for unsigned integer types. Previously, we were
converting from signed types to unsigned types using `as`, which can
lead to surprise integer values conversions (rust-cli#93).

Fixes rust-cli#352 and rust-cli#93
kesyog added a commit to kesyog/config-rs that referenced this issue Jun 29, 2022
* Attempt to convert between integer types using `TryInto`-based
conversions rather than blanket failing for some source and
destination types.
* Use `into_uint` instead of `into_int` in `Value` Deserialize
implementations for unsigned integer types. Previously, we were
converting from signed types to unsigned types using `as`, which can
lead to surprise integer values conversions (rust-cli#93).

Fixes rust-cli#352 and rust-cli#93
matthiasbeyer pushed a commit to matthiasbeyer/config-rs that referenced this issue Aug 2, 2022
* Attempt to convert between integer types using `TryInto`-based
conversions rather than blanket failing for some source and
destination types.
* Use `into_uint` instead of `into_int` in `Value` Deserialize
implementations for unsigned integer types. Previously, we were
converting from signed types to unsigned types using `as`, which can
lead to surprise integer values conversions (rust-cli#93).

Fixes rust-cli#352 and rust-cli#93

(cherry picked from commit 7db2e8b)
matthiasbeyer pushed a commit that referenced this issue Aug 2, 2022
* Attempt to convert between integer types using `TryInto`-based
conversions rather than blanket failing for some source and
destination types.
* Use `into_uint` instead of `into_int` in `Value` Deserialize
implementations for unsigned integer types. Previously, we were
converting from signed types to unsigned types using `as`, which can
lead to surprise integer values conversions (#93).

Fixes #352 and #93

(cherry picked from commit 7db2e8b)
Signed-off-by: Matthias Beyer <[email protected]>
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.

2 participants