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

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

Closed
Nkmol opened this issue Apr 3, 2024 · 10 comments
Assignees
Labels
Go help wanted Issue caused by core project dependency modules or library type:bug A broken experience
Milestone

Comments

@Nkmol
Copy link

Nkmol commented Apr 3, 2024

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.

@andrueastman
Copy link
Member

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 #4381

@Nkmol
Copy link
Author

Nkmol commented Apr 5, 2024

@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.

@andrueastman
Copy link
Member

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 andrueastman added the type:bug A broken experience label Apr 12, 2024
@Nkmol
Copy link
Author

Nkmol commented Apr 12, 2024

@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;
    }
}

@baywet baywet added the Go label Apr 17, 2024
@baywet baywet added this to the Backlog milestone Apr 17, 2024
@baywet baywet added the help wanted Issue caused by core project dependency modules or library label Apr 17, 2024
@baywet
Copy link
Member

baywet commented Apr 17, 2024

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

@Nkmol
Copy link
Author

Nkmol commented Apr 20, 2024

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…

@baywet
Copy link
Member

baywet commented Apr 22, 2024

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
Copy link
Member

baywet commented May 17, 2024

@Nkmol are you still looking into this?

@baywet baywet added the status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close label May 17, 2024
@Nkmol
Copy link
Author

Nkmol commented May 22, 2024

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!

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close Status: No Recent Activity labels May 22, 2024
@baywet baywet added status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close and removed Needs: Attention 👋 status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels May 22, 2024
@andrueastman andrueastman modified the milestones: Backlog, Kiota v1.15 May 29, 2024
@andrueastman andrueastman self-assigned this May 29, 2024
@andrueastman
Copy link
Member

@Nkmol I've validated that is now resolved after changes made in #4723 and was related to the allOf scenarios.

We'll close this one for now. Feel free to re-open if you run into any issues.

@github-project-automation github-project-automation bot moved this from Todo 📃 to Done ✔️ in Kiota May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Go help wanted Issue caused by core project dependency modules or library type:bug A broken experience
Projects
Archived in project
Development

No branches or pull requests

3 participants