Merge pull request #4751 from peterwurzinger/fix/4475

Using full names of generated types to avoid ambiguities with existing namespaces
microsoft · Jun 4, 2024 · c05fb25 · c05fb25
2 parents cfb9157 + 4ea2012
commit c05fb25
Show file tree

Hide file tree

Showing 8 changed files with 216 additions and 91 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -46,6 +46,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Fixes a bug where indexers in include/exclude patters were not normalized if the indexer was the last segment without a slash at the end [#4715](https://github.com/microsoft/kiota/issues/4715)
 - Fixes a bug where CLI generation doesnot handle parameters of type string array. [#4707](https://github.com/microsoft/kiota/issues/4707)
 - Fixed a bug where models would not be created when a multipart content schema existed with no encoding [#4734](https://github.com/microsoft/kiota/issues/4734)
+- Types generated by Kiota are now referenced with their full name to avoid namespace ambiguities [#4475](https://github.com/microsoft/kiota/issues/4475)
 - Fixes a bug where warnings about discriminator not being inherited were generated [#4761](https://github.com/microsoft/kiota/issues/4761)
 
 ## [1.14.0] - 2024-05-02

diff --git a/src/Kiota.Builder/Writers/CSharp/CSharpConventionService.cs b/src/Kiota.Builder/Writers/CSharp/CSharpConventionService.cs
@@ -177,7 +177,7 @@ public string GetTypeString(CodeTypeBase code, CodeElement targetElement, bool i
             throw new InvalidOperationException($"CSharp does not support union types, the union type {code.Name} should have been filtered out by the refiner");
         if (code is CodeType currentType)
         {
-            var typeName = TranslateTypeAndAvoidUsingNamespaceSegmentNames(currentType, targetElement);
+            var typeName = TranslateType(currentType);
             var nullableSuffix = ShouldTypeHaveNullableMarker(code, typeName) && includeNullableInformation ? NullableMarkerAsString : string.Empty;
             var collectionPrefix = currentType.CollectionKind == CodeTypeCollectionKind.Complex && includeCollectionInformation ? "List<" : string.Empty;
             var collectionSuffix = currentType.CollectionKind switch
@@ -196,76 +196,14 @@ public string GetTypeString(CodeTypeBase code, CodeElement targetElement, bool i
 
         throw new InvalidOperationException($"type of type {code?.GetType()} is unknown");
     }
-    private string TranslateTypeAndAvoidUsingNamespaceSegmentNames(CodeType currentType, CodeElement targetElement)
-    {
-        var parentElementsHash = targetElement.Parent is CodeClass parentClass ?
-            parentClass.Methods.Select(static x => x.Name)
-                .Union(parentClass.Properties.Select(static x => x.Name))
-                .Distinct(StringComparer.OrdinalIgnoreCase)
-                .ToHashSet(StringComparer.OrdinalIgnoreCase) :
-            new HashSet<string>(0, StringComparer.OrdinalIgnoreCase);
-
-        var typeName = TranslateType(currentType);
-        var areElementsInSameNamesSpace = DoesTypeExistsInSameNamesSpaceAsTarget(currentType, targetElement);
-        if (currentType.TypeDefinition != null &&
-                (
-                    GetNamesInUseByNamespaceSegments(targetElement).Contains(typeName) && !areElementsInSameNamesSpace         // match if elements are not in the same namespace and the type name is used in the namespace segments
-                || parentElementsHash.Contains(typeName)                                                                   // match if type name is used in the parent elements segments
-                || !areElementsInSameNamesSpace && DoesTypeExistsInTargetAncestorNamespace(currentType, targetElement)     // match if elements are not in the same namespace and the type exists in target ancestor namespace
-                || !areElementsInSameNamesSpace && DoesTypeExistsInOtherImportedNamespaces(currentType, targetElement)          // match if elements is not imported already by another namespace.
-                )
-            )
-            return $"{currentType.TypeDefinition.GetImmediateParentOfType<CodeNamespace>().Name}.{typeName}";
-        return typeName;
-    }
-
-    private static bool DoesTypeExistsInSameNamesSpaceAsTarget(CodeType currentType, CodeElement targetElement)
-    {
-        return currentType?.TypeDefinition?.GetImmediateParentOfType<CodeNamespace>()?.Name.Equals(targetElement?.GetImmediateParentOfType<CodeNamespace>()?.Name, StringComparison.OrdinalIgnoreCase) ?? false;
-    }
-
-    private static bool DoesTypeExistsInTargetAncestorNamespace(CodeType currentType, CodeElement targetElement)
-    {
-        // Avoid type ambiguity on similarly named classes. Currently, if we have namespaces A and A.B where both namespaces have type T,
-        // Trying to use type A.B.T in namespace A without using a qualified name will break the build.
-        // Similarly, if we have type A.B.C.D.T1 that needs to be used within type A.B.C.T2, but there's also a type
-        // A.B.T1, using T1 in T2 will resolve A.B.T1 even if you have a using statement with A.B.C.D.
-        var hasChildWithName = false;
-        if (currentType != null && currentType.TypeDefinition != null && !currentType.IsExternal && targetElement != null)
-        {
-            var typeName = currentType.TypeDefinition.Name;
-            var ns = targetElement.GetImmediateParentOfType<CodeNamespace>();
-            var rootNs = ns?.GetRootNamespace();
-            while (ns is not null && ns != rootNs && !hasChildWithName)
-            {
-                hasChildWithName = ns.GetChildElements(true).OfType<CodeClass>().Any(c => c.Name?.Equals(typeName, StringComparison.OrdinalIgnoreCase) == true);
-                ns = ns.Parent is CodeNamespace n ? n : (ns.GetImmediateParentOfType<CodeNamespace>());
-            }
-        }
-        return hasChildWithName;
-    }
-
-    private static bool DoesTypeExistsInOtherImportedNamespaces(CodeType currentType, CodeElement targetElement)
-    {
-        if (currentType.TypeDefinition is CodeClass { Parent: CodeNamespace currentTypeNamespace } codeClass)
-        {
-            var targetClass = targetElement.GetImmediateParentOfType<CodeClass>();
-            var importedNamespaces = targetClass.StartBlock.Usings
-                .Where(codeUsing => !codeUsing.IsExternal // 1. Are defined during generation(not external) 
-                                    && codeUsing.Declaration?.TypeDefinition != null
-                                    && !codeUsing.Name.Equals(currentTypeNamespace.Name, StringComparison.OrdinalIgnoreCase))  // 2. Do not match the namespace of the current type
-                .Select(static codeUsing => codeUsing.Declaration!.TypeDefinition!.GetImmediateParentOfType<CodeNamespace>())
-                .DistinctBy(static declaredNamespace => declaredNamespace.Name);
-
-            return importedNamespaces.Any(importedNamespace => (importedNamespace.FindChildByName<CodeClass>(codeClass.Name, false) != null)
-                                                               || (importedNamespace.FindChildByName<CodeEnum>(codeClass.Name, false) != null));
-        }
-        return false;
-    }
 
     public override string TranslateType(CodeType type)
     {
         ArgumentNullException.ThrowIfNull(type);
+
+        if (type.TypeDefinition is ITypeDefinition typeDefinition)
+            return typeDefinition.GetFullName();
+
         return type.Name switch
         {
             "integer" => "int",

diff --git a/src/Kiota.Builder/Writers/CSharp/CodeMethodWriter.cs b/src/Kiota.Builder/Writers/CSharp/CodeMethodWriter.cs
@@ -105,7 +105,9 @@ private void WriteRawUrlBuilderBody(CodeClass parentClass, CodeMethod codeElemen
     {
         var rawUrlParameter = codeElement.Parameters.OfKind(CodeParameterKind.RawUrl) ?? throw new InvalidOperationException("RawUrlBuilder method should have a RawUrl parameter");
         var requestAdapterProperty = parentClass.GetPropertyOfKind(CodePropertyKind.RequestAdapter) ?? throw new InvalidOperationException("RawUrlBuilder method should have a RequestAdapter property");
-        writer.WriteLine($"return new {parentClass.Name.ToFirstCharacterUpperCase()}({rawUrlParameter.Name.ToFirstCharacterLowerCase()}, {requestAdapterProperty.Name.ToFirstCharacterUpperCase()});");
+
+        var fullName = parentClass.GetFullName();
+        writer.WriteLine($"return new {fullName}({rawUrlParameter.Name.ToFirstCharacterLowerCase()}, {requestAdapterProperty.Name.ToFirstCharacterUpperCase()});");
     }
     private static readonly CodePropertyTypeComparer CodePropertyTypeForwardComparer = new();
     private static readonly CodePropertyTypeComparer CodePropertyTypeBackwardComparer = new(true);
@@ -117,13 +119,13 @@ private void WriteFactoryMethodBodyForInheritedModel(CodeMethod codeElement, Cod
         {
             writer.WriteLine($"\"{mappedType.Key}\" => new {conventions.GetTypeString(mappedType.Value.AllTypes.First(), codeElement)}(),");
         }
-        writer.WriteLine($"_ => new {parentClass.Name.ToFirstCharacterUpperCase()}(),");
+        writer.WriteLine($"_ => new {parentClass.GetFullName()}(),");
         writer.CloseBlock("};");
     }
     private const string ResultVarName = "result";
     private void WriteFactoryMethodBodyForUnionModel(CodeMethod codeElement, CodeClass parentClass, CodeParameter parseNodeParameter, LanguageWriter writer)
     {
-        writer.WriteLine($"var {ResultVarName} = new {parentClass.Name.ToFirstCharacterUpperCase()}();");
+        writer.WriteLine($"var {ResultVarName} = new {parentClass.GetFullName()}();");
         var includeElse = false;
         foreach (var property in parentClass.GetPropertiesOfKind(CodePropertyKind.Custom)
                                             .OrderBy(static x => x, CodePropertyTypeForwardComparer)
@@ -150,7 +152,7 @@ private void WriteFactoryMethodBodyForUnionModel(CodeMethod codeElement, CodeCla
     }
     private void WriteFactoryMethodBodyForIntersectionModel(CodeMethod codeElement, CodeClass parentClass, CodeParameter parseNodeParameter, LanguageWriter writer)
     {
-        writer.WriteLine($"var {ResultVarName} = new {parentClass.Name.ToFirstCharacterUpperCase()}();");
+        writer.WriteLine($"var {ResultVarName} = new {parentClass.GetFullName()}();");
         var includeElse = false;
         foreach (var property in parentClass.GetPropertiesOfKind(CodePropertyKind.Custom)
                                             .Where(static x => x.Type is not CodeType propertyType || propertyType.IsCollection || propertyType.TypeDefinition is not CodeClass)
@@ -202,7 +204,7 @@ private void WriteFactoryMethodBody(CodeMethod codeElement, CodeClass parentClas
         else if (parentClass.DiscriminatorInformation.ShouldWriteDiscriminatorForIntersectionType)
             WriteFactoryMethodBodyForIntersectionModel(codeElement, parentClass, parseNodeParameter, writer);
         else
-            writer.WriteLine($"return new {parentClass.Name.ToFirstCharacterUpperCase()}();");
+            writer.WriteLine($"return new {parentClass.GetFullName()}();");
     }
     private void WriteRequestBuilderBody(CodeClass parentClass, CodeMethod codeElement, LanguageWriter writer)
     {
@@ -357,7 +359,7 @@ private string GetDeserializationMethodName(CodeTypeBase propType, CodeMethod me
                     return $"GetCollectionOfObjectValues<{propertyType}>({propertyType}.CreateFromDiscriminatorValue){collectionMethod}";
             }
             else if (currentType.TypeDefinition is CodeEnum enumType)
-                return $"GetEnumValue<{enumType.Name.ToFirstCharacterUpperCase()}>()";
+                return $"GetEnumValue<{enumType.GetFullName()}>()";
         }
         return propertyType switch
         {
@@ -662,7 +664,7 @@ private string GetSerializationMethodName(CodeTypeBase propType, CodeMethod meth
                 else
                     return $"WriteCollectionOfObjectValues<{propertyType}>";
             else if (currentType.TypeDefinition is CodeEnum enumType)
-                return $"WriteEnumValue<{enumType.Name.ToFirstCharacterUpperCase()}>";
+                return $"WriteEnumValue<{enumType.GetFullName()}>";
 
         }
         return propertyType switch

diff --git a/src/Kiota.Builder/Writers/CSharp/TypeDefinitionExtensions.cs b/src/Kiota.Builder/Writers/CSharp/TypeDefinitionExtensions.cs
@@ -0,0 +1,44 @@
+using System;
+using System.Text;
+using Kiota.Builder.CodeDOM;
+using Kiota.Builder.Extensions;
+
+namespace Kiota.Builder.Writers.CSharp;
+
+internal static class TypeDefinitionExtensions
+{
+    public static string GetFullName(this ITypeDefinition typeDefinition)
+    {
+        ArgumentNullException.ThrowIfNull(typeDefinition);
+
+        var fullNameBuilder = new StringBuilder();
+        return AppendTypeName(typeDefinition, fullNameBuilder).ToString();
+    }
+
+    private static StringBuilder AppendTypeName(ITypeDefinition typeDefinition, StringBuilder fullNameBuilder)
+    {
+        if (string.IsNullOrEmpty(typeDefinition.Name))
+            throw new ArgumentException("Cannot append a full name for a type without a name.", nameof(typeDefinition));
+
+        fullNameBuilder.Insert(0, typeDefinition.Name.ToFirstCharacterUpperCase());
+        switch (typeDefinition.Parent)
+        {
+            case null:
+                return fullNameBuilder;
+            case ITypeDefinition parentTypeDefinition:
+                {
+                    fullNameBuilder.Insert(0, '.');
+                    return AppendTypeName(parentTypeDefinition, fullNameBuilder);
+                }
+            case CodeNamespace codeNamespace:
+                {
+                    if (!string.IsNullOrEmpty(codeNamespace.Name))
+                        fullNameBuilder.Insert(0, $"{codeNamespace.Name}.");
+
+                    return fullNameBuilder;
+                }
+            default:
+                throw new InvalidOperationException($"Type {typeDefinition.Name} contains an invalid parent of type {typeDefinition.Parent.GetType().FullName}.");
+        }
+    }
+}
diff --git a/tests/Kiota.Builder.Tests/Writers/CLI/CliCodeMethodWriterTests.cs b/tests/Kiota.Builder.Tests/Writers/CLI/CliCodeMethodWriterTests.cs
@@ -241,7 +241,7 @@ public void WritesIndexerCommands()
         writer.Write(method);
         var result = tw.ToString();
 
-        Assert.Contains("var builder = new TestClass", result);
+        Assert.Contains("var builder = new Test.Name.Sub.TestClass", result);
         Assert.Contains("var commands = new List<Command>();", result);
         Assert.Contains("commands.Add(builder.BuildTestMethod1());", result);
         Assert.Contains("commands.AddRange(builder.BuildTestMethod2());", result);
@@ -325,7 +325,7 @@ public void WritesMatchingIndexerCommandsIntoExecutableCommand()
         writer.Write(method);
         var result = tw.ToString();
 
-        Assert.Contains("var testItemIdx = new TestItemRequestBuilder();", result);
+        Assert.Contains("var testItemIdx = new Test.Name.Sub.TestItemRequestBuilder();", result);
         Assert.Contains("var command = testItemIdx.BuildTestMethod1();", result);
         Assert.Contains("var cmds = testItemIdx.BuildTestMethod2();", result);
         Assert.DoesNotContain("execCommands.AddRange(cmds.Item1)", result);
@@ -418,12 +418,12 @@ public void WritesMatchingIndexerCommandsIntoContainerCommand()
         writer.Write(method);
         var result = tw.ToString();
 
-        Assert.Contains("var testItemIndexer = new TestIndexItemRequestBuilder();", result);
+        Assert.Contains("var testItemIndexer = new Test.Name.Sub.TestIndexItemRequestBuilder();", result);
         Assert.Contains("var command = testItemIndexer.BuildTestMethod1();", result);
         Assert.Contains("var cmds = testItemIndexer.BuildTestMethod2();", result);
         Assert.DoesNotContain("execCommands.AddRange(cmds.Item1);", result);
         Assert.Contains("nonExecCommands.AddRange(cmds.Item2);", result);
-        Assert.Contains("var builder = new TestNavItemRequestBuilder", result);
+        Assert.Contains("var builder = new Test.TestNavItemRequestBuilder", result);
         Assert.Contains("nonExecCommands.Add(builder.BuildTestMethod11());", result);
         Assert.Contains("return command;", result);
         Assert.DoesNotContain("nonExecCommands.Add(builder.BuildTestMethod3());", result);
@@ -553,7 +553,7 @@ public void WritesNavCommandThatSkipsReusedNavCommandInstance()
         var result = tw.ToString();
 
         Assert.Contains("var command = new Command(\"user\");", result);
-        Assert.Contains("var builder = new TestNavItemRequestBuilder();", result);
+        Assert.Contains("var builder = new Test.TestNavItemRequestBuilder();", result);
         Assert.Contains("execCommands.Add(builder.BuildExecutableTestMethod());", result);
         Assert.Contains("return command;", result);
         Assert.DoesNotContain("BuildNavTestMethod", result);
@@ -596,7 +596,7 @@ public void WritesContainerCommands()
         var result = tw.ToString();
 
         Assert.Contains("var command = new Command(\"user\");", result);
-        Assert.Contains("var builder = new TestClass1", result);
+        Assert.Contains("var builder = new Test.Name.Sub1.Sub2.TestClass1", result);
         Assert.Contains("nonExecCommands.Add(builder.BuildTestMethod1());", result);
         Assert.Contains("nonExecCommands.Add(builder.BuildTestMethod2());", result);
         Assert.Contains("return command;", result);
@@ -637,7 +637,7 @@ public void WritesRequestBuilderWithParametersCommands()
         var result = tw.ToString();
 
         Assert.Contains("var command = new Command(\"user\");", result);
-        Assert.Contains("var builder = new TestClass1", result);
+        Assert.Contains("var builder = new Test.Name.Sub1.Sub2.TestClass1", result);
         Assert.Contains("nonExecCommands.Add(builder.BuildTestMethod1());", result);
         Assert.Contains("nonExecCommands.Add(builder.BuildTestMethod2());", result);
         Assert.Contains("return command;", result);
@@ -1085,7 +1085,7 @@ public void WritesExecutableCommandForPostRequestWithModelBody()
         Assert.Contains("command.AddOption(bodyOption);", result);
         Assert.Contains("var body = invocationContext.ParseResult.GetValueForOption(bodyOption) ?? string.Empty;", result);
         Assert.Contains("using var stream = new MemoryStream(Encoding.UTF8.GetBytes(body));", result);
-        Assert.Contains("var model = parseNode.GetObjectValue<Content>(Content.CreateFromDiscriminatorValue);", result);
+        Assert.Contains("var model = parseNode.GetObjectValue<Test.Content>(Test.Content.CreateFromDiscriminatorValue);", result);
         Assert.Contains("if (model is null)", result);
         Assert.Contains("Console.Error.WriteLine(\"No model data to send.\")", result);
         Assert.Contains("var requestInfo = CreatePostRequestInformation", result);
@@ -1171,7 +1171,7 @@ public void WritesExecutableCommandForPostRequestWithModelBodyAndContentType()
         Assert.Contains("var body = invocationContext.ParseResult.GetValueForOption(bodyOption) ?? string.Empty;", result);
         Assert.Contains("var contentType = invocationContext.ParseResult.GetValueForOption(contentTypeOption);", result);
         Assert.Contains("using var stream = new MemoryStream(Encoding.UTF8.GetBytes(body));", result);
-        Assert.Contains("var model = parseNode.GetObjectValue<Content>(Content.CreateFromDiscriminatorValue);", result);
+        Assert.Contains("var model = parseNode.GetObjectValue<Test.Content>(Test.Content.CreateFromDiscriminatorValue);", result);
         Assert.Contains("if (model is null)", result);
         Assert.Contains("Console.Error.WriteLine(\"No model data to send.\")", result);
         Assert.Contains("var requestInfo = CreatePostRequestInformation(model, contentType", result);
@@ -1242,7 +1242,7 @@ public void WritesExecutableCommandForPostRequestWithCollectionModel()
         Assert.Contains("command.AddOption(bodyOption);", result);
         Assert.Contains("var body = invocationContext.ParseResult.GetValueForOption(bodyOption) ?? string.Empty;", result);
         Assert.Contains("using var stream = new MemoryStream(Encoding.UTF8.GetBytes(body));", result);
-        Assert.Contains("var model = parseNode.GetCollectionOfObjectValues<Content>(Content.CreateFromDiscriminatorValue)?.ToList();", result);
+        Assert.Contains("var model = parseNode.GetCollectionOfObjectValues<Test.Content>(Test.Content.CreateFromDiscriminatorValue)?.ToList();", result);
         Assert.Contains("if (model is null)", result);
         Assert.Contains("Console.Error.WriteLine(\"No model data to send.\")", result);
         Assert.Contains("var requestInfo = CreatePostRequestInformation", result);