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

Handle skip_serializing_if #387

Closed
Manishearth opened this issue Apr 5, 2021 · 4 comments
Closed

Handle skip_serializing_if #387

Manishearth opened this issue Apr 5, 2021 · 4 comments

Comments

@Manishearth
Copy link

#[derive(serde::Serialize, serde::Deserialize, Debug, Clone)]
struct Foo {
    #[serde(skip_serializing_if = "test_fn")]
    foo: Option<u8>,
}

fn test_fn(x: &Option<u8>) -> bool { x.is_none() }

The above code will panic because of serde-rs/serde#1732. I do have a proposal to fix it on the serde side (serde-rs/serde#2012), but I haven't gotten confirmation they want it yet.

A way to fix this on the bincode side would be to start calling serialize_len for structs as well. This would be a breaking change on the format; I'm not sure if that's possible in bincode.

It would also impact the size of every bincode'd struct, which might not be desired.

@VictorKoenders
Copy link
Contributor

I don't think we'd want this as a default feature, as it will be a breaking change in the binary format of existing data.

However we could add this to the Options trait (e.g. .allow_skipping_empty_fields()), so people can opt-in to this functionality.

@Manishearth
Copy link
Author

Oh that would be neat.

@TedDriggs
Copy link

This would definitely have helped me recently; I have a large struct that was originally made for JSON, so I use skip_serializing_if liberally in it. To add bincode support, I had to copy/paste the struct without those attributes, and then added some unit tests that will (hopefully) fail if the two get out of sync.

Could bincode's default behavior detect when someone has used this and recommend they enable the option? If, say, the struct len was greater than the number of fields actually seen, error out with a message "If you are using skip_serializing_if, enable allow_skipping_empty_fields in your Options"?

@ZoeyR
Copy link
Collaborator

ZoeyR commented Jun 13, 2021

I don't think its possible to handle skip_serializing_if in bincode. Imagine a situation like this:

#[derive(serde::Serialize, serde::Deserialize, Debug, Clone)]
struct Foo {
    #[serde(skip_serializing_if = "test_fn")]
    foo: Option<u8>,
    #[serde(skip_serializing_if = "test_fn")]
    bar: Option<u8>,
}

fn test_fn(x: &Option<u8>) -> bool { x.is_none() }

If only one of the fields gets skipped, on deserialize we have no way to know which field was skipped.

@ZoeyR ZoeyR closed this as completed Jun 13, 2021
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

4 participants