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

Optional fields with #[serde(default)] for deserialization #175

Closed
BenJeau opened this issue Oct 19, 2023 · 23 comments
Closed

Optional fields with #[serde(default)] for deserialization #175

BenJeau opened this issue Oct 19, 2023 · 23 comments

Comments

@BenJeau
Copy link

BenJeau commented Oct 19, 2023

Hi thanks for the library!

For a struct with serde default for some fields, the fields becomes optional when Deserializing and if not define will use the default value. I think the resulting TypeScript code should reflect that. The following Rust code:

#[derive(Deserialize, TS)]
#[ts(export)]
pub struct Request {
    url: String,
    #[serde(default)]
    method: Method,
    #[serde(default = "default_timeout")]
    timeout: u32,
}

#[derive(Deserialize, TS, Default)]
#[ts(export)]
enum Method {
    #[default]
    Get,
    Post,
    Put,
    Delete,
}

fn default_timeout() -> u32  {
    10_000
}

Should result in the following TypeScript types if we only care about deserialization

interface Request {
    url: string;
    method?: Method;
    timeout?: number;
}
@escritorio-gustavo
Copy link
Contributor

Use #[ts(optional)]

@escritorio-gustavo escritorio-gustavo closed this as not planned Won't fix, can't repro, duplicate, stale Jan 26, 2024
@abhay-agarwal
Copy link

For individual fields labeled as #[serde(default)], ts_rs could label the fields as optional. Currently a #[ts(optional)] marker only works on Option<> types, though. This would help me specifically with boolean types which are often given as optional types and only serialized if true.

@escritorio-gustavo
Copy link
Contributor

For individual fields labeled as #[serde(default)], ts_rs could label the fields as optional. Currently a #[ts(optional)] marker only works on Option<> types, though. This would help me specifically with boolean types which are often given as optional types and only serialized if true.

You could do it like this:

use serde::Serialize;

#[derive(TS, Serialize)]
#[ts(export)]
struct Foo {
    #[ts(optional)]
    #[serde(skip_serializing_if = "should_skip", default = "Default::default")]
    my_optional_bool: Option<bool>,
}

fn should_skip(field: &Option<bool>) -> bool {
    matches!(field, None | Some(false))
}

@abhay-agarwal
Copy link

Right, this is what we were doing before. However, because Rust doesn't have a True object, our code was required to "know" that the Option was always Option(true) or None, but never Option(false), and sometimes we would unwittingly add Option(false) in there if, for example, option chaining wasn't correctly written. So my preference would be to allow ts_rs to generate the question mark property if there was a serde(default), as that should in theory allow for the property to be declared optional.

@escritorio-gustavo
Copy link
Contributor

escritorio-gustavo commented Mar 28, 2024

I think this is data modeling error. You're saying your data should only have two valid states, but it has three. Something more appropriate would be using Option<()> and implementing (De)Serialize to convert it to ?: boolean:

use ts_rs::TS;
use serde::{Deserialize, Serialize};

#[derive(TS, Serialize, Deserialize, PartialEq, Debug)]
#[ts(export)]
struct Foo {
    #[ts(optional, as = "Option<bool>")]
    #[serde(skip_serializing_if = "Option::is_none", default = "Default::default", with = "deser")]
    my_optional_bool: Option<()>,
}

mod deser {
    use serde::{Serializer, Serialize, Deserializer, Deserialize};

    pub fn serialize<S: Serializer>(value: &Option<()>, serializer: S) -> Result<S::Ok, S::Error> {
        value.map(|_| true).serialize(serializer)
    }

    pub fn deserialize<'de, D: Deserializer<'de>>(deserializer: D) -> Result<Option<()>, D::Error> {
        Ok(Option::<bool>::deserialize(deserializer)?.filter(|&x| x).map(|_| ()))
    }
}

#[test]
fn test() {
    let none = Foo { my_optional_bool: None };
    let some = Foo { my_optional_bool: Some(()) };

    // Type definition
    assert_eq!(Foo::inline(), "{ my_optional_bool?: boolean, }");

    // Serializing
    assert_eq!(serde_json::to_string(&none).unwrap(), "{}");
    assert_eq!(serde_json::to_string(&some).unwrap(), r#"{"my_optional_bool":true}"#);

    // Deserializing
    assert_eq!(serde_json::from_str::<Foo>(r#"{"my_optional_bool":true}"#).unwrap(), some);
    assert_eq!(serde_json::from_str::<Foo>(r#"{"my_optional_bool":false}"#).unwrap(), none); // `false` becomes `None`!
    assert_eq!(serde_json::from_str::<Foo>("{}").unwrap(), none);
}

@escritorio-gustavo
Copy link
Contributor

escritorio-gustavo commented Mar 28, 2024

If you really want to use Option<bool>, then your code should handle Some(false), because it is a valid value for that type, and therefore, sooner or later, it will show up in your codebase. You can't just go "Eh, it'll never happen", because as you said, it already does. Make your invalid states unrepresentable with Option<()> or handle all representable states appropriately with something like

match optional_bool {
    Some(true) => todo!("True case"),
    Some(false) | None => todo!("False case")
}

or

if optional_bool.filter(|&x| x).is_some() {
    todo!("True case"),
} else {
    todo!("False case")
}

or something else that handles all three cases

@abhay-agarwal
Copy link

abhay-agarwal commented Mar 28, 2024

My usecase is one where nonexistence of a value is equivalent to passing that value as false. Serde allows that using the default macro. In our code, we treat the value as boolean because there is either true or [false/undefined]. We don’t want to use Option for the very reason you described — it creates a third state in the data model.

Unfortunately, ts_rs only allows for optional fields to be declared as Option. If Rust had a “true” object, then we could do Option or just Option<()> however I prefer that our rust code use boolean rather than Option<()>.

For me, it makes complete sense for a non-Option field with #[serde(default)] to be rendered as “field?: type” in ts. I would support a feature that made that conversion possible.

@NyxCode
Copy link
Collaborator

NyxCode commented Mar 29, 2024

For me, it makes complete sense for a non-Option field with #[serde(default)] to be rendered as “field?: type” in ts. I would support a feature that made that conversion possible.

We've had something somewhat similar in a previous version, but ended up removing it.
The problem is that we don't know what you're going to do with your types - If you're sending them to a rust server, for example, then yeah, #[serde(default)] should be field?: Type, but if they represent a server response, then they should be field: Type.

Like #[serde(default)], there are a lot of "asymmetric" serde attributes we don't support, like skip_serializing_if, skip_serializing and skip_deserializing. Whichever way we handeled them, it'd be wrong in 50% of the cases.

So yeah, this all gets kinda tricky.
Whether we can make #[ts(optional)] work on non-Option fields might be worth investigating, but I'm afraid we'll hit lots of edge-cases doing that. Maybe just doing #[ts(type = "number | undefined")] is good enough?

@abhay-agarwal
Copy link

In my mind ts_rs is a code generator, but doesn’t explicitly guarantee that the generated data is consistent or logical. So for our case, generating an optional parameter with a question mark should be something the user can do at their own behest. In typescript, a parameter that is declared optional can be removed from the instantiation, but one declared type | optional still needs to be instantiated.

I think a ts(optional) would be great that supports arbitrary objects. And yes, if we were to be fully bidirectionally consistent then we would need to use serde(default) and skip_serializing_if(x). But that’s our own responsibility IMO.

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 3, 2024

Fair enough! I've noted this in #294.
Some thought needs to be put into this to figure out if

  • it makes sense to just support #[ts(optional)] for any type
    • it might not, since the current semantics of #[ts(optional)] are to add the ?, but to remove the | null from the type.
  • or if this would be something for the next breaking release, where we're free to re-think how #[ts(optional)] should work.

@abhay-agarwal
Copy link

Given that ts optional is only available for Option types, this won’t be a breaking release if the functionality is kept as is for option types. In fact it should basically “just work” if you remove the requirement for Option in the macro. Can submit a PR if helpful

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 3, 2024

I think it's important that #[ts(optional)] behaves predictably, so using it on Option<T> and T should do the same.
However, on Option<T>, it generates field?: T.

Only #[ts(optional = nullable)] generates field?: T | null. That's the behaviour we'd want when used on a non-Option type - keep the type the same, and add a ? to the field.

So right now, we could support #[ts(optional = nullable)] on every type, while disallowing #[ts(optional)].
Or we accept that they behave differently, but I'm really not sure about that.

@escritorio-gustavo
Copy link
Contributor

escritorio-gustavo commented Apr 3, 2024

I prefer that our rust code use boolean rather than Option<()>.

Then this becomes even simpler, no custom (de)serialization required:

use ts_rs::TS;
use serde::{Deserialize, Serialize};

#[derive(TS, Serialize, Deserialize, PartialEq, Debug)]
#[ts(export)]
struct Foo {
    #[ts(optional, as = "Option<bool>")]
    #[serde(skip_serializing_if = "std::ops::Not::not", default = "Default::default")]
    my_optional_bool: bool,
}

#[test]
fn test() {
    let falsy = Foo { my_optional_bool: false };
    let truthy = Foo { my_optional_bool: true };

    // Type definition
    assert_eq!(Foo::inline(), "{ my_optional_bool?: boolean, }");

    // Serializing
    assert_eq!(serde_json::to_string(&falsy).unwrap(), "{}"); // `false` is not serialized
    assert_eq!(serde_json::to_string(&truthy).unwrap(), r#"{"my_optional_bool":true}"#);

    // Deserializing
    assert_eq!(serde_json::from_str::<Foo>(r#"{"my_optional_bool":true}"#).unwrap(), truthy);
    assert_eq!(serde_json::from_str::<Foo>(r#"{"my_optional_bool":false}"#).unwrap(), falsy);
    assert_eq!(serde_json::from_str::<Foo>("{}").unwrap(), falsy); // `undefined` is deserialized into `false`
}

@escritorio-gustavo
Copy link
Contributor

With #[ts(as = "...")] you can just make TS interpret your type as if it were Option<T> instead of T, and then add #[ts(optional)] to change the type to field?: T instead of field: T | null.

The feature you're asking for already exists through as + optional

@NyxCode
Copy link
Collaborator

NyxCode commented Apr 3, 2024

That's a very nice solution! That didn't even cross my mind.

@abhay-agarwal
Copy link

This is a good intermediate solution for sure! It would be nice to have a solution that can use the type as defined, so that there's no duplication between as = Option<X> and field: X

But I appreciate that recommendation, I can implement that solution now.

@abhay-agarwal
Copy link

As a real example, here's my (now updated) definition of a struct:

#[derive(Debug, Deserialize, Serialize, TS, Clone, PartialEq)]
#[serde(rename_all = "camelCase")]
#[ts(export, export_to = "ts/")]
pub struct FilePropertySelection {
    #[ts(optional, as = "Option<bool>")]
    #[serde(default, skip_serializing_if = "std::ops::Not::not")]
    pub everything: bool,
    /// Return all properties within the given groups (along with properties)
    #[ts(optional, as = "Option<HashSet<EntityPropertyGroupId>>")]
    #[serde(default, skip_serializing_if = "HashSet::is_empty")]
    pub groups: HashSet<EntityPropertyGroupId>,
    /// Return all given properties (along with groups)
    /// WARN: If a property is specified, and it doesn't exist, and error will
    /// occur
    #[ts(optional, as = "Option<HashSet<FilePropertyId>>")]
    #[serde(default, skip_serializing_if = "HashSet::is_empty")]
    pub properties: HashSet<FilePropertyId>,
}

As you can see, the use of as = ... does work, it just doesn't quite get me to the point where I feel like the problem is "solved". I would love to not have to use the duplicative syntax if possible. But again, this is a choice for your side, as I can technically achieve what I want.

@escritorio-gustavo
Copy link
Contributor

escritorio-gustavo commented Apr 4, 2024

I would love to not have to use the duplicative syntax if possible.

We could allow as to parse Option<_> and have _ be converted into the field's original type

use ts_rs::TS;
use serde::{Deserialize, Serialize};

#[derive(TS, Serialize, Deserialize, PartialEq, Debug)]
#[ts(export)]
struct Foo {
    #[ts(optional, as = "Option<_>")]
    #[serde(skip_serializing_if = "std::ops::Not::not", default)]
    my_optional_bool: bool,
}

#[test]
fn test() {
    let falsy = Foo { my_optional_bool: false };
    let truthy = Foo { my_optional_bool: true };

    // Type definition
    assert_eq!(Foo::inline(), "{ my_optional_bool?: boolean, }");

    // Serializing
    assert_eq!(serde_json::to_string(&falsy).unwrap(), "{}"); // `false` is not serialized
    assert_eq!(serde_json::to_string(&truthy).unwrap(), r#"{"my_optional_bool":true}"#);

    // Deserializing
    assert_eq!(serde_json::from_str::<Foo>(r#"{"my_optional_bool":true}"#).unwrap(), truthy);
    assert_eq!(serde_json::from_str::<Foo>(r#"{"my_optional_bool":false}"#).unwrap(), falsy);
    assert_eq!(serde_json::from_str::<Foo>("{}").unwrap(), falsy); // `undefined` is deserialized into `false`
}

@escritorio-gustavo
Copy link
Contributor

Your example would then become

#[derive(Debug, Deserialize, Serialize, TS, Clone, PartialEq)]
#[serde(rename_all = "camelCase")]
#[ts(export, export_to = "ts/")]
pub struct FilePropertySelection {
    #[ts(optional, as = "Option<_>")]
    #[serde(default, skip_serializing_if = "std::ops::Not::not")]
    pub everything: bool,
    /// Return all properties within the given groups (along with properties)
    #[ts(optional, as = "Option<_>")]
    #[serde(default, skip_serializing_if = "HashSet::is_empty")]
    pub groups: HashSet<EntityPropertyGroupId>,
    /// Return all given properties (along with groups)
    /// WARN: If a property is specified, and it doesn't exist, and error will
    /// occur
    #[ts(optional, as = "Option<_>")]
    #[serde(default, skip_serializing_if = "HashSet::is_empty")]
    pub properties: HashSet<FilePropertyId>,
}

@abhay-agarwal
Copy link

Sounds great!

@abhay-agarwal
Copy link

I still think the best/easiest solution is to just allow for the #[ts(optional)] to be used on non-option types, but your solution is appropriate as well.

@gustavo-shigueo
Copy link
Collaborator

Great! I have already coded it on my work computer and will submit a PR on monday

@escritorio-gustavo
Copy link
Contributor

Check out #299

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

5 participants