Idempotency tests fail for NOTION.COM (multiple descriptions for same model class)

[WolfgangHG]

Found as part of #7774: the idempotency tests fail occasionally, mostly for the recent NOTION.COM api.

One failing test is https://github.com/microsoft/kiota/actions/runs/28884299669/job/85681452557:
==>In this case, a difference in file "models\partial_user_object_response.py" occured:
First one:

@dataclass
class PartialUserObjectResponse(Parsable):
    """
    The user who created the data source.
    """

Second one:

@dataclass
class PartialUserObjectResponse(Parsable):
    """
    User who created the page.
    """

Interesting here: the "partialUserObjectResponse" is referenced from two properties, and they have differing descriptions:

      "pageObjectResponse": {
        "properties": {
          ...
          "created_by": {
            "$ref": "#/components/schemas/partialUserObjectResponse",
            "description": "User who created the page."
          },


      "dataSourceObjectResponse": {
        "type": "object",
        "properties": {
          ...
          "created_by": {
            "$ref": "#/components/schemas/partialUserObjectResponse",
            "description": "The user who created the data source."
          },

Note: in #7774, I added code to "compare-generation.ps1" to report the files containing a difference - it is easier to track down those problems with the modified script from the pull request.

[WolfgangHG]

On my machine, I could reproduce a similar error with this command (and a Kiota debug build):

.\it\compare-generation.ps1 -descriptionUrl "./description/openapi.json" -descriptionKey https://developers.notion.com/openapi.json -language "python" -dev -preserveOutput

Here it happened twice in file "models\internal_file_response.py":

Version 1:

@dataclass
class InternalFileResponse(Parsable):
    # The time when the URL will expire.
    expiry_time: Optional[datetime.datetime] = None
    # The URL of the file.
    url: Optional[str] = None

Version 2:

@dataclass
class InternalFileResponse(Parsable):
    """
    The file URL for the icon.
    """
    # The time when the URL will expire.
    expiry_time: Optional[datetime.datetime] = None
    # The URL of the file.
    url: Optional[str] = None

In openapi.json, the component "internalFileResponse" is referenced multiple times, and some of the references have a comment, others don't.

[WolfgangHG]

I think it might be caused by the method KiotaBuilder.AddModelDeclarationIfDoesntExist:

kiota/src/Kiota.Builder/KiotaBuilder.cs

Lines 2009 to 2035 in bc1a7ba

	private CodeElement AddModelDeclarationIfDoesntExist(OpenApiUrlTreeNode currentNode, OpenApiOperation? currentOperation, IOpenApiSchema schema, string declarationName, CodeNamespace currentNamespace, CodeClass? inheritsFrom = null)
	{
	if (GetExistingDeclaration(currentNamespace, currentNode, declarationName) is not CodeElement existingDeclaration) // we can find it in the components
	{
	if (AddEnumDeclaration(currentNode, schema, declarationName, currentNamespace) is CodeEnum enumDeclaration)
	return enumDeclaration;
	if (schema.IsIntersection() && schema.MergeIntersectionSchemaEntries() is { } mergedSchema &&
	AddModelDeclarationIfDoesntExist(currentNode, currentOperation, mergedSchema, declarationName, currentNamespace, inheritsFrom) is CodeClass createdClass)
	{
	// multiple allOf entries that do not translate to inheritance
	return createdClass;
	}
	else if (schema.MergeInclusiveUnionSchemaEntries() is { } iUMergedSchema &&
	AddModelClass(currentNode, iUMergedSchema, declarationName, currentNamespace, currentOperation, inheritsFrom) is CodeClass uICreatedClass)
	{
	return uICreatedClass;
	}
	else if (schema.MergeExclusiveUnionSchemaEntries() is { } eUMergedSchema &&
	AddModelClass(currentNode, eUMergedSchema, declarationName, currentNamespace, currentOperation, inheritsFrom) is CodeClass uECreatedClass)
	{
	return uECreatedClass;
	}
	return AddModelClass(currentNode, schema, declarationName, currentNamespace, currentOperation, inheritsFrom);
	}
	return existingDeclaration;
	}

It checks whether a model element is found, and if not it is added.

In the NOTION.COM api, "internalFileResponse" is referenced in six places. The method is called from multiple parallel tasks, so it might happen that the first task calls GetExistingDeclaration with result = false, so it enters the IF block. Before it has finished, the second task performs the same check and it also returns "false". So, two model classes are added to my understanding.

But I think even when adding locking to the "exists" check so that a class is added only once, it might still happen that the second/third/n-th class reference in the OpenAPI spec adds the first class and thus is written to the generated code. And this is the idempotency problem: a random description is generated.

Is my understanding basically correct? If yes: fixing this is far off my head. Could it mean removing parallel processing?

[WolfgangHG]

Or should the documentation of the schema reference not be copied to the class?
Here are two samples: "internalFileResponse" is used twice with different descriptions, and apparently a random description is picked.

      "filePageCoverResponse": {
        "type": "object",
        "properties": {
          "type": {
            "type": "string",
            "const": "file",
            "description": "Type of cover. In this case, a file."
          },
          "file": {
            "$ref": "#/components/schemas/internalFileResponse",
            "description": "The file URL for the cover."
          }
        },
        "additionalProperties": false,
        "required": [
          "type",
          "file"
        ],
        "title": "File"
      },

      "filePageIconResponse": {
        "type": "object",
        "properties": {
          "type": {
            "type": "string",
            "const": "file",
            "description": "Type of icon. In this case, a file."
          },
          "file": {
            "$ref": "#/components/schemas/internalFileResponse",
            "description": "The file URL for the icon."
          }
        },

Wouldn't it be better to pick the class documentation only from the schema description (internalFileResponse) itself?
The descriptions in this snippet are just property descriptions I think.

Note: I tried to add a description to the schema class internalFileResponse in the local copy of the OpenAPI file, but it did not appear. The model class is probably generated as part of the reference handling, and the schema documentation is not written?

[WolfgangHG]

Following the previous suggestion, I modified this code snippet from KiotaBuilder.AddModelClass locally:

kiota/src/Kiota.Builder/KiotaBuilder.cs

Lines 2121 to 2126 in bc1a7ba

	Documentation = new()
	{
	DocumentationLabel = schema.ExternalDocs?.Description ?? string.Empty,
	DocumentationLink = schema.ExternalDocs?.Url,
	DescriptionTemplate = (string.IsNullOrEmpty(schema.Description) ? schema.AllOf?.FirstOrDefault(static x => !x.IsReferencedSchema() && !string.IsNullOrEmpty(x.Description))?.Description : schema.Description).CleanupDescription(),
	},

I changed it to:

string description;
if (schema is OpenApiSchemaReference schemaRef)
{
    //Schema reference:
    description = schemaRef.Target?.Description?.CleanupDescription() ?? "";
}
else
{
    //Old code - don't know whether it is required?
    description = (string.IsNullOrEmpty(schema.Description) ? schema.AllOf?.FirstOrDefault(static x => !x.IsReferencedSchema() && !string.IsNullOrEmpty(x.Description))?.Description : schema.Description).CleanupDescription();
}
var newClassStub = new CodeClass
{
    Name = declarationName,
    Kind = CodeClassKind.Model,
    Documentation = new()
    {
        DocumentationLabel = schema.ExternalDocs?.Description ?? string.Empty,
        DocumentationLink = schema.ExternalDocs?.Url,
        DescriptionTemplate = description,
    },
    Deprecation = schema.GetDeprecationInformation(),
};

This seemed to work: the locally executed idempotency test did not fail any more, and if I define a description for the schema class itself, it was written to the generated code class.

Question one: what about the code snippet tagged with "old code"? Is it required any more?
Question two: Is my approach reasonable at all ;-)? If yes: maybe it would be best to add it to #7774 where the idempotency problem comes up more often.

The threading problem (model class is added multiple times) might still happen, but it does not seem to have an any further impact.