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

Populate the discriminator property for derrived types. #4618

Open
greg-ww opened this issue May 7, 2024 · 12 comments
Open

Populate the discriminator property for derrived types. #4618

greg-ww opened this issue May 7, 2024 · 12 comments
Labels
Needs: Attention 👋 question Further information is requested type:question An issue that's a question
Milestone

Comments

@greg-ww
Copy link

greg-ww commented May 7, 2024

Is your feature request related to a problem? Please describe the problem.

I could be doing something wrong here but I couldn't find any documentation on sending heterogeneous collections in a post, only on receiving them. I have an endpoint that takes a list of derived types, the open API spec defines the discriminator property and the mappings for the derived types. The models for the derived types that Kiota generates have a Type property I can assign the correct value to but by default it is empty.

Client library/SDK language

Csharp

Describe the solution you'd like

Since the desired value for these types is constant and defined in the open API spec I don't understand why I need to set it when working with these models. Can it not just be hard coded to the correct value?

Additional context

It's possible there is something wrong with my open API spec but I couldn't find any confirmation in the documentation of what the desired behavior is for this use case is.

@greg-ww greg-ww added status:waiting-for-triage An issue that is yet to be reviewed or assigned type:feature New experience request labels May 7, 2024
@baywet
Copy link
Member

baywet commented May 7, 2024

Hi @greg-ww
Thanks for using kiota and for reaching out.
Could you please share a small reproduction OpenAPI description?

@baywet baywet added question Further information is requested Needs: Author Feedback status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close type:question An issue that's a question and removed status:waiting-for-triage An issue that is yet to be reviewed or assigned type:feature New experience request labels May 7, 2024
@baywet baywet added this to the Backlog milestone May 7, 2024
@greg-ww
Copy link
Author

greg-ww commented May 7, 2024

The request body takes an array of a base type, the base type has a discriminator mapping similar to below.

          "propertyName": "type",
          "mapping": {
            "derived1": "#/components/schemas/DerivedType1",
            "derived2": "#/components/schemas/DerivedType2",
            "derived3": "#/components/schemas/DerivedType3"
          }
        }

@greg-ww greg-ww closed this as completed May 7, 2024
@greg-ww greg-ww reopened this May 7, 2024
@greg-ww
Copy link
Author

greg-ww commented May 7, 2024

These values appear to be used in the generated code for deserializing the models but not when serializing them. I was wondering if this is an intentional design choice.

@andrueastman
Copy link
Member

Any chance you can share the schema of the derive type? Ideally, the type property should be set with a default for it to be set automatically.

    microsoft.graph.managedIOSLobApp:
      allOf:
        - $ref: '#/components/schemas/microsoft.graph.managedMobileLobApp'
        - title: managedIOSLobApp
          required:
            - '@odata.type'
          type: object
          properties:
....

....
            '@odata.type':
              type: string
              default: '#microsoft.graph.managedIOSLobApp'
          description: Contains properties and inherited properties for Managed iOS Line Of Business apps.
      x-ms-discriminator-value: '#microsoft.graph.managedIOSLobApp'

The code generated would then have it set in the constructor similar to
https://github.com/microsoftgraph/msgraph-sdk-dotnet/blob/26bb5d0c54555077046596b05ee15f6c61e05c5f/src/Microsoft.Graph/Generated/Models/ManagedIOSLobApp.cs#L93

@greg-ww
Copy link
Author

greg-ww commented May 9, 2024

I was kind of hoping this could be treated as a serialization concern in C#. When you deserialize a list of a base type you use the discriminator mapping to determine the derived type to use, when serializing to a list of a base type you set the discriminator value based on the mapping. Would save people having to add stuff to their Open API spec just to get the discriminator to work automatically when all the information needed is in the mapping.

@greg-ww
Copy link
Author

greg-ww commented May 9, 2024

I also found some other issues while looking for a work around for this. In .Net setting a default value works but to support type narrowing in TS we wanted the discriminator value to be hard coded on the derived types. (For context the typescript team I am supporting isn't using Kiota as their generator). Since OpenAPI.NET(https://github.com/microsoft/OpenAPI.NET) doesn't yet support const values I settled for defining the discriminator property as having an enum with only one type. This works well for the typescript clients giving them a hard coded value. However in the .Net client I generate with kiota the single value enum appears to have to effect at all, it would be great if this also hard coded the value. Should I make another issue for this request?

Additionally to work around this I tried adding both a default value to support the Kiota .Net client and an enum with only one value for the TS client, unfortunately this generates code that doesn't compile. Weirdly giving it a default value that matches the only value in the enum makes it try to assign an enum type to my string discriminator property. Should I make another issue for this also? I can provide simplified example open api specs in json that show case these two other issues.

@greg-ww
Copy link
Author

greg-ww commented May 9, 2024

image

{
  "openapi": "3.0.0",
  "info": {
    "title": "Polymorphic API",
    "version": "1.0.0"
  },
  "paths": {
    "/polymorphic-list": {
      "post": {
        "summary": "Accepts a list of polymorphic types and returns a list",
        "requestBody": {
          "content": {
            "application/json": {
              "schema": {
                "type": "array",
                "items": {
                  "$ref": "#/components/schemas/BaseType"
                }
              }
            }
          },
          "required": true
        },
        "responses": {
          "200": {
            "description": "Success",
            "content": {
              "application/json": {
                "schema": {
                  "type": "array",
                  "items": {
                    "$ref": "#/components/schemas/BaseType"
                  }
                }
              }
            }
          }
        }
      }
    }
  },
  "components": {
    "schemas": {
      "BaseType": {
        "type": "object",
        "properties": {
          "$type": {
            "type": "string"
          }
        },
        "required": ["$type"],
        "discriminator": {
          "propertyName": "$type",
          "mapping": {
            "A": "#/components/schemas/A",
            "B": "#/components/schemas/B"
          }
        }
      },
      "A": {
        "allOf": [
          { "$ref": "#/components/schemas/BaseType" },
          {
            "type": "object",
            "properties": {
              "$type": {
                "type": "string",
                "enum": ["onlyValue"],
                "default": "onlyValue"
              }
            },
            "required": ["$type"]
          }
        ]
      },
      "B": {
        "allOf": [
          { "$ref": "#/components/schemas/BaseType" },
          {
            "type": "object",
            "properties": {
              "$type": {
                "type": "string",
                "enum": ["onlyValue"],
                "default": "onlyValue"
              }
            },
            "required": ["$type"]
          }
        ]
      }
    }
  }
}

@andrueastman
Copy link
Member

I believe the issue here is that the base has a type that is a string while the derived instances have the enum declaration. This causes a conflict by setting set the default as an enum while the type information from the base is a string.

@greg-ww
Copy link
Author

greg-ww commented May 13, 2024

Am I mistaken in thinking that restricting the values for inherited properties in derived types using "enum" is valid in the Open API spec?

@baywet baywet removed the status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close label May 17, 2024
@andrueastman
Copy link
Member

As the property is defined in the base type and redefined in the derived type, the type of the base would be a string while the derived types would have the properties be inferred to be enums. Furthermore, since the schemas are inlined, Kiota would generate different enum types models for each derived type property which would conflict and cause an error.

You would need to have consistency in the typing down the inheritance by either.

  • Leaving the type as string without enum contraint anywhere. The property will be set as a string value default in the constructor
  • Have a shared component that is the enum and have it set the default so that the default value and type are the same accross the properties as below.
{
  "openapi": "3.0.0",
  "info": {
    "title": "Polymorphic API",
    "version": "1.0.0"
  },
  "paths": {
    "/polymorphic-list": {
      "post": {
        "summary": "Accepts a list of polymorphic types and returns a list",
        "requestBody": {
          "content": {
            "application/json": {
              "schema": {
                "type": "array",
                "items": {
                  "$ref": "#/components/schemas/BaseType"
                }
              }
            }
          },
          "required": true
        },
        "responses": {
          "200": {
            "description": "Success",
            "content": {
              "application/json": {
                "schema": {
                  "type": "array",
                  "items": {
                    "$ref": "#/components/schemas/BaseType"
                  }
                }
              }
            }
          }
        }
      }
    }
  },
  "components": {
    "schemas": {
      "BaseType": {
        "type": "object",
        "properties": {
          "$type": {
            "$ref": "#/components/schemas/TypeValues"
          }
        },
        "required": ["$type"],
        "discriminator": {
          "propertyName": "$type",
          "mapping": {
            "A": "#/components/schemas/A",
            "B": "#/components/schemas/B"
          }
        }
      },
      "A": {
        "allOf": [
          { "$ref": "#/components/schemas/BaseType" },
          {
            "type": "object",
            "properties": {
              "$type": {
                "$ref": "#/components/schemas/TypeValues"
              }
            },
            "required": ["$type"]
          }
        ]
      },
      "B": {
        "allOf": [
          { "$ref": "#/components/schemas/BaseType" },
          {
            "type": "object",
            "properties": {
              "$type": {
                "$ref": "#/components/schemas/TypeValues"
              }
            },
            "required": ["$type"]
          }
        ]
      },
      "TypeValues":{
        "type": "string",
        "enum": ["onlyValue", "alternativeValue"],
        "default": "onlyValue"
      }
    }
  }
}

@andrueastman andrueastman added status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close and removed Needs: Attention 👋 labels May 23, 2024
@greg-ww
Copy link
Author

greg-ww commented May 25, 2024

@andrueastman You seem to be explaining how Kiota does not support this use case, I am more interested in discussion of if it should support it.

Enums are not types in the OpenAPI spec, I do not believe there is anything inconsistent about the typing of my property in OpenAPI. I am under the impression that my spec is valid (although admittedly not designed with the sole focus of making Kiota generate compiling code).

If my spec is valid (in the eyes of OpenAPI) should Kiota be able to support it?

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels May 25, 2024
@darrelmiller
Copy link
Member

@greg-ww Thanks for raising this issue and providing the details. I have a few thoughts.

  • We have this exact scenario in Microsoft Graph, so Kiota should have a good support for it (I'm not saying it currently does).
  • As you mention OpenAPI.NET does not support const because we don't yet have support for OpenAPI 3.1. Once we release 3.1 support, we will be able to use const.
  • You are correct that enum is not a type specifier in JSON Schema, it is simply a constraint over the existing type. In languages that natively support enum types, we translate JSON Schemas with an enum constraint as a kind of type. However, this is just a convention.
  • We should explore special casing an enum constraint with one value as functionally equivalent to the JSON Schema const keyword. This will set us up well for supporting OpenAPI 3.1.
  • Defaulting const values when serializing sounds like a reasonable thing to do and would make for a good experience for developers creating derived types in a heterogenous collection.

@andrueastman What do you think about special casing enums with one value to be a const of the type defined in the schema? That would avoid the problem you describe of base classes redefining the type in derived classes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Attention 👋 question Further information is requested type:question An issue that's a question
Projects
Status: In Progress 🚧
Development

No branches or pull requests

4 participants