microsoft / kiota

OpenAPI based HTTP Client code generator
https://aka.ms/kiota/docs
MIT License
2.83k stars 194 forks source link

Enhance polymorphic deserialization to handle multi-level inheritance #2432

Open darrelmiller opened 1 year ago

darrelmiller commented 1 year ago

Currently the deserializer cannot instantiate types that are not direct descendants of the declared return type. Instead it will default to the base type.

This was a performance compromise because in Microsoft Graph all types derive from a base type "entity" and if any operation returned an entity it would be expensive to search the entire tree of types looking for the correct type.

One solution to consider in the future is to create a global mapping table that is aware that Graph has globally unique discriminators so that it can reduce the duplication of mapping tables. We would then move the type mappings for the types into request builders and property deserializers.

baywet commented 1 year ago

@darrelmiller should we move this one to v3?

da1910 commented 2 months ago

I would be keen to see this fixed in a way which doesn't add additional hidden requirements on a schema - if we have a nested hierarchy of types with discriminators at each level the currently generated code is so very close to working out of the box.

Perhaps a switch if the main internal use of the generated code is for an API where every type is a subclass of the same base? "useGlobalDiscriminatorTable" or something? That way schemas which can tolerate the performance impact of recursive descent can avoid the need for undocumented globally unique discriminators?

baywet commented 2 months ago

Thanks for your interest in this. We unlikely to add a switch for that aspect, it's too specific. We more likely to ship this as a breaking change (major version required). This is because for a same type, multiple discriminator information can be present depending on whether it's part of an inheritance structure, union type, or a specific endpoint has restrictions. (e.g. a rollingVehicules endpoint that'd return car, bus, train but not plane or boat)

da1910 commented 2 months ago

Thanks for your interest in this. We unlikely to add a switch for that aspect, it's too specific. We more likely to ship this as a breaking change (major version required). This is because for a same type, multiple discriminator information can be present depending on whether it's part of an inheritance structure, union type, or a specific endpoint has restrictions. (e.g. a rollingVehicules endpoint that'd return car, bus, train but not plane or boat)

That's also a reasonable fix, the impression I got from the comments above is that it was deliberately not done initially because the design of the Graph API was such that it would add a reasonable overhead because every single type would need to traverse t the same object hierarchy.

baywet commented 2 months ago

You are correct, that's the initial design and limitations trade-off we made. Effectively, at the time, every way we looked at it, we ended up with either bad performance or circular dependencies. We then decided to take on this limitation in order to ship something that worked in time. And so far this decision has worked well both for Microsoft Graph and other APIs. In the case of Microsoft Graph, we have a couple of edge cases (notably education scenarios as far as I can remember), but no broad impact to the customers.