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

Using full names of generated types to avoid ambiguities with existing namespaces #4751

Merged
merged 11 commits into from
Jun 4, 2024
72 changes: 5 additions & 67 deletions src/Kiota.Builder/Writers/CSharp/CSharpConventionService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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",
Expand Down
16 changes: 9 additions & 7 deletions src/Kiota.Builder/Writers/CSharp/CodeMethodWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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
{
Expand Down Expand Up @@ -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
Expand Down
44 changes: 44 additions & 0 deletions src/Kiota.Builder/Writers/CSharp/TypeDefinitionExtensions.cs
Original file line number Diff line number Diff line change
@@ -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)
andrueastman marked this conversation as resolved.
Show resolved Hide resolved
{
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());
if (typeDefinition.Parent is null)
return fullNameBuilder;

if (typeDefinition.Parent is ITypeDefinition parentTypeDefinition)
{
fullNameBuilder.Insert(0, '.');
return AppendTypeName(parentTypeDefinition, fullNameBuilder);
}
else if (typeDefinition.Parent is CodeNamespace codeNamespace)
{
if (!string.IsNullOrEmpty(codeNamespace.Name))
fullNameBuilder.Insert(0, $"{codeNamespace.Name}.");

return fullNameBuilder;
}
else
{
throw new InvalidOperationException($"Type {typeDefinition.Name} contains an invalid parent of type {typeDefinition.Parent.GetType().FullName}.");
}
peterwurzinger marked this conversation as resolved.
Show resolved Hide resolved
}
}
20 changes: 10 additions & 10 deletions tests/Kiota.Builder.Tests/Writers/CLI/CliCodeMethodWriterTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
Loading
Loading