Discriminator object refers to missing property

[andrueastman]

Latest update of the conversion library includes adding some discriminator mappings to the openApi document. Possibly related to changes fixing #233

Taking a look at the previous description of the apiAuthenticationConfigurationBase

    microsoft.graph.apiAuthenticationConfigurationBase:
      title: apiAuthenticationConfigurationBase
      type: object

And the new description

    microsoft.graph.apiAuthenticationConfigurationBase:
      title: apiAuthenticationConfigurationBase
      type: object
      discriminator:
        propertyName: '@odata.type'
        mapping:
          '#microsoft.graph.basicAuthentication': '#/components/schemas/microsoft.graph.basicAuthentication'
          '#microsoft.graph.clientCertificateAuthentication': '#/components/schemas/microsoft.graph.clientCertificateAuthentication'
          '#microsoft.graph.pkcs12Certificate': '#/components/schemas/microsoft.graph.pkcs12Certificate'

The discriminator mapping mentions the propertyName @odata.type but the property collection (this instance has no properties) does not contain the property @odata.type.

This leads to validation errors as described in microsoft/kiota#1668.

The conversion library should probably make sure the property exists before adding the mapping.

[andrueastman]

According to https://spec.openapis.org/oas/v3.1.0#fixed-fields-20,

The discriminator object is legal only when using one of the composite keywords oneOf, anyOf, allOf.

[baywet]

related

The conversion mechanism should add the @odata.type field to the required field list in the type the field is defined.

Also related #240.

Here the spec contradicts itself by providing the following example where Pet does not have any/all/one of but has a discriminator node. I do think we should ignore this directive from the spec as if you follow it, the only way to implement inheritance + discriminator + mapping would be to have the discriminator field defined on each derived type AND have the mapping entry containing the derived type it's defined on, which would be both redundant, and make for a complex guessing game when looking at the base type. What do you think @darrelmiller ?

components:
  schemas:
    Pet:
      type: object
      required:
      - petType
      properties:
        petType:
          type: string
      discriminator:
        propertyName: petType
        mapping:
          dog: Dog
    Cat:
      allOf:
      - $ref: '#/components/schemas/Pet'
      - type: object
        # all other properties specific to a `Cat`
        properties:
          name:
            type: string
   Dog:
      allOf:
      - $ref: '#/components/schemas/Pet'
      - type: object
        # all other properties specific to a `Dog`
        properties:
          bark:
            type: string
    Lizard:
      allOf:
      - $ref: '#/components/schemas/Pet'
      - type: object
        # all other properties specific to a `Lizard`
        properties:
          lovesRocks:
            type: boolean

[andrueastman]

I do think we should ignore this directive from the spec as if you follow it, the only way to implement inheritance + discriminator + mapping would be to have the discriminator field defined on each derived type AND have the mapping entry containing the derived type it's defined on, which would be both redundant, and make for a complex guessing game when looking at the base type.

+1

[baywet]

So to recap for implementers to have clarity on the changes required by this issue:

• Some base types are missing the odata.type property when the discriminator is added, the lib should walk the inheritance tree to make sure the property is present and add it if not
• The odata type property should be declared as required on the schema it is being declared

(and the OpenAPI lib will be fixed to properly validate the inheritance structure instead of looking only at the current schema)

[baywet]

Additionally, after some more thinking, could we also take that opportunity to set a default value for the odata.type property to the actual odata type? i.e. #microsoft.graph.entity.
This would enable kiota to initialize this property at runtime and send it to the service.