We should always capture additional properties

[heaths]

Currently, whether to capture additional properties in a model is up to the TypeSpec. No language currently captures additional properties by default even for extensible enums' base discriminants. This can lead to data loss if a service does not implement PATCH correctly or if they use PUT for updates because added fields will be missing. It should work correctly if PATCH for JSON merge+patch was implemented correctly.

We should consider always capturing additional properties for any model, or at least any DU (polymorphic model).

[jhendrixMSFT]

There is risk for a breaking change if the tsp is updated to explicitly define additional properties. Consider the following (this could be a base for polymorphic DU or not, doesn't really matter).

model Foo {
    name: string
}

We would emit this as follows.

pub struct Foo {
    pub additional_properties: Option<HashMap<String, Value>>,
    pub name: Option<String>,
}

Since no explicit type is defined for additional properties, we must default to azure_core::Value.

Later, the tsp is updated to explicitly define additional properties with a value of type string.

model Foo extends Record<string> {
    name: string
}

We would now emit this.

pub struct Foo {
    pub additional_properties: Option<HashMap<String, String>>,
    pub name: Option<String>,
}

Which would be a breaking change.

Granted, this is probably uncommon, but it's not impossible. Do we think the benefits outweigh the risk?

[jhendrixMSFT]

I happened to look at a tsp PR for a Go SDK breaking change and came across this comment.

Use of additionalProperties is not allowed for properties owned by the service. The only time its ok to use it is when the properties are pass thru (user defined) and not subject to any validations. Please make this is an array or provide an explanation for why you need this.

Which makes me think that, at least for ARM, that adding this everywhere might not be providing real value, i.e. we'd be adding it to models where it's not legal and would never be used.

I can somewhat see the value for base types in polymorphic DUs and could live with it there if we're ok with the risk of a breaking change (which might not be a real risk [for ARM] per the above comment from ARM).

[heaths]

Any service that uses a PUT to update models risks data loss, though. I've brought this up numerous times in the past with @KrzysztofCwalina when I was on the Azure SDK for .NET team. If a polymorphic type is returned, untouched, and send back, a lot of data was just unset. The REST API board recommends PATCH for create-or-update, but many older services (and perhaps some new services) don't always.

We could decide, for example, that if we were to add additional_properties: HashMap<String, azure_core::Valkue>, the value type will never change. A String is still a Value - you just have to do a little more work to get it, but that's idiomatic to Rust, especially to those used to dealing with serde_json.

[jhendrixMSFT]

I agree with the theory, I've just never seen this play out in practice (in the Go SDK).

At any rate, if we want to always add this to the "base" type in a polymorphic DU and hard code the value to azure_core::Value I can support that. But I don't think we should add this everywhere until we have a customer ask/evidence that this is a real issue.

[heaths]

But I don't think we should add this everywhere until we have a customer ask/evidence that this is a real issue.

I don't disagree with that. If anything, this tracking issue is just that: to discuss - across Azure SDK languages, even - why we as a team have never done this. That said, the fact no one to my knowledge has complains means it may be a non-issue. It's just that the potential for it worries me some.