AllOf C# Inheritence

[pettijohn]

What are you generating using Kiota, clients or plugins?

API Client/SDK

In what context or format are you using Kiota?

Linux executable

Client library/SDK language

Csharp

Describe the bug

I am working with the Schwab API and their schema. I have published a repo to go along with this issue. https://github.com/pettijohn/KiotaIssue/

In the schema, there are a number of models that extend AccountsBaseInstrument using allOf.

Most of these generated C# classes extend AccountsBaseInstrument, as expected (AccountCashEquivalent, AccountFixedIncome, AccountOption, CollectiveInvestment generate as expected). Two do not - AccountEquity, AccountMutualFund. E.g.,

"AccountEquity": {
                "allOf": [
                    {
                        "$ref": "#/components/schemas/AccountsBaseInstrument"
                    }
                ]
            },

Reviewing the documentation, I think this is actually by design. If I understand this correctly: https://learn.microsoft.com/en-us/openapi/kiota/models#components-versus-inline-models AccountEquity and AccountMutualFund fall into the rule "Class/interface with properties from the referenced schema and without a parent type."

That's a bummer, because there are instances where I only care about properties on the base type and I want to be able to treat them all the same:

AccountsBaseInstrument? instrument = 
        position.Instrument.AccountCashEquivalent as AccountsBaseInstrument ??
        position.Instrument.AccountEquity as AccountsBaseInstrument ??
        position.Instrument.AccountFixedIncome as AccountsBaseInstrument ??
        position.Instrument.AccountMutualFund as AccountsBaseInstrument ??
        position.Instrument.AccountOption as AccountsBaseInstrument ??
        position.Instrument.CollectiveInvestment as AccountsBaseInstrument;
var s = instrument.Symbol;

To work around, I added an empty property to the classes to get it to generate inheriting from the base class.

"AccountEquity": {
                "allOf": [
                    {
                        "$ref": "#/components/schemas/AccountsBaseInstrument"
                    }
                ],
                "properties": {
                    "ignoreme": {
                        "type": "string"
                    }
                }
            },

Is there a better way? Or maybe this is a feature request - if other generated classes inherit from the base class, then so should all generated classes.

Expected behavior

Generated classes inherit from AccountsBaseInstrument

How to reproduce

See schema in initial commit and generate https://github.com/pettijohn/KiotaIssue/

Open API description file

No response

Kiota Version

1.16.0

Latest Kiota version known to work for scenario above?(Not required)

No response

Known Workarounds

No response

Configuration

Docker, WSL, Ubuntu 22.04, Windows 11

Debug output

Click to expand log

```

</details>


### Other information

_No response_

[baywet]

Hi @pettijohn
Thanks for using kiota and for reaching out.
To make sure I understand your concern correctly: your OAI description contains a schema named AccountEquity which before any change only contains a single allOf entry (no other properties). This results in that schema being "squashed" as it doesn't add any other meaning (besides maybe the type hierarchy). And you'd like the type to be projected anyway? is that correct?

If so, this is by design, in most scenarios allOf with single entries result from improper OAS description generation from the server-side tooling and was adding noise. This is why we decided to implement "squashing". Unless we get overwhelming feedback this is wrong, we're unlikely to change that at this point.

[pettijohn]

Mostly correct. The schema has SIX components that extend AccountsBaseInstrument. Four of them add additional properties and as such extend from the base class. Two of them do not add any additional properties and so get squashed. The squashing of some but not others means that they don't all extend the c# class AccountsBaseInstrument.

I certainly understand and agree with your rationale for squashing when there is no added value. This is maybe a different case because there's value in the polymorphism and being able to treat everything as an AccountsBaseInstrument.

This isn't my schema (again I'm getting it from Schwab). But maybe the solution is to modify the discriminator to reference the base class directly instead of through an abstraction that doesn't add any value. "EQUITY": "#/components/schemas/AccountEquity" could become EQUITY": "#/components/schemas/AccountsBaseInstrument" for instance.

AccountsInstrument:

"discriminator": {
                    "propertyName": "assetType",
                    "mapping": {
                        "CASH_EQUIVALENT": "#/components/schemas/AccountCashEquivalent",
                        "EQUITY": "#/components/schemas/AccountEquity",
                        "FIXED_INCOME": "#/components/schemas/AccountFixedIncome",
                        "MUTUAL_FUND": "#/components/schemas/AccountMutualFund",
                        "OPTION": "#/components/schemas/AccountOption",
                        "COLLECTIVE_INVESTMENT": "#/components/schemas/CollectiveInvestment"
                    }
                }

Here's a direct link to the open api schema: https://github.com/pettijohn/KiotaIssue/blob/95917e105b5e99533d164d43d12be52df71fe324/openapi3.json If you look at the latest version of the same file you can see how I added "ignorme" properties. I generated C# classes with this command (WSL, docker) https://github.com/pettijohn/KiotaIssue/blob/main/kiota-generate.sh

[baywet]

Thanks for the additional context here.
The difficulty lies in establishing clear rules for when to squash or not squash.
I guess we could add an exception which would avoid squashing a derived schema if the parent has a discriminator entry with it.
But that creates another issue: we default the discriminators when they are absent which would mean not squashing anything anymore.

[baywet]

My guess is that they added this additional empty type because they either want to enable type assertion / matching to be used as logic control flow. Or because they anticipate additional properties for that derived type in the future. Or both.

[baywet]

Would you be willing to look into the solution I have suggested and submit a pull request given some guidance?

[pettijohn]

Can you give me a code pointer to get me started on changing the squashing logic? I can try to look at it this weekend.

[baywet]

Great! The triage and merging logic is here.

kiota/src/Kiota.Builder/KiotaBuilder.cs

Line 1634 in 44d61a4

AddModelDeclarationIfDoesntExist(currentNode, operation, schema.MergeAllOfSchemaEntries([.. referencedSchemas], static x => x.Reference is null)!, className, shortestNamespace, CreateInheritedModelDeclaration(currentNode, referencedSchemas[0], operation, classNameSuffix, codeNamespace, isRequestBody, string.Empty)),

And the tests start here

kiota/tests/Kiota.Builder.Tests/KiotaBuilderTests.cs

Line 7773 in 22089d6

public async Task InheritanceWithAllOfWith3Parts3SchemaChildClass()

Let us know if you have other questions

[pettijohn]

Before I test this and submit a PR, can you please let me know if I'm doing about what you had in mind?

pettijohn@5f5374b

In the switch statement you referenced in CreateInheritedModelDeclaration, I added an additional parameter referencedByOneOf to indicate if we are generating this class off of a discriminator and added the appropriate cases.

I'm not sure if I'm determining if it was referenced by oneOf in the right/clean/optimal way. GetCodeTypeForMapping (which is called by GetDiscriminatorMappings) now passes a bool down the stack indicating that referencedByOneOf=true, particularly into CreateModelDeclarations so that we can decision in the schema.IsInherited branch.

I suspect there's a more elegant way to do that? Or maybe not and I should just add comments & tests.

[baywet]

I think this is a good start.
Thank you for sharing .
It's probably only addressing wonderful paths there are probably additional code paths out there, but we can start from here.
I would also name the additional perimeter differently because one off can be confused with oneof in JSON schema.
You can probably go ahead and open a pull request in the draft and we will iterate from here.

[pettijohn]

Appreciate the early feedback. I expect to have a busy week at work so it'll be a bit before I return to this. I'll make the changes you suggest and submit a PR for iterating.