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

adds support for type discriminator during deserialization #1174

Merged
merged 68 commits into from
Mar 2, 2022

Conversation

baywet
Copy link
Member

@baywet baywet commented Feb 8, 2022

Summary

fixes #648

What I thought would be a rather short PR ended up being much larger than anticipated. The background story is that because of the type system in Go, I add to introduce interfaces generation support in Kiota CodeDOM. Which led to a whole bunch of refactoring so we don't have to duplicate things like import statements, refiners logic, writing methods prototypes etc...
Because of the TypeScript limitations on static methods, I also had to introduce global functions and do a bunch of refactoring.
This PR is adding new capabilities to the Kiota engine, which is overall a good thing for Kiota to support additional scenarios and languages. Apologies to the reviewers, this is going to be large.

To illustrate what this does, let's take the example of /groups/id/members which returns a collection of directory objects. User, Service Principal, Group and many other types derive from directory object. Before this change, everything was deserialized to a directory object, and the additional properties available on derived types (like userPrincipalName for user) would end up in the additional data manager.
After that change, and once the conversion lib releases, the SDK will use the newly introduced factories for each model types to read in the odata.type (contains values like #microsoft.graph.user) property and instantiate the derived type when possible. Making pattern matching possible in the client code and access to derived types properties easier.

Generation diff

microsoft/kiota-samples#513

using this description (see outlook item)
microsoftgraph/msgraph-sdk-powershell#1060

Global todo

  • move the discriminator mappings property from code class to code method
  • introduce a new factory method type
  • add a common refiner to add usings for discriminator mappings
  • add a common refiner to add usings for factory methods in request executors
  • check that code block end is not adding unecessary closings for php/ruby/java/ts/csharp/python
  • unit tests for interface cloning refiners
  • update conceptual documentation
  • ping php and python teams to reflect the updates
  • update the ruby checklist to reflect the updates
  • changelog entry

CSharp todo

  • update parse node interface to take the new factories as parameters
  • update deserializer method gen to pass the factory for the properties
  • remove the new constraint on generic types for request adapter
  • add factory to request adapter as parameter
  • update request executor to pass the factory + update unit tests
  • bump abstractions, http and serialization packages
  • implement factory method generation + unit tests
  • remove the get object value that relies on new generic type constraint in parse node
  • rename the get error value method into get object value
  • use refiner method to add reference to types in discriminator mapping
  • implement using the factory + discriminator property name in getObjectValue

Go todo

  • update parse node interface to take the new factories as parameters
  • update deserializer method gen to pass the factory for the properties
  • add factory to request adapter as parameter
  • update request executor to pass the factory + update unit tests
  • bump abstractions, http and serialization packages
  • implement factory method generation + unit tests
  • use refiner method to add reference to types in discriminator mapping
  • implement using the factory + discriminator property name in getObjectValue

Java todo

  • update parse node interface to take the new factories as parameters
  • update deserializer method gen to pass the factory for the properties
  • add factory to request adapter as parameter
  • update request executor to pass the factory + update unit tests
  • bump abstractions, http and serialization packages
  • implement factory method generation + unit tests
  • use refiner method to add reference to types in discriminator mapping
  • implement using the factory + discriminator property name in getObjectValue

TypeScript todo

  • update parse node interface to take the new factories as parameters
  • update deserializer method gen to pass the factory for the properties
  • add factory to request adapter as parameter
  • update request executor to pass the factory + update unit tests
  • bump abstractions, http and serialization packages
  • implement factory method generation (read the discriminator value from parse node, switch case) + unit tests
  • use refiner method to add reference to types in discriminator mapping
  • implement using the factory + discriminator property name in getObjectValue

@baywet baywet added this to the TypeWriter Replacement milestone Feb 8, 2022
@baywet baywet added Go generator Issues or improvements relater to generation capabilities. Csharp Pull requests that update .net code TypeScript Pull requests that update Javascript code Java labels Feb 8, 2022
@baywet baywet self-assigned this Feb 8, 2022
@baywet baywet marked this pull request as draft February 8, 2022 21:27
@baywet baywet changed the base branch from main to feature/error-description February 9, 2022 00:47
Base automatically changed from feature/error-description to main February 10, 2022 18:51
@baywet baywet force-pushed the feature/discriminator branch from 521e342 to a31206c Compare February 10, 2022 19:47
@baywet

This comment was marked as outdated.

@baywet baywet force-pushed the feature/discriminator branch from 44aeb12 to d828141 Compare February 16, 2022 17:03
@nikithauc
Copy link
Contributor

running into an issue with Go where the type system for structs doesn't allow for up casting of structs (because it's composition over inheritance). Here is the roadmap to solve it:

  1. interfaces for all the models with inheritance structure
  2. all the models implement their respective interfaces
  3. properties of objects/collection of objects are interfaces
  4. the deserializer infers to parent interface
  5. the client does a type inferance
  6. put the interfaces in their dedicated package structures

Can you elaborate on put the interfaces in their dedicated package structures ? How would that look ? Would this be some pattern that we apply for typescript when we use model interfaces?

@baywet
Copy link
Member Author

baywet commented Feb 18, 2022

@nikithauc I'm still working with @darrelmiller to understand how much this is a requirement. But the gist of it is that interfaces for models would be in a sub namespace like so

  • mainnamespace -> contains the api client and the request builders
  • mainnamespace.models -> contains the model classes
  • mainnamespace.interfaces -> contains the model interfaces

Of course the infrastructure I'm laying out would leave that as a choice for each language (whether or not we have interfaces in this language and where they are placed).
Hopefully this clarifies the ongoing work here.

@darrelmiller
Copy link
Member

darrelmiller commented Feb 18, 2022

From conversation with Vincent, decision is that we put interfaces and classes all in the same models folders.
We should use interfaces for request payload and responses payload in chained API.

Signed-off-by: Vincent Biret <vibiret@microsoft.com>
@baywet baywet force-pushed the feature/discriminator branch from e7e2ef5 to 52f73e9 Compare February 23, 2022 19:10
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
@baywet baywet marked this pull request as ready for review February 23, 2022 20:34
@baywet baywet requested a review from andrueastman February 23, 2022 20:34
@baywet
Copy link
Member Author

baywet commented Feb 23, 2022

@andrueastman this is ready for review, thanks for your help during the design time
@nikithauc @ramsessanchez @jobala if you have time to look at the languages you contribute to, it'd be greatly appreciated
@jasonjoh if you have time to look at the markdown modifications, it'd be appreciated as well.

I'm aiming to merge this this in by Friday so we can start testing that in Go/Dotnet/TS previews as the newer conversion lib releases.

docs/extending/models.md Outdated Show resolved Hide resolved
@nikithauc
Copy link
Contributor

From conversation with Vincent, decision is that we put interfaces and classes all in the same models folders. We should use interfaces for request payload and responses payload in chained API.

TS does not append an I- to the interfaces. In cases like that, how would the class names and interface names for the same model be conflict free?

@nikithauc
Copy link
Contributor

If we fix the problem with circular dependency, will we still need the new CreateDiscriminator classes right?

Co-authored-by: Jason Johnston <jasonjoh@users.noreply.github.com>
@baywet baywet requested a review from jasonjoh February 24, 2022 12:42
@baywet
Copy link
Member Author

baywet commented Feb 24, 2022

@nikithauc

TS does not append an I- to the interfaces. In cases like that, how would the class names and interface names for the same model be conflict free?

This PR still adds most of the infrastructure needed for that change, the only missing piece would be to create the interfaces into a different namespace/package. (which I reverted after my convo with Darrel)

If we fix the problem with circular dependency, will we still need the new CreateDiscriminator classes right?

They are functions but yes, this part would still be needed after models classes inheritance is broken up and interfaces are introduced (like Go).

jasonjoh
jasonjoh previously approved these changes Feb 24, 2022
Copy link
Member

@andrueastman andrueastman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼 On the reorganization of the CodeDOM

Otherwise just a few comments from me.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 2, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

88.0% 88.0% Coverage
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Csharp Pull requests that update .net code generator Issues or improvements relater to generation capabilities. Go Java TypeScript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for discriminator and type downcast during deserialization
5 participants