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

Discriminator mapping should list all derived types #240

Closed
andrueastman opened this issue Jun 27, 2022 · 2 comments · Fixed by #256
Closed

Discriminator mapping should list all derived types #240

andrueastman opened this issue Jun 27, 2022 · 2 comments · Fixed by #256
Assignees
Labels
priority:p2 Medium. Generally has a work-around and a smaller sub-set of customers is affected. SLA <=30 days type:bug A broken experience
Milestone

Comments

@andrueastman
Copy link
Member

The mapping should list all derived types, not just those that are direct descendants of the base type. Any derived type could be passed or returned and we need to be able to identify the corresponding schema of that type.

Originally posted by @darrelmiller in #219 (comment)

@andrueastman
Copy link
Member Author

Taking a look at the openApi description of the MobileApp type we have a number of types listed in the discriminator mapping.

    microsoft.graph.mobileApp:
      allOf:
        - $ref: '#/components/schemas/microsoft.graph.entity'
        - title: mobileApp
          type: object
          properties:
....

....
          description: An abstract class containing the base properties for Intune mobile apps.
          discriminator:
            propertyName: '@odata.type'
            mapping:
              '#microsoft.graph.androidForWorkApp': '#/components/schemas/microsoft.graph.androidForWorkApp'
              '#microsoft.graph.mobileLobApp': '#/components/schemas/microsoft.graph.mobileLobApp'
              '#microsoft.graph.androidManagedStoreApp': '#/components/schemas/microsoft.graph.androidManagedStoreApp'
              '#microsoft.graph.androidStoreApp': '#/components/schemas/microsoft.graph.androidStoreApp'
              '#microsoft.graph.iosiPadOSWebClip': '#/components/schemas/microsoft.graph.iosiPadOSWebClip'
              '#microsoft.graph.iosStoreApp': '#/components/schemas/microsoft.graph.iosStoreApp'
              '#microsoft.graph.iosVppApp': '#/components/schemas/microsoft.graph.iosVppApp'
              '#microsoft.graph.macOSMdatpApp': '#/components/schemas/microsoft.graph.macOSMdatpApp'
              '#microsoft.graph.macOSMicrosoftEdgeApp': '#/components/schemas/microsoft.graph.macOSMicrosoftEdgeApp'
              '#microsoft.graph.macOSOfficeSuiteApp': '#/components/schemas/microsoft.graph.macOSOfficeSuiteApp'
              '#microsoft.graph.macOsVppApp': '#/components/schemas/microsoft.graph.macOsVppApp'
              '#microsoft.graph.managedApp': '#/components/schemas/microsoft.graph.managedApp'
              '#microsoft.graph.microsoftStoreForBusinessApp': '#/components/schemas/microsoft.graph.microsoftStoreForBusinessApp'
              '#microsoft.graph.officeSuiteApp': '#/components/schemas/microsoft.graph.officeSuiteApp'
              '#microsoft.graph.webApp': '#/components/schemas/microsoft.graph.webApp'
              '#microsoft.graph.windowsMicrosoftEdgeApp': '#/components/schemas/microsoft.graph.windowsMicrosoftEdgeApp'
              '#microsoft.graph.windowsPhone81StoreApp': '#/components/schemas/microsoft.graph.windowsPhone81StoreApp'
              '#microsoft.graph.windowsStoreApp': '#/components/schemas/microsoft.graph.windowsStoreApp'
              '#microsoft.graph.windowsWebApp': '#/components/schemas/microsoft.graph.windowsWebApp'

Taking a look at the openApi description of the derived type of MobileLobApp type we have a number of types listed in the discriminator mapping.

    microsoft.graph.mobileLobApp:
      allOf:
        - $ref: '#/components/schemas/microsoft.graph.mobileApp'
        - title: mobileLobApp
          type: object
          properties:

....

....
          description: An abstract base class containing properties for all mobile line of business apps.
          discriminator:
            propertyName: '@odata.type'
            mapping:
              '#microsoft.graph.androidLobApp': '#/components/schemas/microsoft.graph.androidLobApp'
              '#microsoft.graph.iosLobApp': '#/components/schemas/microsoft.graph.iosLobApp'
              '#microsoft.graph.macOSDmgApp': '#/components/schemas/microsoft.graph.macOSDmgApp'
              '#microsoft.graph.macOSLobApp': '#/components/schemas/microsoft.graph.macOSLobApp'
              '#microsoft.graph.win32LobApp': '#/components/schemas/microsoft.graph.win32LobApp'
              '#microsoft.graph.windowsAppX': '#/components/schemas/microsoft.graph.windowsAppX'
              '#microsoft.graph.windowsMobileMSI': '#/components/schemas/microsoft.graph.windowsMobileMSI'
              '#microsoft.graph.windowsPhone81AppX': '#/components/schemas/microsoft.graph.windowsPhone81AppX'
              '#microsoft.graph.windowsPhoneXAP': '#/components/schemas/microsoft.graph.windowsPhoneXAP'
              '#microsoft.graph.windowsUniversalAppX': '#/components/schemas/microsoft.graph.windowsUniversalAppX'

As the MobileLobApp is a derived type of MobileApp the mappings in the parent should also show up in the parent. This is because, an API marked to return the parent (MobileApp) could return the children in the MobileLobApp discriminator.

@andrueastman andrueastman changed the title The mapping should list all derived types, not just those that are direct descendants of the base type. Any derived type could be passed or returned and we need to be able to identify the corresponding schema of that type. Discriminator mapping should list all derived types Jun 27, 2022
@baywet baywet added type:bug A broken experience priority:p3 Nice to have. Customer impact is very minimal labels Jun 28, 2022
@baywet
Copy link
Member

baywet commented Jun 28, 2022

Thanks for reporting this. I do think it makes sense to "duplicate the information" to cover more cases, I'm a bit worried about circular dependencies (TypeScript), but we'll see if we get in any kind of trouble.

Thinking process

Here is the thinking process I went through to get to this conclusion.

There's something that's not clear from the spec to me: is recursion allowed?

I.E. if in the example they provide, I also have different types of dogs (fox terrier, labradoodle, pug), (being defined as allOf dog + one additional property), does that mean the different types of dogs MUST be listed in the discriminator mapping of Pet? That would surely conflict / be redundant in the case of implicit mapping.

The reason I'm bringing this up is because we could rebuild the createFromDiscriminator method to be recursive instead of bringing so much redundant information, but it'd add quite some complexity in the kiota generated code, and we'd move from a o(1) time complexity to o(n) (n types).
And because of the added complexity, I think we should go the more explicit route.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:p2 Medium. Generally has a work-around and a smaller sub-set of customers is affected. SLA <=30 days type:bug A broken experience
Projects
None yet
3 participants