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

serde-wasm-bindgen doesn't handle serde(deny_unknown_fields) properly #39

Closed
sergeyboyko0791 opened this issue Jan 5, 2023 · 3 comments

Comments

@sergeyboyko0791
Copy link

An example:

use wasm_bindgen_test::*;
use serde::{Deserialize, Serialize};

wasm_bindgen_test_configure!(run_in_browser);

#[wasm_bindgen_test]
fn test_deny_unknown_fields() {
    #[derive(Debug, Deserialize, Serialize)]
    #[serde(deny_unknown_fields)]
    pub struct RPCErrorExtra {
        pub code: i32,
        pub message: String,
        pub data: Option<serde_json::Value>,
        pub stack: Option<serde_json::Value>,
    }

    let serializer = serde_wasm_bindgen::Serializer::json_compatible();

    let json = r#"{"code": 1, "message": "", "extra": true}"#;
    let json_value = serde_json::from_str::<serde_json::Value>(json).unwrap();
    let js_value = json_value.serialize(&serializer).unwrap();
    // This should fail, but it doesn't.
    serde_wasm_bindgen::from_value::<RPCErrorExtra>(js_value).unwrap_err();
}

Panics with:

panicked at 'called Result::unwrap_err() on an Ok value: RPCErrorExtra { code: 1, message: "", data: None, stack: None }

@RReverser
Copy link
Owner

Yeah I'm afraid you'll have to manually capture extra fields like here: https://serde.rs/attr-flatten.html#capture-additional-fields

The reason is that, unlike flatten, deny_unknown_fields doesn't switch deserialization from structs to maps, and we have no way of knowing whether Serde will want those extra fields or not.

Being able to pick only known properties from JS object instead of iterating over its all key-value pairs as a map is a significant perf optimisation, and I wouldn't want to remove it for the relatively rare attribute.

Ideally Serde upstream should switch to deserialize_map when deny_unknown_fields is used, but I'm not sure if they'd be open to do so.

@RReverser
Copy link
Owner

The easiest solution for now might be to use a helper like this:

fn deserialize_deny_unknown_fields<'de, T: Deserialize<'de>>(value: JsValue) -> Result<T, serde_wasm_bindgen::Error> {
    #[derive(Deserialize)]
    struct Wrap<T> {
        #[serde(flatten)]
        result: T,
        #[serde(flatten)]
        extra_fields: HashMap<String, serde::de::IgnoredAny>,
    }

    let Wrap {
        result,
        extra_fields,
    } = serde_wasm_bindgen::from_value(value)?;
    if !extra_fields.is_empty() {
        return Err(serde::de::Error::custom(format_args!(
            "Unexpected fields: {:?}",
            extra_fields.keys().collect::<Vec<_>>()
        )));
    }
    Ok(result)
}

@sergeyboyko0791
Copy link
Author

@RReverser thank you for the answer! Will follow your advice.

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