Relations between inheritance trees breaks code generation

[papegaaij]

While trying to generate code for our project, I noticed some types got pruned which shouldn't have been pruned. Closer investigation showed that the inheritance chain for some of the types is broken. I've managed to produce a minimal testcase, which I will submit as a PR against current main. In the spec in this PR, you will see some types that are not used, but that's only because I removed the operations referencing those types. The operations were not needed to reproduce the issue, but the types were. The issue is present in at least C#, Go and Java, so it seems unrelated to the target language.

The result of the code generation should have been an inheritance chain with Linkable > GroupClassificationPrimer > GroupClassification. However, the primer class is missing, and strangely, the GroupClassification has the properties of the primer, not its own properties. In this example, this results in a GroupClassification extending Linkable and with only a name, not the description. All the other parts in the spec seem to be needed to reproduce this issue. For example, when you remove the property markers from GroupPrimer, which references ItemMarkers, the code is generated correctly. Both these types have nothing to do with GroupClassification, but still seem to influence its generation.

[baywet]

Thanks for submitting PR #2782 (and the other one before). I've been a bit overloaded after coming back from a long leave, but I'll take a look at both as soon as I can.

[papegaaij]

No problem. I think Kiota does a much better job at generating code from an OpenAPI spec than what I've seen from other projects. This one is just an unfortunate bug. With Kiota, we finally see a route to delivering high quality generated SDKs for our API.

[papegaaij]

I've done some debugging, and I think I may have found the location of the problem. In KiotaBuilder.CreateInheritedModelDeclaration, the class name for GroupClassificationPrimer is resolved incorrectly to GroupClassification. When stepping through the code, it flattens the allOfs to a list of 3. The first on correctly resolves to Linkable, but the second one gets a className == GroupClassification, which should have been GroupClassificationPrimer. This in turn causes the contents of GroupClassificationPrimer to be placed in the GroupClassification class and the actual GroupClassification class is skipped. When I correct the value of className, the test passes and it seems the code is generated correctly. I'll see if I can find the cause of the incorrect name.

[papegaaij]

The FlattenEmptyEntries call returns a schema for GroupClassificationPrimer without a Reference. I'm not sure if this is by design, but it seems to be the cause of the incorrect class name.

[baywet]

Thank you for looking into this further and sharing your progress.
I haven't been able to look into it myself yet due to other priorities.

Effectively we're flattening some schemas because they don't hold meaning. They are usually the result of a bad generation from certain services. Or they only carry some description, nullable information, etc...
This is to avoid generating empty types or too many types.

At the time we implemented that, the rules about which property to consider and which to ignore were not clearly defined and this was done in an exploratory way.

But we have an issue to address the consistency aspect in the near future. #2434

If you have additional questions to get further context don't hesitate.

[papegaaij]

Given a type hierarchy with 3 levels, the base class has a single schema. The intermediate class has a schema with an allOf with 2 parts (reference to super class and it's own properties). The subclass also has a schema with an allOf with 2 parts. This looks like this:

- BaseClass
- IntermediateClass
  - IntermediateRefToBase
  - IntermediateProps
- SubClass
  - SubClassRefToIntermediate
  - SubClassProps

The FlattenEmptyEntries returns a list with [IntermediateRefToBase, IntermediateProps, SubClassProps]. For the first schema it can resolve the type name from the reference. However, for the 2nd and 3rd it cannot, so it does a guess and it guesses wrong. Perhaps CreateInheritedModelDeclaration should search for the schema containing the subschema to resolve the reference.

For now, I've managed to find a workaround for this issue by giving the subschemas with the properties (IntermediateProps and SubClassProps) a title with the name of the type they belong to. This hints Kiota into selecting the correct class name. It's not clear though why this title attribute is used for this, as the JSON Schema specification seems to suggest the title is meant to be human readable.

[baywet]

Thanks for digging further. This is effectively what #2434 is meant to address.

[papegaaij]

Well, yes and no. At the moment I actually use the behavior described in #2434 to work around the issue with the name generation. So if that ticket is fixed, generation will be broken again for our spec. I do agree that Kiota should not use the title to generate a name, but it should generate predictable names in some way.

In #2745 I proposed a solution for that specific issue: preprocessing of the OpenAPI spec. That might work for this issue as well. Suppose you add a new step in the generator that takes the OpenAPI spec as it is read from file and produces a normalized and annotated OpenAPI spec. This step could merge allOf definitions in types, remove superfluous allOf definitions if needed, but also add extension attributes, like x-kiota-typename. This can then hold the name of the type to be used by the code generation. This can reduce the complexity of the code in the generation phase a lot and adding type information in such a preprocessing step is probably easier than doing it during the generation. Another benefit is that such attributes can be made available to users to override the default names generated by Kiota. A preprocessing step also is quite easy to test and the result can be inspected simply by dumping it back to a file.

Unfortunately, being mainly a Java developer, my knowledge of C# is limited and our schedule at the moment does not give me the room to both learn a new language and work on such an enhancement.

[baywet]

Let's explore this pre-processing step further. Besides normalizing the JSON schemas, what else would it do? Would you expose it as a separate command to users?

[papegaaij]

It could perform a lot of the name resolution and conversion that is now part of the code generation. The results of this could be stored in dedicated extension properties in the resulting OpenAPI. This could, for example solve many of the concerns raised in #2498. It would also solve another issue we are facing at the moment, where a $type and type property both get named Type and conflict. For the example in #2782 , the result could be:

components:
  schemas:
    group.GroupClassificationPrimer:
      x-kiota-extends: `Linkable`
      allOf:
      - '$ref': '#/components/schemas/Linkable'
      - required:
        - '$type'
        - name
        type: object
        properties:
          '$type':
            type: string
            x-kiota-propertyname: 'dtype'
          name:
            type: string
            x-kiota-propertyname: 'name'
        discriminator:
          propertyName: '$type'
          mapping:
            group.GroupClassification: '#/components/schemas/group.GroupClassification'
        x-kiota-typename: 'group.GroupClassificationPrimer'

These new properties x-kiota-typename and x-kiota-propertyname can then be used throughout the code generation. This then serves both as a way to give users a way to customize name generation (and possibly more, like inheritance) and at the same time simplify the generation code.

I would also expose this as a standalone command. The output will serve several different purposes. It allows users to see how Kiota interprets the OpenAPI spec. It can be used to normalize a spec (perhaps with the option to drop the x-kiota-* properties). It can be used to troubleshoot Kiota itself. And, it will also serve as a living documentation of the supported x-kiota-* properties.

[baywet]

Thanks for the additional context. This is an interesting suggestion. We've never had a pre-processing step in mind (besides maybe what OpenAPI.net and associated validation rules do) throughout the history of this project. More often than not, description quality/accuracy gets in the way of the generation work. Having a pre-processing step to remove the noise might be beneficial. I'm a bit worried about the refactoring cost at this point.
@darrelmiller: any opinion on the matter?