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.

Sign up for GitHub

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

Fix for type collisions for types referenced from OData actions #1215

Merged
merged 1 commit into from
Feb 16, 2022
Merged

Fix for type collisions for types referenced from OData actions #1215

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions src/Kiota.Builder/KiotaBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
Expand Down Expand Up @@ -697,7 +697,7 @@ private string GetModelsNamespaceNameFromReferenceId(string referenceId) {
}
private CodeType CreateModelDeclarationAndType(OpenApiUrlTreeNode currentNode, OpenApiSchema schema, OpenApiOperation operation, CodeNamespace codeNamespace, string classNameSuffix = "", OpenApiResponse response = default) {
var className = currentNode.GetClassName(operation: operation, suffix: classNameSuffix, response: response, schema: schema);
var codeDeclaration = AddModelDeclarationIfDoesntExit(currentNode, schema, className, codeNamespace);
var codeDeclaration = AddModelDeclarationIfDoesntExist(currentNode, schema, className, codeNamespace);
return new CodeType {
TypeDefinition = codeDeclaration,
Name = className,
Expand All @@ -714,7 +714,7 @@ private CodeTypeBase CreateInheritedModelDeclaration(OpenApiUrlTreeNode currentN
if(shortestNamespace == null)
shortestNamespace = rootNamespace.AddNamespace(shortestNamespaceName);
className = currentSchema.GetSchemaTitle() ?? currentNode.GetClassName(operation: operation, schema: schema);
codeDeclaration = AddModelDeclarationIfDoesntExit(currentNode, currentSchema, className, shortestNamespace, codeDeclaration as CodeClass, true);
codeDeclaration = AddModelDeclarationIfDoesntExist(currentNode, currentSchema, className, shortestNamespace, codeDeclaration as CodeClass, !currentSchema.IsReferencedSchema());
}

return new CodeType {
Expand Down Expand Up @@ -749,7 +749,7 @@ private CodeTypeBase CreateUnionModelDeclaration(OpenApiUrlTreeNode currentNode,
if(shortestNamespace == null)
shortestNamespace = rootNamespace.AddNamespace(shortestNamespaceName);
var className = currentSchema.GetSchemaTitle();
var codeDeclaration = AddModelDeclarationIfDoesntExit(currentNode, currentSchema, className, shortestNamespace);
var codeDeclaration = AddModelDeclarationIfDoesntExist(currentNode, currentSchema, className, shortestNamespace);
unionType.AddType(new CodeType {
TypeDefinition = codeDeclaration,
Name = className,
Expand All @@ -774,8 +774,11 @@ private CodeTypeBase CreateModelDeclarations(OpenApiUrlTreeNode currentNode, Ope
} else if (schema.IsArray()) {
// collections at root
var type = GetPrimitiveType(schema?.Items, string.Empty);
if(type == null)
type = CreateModelDeclarationAndType(currentNode, schema?.Items, operation, codeNamespace);
if (type == null)
{
var targetNamespace = schema?.Items == null ? codeNamespace : GetShortestNamespace(codeNamespace, schema.Items);
Copy link
Member Author

@andrueastman andrueastman Feb 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We evaluate this case similar to objects so that if a collection item has a schema, we will look at the same place(namespace) and avoid duplicating models in different namespaces.

type = CreateModelDeclarationAndType(currentNode, schema?.Items, operation, targetNamespace);
}
type.CollectionKind = CodeTypeBase.CodeTypeCollectionKind.Array;
return type;
} else if(!string.IsNullOrEmpty(schema.Type))
Expand All @@ -791,7 +794,7 @@ private CodeNamespace GetSearchNamespace(bool checkInAllNamespaces, OpenApiUrlTr
else if (currentNode.DoesNodeBelongToItemSubnamespace()) return currentNamespace.EnsureItemNamespace();
else return currentNamespace;
}
private CodeElement AddModelDeclarationIfDoesntExit(OpenApiUrlTreeNode currentNode, OpenApiSchema schema, string declarationName, CodeNamespace currentNamespace, CodeClass inheritsFrom = null, bool checkInAllNamespaces = false) {
private CodeElement AddModelDeclarationIfDoesntExist(OpenApiUrlTreeNode currentNode, OpenApiSchema schema, string declarationName, CodeNamespace currentNamespace, CodeClass inheritsFrom = null, bool checkInAllNamespaces = false) {
var existingDeclaration = GetExistingDeclaration(checkInAllNamespaces, currentNamespace, currentNode, declarationName);
if(existingDeclaration == null) // we can find it in the components
{
Expand Down Expand Up @@ -820,7 +823,7 @@ private CodeClass AddModelClass(OpenApiUrlTreeNode currentNode, OpenApiSchema sc
var parentSchema = referencedAllOfs.FirstOrDefault();
if(parentSchema != null) {
var parentClassNamespace = GetShortestNamespace(currentNamespace, parentSchema);
inheritsFrom = AddModelDeclarationIfDoesntExit(currentNode, parentSchema, parentSchema.GetSchemaTitle(), parentClassNamespace, null, true) as CodeClass;
inheritsFrom = AddModelDeclarationIfDoesntExist(currentNode, parentSchema, parentSchema.GetSchemaTitle(), parentClassNamespace, null, !parentSchema.IsReferencedSchema()) as CodeClass;
Copy link
Member Author

@andrueastman andrueastman Feb 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only set to true if we don't have schema. The parentClassNamespace should ideally contain the type if the schema is referenced as the namespace is consistently evaluated from the schema (if it exists).
With this change, we know which namespace the type should be if we have a reference schema so no need to search all namespaces.

}
}
var newClass = currentNamespace.AddClass(new CodeClass {
Expand Down Expand Up @@ -851,7 +854,7 @@ private void CreatePropertiesForModelClass(OpenApiUrlTreeNode currentNode, OpenA
var shortestNamespaceName = GetModelsNamespaceNameFromReferenceId(propertyDefinitionSchema.Reference?.Id);
var targetNamespace = string.IsNullOrEmpty(shortestNamespaceName) ? ns :
(rootNamespace.FindNamespaceByName(shortestNamespaceName) ?? rootNamespace.AddNamespace(shortestNamespaceName));
definition = AddModelDeclarationIfDoesntExit(currentNode, propertyDefinitionSchema, className, targetNamespace, null, true);
definition = AddModelDeclarationIfDoesntExist(currentNode, propertyDefinitionSchema, className, targetNamespace, null, !propertyDefinitionSchema.IsReferencedSchema());
Copy link
Member Author

@andrueastman andrueastman Feb 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only set to true if we don't have schema. The targetNamespace should ideally contain the type if the schema is referenced as the namespace is consistently evaluated from the schema (if it exists).
Leaving to true results to flaky generation of Types as sometimes it might resolve something like Microsoft.Graph.Group vs Microsoft.Graph.TermStore.Group depending on which namespace is dug up first(if set to true).
With this change, we know which namespace the type should be if we have a reference schema so no need to search all namespaces.

}
return CreateProperty(x.Key, className ?? x.Value.Type, typeSchema: x.Value, typeDefinition: definition);
})
Expand Down