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

Warn about unknown fields when deserialising JSON files #6436

Open
mina86 opened this issue Mar 15, 2022 · 6 comments
Open

Warn about unknown fields when deserialising JSON files #6436

mina86 opened this issue Mar 15, 2022 · 6 comments
Labels
C-good-first-issue Category: issues that are self-contained and easy for newcomers to work on. Groomed Node Node team P-low Priority: low T-node Team: issues relevant to the node experience team

Comments

@mina86
Copy link
Contributor

mina86 commented Mar 15, 2022

Currently, if serde encounters an unknown field it silently ignores it. We might want to at least warn if this happens. I’ve just spent noticeable amount of time debugging an issue caused by a typo in config.json file.

@bowenwang1996 bowenwang1996 added the C-good-first-issue Category: issues that are self-contained and easy for newcomers to work on. label Mar 16, 2022
@kakoc
Copy link
Contributor

kakoc commented Mar 23, 2022

If I understand correctly we may try to use that crate?:
https://github.com/dtolnay/serde-ignored

@mina86
Copy link
Contributor Author

mina86 commented Mar 23, 2022

Yes, looks exactly like what we want.

@kakoc
Copy link
Contributor

kakoc commented Mar 23, 2022

If so, am I right that we can do that in the following way:
We can create a de wrapper type and use it for debug deserializations?
Something like that:

impl<T> DeserializedWithDebug <T> {
    fn deserialize<'de, D>(deserializer: D) -> Result<T, D::Error>
    where
        T: Deserialize<'de>,
        D: Deserializer<'de>,
    {
        let mut unused = Set::new();
        let p: Package = serde_ignored::deserialize(deserializer, |path| {
            unused.insert(path.to_string());
        });

        debug!(unused);

        p
    }
}

@mina86
Copy link
Contributor Author

mina86 commented Mar 23, 2022

That looks reasonable. The exact formatting for the debug message would need some work of course and maybe even have the function return (T, Vec<String>) rather than printing the unrecognised keys, but in general seems to be what we want.

@kakoc
Copy link
Contributor

kakoc commented Mar 24, 2022

Thanks for the comments.
So I'm ready to tackle on that.

kakoc pushed a commit to kakoc/nearcore that referenced this issue Mar 25, 2022
kakoc pushed a commit to kakoc/nearcore that referenced this issue Mar 25, 2022
kakoc pushed a commit to kakoc/nearcore that referenced this issue Mar 25, 2022
kakoc pushed a commit to kakoc/nearcore that referenced this issue Mar 25, 2022
kakoc pushed a commit to kakoc/nearcore that referenced this issue Mar 25, 2022
kakoc pushed a commit to kakoc/nearcore that referenced this issue Mar 25, 2022
kakoc pushed a commit to kakoc/nearcore that referenced this issue Mar 28, 2022
kakoc pushed a commit to kakoc/nearcore that referenced this issue Mar 28, 2022
kakoc pushed a commit to kakoc/nearcore that referenced this issue Mar 28, 2022
@mina86 mina86 added the T-node Team: issues relevant to the node experience team label May 2, 2022
@janewang janewang added this to Incoming Requests (to be evaluated by the team and are not committed) in Node Q2 2022 via automation May 9, 2022
@exalate-issue-sync exalate-issue-sync bot added T-nodeX and removed T-node Team: issues relevant to the node experience team labels Jun 28, 2022
@matklad matklad added T-node Team: issues relevant to the node experience team and removed T-nodeX labels Aug 4, 2022
@exalate-issue-sync exalate-issue-sync bot added the P-low Priority: low label Aug 9, 2022
@gmilescu gmilescu added the Node Node team label Oct 19, 2023
@jancionear
Copy link
Contributor

It looks like this has been implemented in #6490, but I don't get any warnings when I add an invalid option to the config. Opened another issue about it: #10309

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-good-first-issue Category: issues that are self-contained and easy for newcomers to work on. Groomed Node Node team P-low Priority: low T-node Team: issues relevant to the node experience team
Projects
Node Q2 2022
Incoming Requests (to be evaluated by...
Development

No branches or pull requests

6 participants