wyyerd / stripe-rs

Rust API bindings for the Stripe HTTP API.
Apache License 2.0
224 stars 88 forks source link

Incorrect handling for nullable booleans in codegen #84

Closed gperinazzo closed 5 years ago

gperinazzo commented 5 years ago

The openapi codegen currently handles non-required booleans using #[serde(default)]. This works to set the value to false if the field is missing in the input json, but if the attribute is nullable deserialization will fail if it finds a null.

The assumption seems to be documented in line 1392 of the codegen source

I've written a small example:

use serde::Deserialize;

#[derive(Debug, Deserialize)]
struct NoAttribute {
    field: bool,
}

#[derive(Debug, Deserialize)]
struct DefaultAttribute {
    #[serde(default)]
    field: bool,
}

#[derive(Debug, Deserialize)]
struct OptionalField {
    field: Option<bool>,
}

fn main() {
    let json = r#"{"field":null}"#;

    dbg!(serde_json::from_str::<NoAttribute>(json));
    dbg!(serde_json::from_str::<DefaultAttribute>(json));
    dbg!(serde_json::from_str::<OptionalField>(json));
}

Which outputs:

[src/main.rs:22] serde_json::from_str::<NoAttribute>(json) = Err(
    Error("invalid type: null, expected a boolean", line: 1, column: 13),
)
[src/main.rs:23] serde_json::from_str::<DefaultAttribute>(json) = Err(
    Error("invalid type: null, expected a boolean", line: 1, column: 13),
)
[src/main.rs:24] serde_json::from_str::<OptionalField>(json) = Ok(
    OptionalField {
        field: None,
    },
)

I had this issue trying to receive a product.created event where shippable seems to be set to null.

kestred commented 5 years ago

I've made a fix for this issue in https://github.com/wyyerd/stripe-rs/pull/93.

Thanks for submitting the issue.

gperinazzo commented 5 years ago

Thanks for the quick fix! I was going to propose removing that condition, but wasn't sure if the changes in the resulting objects were acceptable (as those are breaking changes).

There may be a way to maintain the previous API by telling serde to use a custom deserialization function for those fields, but I didn't have the time to test it out.

kestred commented 5 years ago

This change was made in the "major" v0.11.0 release