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

Multiple identical classes are generated for a component schema with allOf inheritance #4191

Closed
bkoelman opened this issue Feb 18, 2024 · 14 comments · Fixed by #4381, #4694 or #4723
Closed
Assignees
Labels
generator Issues or improvements relater to generation capabilities. help wanted Issue caused by core project dependency modules or library type:bug A broken experience WIP
Milestone

Comments

@bkoelman
Copy link

bkoelman commented Feb 18, 2024

When using inheritance using allOf, Kiota generates multiple identical classes for the same component schema. As a result, the types are incompatible when trying to consume the C# code generated by Kiota.

I've tried to create a minimal repro, but the OAS is still quite large. The full file is provided below. At a high level, the structure contains the following base component schema:

"dataInResponse": {
  "required": [
    "id",
    "type"
  ],
  "type": "object",
  "properties": {
    "type": {
      "minLength": 1,
      "type": "string"
    },
    "id": {
      "minLength": 1,
      "type": "string"
    }
  },
  "additionalProperties": false,
  "discriminator": {
    "propertyName": "type",
    "mapping": {
      "tags": "#/components/schemas/tagDataInResponse",
      "todoItems": "#/components/schemas/todoItemDataInResponse"
    }
  },
  "x-abstract": true
}

with derived schemas tagDataInResponse and todoItemDataInResponse, ie:

"todoItemDataInResponse": {
  "allOf": [
    {
      "$ref": "#/components/schemas/dataInResponse"
    },
    {
      "type": "object",
      "properties": {
        "attributes": {
          "allOf": [
            {
              "$ref": "#/components/schemas/todoItemAttributesInResponse"
            }
          ]
        },
        "relationships": {
          "allOf": [
            {
              "$ref": "#/components/schemas/todoItemRelationshipsInResponse"
            }
          ]
        }
      },
      "additionalProperties": false
    }
  ],
  "additionalProperties": false
}

These schemas are used from two GET endpoints, one returning a collection and the other a singular item. The response schema in both cases contains a data property (derived schema reference, or an array of that) and an included property (array of base schema reference).

The first endpoint (paths./api/todoItems.get) uses the following response schema:

"todoItemCollectionResponseDocument": {
  "required": [
    "data"
  ],
  "type": "object",
  "properties": {
    "data": {
      "type": "array",
      "items": {
        "$ref": "#/components/schemas/todoItemDataInResponse"
      }
    },
    "included": {
      "type": "array",
      "items": {
        "$ref": "#/components/schemas/dataInResponse"
      }
    }
  },
  "additionalProperties": false
}

And the second endpoint (paths./api/todoItems/{id}.get) uses the following schema:

"todoItemPrimaryResponseDocument": {
  "required": [
    "data"
  ],
  "type": "object",
  "properties": {
    "data": {
      "allOf": [
        {
          "$ref": "#/components/schemas/todoItemDataInResponse"
        }
      ]
    },
    "included": {
      "type": "array",
      "items": {
        "$ref": "#/components/schemas/dataInResponse"
      }
    }
  },
  "additionalProperties": false
}

I would have expected Kiota to generate the base class DataInResponse, with the two derived classes TodoItemDataInResponse and TagDataInResponse. What happens is that Kiota also generates the derived class TodoItems, whose content is identical to TodoItemDataInResponse. The unexpected TodoItems class is only used by the singular endpoint.

As a result, it's not possible to define a method that takes a parameter of type TodoItemDataInResponse and is called with the response from both endpoints. I would have expected to be able to write:

TodoItemPrimaryResponseDocument? getSingleResponse = await client.Api.TodoItems["1"].GetAsync();
PrintTodoItem(getSingleResponse!.Data!, getSingleResponse.Included);

TodoItemCollectionResponseDocument? getMultiResponse = await client.Api.TodoItems.GetAsync();
foreach (TodoItemDataInResponse todoItem in getMultiResponse!.Data!)
{
    PrintTodoItem(todoItem, getMultiResponse.Included);
}

static void PrintTodoItem(TodoItemDataInResponse todoItem, ICollection<DataInResponse>? included)
{
    // ...
}

Instead, the code for method PrintTodoItem needs to be duplicated, because the types are incompatible:

TodoItemPrimaryResponseDocument? getSingleResponse = await client.Api.TodoItems["1"].GetAsync();
PrintTodoItem1(getSingleResponse!.Data!, getSingleResponse.Included);

TodoItemCollectionResponseDocument? getMultiResponse = await client.Api.TodoItems.GetAsync();
foreach (TodoItemDataInResponse todoItem in getMultiResponse!.Data!)
{
    PrintTodoItem2(todoItem, getMultiResponse.Included);
}

static void PrintTodoItem1(TodoItems todoItem, ICollection<DataInResponse>? included)
{
    // ...
}

static void PrintTodoItem2(TodoItemDataInResponse todoItem, ICollection<DataInResponse>? included)
{
    // ...
}

Additionally, because there are duplicate types, it's unclear for consumers of the API what to upcast/type-check for when looping over the entries in included.

When using NSwag to generate the client, types appear as expected, which makes me believe the OAS is correct.

Expand to view the full OAS file
{
  "openapi": "3.0.1",
  "info": {
    "title": "JsonApiDotNetCoreExample",
    "version": "1.0"
  },
  "servers": [
    {
      "url": "https://localhost:44340"
    }
  ],
  "paths": {
    "/api/todoItems": {
      "get": {
        "tags": [
          "todoItems"
        ],
        "summary": "Retrieves a collection of todoItems.",
        "operationId": "getTodoItemCollection",
        "parameters": [
          {
            "name": "query",
            "in": "query",
            "description": "For syntax, see the documentation for the [`include`](https://www.jsonapi.net/usage/reading/including-relationships.html)/[`filter`](https://www.jsonapi.net/usage/reading/filtering.html)/[`sort`](https://www.jsonapi.net/usage/reading/sorting.html)/[`page`](https://www.jsonapi.net/usage/reading/pagination.html)/[`fields`](https://www.jsonapi.net/usage/reading/sparse-fieldset-selection.html) query string parameters.",
            "schema": {
              "type": "object",
              "additionalProperties": {
                "type": "string",
                "nullable": true
              },
              "example": ""
            }
          }
        ],
        "responses": {
          "200": {
            "description": "Successfully returns the found todoItems, or an empty array if none were found.",
            "content": {
              "application/vnd.api+json": {
                "schema": {
                  "$ref": "#/components/schemas/todoItemCollectionResponseDocument"
                }
              }
            }
          },
          "400": {
            "description": "The query string is invalid."
          }
        }
      }
    },
    "/api/todoItems/{id}": {
      "get": {
        "tags": [
          "todoItems"
        ],
        "summary": "Retrieves an individual todoItem by its identifier.",
        "operationId": "getTodoItem",
        "parameters": [
          {
            "name": "id",
            "in": "path",
            "description": "The identifier of the todoItem to retrieve.",
            "required": true,
            "schema": {
              "type": "string"
            }
          },
          {
            "name": "query",
            "in": "query",
            "description": "For syntax, see the documentation for the [`include`](https://www.jsonapi.net/usage/reading/including-relationships.html)/[`filter`](https://www.jsonapi.net/usage/reading/filtering.html)/[`sort`](https://www.jsonapi.net/usage/reading/sorting.html)/[`page`](https://www.jsonapi.net/usage/reading/pagination.html)/[`fields`](https://www.jsonapi.net/usage/reading/sparse-fieldset-selection.html) query string parameters.",
            "schema": {
              "type": "object",
              "additionalProperties": {
                "type": "string",
                "nullable": true
              },
              "example": ""
            }
          }
        ],
        "responses": {
          "200": {
            "description": "Successfully returns the found todoItem.",
            "content": {
              "application/vnd.api+json": {
                "schema": {
                  "$ref": "#/components/schemas/todoItemPrimaryResponseDocument"
                }
              }
            }
          },
          "400": {
            "description": "The query string is invalid."
          },
          "404": {
            "description": "The todoItem does not exist."
          }
        }
      }
    }
  },
  "components": {
    "schemas": {
      "dataInResponse": {
        "required": [
          "id",
          "type"
        ],
        "type": "object",
        "properties": {
          "type": {
            "minLength": 1,
            "type": "string"
          },
          "id": {
            "minLength": 1,
            "type": "string"
          }
        },
        "additionalProperties": false,
        "discriminator": {
          "propertyName": "type",
          "mapping": {
            "tags": "#/components/schemas/tagDataInResponse",
            "todoItems": "#/components/schemas/todoItemDataInResponse"
          }
        },
        "x-abstract": true
      },
      "tagAttributesInResponse": {
        "type": "object",
        "properties": {
          "name": {
            "minLength": 1,
            "type": "string"
          }
        },
        "additionalProperties": false
      },
      "tagDataInResponse": {
        "allOf": [
          {
            "$ref": "#/components/schemas/dataInResponse"
          },
          {
            "type": "object",
            "properties": {
              "attributes": {
                "allOf": [
                  {
                    "$ref": "#/components/schemas/tagAttributesInResponse"
                  }
                ]
              },
              "relationships": {
                "allOf": [
                  {
                    "$ref": "#/components/schemas/tagRelationshipsInResponse"
                  }
                ]
              }
            },
            "additionalProperties": false
          }
        ],
        "additionalProperties": false
      },
      "tagIdentifier": {
        "required": [
          "id",
          "type"
        ],
        "type": "object",
        "properties": {
          "type": {
            "$ref": "#/components/schemas/tagResourceType"
          },
          "id": {
            "minLength": 1,
            "type": "string"
          }
        },
        "additionalProperties": false
      },
      "tagRelationshipsInResponse": {
        "type": "object",
        "properties": {
          "todoItems": {
            "allOf": [
              {
                "$ref": "#/components/schemas/toManyTodoItemInResponse"
              }
            ]
          }
        },
        "additionalProperties": false
      },
      "tagResourceType": {
        "enum": [
          "tags"
        ],
        "type": "string",
        "additionalProperties": false
      },
      "toManyTagInResponse": {
        "type": "object",
        "properties": {
          "data": {
            "type": "array",
            "items": {
              "$ref": "#/components/schemas/tagIdentifier"
            }
          }
        },
        "additionalProperties": false
      },
      "toManyTodoItemInResponse": {
        "type": "object",
        "properties": {
          "data": {
            "type": "array",
            "items": {
              "$ref": "#/components/schemas/todoItemIdentifier"
            }
          }
        },
        "additionalProperties": false
      },
      "todoItemAttributesInResponse": {
        "type": "object",
        "properties": {
          "description": {
            "type": "string"
          }
        },
        "additionalProperties": false
      },
      "todoItemCollectionResponseDocument": {
        "required": [
          "data"
        ],
        "type": "object",
        "properties": {
          "data": {
            "type": "array",
            "items": {
              "$ref": "#/components/schemas/todoItemDataInResponse"
            }
          },
          "included": {
            "type": "array",
            "items": {
              "$ref": "#/components/schemas/dataInResponse"
            }
          }
        },
        "additionalProperties": false
      },
      "todoItemDataInResponse": {
        "allOf": [
          {
            "$ref": "#/components/schemas/dataInResponse"
          },
          {
            "type": "object",
            "properties": {
              "attributes": {
                "allOf": [
                  {
                    "$ref": "#/components/schemas/todoItemAttributesInResponse"
                  }
                ]
              },
              "relationships": {
                "allOf": [
                  {
                    "$ref": "#/components/schemas/todoItemRelationshipsInResponse"
                  }
                ]
              }
            },
            "additionalProperties": false
          }
        ],
        "additionalProperties": false
      },
      "todoItemIdentifier": {
        "required": [
          "id",
          "type"
        ],
        "type": "object",
        "properties": {
          "type": {
            "$ref": "#/components/schemas/todoItemResourceType"
          },
          "id": {
            "minLength": 1,
            "type": "string"
          }
        },
        "additionalProperties": false
      },
      "todoItemPrimaryResponseDocument": {
        "required": [
          "data"
        ],
        "type": "object",
        "properties": {
          "data": {
            "allOf": [
              {
                "$ref": "#/components/schemas/todoItemDataInResponse"
              }
            ]
          },
          "included": {
            "type": "array",
            "items": {
              "$ref": "#/components/schemas/dataInResponse"
            }
          }
        },
        "additionalProperties": false
      },
      "todoItemRelationshipsInResponse": {
        "type": "object",
        "properties": {
          "tags": {
            "allOf": [
              {
                "$ref": "#/components/schemas/toManyTagInResponse"
              }
            ]
          }
        },
        "additionalProperties": false
      },
      "todoItemResourceType": {
        "enum": [
          "todoItems"
        ],
        "type": "string",
        "additionalProperties": false
      }
    }
  }
}

I'm using the next command to generate the client code:

dotnet kiota generate --language CSharp --class-name ApiClient --namespace-name GeneratedClient --output ./GeneratedClient --clean-output --clear-cache --openapi ..\JsonApiDotNetCoreExample\GeneratedSwagger\JsonApiDotNetCoreExample.json

Used versions:

  • .NET 8 SDK v8.0.201 on Windows 11 23H2
  • microsoft.openapi.kiota global tool v1.11.1 (latest available)
  • Latest available NuGet package versions, as of today:
    <PackageReference Include="Microsoft.Kiota.Abstractions" Version="1.7.9" />
    <PackageReference Include="Microsoft.Kiota.Http.HttpClientLibrary" Version="1.3.6" />
    <PackageReference Include="Microsoft.Kiota.Serialization.Form" Version="1.1.3" />
    <PackageReference Include="Microsoft.Kiota.Serialization.Json" Version="1.1.5" />
    <PackageReference Include="Microsoft.Kiota.Serialization.Multipart" Version="1.1.2" />
    <PackageReference Include="Microsoft.Kiota.Serialization.Text" Version="1.1.2" />
    
@andrueastman
Copy link
Member

Thanks for raising this @bkoelman

I believe the issue is that second endpoint (paths./api/todoItems/{id}.get) uses an inline schema for its response type with allOf. As schema is declared inline, kiota will interpret this as a different type and generate the inline model.

https://learn.microsoft.com/en-us/openapi/kiota/models#components-versus-inline-models

As the allOf is only of one type, any chance you get a different result if you modify the description to directly reference the todoItemDataInResponse without the allOf?

@bkoelman
Copy link
Author

bkoelman commented Feb 20, 2024

Thanks for investigating. If I patch the OAS manually as you suggested, the problem goes away.

We're using Swashbuckle's UseAllOfToExtendReferenceSchemas because the OpenAPI 3.0 spec doesn't allow the use of nullable and description without it (see here). So we get this output as a result.

Shouldn't Kiota just treat a single-element allOf the same as what's inside it? I think I've seen Kiota do that in other places already.

@andrueastman
Copy link
Member

Shouldn't Kiota just treat a single-element allOf the same as what's inside it? I think I've seen Kiota do that in other places already.

I believe, you are correct. It probably should.

To resolve this, I suspect need to check what happens around here when the model declarations are being created.

if (schema.IsReferencedSchema() && !string.IsNullOrEmpty(typeNameForInlineSchema))

@bkoelman Would by any chance be willing to submit a PR for a failing test in this scenario to help out. I think the test would look very similar to this one.

public async Task DoesNotIntroduceIntermediateTypesForMeaninglessProperties(string additionalInformation)

@andrueastman andrueastman added generator Issues or improvements relater to generation capabilities. Needs: Author Feedback and removed needs more information Needs: Attention 👋 labels Feb 21, 2024
@bkoelman
Copy link
Author

I wish there were more hours in a day. It's taken me weeks before finding time to create the minimal repro, which was needed to know if we were doing something wrong. I'd like to contribute, but don't expect to be able to anytime soon, unfortunately.

@baywet baywet added this to the Backlog milestone Feb 22, 2024
@baywet baywet added type:bug A broken experience help wanted Issue caused by core project dependency modules or library and removed Needs: Attention 👋 labels Feb 22, 2024
@SilasKenneth SilasKenneth self-assigned this Apr 24, 2024
@SilasKenneth SilasKenneth moved this from Todo 📃 to In Progress 🚧 in Kiota Apr 24, 2024
@baywet
Copy link
Member

baywet commented May 17, 2024

I just tested this with the latest preview from yesterday. This is fixed by #4381
Thanks for your patience!
Closing!

@baywet baywet closed this as completed May 17, 2024
@github-project-automation github-project-automation bot moved this from In Progress 🚧 to Done ✔️ in Kiota May 17, 2024
@baywet baywet modified the milestones: Backlog, Kiota v1.15 May 17, 2024
@baywet baywet assigned baywet and unassigned SilasKenneth May 17, 2024
@bkoelman
Copy link
Author

Nope, it is still broken, although the set of redundant types being generated is slightly different.

I wonder how this was tested. Running the repro OAS against kiota v1.15.0-preview.202405160001 generates the following types:

image

@bkoelman
Copy link
Author

bkoelman commented May 18, 2024

And why generate type names with an underscore and lowercase, which goes against .NET naming conventions? I've seen this happen in other places too, pretty ugly.

@andrueastman
Copy link
Member

Re-opening this one for now to investigate

@andrueastman andrueastman reopened this May 20, 2024
@github-project-automation github-project-automation bot moved this from Done ✔️ to In Progress 🚧 in Kiota May 20, 2024
@andrueastman andrueastman assigned andrueastman and unassigned baywet May 21, 2024
@github-project-automation github-project-automation bot moved this from In Progress 🚧 to Done ✔️ in Kiota May 21, 2024
@bkoelman
Copy link
Author

bkoelman commented May 22, 2024

Thanks for trying to fix this. Unfortunately, it's still broken. I've tested with the binary downloaded from the windows-latest build artefact.

Duplicate types are not being generated anymore, but now the declared type of the Data property has changed from derived type to base type. Pasting the C# code from the repro steps shows the following compile error:

image

The declared type of TodoItemPrimaryResponseDocument.Data should be TodoItemsDataInResponse, but it changed to DataInResponse. Here's the related OAS fragment from repro steps:

"todoItemPrimaryResponseDocument": {
  "required": [
    "data"
  ],
  "type": "object",
  "properties": {
    "data": {
      "allOf": [
        {
          "$ref": "#/components/schemas/todoItemDataInResponse"
        }
      ]
    },
    "included": {
      "type": "array",
      "items": {
        "$ref": "#/components/schemas/dataInResponse"
      }
    }
  },
  "additionalProperties": false
}

Edit: Corrected the wrong OAS snippet.

@bkoelman
Copy link
Author

@andrueastman Can you please revert the changes from the last PR, #4694? Our code doesn't compile anymore with it, whereas before it was just annoying that we had to duplicate logic. And can you reopen this issue until a fix is available?

@andrueastman andrueastman reopened this May 28, 2024
@github-project-automation github-project-automation bot moved this from Done ✔️ to In Progress 🚧 in Kiota May 28, 2024
@andrueastman
Copy link
Member

Re-opening this one to investigate

@andrueastman
Copy link
Member

This should also be resolved with changes in #4723

@github-project-automation github-project-automation bot moved this from In Progress 🚧 to Done ✔️ in Kiota May 28, 2024
@bkoelman
Copy link
Author

Confirming this works properly now. Thanks!

@andrueastman
Copy link
Member

Thanks for confirming @bkoelman

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
generator Issues or improvements relater to generation capabilities. help wanted Issue caused by core project dependency modules or library type:bug A broken experience WIP
Projects
Archived in project
4 participants