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

Fix superfluous fields when converting composed types to wrapper types. #5657

Merged
merged 2 commits into from
Oct 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Fixed a bug where composed types wrappers would not build in CSharp.
- Fixed a bug where the type name for inherited inline models would be incorrect. [#5610](https://github.com/microsoft/kiota/issues/5610)
- Fixes typing inconsistencies in generated code and libraries in Python [kiota-python#333](https://github.com/microsoft/kiota-python/issues/333)
- Fixes generation of superfluous fields for Models with discriminator due to refiners adding the same properties to the same model [#4178](https://github.com/microsoft/kiota/issues/4178).

## [1.19.1] - 2024-10-11

Expand Down
28 changes: 26 additions & 2 deletions src/Kiota.Builder/Refiners/CommonLanguageRefiner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,11 @@
.Union(parseNodeFactoryInterfaceAndRegistrationFullName)
.Where(x => !string.IsNullOrEmpty(x))
.ToList();
currentMethod.DeserializerModules = currentMethod.DeserializerModules.Select(x => x.Split(separator).Last()).ToHashSet(StringComparer.OrdinalIgnoreCase);

Check warning on line 39 in src/Kiota.Builder/Refiners/CommonLanguageRefiner.cs

View workflow job for this annotation

GitHub Actions / Build

Indexing at Count-1 should be used instead of the "Enumerable" extension method "Last" (https://rules.sonarsource.com/csharp/RSPEC-6608)
currentMethod.SerializerModules = currentMethod.SerializerModules.Select(x => x.Split(separator).Last()).ToHashSet(StringComparer.OrdinalIgnoreCase);

Check warning on line 40 in src/Kiota.Builder/Refiners/CommonLanguageRefiner.cs

View workflow job for this annotation

GitHub Actions / Build

Indexing at Count-1 should be used instead of the "Enumerable" extension method "Last" (https://rules.sonarsource.com/csharp/RSPEC-6608)
declaration.AddUsings(cumulatedSymbols.Select(x => new CodeUsing
{
Name = x.Split(separator).Last(),

Check warning on line 43 in src/Kiota.Builder/Refiners/CommonLanguageRefiner.cs

View workflow job for this annotation

GitHub Actions / Build

Indexing at Count-1 should be used instead of the "Enumerable" extension method "Last" (https://rules.sonarsource.com/csharp/RSPEC-6608)
Declaration = new CodeType
{
Name = x.Split(separator).SkipLast(1).Aggregate((x, y) => $"{x}{separator}{y}"),
Expand Down Expand Up @@ -159,7 +159,7 @@
}
CrawlTree(current, x => ReplacePropertyNames(x, propertyKindsToReplace!, refineAccessorName));
}
protected static void AddGetterAndSetterMethods(CodeElement current, HashSet<CodePropertyKind> propertyKindsToAddAccessors, Func<CodeElement, string, string> refineAccessorName, bool removeProperty, bool parameterAsOptional, string getterPrefix, string setterPrefix, string fieldPrefix = "_", AccessModifier propertyAccessModifier = AccessModifier.Private)

Check warning on line 162 in src/Kiota.Builder/Refiners/CommonLanguageRefiner.cs

View workflow job for this annotation

GitHub Actions / Build

Refactor this method to reduce its Cognitive Complexity from 20 to the 15 allowed. (https://rules.sonarsource.com/csharp/RSPEC-3776)

Check warning on line 162 in src/Kiota.Builder/Refiners/CommonLanguageRefiner.cs

View workflow job for this annotation

GitHub Actions / Build

Method has 9 parameters, which is greater than the 7 authorized. (https://rules.sonarsource.com/csharp/RSPEC-107)
{
ArgumentNullException.ThrowIfNull(refineAccessorName);
var isSetterPrefixEmpty = string.IsNullOrEmpty(setterPrefix);
Expand Down Expand Up @@ -301,7 +301,7 @@
&& codeClass.Properties.FirstOrDefault(classProp => provider.ReservedNames.Contains(classProp.Name)) is { } matchingProperty && matchingProperty.Name.Equals(codeClass.Name, StringComparison.OrdinalIgnoreCase)) // rename the class if it has a matching property and the class has the same name as the property.
);
}
protected static void ReplaceReservedNames(CodeElement current, IReservedNamesProvider provider, Func<string, string> replacement, HashSet<Type>? codeElementExceptions = null, Func<CodeElement, bool>? shouldReplaceCallback = null)

Check warning on line 304 in src/Kiota.Builder/Refiners/CommonLanguageRefiner.cs

View workflow job for this annotation

GitHub Actions / Build

Refactor this method to reduce its Cognitive Complexity from 20 to the 15 allowed. (https://rules.sonarsource.com/csharp/RSPEC-3776)
{
ArgumentNullException.ThrowIfNull(current);
ArgumentNullException.ThrowIfNull(provider);
Expand Down Expand Up @@ -370,7 +370,7 @@
currentDeclaration.Usings
.Where(static codeUsing => codeUsing is { IsExternal: false })
.Select(static codeUsing => new Tuple<CodeUsing, string[]>(codeUsing, codeUsing.Name.Split('.')))
.Where(tuple => tuple.Item2.Any(x => provider.ReservedNames.Contains(x)))

Check warning on line 373 in src/Kiota.Builder/Refiners/CommonLanguageRefiner.cs

View workflow job for this annotation

GitHub Actions / Build

Collection-specific "Exists" method should be used instead of the "Any" extension. (https://rules.sonarsource.com/csharp/RSPEC-6605)
.ToList()
.ForEach(tuple =>
{
Expand Down Expand Up @@ -414,7 +414,7 @@
CrawlTree(current, c => AddDefaultImports(c, evaluators));
}
private static readonly HashSet<string> BinaryTypes = new(StringComparer.OrdinalIgnoreCase) { "binary", "base64", "base64url" };
protected static void ReplaceBinaryByNativeType(CodeElement currentElement, string symbol, string ns, bool addDeclaration = false, bool isNullable = false)

Check warning on line 417 in src/Kiota.Builder/Refiners/CommonLanguageRefiner.cs

View workflow job for this annotation

GitHub Actions / Build

Refactor this method to reduce its Cognitive Complexity from 16 to the 15 allowed. (https://rules.sonarsource.com/csharp/RSPEC-3776)
{
if (currentElement is CodeMethod currentMethod)
{
Expand Down Expand Up @@ -485,8 +485,10 @@
if (!supportsInnerClasses)
{
var @namespace = codeClass.GetImmediateParentOfType<CodeNamespace>();
if (@namespace.FindChildByName<CodeClass>(codeComposedType.Name, false) is CodeClass { OriginalComposedType: null })
if (@namespace.FindChildByName<CodeClass>(codeComposedType.Name, false) is { OriginalComposedType: null })
codeComposedType.Name = $"{codeComposedType.Name}Wrapper";
if (GetAlreadyExistingComposedCodeType(@namespace, codeComposedType) is { } existingCodeType)
return existingCodeType;
newClass = @namespace.AddClass(new CodeClass
{
Name = codeComposedType.Name,
Expand All @@ -499,8 +501,10 @@
Deprecation = codeComposedType.Deprecation,
}).Last();
}
else if (codeComposedType.TargetNamespace is CodeNamespace targetNamespace)
else if (codeComposedType.TargetNamespace is { } targetNamespace)
{
if (GetAlreadyExistingComposedCodeType(targetNamespace, codeComposedType) is { } existingCodeType)
return existingCodeType;
newClass = targetNamespace.AddClass(new CodeClass
{
Name = codeComposedType.Name,
Expand All @@ -523,6 +527,8 @@
{
if (codeComposedType.Name.Equals(codeClass.Name, StringComparison.OrdinalIgnoreCase) || codeClass.FindChildByName<CodeProperty>(codeComposedType.Name, false) is not null)
codeComposedType.Name = $"{codeComposedType.Name}Wrapper";
if (GetAlreadyExistingComposedCodeType(codeClass, codeComposedType) is { } existingCodeType)
return existingCodeType;
newClass = codeClass.AddInnerClass(new CodeClass
{
Name = codeComposedType.Name,
Expand Down Expand Up @@ -602,6 +608,24 @@
ActionOf = codeComposedType.ActionOf,
};
}

private static CodeType? GetAlreadyExistingComposedCodeType(IBlock targetNamespace, CodeComposedTypeBase codeComposedType)
{
if (targetNamespace.FindChildByName<CodeClass>(codeComposedType.Name, false) is { OriginalComposedType: not null } existingClass && existingClass.OriginalComposedType.Name.Equals(codeComposedType.Name, StringComparison.OrdinalIgnoreCase))
{ // the composed type was already added/created and the typeDefinition(codeclass) is already present in the namespace/class.
return new CodeType
{
Name = codeComposedType.Name,
TypeDefinition = existingClass,
CollectionKind = codeComposedType.CollectionKind,
IsNullable = codeComposedType.IsNullable,
ActionOf = codeComposedType.ActionOf,
};
}

return null;
}

protected static void MoveClassesWithNamespaceNamesUnderNamespace(CodeElement currentElement)
{
if (currentElement is CodeClass currentClass &&
Expand All @@ -625,9 +649,9 @@
if (currentElement is CodeIndexer currentIndexer &&
currentElement.Parent is CodeClass indexerParentClass)
{
if (indexerParentClass.ContainsMember(currentElement.Name)) // TODO remove condition for v2 necessary because of the second case of Go block

Check warning on line 652 in src/Kiota.Builder/Refiners/CommonLanguageRefiner.cs

View workflow job for this annotation

GitHub Actions / Build

Complete the task associated to this 'TODO' comment. (https://rules.sonarsource.com/csharp/RSPEC-1135)
indexerParentClass.RemoveChildElement(currentElement);
//TODO remove whole block except for last else if body for v2

Check warning on line 654 in src/Kiota.Builder/Refiners/CommonLanguageRefiner.cs

View workflow job for this annotation

GitHub Actions / Build

Complete the task associated to this 'TODO' comment. (https://rules.sonarsource.com/csharp/RSPEC-1135)
if (language == GenerationLanguage.Go)
{
if (currentIndexer.IsLegacyIndexer)
Expand Down
97 changes: 97 additions & 0 deletions tests/Kiota.Builder.Tests/KiotaBuilderTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,103 @@ public async Task DoesntConflictOnModelsNamespaceAsync()
Assert.NotNull(innerRequestBuilderNS.FindChildByName<CodeClass>("InnerRequestBuilder", false));

}
[Theory]
[InlineData(GenerationLanguage.CSharp)]
[InlineData(GenerationLanguage.Java)]
[InlineData(GenerationLanguage.TypeScript)]
[InlineData(GenerationLanguage.Python)]
[InlineData(GenerationLanguage.Go)]
[InlineData(GenerationLanguage.PHP)]
[InlineData(GenerationLanguage.Ruby)]
public async Task DoesNotAddSuperflousFieldsToModelsAsync(GenerationLanguage language)
{
var tempFilePath = Path.Combine(Path.GetTempPath(), Path.GetTempFileName());
await using var fs = await GetDocumentStreamAsync(@"openapi: 3.0.3
info:
title: Example API
version: 1.0.0
servers:
- url: ""https://localhost:8080""
paths:
""/api/all"":
post:
requestBody:
content:
application/json:
schema:
$ref: ""#/components/schemas/AorB""
required: true
responses:
""200"":
$ref: ""#/components/responses/AorBResponse""
components:
schemas:
A:
type: object
required:
- type
properties:
type:
type: string
default: ""a""
B:
type: object
required:
- type
properties:
type:
type: string
default: ""b""
AorB:
oneOf:
- $ref: ""#/components/schemas/A""
- $ref: ""#/components/schemas/B""
discriminator:
propertyName: type
mapping:
a: ""#/components/schemas/A""
b: ""#/components/schemas/B""
responses:
AorBResponse:
description: mandatory
content:
application/json:
schema:
$ref: ""#/components/schemas/AorB""");
var mockLogger = new Mock<ILogger<KiotaBuilder>>();
var generationConfiguration = new GenerationConfiguration { ClientClassName = "Graph", OpenAPIFilePath = tempFilePath, Language = language }; // we can use any language that creates wrapper types for composed types in different ways
var builder = new KiotaBuilder(mockLogger.Object, generationConfiguration, _httpClient);
var document = await builder.CreateOpenApiDocumentAsync(fs);
var node = builder.CreateUriSpace(document);
var codeModel = builder.CreateSourceModel(node);
await builder.ApplyLanguageRefinementAsync(generationConfiguration, codeModel, CancellationToken.None);
var requestBuilderNamespace = codeModel.FindNamespaceByName("ApiSdk.api.all");
Assert.NotNull(requestBuilderNamespace);
if (language == GenerationLanguage.TypeScript || language == GenerationLanguage.Go)
{// these languages use CodeFiles
var requestExecutorMethod = codeModel.FindChildByName<CodeMethod>("post");
Assert.NotNull(requestExecutorMethod);
var returnType = requestExecutorMethod.ReturnType as CodeType;
var returnTypeDefinition = returnType.TypeDefinition as CodeInterface;
if (language == GenerationLanguage.TypeScript)
{
Assert.Equal(2, returnTypeDefinition.Properties.Count());
}
if (language == GenerationLanguage.Go)
{
Assert.Equal(4, returnTypeDefinition.Methods.Count());// a getter and a setter for each property in Go.
}
}
else
{
var allRequestBuilderClass = requestBuilderNamespace.FindChildByName<CodeClass>("allRequestBuilder", false);
var executor = allRequestBuilderClass.Methods.FirstOrDefault(m => m.IsOfKind(CodeMethodKind.RequestExecutor));
Assert.NotNull(executor);
var returnType = executor.ReturnType as CodeType;
var returnTypeDefinition = returnType.TypeDefinition as CodeClass;
Assert.Equal(2, returnTypeDefinition.Properties.Count());
}
}
[Fact]
public async Task NamesComponentsInlineSchemasProperlyAsync()
{
Expand Down
Loading