Populate the discriminator property for derrived types.

greg-ww commented 2 months ago

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.

baywet commented 2 months ago

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

greg-ww commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

{
  "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 commented 2 months ago

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 commented 2 months ago

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

andrueastman commented 2 months ago

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"
      }
    }
  }
}

greg-ww commented 2 months ago

@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?

darrelmiller commented 2 months ago

@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.

MateuszMaleckiAnsys commented 1 month ago

I'm also having this issue, but I am using enum as a discriminator. If I were using string I would be able to add default value to the property in derived type to make it work, but for enums it is not possible because I will either end up assigning a string to an enum in the derived type constructor or assigning a different enum type, which will also fail.

The workaround proposed in https://github.com/microsoft/kiota/issues/4618#issuecomment-2126708631 (@andrueastman ) won't work unfortunately as it sets 'onlyValue' as a default value for every type and I would need to have 'onlyValue' as a default for 'A' type and 'alternativeValue' as a default for 'B' type.

baywet commented 1 month ago

Hey everyone, Thanks for the great conversation here. I want to be transparent with the fact that Andrew is on leave now, we don't know yet when he'll be back. In the meantime, I'll try to keep the momentum going so we get to a resolution.

One feature improvement we've already mentioned on this thread: const support. This depends on 3.1 support by OAI.net and integration of the changes here. Let's table that for later. (if one of you wants to create an issue specifically about that so we don't forget, please go ahead)

Another improvement we eluded to would be to "ignore the enum information when the property is a discriminator" so the deduplication logic doesn't kick in, and we consider it a "basic string value used for discrimination". Does that make sense?

I believe that if we address this later point, and assuming the property documents a default, we should address the initial ask for serialization.

Thoughts?

MateuszMaleckiAnsys commented 1 month ago

I would actually like the discriminator to keep being an enum. However I don't know if it is doable in OAS to asign a default value to it in a subtype in a way that makes sense. Maybe asign a string default value in OpenApi spec and parse string into enum in the constructor if property is an enum?

However this issue as a whole can also be addressed by resolving the discriminator properties during serialization (both for strings and enums) as mentioned above. Something along these lines:

baywet commented 1 month ago

supporting enums for discriminators: this should be possible with the serialization/deserialization code we have in place. Including with defaults. It'd just require more work overall.

setting the discriminator value in the parent serialization: I'm not sure we want to do that. Not only this is going to require type testing (not possible in some languages/overly complex), but also this should be unnecessary assuming the default value is set in each derived type.

Does that make sense?

For the people involved on this thread. From a purely functional perspective, what would be preferred? supporting enums for discriminator? or ignoring enums and always using strings?

MateuszMaleckiAnsys commented 1 month ago

Personally, I'd like to be able to use enums as discriminators.

greg-ww commented 1 month ago

supporting enums for discriminators: this should be possible with the serialization/deserialization code we have in place. Including with defaults. It'd just require more work overall.

setting the discriminator value in the parent serialization: I'm not sure we want to do that. Not only this is going to require type testing (not possible in some languages/overly complex), but also this should be unnecessary assuming the default value is set in each derived type.

Does that make sense?

For the people involved on this thread. From a purely functional perspective, what would be preferred? supporting enums for discriminator? or ignoring enums and always using strings?

The problem with putting the onus on the Open API spec to specify default values for the discriminators is that Kiota isn't the only client generator people might be using, assigning default values to enums might also break other clients who didn't think to cover that use case or who need their own contradictory work arounds.

If Kiota could handle it as part of serialization / deserialization it would avoid creating more hoops for the spec to jump through and improve compatibility with other generators, meaning no matter what other client generators our consumers use we always support Kiota.

In my opinion the goal should be to make it so we never have to add anything beyond the discriminator mapping to our spec for this.

greg-ww commented 1 month ago

On enums for discriminators I do not have strong opinions. I do think that an enum with only one value listed should be treated as equivalent to const though.

baywet commented 1 month ago

Thanks for the additional input. I think a first stopgap would be to "ignore" enum information if the property is a discriminator. This should unblock the original issue here. And we can later on see whether there's additional demand to support enums in discriminators. It'll also reduce the work that needs to be done on the topic.

As for setting the default for the discriminator property I believe this is already done by kiota today. There might be some side effects with the backing store though.

baywet commented 1 month ago

@greg-ww @MateuszMaleckiAnsys is one of you willing to submit a pull request for the first part of my last message?

microsoft / kiota