do not skip_serializing of read only schemas #317
Conversation
| @@ -240,15 +240,11 @@ impl CodeGen { | |||
| if &nm.to_string() != property_name { | |||
| serde_attrs.push(quote! { rename = #property_name }); | |||
| } | |||
| if property.schema.read_only == Some(true) { | |||
| serde_attrs.push(quote! { skip_serializing }); | |||
There was a problem hiding this comment.
Was this just because the read-only authoring couldn't necessarily be trusted? More so just curious. I think the change here looks safe.
There was a problem hiding this comment.
It is specific to the client side. I want to be able to do round-trip testing and I want to use the models from the server-side. One reason for read-only is so that the client doesn't send a bunch of the data back to the server. It is an optimization that isn't needed right now. Another design would be to generate a function that could clear the read only values, may be .clear_read_only().
There was a problem hiding this comment.
It's a good optimization that I don't think we do in other language SDKs right now. I imagine the fear is that, for PATCH operations (could be implemented as a PUT as well), missing data handled incorrectly could result in data loss. @pakrym might have more insight into why we still send read-only data back, or if it's just an optimization we haven't yet done.
There was a problem hiding this comment.
@pakrym might have more insight into why we still send read-only data back
Do we? We shouldn't be.
There was a problem hiding this comment.
Good to know we don't for .NET. Thanks.
There was a problem hiding this comment.
And, that said, if it hasn't been a detected problem yet this is probably a safe optimization. Ideally, if we did find a problem it should be a service-side fix since Azure has guidelines on patching resources.
| @@ -83,13 +83,13 @@ pub struct EgressConfig {} | |||
| pub struct AlertContext {} | |||
| #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] | |||
| pub struct Essentials { | |||
| #[serde(skip_serializing)] | |||
| #[serde(default, skip_serializing_if = "Option::is_none")] | |||
There was a problem hiding this comment.
More of a general comment: I'm surprised serde doesn't do this for Option<T> in general like serializers do in our other languages. Is there some way we can modify the behavior and not have to attribute every optional or vectored field?
There was a problem hiding this comment.
The reason would be that some serialization formats require all fields to be present. serde is for more than just json so even if you could argue that for json the field should just not be present (which I don't think is a thing one could claim), it might not work for other seralization formats.
This allows the models to be used from the server-side and makes testing easier.