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

skip_serializing_if also acts like default #2

Open
robin-nitrokey opened this issue Jul 13, 2023 · 1 comment
Open

skip_serializing_if also acts like default #2

robin-nitrokey opened this issue Jul 13, 2023 · 1 comment

Comments

@robin-nitrokey
Copy link
Member

Currently, skip_serializing_if not only affects serialization but also deserialization: If a field is marked with this attribute, it is not unwrapped after deserializing. This also means that the attribute only works for options.

@sosthene-nitrokey
Copy link
Contributor

This actually appears to not be the wanted implementation behaviour. It's actually buggy per the serde data model.

Here there should be a: let #ident = #ident.unwrap_or_default().

Right now this works for options because when de-serializing the map, the value is stored in a temporary option, where for a field of type T that option is of type Option<{Inferred Type}>. Where None means that the field is absent, and Some means that the field is present.

The issue is that it doesn't unwrap_or_default, and directly places it in the value. So if T is Option<S> it means that {Inferred type } is inferred to be S. However this is incorrect, the {Inferred Type} should always be T.

This is an issue because:

  1. This only works for options with a default to None
  2. Even for option this is is incorrect because it will reject the field if it is present but with a value of None. FIDO canonical CBOR appears to allow null values so this could potentially happen, even though it suggests that optional value will be missing, and cbor-smol also handles null values as None.

sosthene-nitrokey added a commit to sosthene-nitrokey/serde-indexed that referenced this issue Nov 27, 2023
This patch adds tests based on `serde_test`. These tests helped showcase
the bug that leads to trussed-dev#2
sosthene-nitrokey added a commit that referenced this issue Jun 25, 2024
This patch adds tests based on `serde_test`. These tests helped showcase
the bug that leads to #2
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