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

deserialize_in_place fills skipped fields with default #2512

Open
h3ndrk opened this issue Jul 18, 2023 · 1 comment · May be fixed by #2513
Open

deserialize_in_place fills skipped fields with default #2512

h3ndrk opened this issue Jul 18, 2023 · 1 comment · May be fixed by #2513

Comments

@h3ndrk
Copy link

h3ndrk commented Jul 18, 2023

Hi, thanks for this awesome library! I have some questions regarding in-place deserialization.

I'm wondering why we are filling skipped fields in deserialize_in_place(). Skipped fields are ignored for maps but not for sequences. Is there a reason for it or should the filling with default be removed? This is the relevant code that writes default values into the in-place struct:

if field.attrs.skip_deserializing() {
let default = Expr(expr_is_missing(field, cattrs));
quote! {
self.place.#member = #default;
}
} else {

My use case is the following: I have a struct (MixedContent below) which contains fields that I want to serialize and fields that cannot be serialized and also don't implement Default. Something like this:

// serde_derive with Feature deserialize_in_place
#[derive(Deserialize, Serialize)]
struct MixedContent {
    #[serde(skip, default = "panic")]
    non_serializable: NonSerializable,

    serializable: Serializable,
}

fn panic() -> NonSerializable {
    panic!("deserialize not implemented")
}

struct NonSerializable {
    // ...
}

#[derive(Deserialize, Serialize)]
struct Serializable {
    // ...
}

Since I cannot implement Default for the NonSerializable but for Serializable, my idea was to use the deserialize_in_place functionality with skipped fields. With that I can create a custom constructor with more fancy logic and then use deserialize_in_place() afterwards on the struct. I think, using some DeserializeSeed is too complicated because it seems I need to implement it manually. Also custom home-grown derive macros doesn't feel right to achieve my scenario.

What I need to adjust from basic serde usage is

  1. Generate a proper deserialize_in_place: Easy, just enable the feature flag
  2. Disable the normal deserialize: Can be done via a serde(default = ...) attribute which never returns anything but fulfills the trait and makes the compiler happy
  3. Deactivate all struct field writes of skipped fields: Doesn't work because the sequence code generation still contains them, see above

I'm willing to create a PR to remove the skipped field filling in sequences but wanted to make sure, nothing else gets broken by it (Chesterton's Fence). Do you have suggestions?

@Mingun
Copy link
Contributor

Mingun commented Jul 18, 2023

Working with skipped fields are broken in many cases, I'm working right now on fixing those issues and will make a PR in a few days.

But in this case that is never-mind. I do not plan to change this behaviour in my PR. Although I agree, that it will be better to does not touch missing fields.

@h3ndrk h3ndrk linked a pull request Jul 18, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants