Remember unknown values in base classes for discriminated types

[heaths]

Just an idea from an internal discussion. We don't declare base classes for discriminated types abstract in case we need to fall back to creating something. In that case, should we store all otherwise unhandled properties in a general, private variable for round-tripping? Many different ways we could do this, even just an IDictionary<string, object> that could contain other such dictionaries for nested properties - similar to what what the JsonSerializer can do via the JsonExtensionDataAttribute.

[heaths]

To note, this may not be a practical concern if service owners don't add new discriminated types to existing api-versions. Currently, we intentionally do not support arbitrary api-versions (or shouldn't; I believe Java currently does) so as long as new versions of services correspond to new packages, we'd have to have regenerated the code.

/cc @tg-msft @KrzysztofCwalina

[heaths]

In discussing Azure/azure-sdk-for-net#9686, one idea is that we make the base classes (no matter how far down the hierarchy) abstract but then define an internal child class that basically just buckets the properties and sends them back. Users won't be able to instantiate it or the base class by design, but they are also protected from a service returning a new model - even under the same api-version - that the SDK doesn't yet handle.

[tg-msft]

To note, this may not be a practical concern if service owners don't add new discriminated types to existing api-versions.

This is a very common occurrence according to the scary stories @johanste tells around the campfire. And I get why they do it. They either have to return a new type or effectively lie about the resource when requested from an older API version. Both options are unpleasant, but at least returning the new resource gives the client a chance to work correctly.

We can't rely on updated versions of the (regenerated) client library to solve the problem. This should probably be supported on every code generated model - not just discriminated unions - because services may return new properties to old versions of the API for arbitrary resources. And it should probably have a common solution described in the guidelines and potentially exposed via Azure.Core with an explicitly implemented interface like IExtraProperties or something.

[johanste]

Not sure if it very common - but I've seen it on multiple occasions. The challenge is generally whenever a service adds a new discriminated type to a server persisted resource. The choice is now for the service to hide any resource that uses the new type, map it into something that an earlier client would know about (this is most often impossible) or hide the resource from old clients (this leads to potential data loss/quota confusion) when clients delete what they think are empty containers (the old client didn't see the new typed resources contained in the container).

[heaths]

I'd personally like an interface IExtraProperties { Dictionary<string, object> Properties { get; } } that everything implements explicitly per the discussion in #650.

Ted mentioned this in the linked bug. Given "Properties" is pretty common and other patterns I've seen, I would think AdditionalProperties might be safer.

...unless we intend for it to be an explicit interface implementation, in which case it doesn't matter.

[heaths]

Because service teams often add properties to existing versions (whether changes are made to swagger or not), should we actually be doing this for all models by default to avoid data loss?

[MiYanni]

Somewhat discussed offline: There already is an "additionalProperties": true swagger property that can be added to do most of the functionality described above. You can read about it in the Free-Form Object section here: https://swagger.io/docs/specification/data-models/data-types/#object

However, the abstract indication would be a very different mechanism that could be automatic. But, I'd need to investigate further to see what is involved in making that change.

[AlexGhiondea]

@heaths can you take point on this and figure out what other code generators are planning to do this (related to Search)? If this is something that all code generators support (or would like to support) we should do this.

[heaths]

@AlexGhiondea, only Java has the concept of abstract classes, though you could say TypeScript could use an interface instead. I can ask what other generators might want to support, but this really is more specific to .NET and Java, and was specifically from archboard feedback about not instantiating "useless" types - i.e., that most of the abstract base classes had no members other than ODataType.

[tg-msft]

This came up again today for Azure/azure-sdk#1892. //cc @kinelski @KrzysztofCwalina @johanste