Incorrect merging of model classes

[andreaTP]

I'm not sure what is going on here (feel free to rephrase the title accordingly), but there is something wrong happening in a (not so)edge case. Here you have a minimal reproducer description:

openapi: 3.0.3
info:
  title: A bug reproducer
  version: v1
paths:
  "/one":
    patch:
      requestBody:
        content:
          application/json:
            schema:
              $ref: "#/components/schemas/ComponentTwo"
      responses:
        "200":
          description: OK
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/ComponentOne"
components:
  schemas:
    WithOne:
      type: object
      properties:
        one:
          type: string
    WithTwo:
      type: object
      properties:
        two:
          type: string
    WithThree:
      type: object
      properties:
        three:
          type: string
    ComponentOne:
      type: object
      allOf:
        - $ref: "#/components/schemas/WithOne"
    ComponentTwo:
      type: object
      allOf:
        - $ref: "#/components/schemas/WithOne"
        - $ref: "#/components/schemas/WithTwo"
        - $ref: "#/components/schemas/WithThree"

Generating code with Kiota we obtain (examples in C# for your convenience but applies to all).

expected:

public class ComponentOne : WithOne, IParsable

not expected:

public class ComponentTwo : WithOne, IParsable 

this is surprising, as it seems that ComponentTwo is being considered equal to ComponentOne for some reason. The expected output for ComponentTwo should be(code trimmed for clarity):

    public class ComponentTwo : IAdditionalDataHolder, IParsable 
    {
        public IDictionary<string, object> AdditionalData { get; set; }
        public string? One { get; set; }
        public string? Two { get; set; }
        public string? Three { get; set; }

If ComponentOne is not used ComponentTwo gets generated with the expected shape(it took forever to minimize 😅 ).

[darrelmiller]

Vincent is out this week on vacation. @andrueastman could you investigate?

[andrueastman]

Kiota is building an inheritance index here of a type and the types that depend on it from the allOf and using this as a mean of building the discriminator mapping.

When the WithOne type is created, it tries to build the discriminator mappings (therefore initialize its derived types in the codeDom which are ComponentOne and ComponentTwo) by calling GetCodeTypeForMapping https://github.com/microsoft/kiota/blob/4335b26876e9aa2cbdbd8cf0b189469212d60bd5/src/Kiota.Builder/KiotaBuilder.cs#L2020

However, when this is done, this is done by passing WithOne as the baseClass parameter (as this is being done from the context of the base class trying to create discriminator therefore assuming there are no other in between the inheritance tree).

To resolve this, the baseClass parameter could maybe be removed and the call to AddModelDeclarationIfDoesntExist will look up the schema and should the correct allOf entry as this is its responsibility.

https://github.com/microsoft/kiota/blob/4335b26876e9aa2cbdbd8cf0b189469212d60bd5/src/Kiota.Builder/KiotaBuilder.cs#L2030

[andrueastman]

@andreaTP Just to confirm this issue is related to the comment at https://github.com/microsoft/kiota/issues/4346#issuecomment-2020409106?

[andreaTP]

Just to confirm this issue is related to the comment at https://github.com/microsoft/kiota/issues/4346#issuecomment-2020409106?

I have no idea ...

[dhirajsb]

Why is Kiota using inheritance for allOf when the spec explicitly states that it's not allowed? From the spec:

allOf can not be used to "extend" a schema to add more details to it in the sense of object-oriented inheritance. Instances must independently be valid against "all of" the schemas in the allOf. See the section on Extending Closed Schemas for more information.

It's meant to be used to compose types, effectively copying the properties of the base allOf.

[baywet]

Why is Kiota using inheritance for allOf when the spec explicitly states that it's not allowed? From the spec:

allOf can not be used to "extend" a schema to add more details to it in the sense of object-oriented inheritance. Instances must independently be valid against "all of" the schemas in the allOf. See the section on Extending Closed Schemas for more information.

It's meant to be used to compose types, effectively copying the properties of the base allOf.

Unfortunately this is one of the instances where using a validation specification as an IDL shows limitations. Effectively kiota implements the status quo of "OAS chose to use JSON Schema, lots of OAS specs use allOf to model inheritance even though it wasn't designed for that, people expect that convention to work".