[bug] Kiota generate is not idempotent

[andreaTP]

Issue discovered in #2438 .

The generator is currently emitting different results in different runs.
We expect the generator to be fully idempotent so that given the same input the same output is produced even in the case of edge cases.

More specifically, even applying all the requested workarounds the output of the generator vary for different runs and is reproducible by running this script:
https://gist.github.com/andreaTP/08180453508dc80dc1ae7953efe0dc20

[baywet]

can you provide more specific details about what's changing from run to run?

[andreaTP]

Sure use this OpenAPI document spec.zip, with the latest kiota (compiled from main, but it's reproducible even with older versions).
And a command like:

kiota generate --openapi /connector_mgmt.yaml --output generated-sources/kiota --language Java --class-name ApiClient --namespace-name com.openshift.cloud.api.connector --clean-output false --clear-cache false --log-level Warning

I can observe various spurious class declarations such as:

public class ConnectorNamespace extends VersionMetadata_collections implements Parsable {

where, sometimes, the expected output is generated instead:

public class ConnectorNamespace extends ObjectReference implements Parsable {

or again:

public class ConnectorType extends VersionMetadata_collections implements Parsable {

instead of (but sometimes it works ...):

public class ConnectorType extends ObjectReference implements Parsable {

all in all the major issue seems to be related to an incorrectly computed hierarchy, but I haven't narrowed down the issue any further.

[andreaTP]

@baywet I started anyhow looking into this issue, but I'm afraid I would need your assistance, sorry for bothering you.

I'm starting to be convinced that the issue is caused by a colliding Key in the InheritanceIndex/DiscriminatorMappings:

• Identified that the issue was caused by VersionMetadata_collections
• Noticed that the inheritance of the collection type was likely to be not correct (at least not as expected)
• To calculate the InheritanceIndex Key we are using the Reference.Id that "seems like" to be colliding with the base type in case of (synthetic) primitive Array collections

changing this part of the code:

kiota/src/Kiota.Builder/Extensions/OpenApiSchemaExtensions.cs

Line 194 in 68bd708

return GetAllInheritanceSchemaReferences(schema.Reference.Id, inheritanceIndex)

to take into account only primitive types or by adding the type to the Key I can see progress (at least the build is reproducible and compiles in Java).
Unfortunately, the changes I'm attempting are somehow breaking the correct inheritance calculation.

[baywet]

No worries, hopefully we get to the bottom of this together and unblock you.
Thanks for all the details, and sorry for the delays in the replies, pretty swamped here.
@andrueastman authored #2446 which we just merged and I believe is somewhat related. Can you try again with the latest from main and see if there's a change in behaviour please?
Also to double check we're not just iterating over #2438 can you update the document you've provided so allOf are only 1 reference entry + 1 inline entry?

A few remarks on the version metadata aspect:

components:
  schemas:
VersionMetadata:
      allOf: #this is a canonical inheritance pattern and should be fine
        - $ref: "#/components/schemas/ObjectReference"
        - type: object
          example:
            kind: "APIVersion"
            id: "v1"
            href: "/api/connector_mgmt/v1"
            collections:
              - id: "kafkas"
                href: "/api/connector_mgmt/v1/kafka_connectors"
                kind: "ConnectorList"
          properties:
            collections:
              type: array
              items:
                allOf: #this is a bit weird but we have strategies in place to flatten empty all/any/one ofs you might want to try to flatten it in the document
                  - $ref: "#/components/schemas/ObjectReference"

I hope all of this helps, sorry I can't spend more time on this at the moment.

[andreaTP]

Thanks a lot for getting back @baywet !

@andrueastman authored #2446 which we just merged and I believe is somewhat related. Can you try again with the latest from main and see if there's a change in behaviour please?

Yes, I noticed and tested this change this morning and sadly it doesn't fix this issue.

Also to double check we're not just iterating over #2438 can you update the document you've provided so allOf are only 1 reference entry + 1 inline entry?

Right, let me do it first 👍

this is a bit weird

You are right, possibly the peculiarity of the document we are looking at is that it contains mutually recursive models, depending on the pattern used to visit the tree this can have consequences.

[andreaTP]

Ok, I spent the time to bisect this issue, this is the minimal reproducer that can be used:

openapi: 3.0.0
info:
  title: Connector Management API
  version: 0.1.0
paths:
  "/api/connector_mgmt/v1":
    get:
      operationId: getVersionMetadata
      summary: Returns the version metadata
      description: Returns the version metadata
      responses:
        "200":
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/VersionMetadata'
          description: Version metadata
  "/api/connector_mgmt/v1/kafka_connector_types":
    get:
      operationId: getConnectorTypes
      summary: Returns a list of connector types
      description: Returns a list of connector types
      responses:
        "200":
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/ConnectorType"
          description: A list of connector types
components:
  schemas:
    ObjectReference:
      type: object
      properties:
        id:
          type: string
    VersionMetadata:
      allOf:
        - $ref: "#/components/schemas/ObjectReference"
        - type: object
          properties:
            collections:
              type: array
              items:
                allOf:
                - $ref: "#/components/schemas/ObjectReference"
    ConnectorType:
      description: Represents a connector type supported by the API
      required:
        - name
      allOf:
        - $ref: "#/components/schemas/ObjectReference"
        - type: object
          properties:
            name:
              description: Name of the connector type.
              type: string

This spec is going to proce about half of the times:

public class ConnectorType extends VersionMetadata_collections implements Parsable {

and:

public class ConnectorType extends ObjectReference implements Parsable {

[andreaTP]

The problem seems to be caused by:

              items:
                allOf:
                - $ref: "#/components/schemas/ObjectReference"

in VersionMetadata, using a type: string in the same position apparently resolves the issue.

(sorry for being chatty on this Issue, I'm trying to dump the updates as I get them)

[baywet]

No, this is great, we can follow the progress this way.
If you keep only a $ref (remove the all of) does that "solve" the behaviour?

[andreaTP]

If you keep only a $ref (remove the all of) does that "solve" the behaviour?

yes, it solves it!

Tentatively opened #2452 , as it seems that "ordering" inside the inheritance index does matter and can be leveraged to fix the issue, but it doesn't cover all the cases, at this point I'm open to alternatives!

[baywet]

I think the following method is getting derailed again by the allOf node.

kiota/src/Kiota.Builder/KiotaBuilder.cs

Line 1324 in a9a0dd1

private CodeTypeBase CreateModelDeclarations(OpenApiUrlTreeNode currentNode, OpenApiSchema schema, OpenApiOperation? operation, CodeElement parentElement, string suffixForInlineSchema, OpenApiResponse? response = default, string typeNameForInlineSchema = "", bool isRequestBody = false)

Because it relies on that method, which doesn't tolerate "empty nodes".

kiota/src/Kiota.Builder/Extensions/OpenApiSchemaExtensions.cs

Line 54 in a9a0dd1

public static bool IsArray(this OpenApiSchema? schema)

Fixing that aspect will probably have a better result than trying to order things.

[andreaTP]

Thanks for the hint @baywet , yes, you are probably right, I have attempted various things with not much result.
At the moment, I'm starting to think that the inner anonymous property is, somehow polluting the index of the discriminator, but I'm not really familiar with this section of the codebase 🙁

i.e.: I can prove that the value extracted by GetDiscriminatorMappings is the incorrect one.

is getting derailed again by the allOf node

do you have any references I can lookup to follow the history?

[baywet]

unfortunately there's not much documentation on that aspect, it's mostly trial and error based on a corpus of API descriptions. And that specific edge case didn't come across during our testing. You might be able to find helpful information by digging through the issues on the repo, but that's about it.
My suggestion is to fix #2438 first, it's needed for your description and it'll get us closer to an exhaustive implementation. Once we have that implementing some rationale to flatten the array case when needed should be easier.
But if you want to switch priorities around, checkout the flatten if empty method.

kiota/src/Kiota.Builder/Extensions/OpenApiSchemaExtensions.cs

Line 135 in a9a0dd1

internal static IEnumerable<OpenApiSchema> FlattenEmptyEntries(this IEnumerable<OpenApiSchema> schemas, Func<OpenApiSchema, IList<OpenApiSchema>> subsequentGetter, int? maxDepth = default)

[andreaTP]

Thanks a lot for the quick turn around.

Your proposal of fixing/verifying #2438 first sounds good to me.
In all transparency I have just submitted a workaround for our specs and I'm currently pretty swamped on other priorities too.

I'll try to find time or someone else to look into, but I cannot guarantee much at this point.
As I have a workaround now, feel free to decrease the priority of this issue accordingly.

[baywet]

While working on #2676 I found out something: renaming code elements has a high potential of introducing fork trees is not done right. Those fork trees mean that sometimes multiple writers fire for the same file. And they don't necessarily always fire in the same order since the code dom is built in a parallels way (no order guaranteed).
We have renames at a bunch of places that are not relying on the code block method dedicated to this and should update the code. Ideally we'd end up in a state where the Name setter is init private.

Additionally @papegaaij comment about locking and duplication of work in the builder made me realize two things:

• yes we're probably missing a couple of locks a couple of places (see original comment)
• we do have some code elements that are stored other places than the code block inner elements concurrent dictionary, (usings, implements, etc...) we might want to streamline that as well.

[papegaaij]

There seem to be 2 major contributors to inconsistencies between runs:

• Race conditions in the multi threading, often resulting in incorrect code.
• Different correct paths to get to the same point but with slightly different input.

The first category can be suppressed by now by setting the KIOTA_GENERATION:MAXDEGREEOFPARALLELISM to 1. These should be fixed, but I think we should leave them out for this ticket.

The second category results from the fact that Kiota can end up generating the same type via different routes. This is expected and fine, but the outcome should always be identical. #2861 and #2863 already address some of the issues mentioned in this ticket, but there could be more. With these changes (and parallelism switched off), I was able to consistently generate the same code over many runs for the Graph API with the Go target. Parallelism does increase the impact of these issues by making the path that generates a type first less predictable.

You can get a quick overview of files that have changed between runs by calculating sha1sums:

find graph/go -not -name ".*" -type f -print0 | sort -z | xargs -0 sha1sum

[sebastienlevert]

@andreaTP recent changes probably have fixed this issue. Can you re-validate idempotency is still an issue on generation? Thanks!

[andreaTP]

Sure, I'm on holiday those days, when I'm back I'll give it a shot!
Meanwhile, probably, @papegaaij can weigh into this discussion.

[papegaaij]

The tests I've ran all produce stable output now, but it could be there are still some issues remaining. I think it would be best if @andreaTP tests this with his cases.

[andreaTP]

Sorry for the delay, I'm now working on verifying this, and I'll get back soon with the results.

[andreaTP]

Sorry again for the delay in giving feedback on this one, I'm afraid the issues are not completely solved just yet.
In #3158 I built a simple script to generate code from a definition 2 times consecutively and compare the results, running it in CI shows that this process is not idempotent in a number of cases.

From the branch is pretty easy to reproduce by using, for example:

./it/compare-generation.ps1 -descriptionUrl apisguru::stripe.com -language java -preserveOutput

consistently fails on my machine and in CI.
cc. @baywet