kiota icon indicating copy to clipboard operation
kiota copied to clipboard

Type references to camel-case interface name where an underscore interface name is used

Open Nkmol opened this issue 10 months ago • 8 comments

Kiota version: 1.12.0 Language: Go

Issue

A generated model is referencing wrongly to a created interface:

type Base_commit struct {
    Object
    hash *string
    parents []BaseCommitable // The problematic reference
}

The interface it should be referencing to:

type Base_commitable interface {
    Objectable
    i878a80d2330e89d26896388a3f487eef27b0a0e6c010c493bf80be1452208f91.Parsable
    GetHash()(*string)
    GetParents()([]BaseCommitable)
    SetHash(value *string)()
    SetParents(value []BaseCommitable)()
}

How to reproduce:

Minimal Open API:

{
  "openapi": "3.0.0",
  "info": {
    "title": "Error self-reference",
    "description": "Model interface is confused with underscore interface name and camelCase interface naming",
    "version": "1.0"
  },
  "servers": [
    {
      "url": "http://localhost/api"
    }
  ],
  "paths": {
    "/commit": {
      "get": {
        "tags": [
          "Commits"
        ],
        "description": "test",
        "summary": "test summary",
        "responses": {
          "200": {
            "description": "The commit object",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/commit"
                }
              }
            }
          }
        }
      }
    }
  },
  "components": {
    "schemas": {
      "commit": {
        "allOf": [
          {
            "$ref": "#/components/schemas/base_commit"
          },
          {
            "type": "object",
            "title": "Commit",
            "description": "A repository commit object.",
            "properties": {
              "repository": {
                "type": "string"
              },
              "participants": {
                "type": "string"
              }
            }
          }
        ]
      },
      "base_commit": {
        "allOf": [
          {
            "$ref": "#/components/schemas/object"
          },
          {
            "type": "object",
            "title": "Base Commit",
            "description": "The common base type for both repository and snippet commits.",
            "properties": {
              "hash": {
                "type": "string",
                "pattern": "[0-9a-f]{7,}?"
              },
              "parents": {
                "type": "array",
                "items": {
                  "$ref": "#/components/schemas/base_commit"
                },
                "minItems": 0
              }
            }
          }
        ]
      },
      "object": {
        "type": "object",
        "description": "Base type for most resource objects. It defines the common `type` element that identifies an object's type. It also identifies the element as Swagger's `discriminator`.",
        "properties": {
          "type": {
            "type": "string"
          }
        },
        "required": [
          "type"
        ],
        "additionalProperties": true,
        "discriminator": {
          "propertyName": "type"
        }
      }
    }
  }
}

Command used:

kiota generate --openapi test.json --language go --namespace-name bitbucket/client --clean-output --output client

Further notes

This issue does not occur when I directly use the commit_base in the /get definition.

Nkmol avatar Apr 03 '24 11:04 Nkmol

Thanks for raising this @Nkmol

Any chance you can try building a client in a different language and confirm if it builds a client that compiles? This could be related to https://github.com/microsoft/kiota/pull/4381

andrueastman avatar Apr 05 '24 10:04 andrueastman

@andrueastman Thank you for the fast response. I was able to get the main branch working, and tested this with a modified launch.json in VSCode. "Go" still shows the same issue, as expected, other results:

TypeScript (Different issue) Does show the same error, but another issue where object referenced is used, whereas the Object name is expected (I suspect because of the reserved word).

export interface Base_commit extends object, Parsable {
    /**
     * The hash property
     */
    hash?: string;
    /**
     * The parents property
     */
    parents?: BaseCommit[];
}
/**
 * The common base type for both repository and snippet commits.
 */
export interface BaseCommit extends object, Parsable {
    /**
     * The hash property
     */
    hash?: string;
    /**
     * The parents property
     */
    parents?: BaseCommit[];
}

Java (Works) Seems to work.

/**
 * The common base type for both repository and snippet commits.
 */
@jakarta.annotation.Generated("com.microsoft.kiota")
public class BaseCommit extends Object implements Parsable {
    /**
     * The hash property
     */
    private String hash;
    /**
     * The parents property
     */
    private java.util.List<BaseCommit> parents;
    /**
     * Instantiates a new {@link BaseCommit} and sets the default values.
     */
    public BaseCommit() {
        super();
    }
...
}

CSharp (Works) Seems to work

/// <summary>
    /// The common base type for both repository and snippet commits.
    /// </summary>
    public class Base_commit : ObjectObject, IParsable 
    {
        ...
        public List<BaseCommit>? Parents { get; set; }
#nullable restore
#else
        public List<BaseCommit> Parents { get; set; }
#endif
        ...
    }

/// <summary>
    /// The common base type for both repository and snippet commits.
    /// </summary>
    public class BaseCommit : ObjectObject, IParsable 
    {
        ...
        public List<BaseCommit>? Parents { get; set; }
#nullable restore
#else
        public List<BaseCommit> Parents { get; set; }
#endif
        ...
    }

Stopped testing, I believe we can conclude this is something specific to the Go writer? Let me know if more info is needed.

Nkmol avatar Apr 05 '24 13:04 Nkmol

I believe we can conclude this is something specific to the Go writer?

Taking a look at the examples, I would expect only one model generated for these scenarios, i.e BaseCommit or Base_commit but not both. @Nkmol Any chance you can confirm the java sample also generated two models?

I suspect the interface is only converted from the class for one of the Go scenarios causing the build error but the root of the error looks like its related to the note.

This issue does not occur when I directly use the commit_base in the /get definition.

andrueastman avatar Apr 12 '24 08:04 andrueastman

@andrueastman Appreciate the feedback!

I re-checked, and there are indeed no double models generated for Java: Screenshot 2024-04-12 at 11 59 18

BaseCommit.java

package bitbucket/client.models;

import com.microsoft.kiota.serialization.Parsable;
import com.microsoft.kiota.serialization.ParseNode;
import com.microsoft.kiota.serialization.SerializationWriter;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
/**
 * The common base type for both repository and snippet commits.
 */
@jakarta.annotation.Generated("com.microsoft.kiota")
public class BaseCommit extends Object implements Parsable {
    /**
     * The hash property
     */
    private String hash;
    /**
     * The parents property
     */
    private java.util.List<BaseCommit> parents;
    /**
     * Instantiates a new {@link BaseCommit} and sets the default values.
     */
    public BaseCommit() {
        super();
    }
    /**
     * Creates a new instance of the appropriate class based on discriminator value
     * @param parseNode The parse node to use to read the discriminator value and create the object
     * @return a {@link BaseCommit}
     */
    @jakarta.annotation.Nonnull
    public static BaseCommit createFromDiscriminatorValue(@jakarta.annotation.Nonnull final ParseNode parseNode) {
        Objects.requireNonNull(parseNode);
        return new BaseCommit();
    }
    /**
     * The deserialization information for the current model
     * @return a {@link Map<String, java.util.function.Consumer<ParseNode>>}
     */
    @jakarta.annotation.Nonnull
    public Map<String, java.util.function.Consumer<ParseNode>> getFieldDeserializers() {
        final HashMap<String, java.util.function.Consumer<ParseNode>> deserializerMap = new HashMap<String, java.util.function.Consumer<ParseNode>>(super.getFieldDeserializers());
        deserializerMap.put("hash", (n) -> { this.setHash(n.getStringValue()); });
        deserializerMap.put("parents", (n) -> { this.setParents(n.getCollectionOfObjectValues(BaseCommit::createFromDiscriminatorValue)); });
        return deserializerMap;
    }
    /**
     * Gets the hash property value. The hash property
     * @return a {@link String}
     */
    @jakarta.annotation.Nullable
    public String getHash() {
        return this.hash;
    }
    /**
     * Gets the parents property value. The parents property
     * @return a {@link java.util.List<BaseCommit>}
     */
    @jakarta.annotation.Nullable
    public java.util.List<BaseCommit> getParents() {
        return this.parents;
    }
    /**
     * Serializes information the current object
     * @param writer Serialization writer to use to serialize this model
     */
    public void serialize(@jakarta.annotation.Nonnull final SerializationWriter writer) {
        Objects.requireNonNull(writer);
        super.serialize(writer);
        writer.writeStringValue("hash", this.getHash());
        writer.writeCollectionOfObjectValues("parents", this.getParents());
    }
    /**
     * Sets the hash property value. The hash property
     * @param value Value to set for the hash property.
     */
    public void setHash(@jakarta.annotation.Nullable final String value) {
        this.hash = value;
    }
    /**
     * Sets the parents property value. The parents property
     * @param value Value to set for the parents property.
     */
    public void setParents(@jakarta.annotation.Nullable final java.util.List<BaseCommit> value) {
        this.parents = value;
    }
}

Nkmol avatar Apr 12 '24 10:04 Nkmol

It'd be interesting to see whether this method is the culprit. Any change you can step through the code @Nkmol ?

baywet avatar Apr 17 '24 14:04 baywet

It'd be interesting to see whether this method is the culprit. Any change you can step through the code @Nkmol ?

Thanks for the reply! I was looking at the part where this "-able" notation was created, and this seems the place :) I am happy to have a look, but I am very unfamiliar with the philosophy and code, so will likely post my findings here.

Looking closer, I can see the CopyClassAsInterface is called twice with the BaseCommit model. Once as {CodeClass} Name [string]: "BaseCommit" and once as {CodeClass} Name [string]: "base_commit" (in this order). The latter is called through the inheritance copy of Commit (which explain why this issue does not happen when directly using "base_commit"). The baseClass is set through modelClass.StartBlock.Inherits?.TypeDefinition is CodeClass baseClass.

I am not sure about the mechanics and meanings of each code entity, but could the latler "base_commit" call overwrite some already set structures of "BaseCommit"?


A bit off-topic; but what is the idea behind the "-able" naming in Kiota? And what is the difference between language specific refiners and language specific writers? What are "usings" in the block code declarations?

This is just a small update, will see if more can be found…

Nkmol avatar Apr 20 '24 13:04 Nkmol

Thanks for the additional information.

This is really helpful. The fact that it runs "twice" is expected as depending on how the graph is structured, we might not get to some sections otherwise. The fact the second run is "erasing" the new name is not however. Can you provide additional code pointers to where the second rename is happening please?

what is the idea behind the "-able" naming in Kiota?

One convention in Go seems to be having interfaces that define single methods (or a couple at most) and use the er suffix to a verb to make it a noun (i.e. interface with Read method becomes Reader). However the interfaces that kiota generates are purely to enable type assertion scenarios that are not possible with pointers to structs. This aspect not following the canonical "er" suffix convention, we had to find another one, and able was used in numerous other interfaces (most notably the Closeable interface)

what is the difference between language specific refiners and language specific writers?

A refiner "tweaks" the code dom into something closer to the target language. Writers are just an implementation of the visitor design pattern for all the code element types, and they are in charge of generating the code for a given element.

What are "usings" in the block code declarations?

Arguably a bit of a dotnetism here. They are effectively imports/references.

baywet avatar Apr 22 '24 15:04 baywet

@Nkmol are you still looking into this?

baywet avatar May 17 '24 14:05 baywet

Thank you very much @baywet for the detailed response! Excuse my lack of response, there has been quite an abrupt shift of time, so I cannot make any promises in the near future.

This is really helpful. The fact that it runs "twice" is expected as depending on how the graph is structured, we might not get to some sections otherwise. The fact the second run is "erasing" the new name is not however. Can you provide additional code pointers to where the second rename is happening please?

I have not confirmed this “erasing” actually happens (it was assumed as the CodeDOM contained two different code style names for the same model), and I am not entirely sure where this behaviour would be best checked.

I am hoping to revisit this soon. If there are any pointers you can give me, please let me know. Thanks for the responses once again!

Nkmol avatar May 22 '24 19:05 Nkmol