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 ErrorMessageOverride collision with classname #4930

Merged
merged 5 commits into from
Jul 3, 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 @@ -25,6 +25,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Ensures HashSet properties in `KiotaLock` maintain IgnoreCase comparer across runs [#4916](https://github.com/microsoft/kiota/issues/4916)
- Dropped `client base url set to` message when generating plugins. [#4905](https://github.com/microsoft/kiota/issues/4905)
- Emit `[GeneratedCode]` attribute for C# types. [#4907](https://github.com/microsoft/kiota/issues/4907)
- Fixes error property disambiguation when the property has the same name as class [#4893](https://github.com/microsoft/kiota/issues/)
- Fixes missing imports for `UntypedNode` for method parameter and return value scenarios. [#4925](https://github.com/microsoft/kiota/issues/4925)

## [1.15.0] - 2024-06-06
Expand Down
12 changes: 11 additions & 1 deletion 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 @@ -157,7 +157,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 160 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 160 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 @@ -294,10 +294,12 @@
provider,
replacement,
null,
static x => ((x is CodeProperty prop && prop.IsOfKind(CodePropertyKind.Custom)) || x is CodeMethod) && x.Parent is CodeClass parent && parent.IsOfKind(CodeClassKind.Model) && parent.IsErrorDefinition
x => (((x is CodeProperty prop && prop.IsOfKind(CodePropertyKind.Custom)) || x is CodeMethod) && x.Parent is CodeClass parent && parent.IsOfKind(CodeClassKind.Model) && parent.IsErrorDefinition) // rename properties or method of error classes matching the reserved names.
|| (x is CodeClass codeClass && codeClass.IsOfKind(CodeClassKind.Model) && codeClass.IsErrorDefinition
&& codeClass.Properties.FirstOrDefault(classProp => provider.ReservedNames.Contains(classProp.Name)) is { } matchingProperty && matchingProperty.Name.Equals(codeClass.Name, StringComparison.OrdinalIgnoreCase)) // rename the a 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 302 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 @@ -366,7 +368,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 371 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 @@ -410,7 +412,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 415 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 @@ -621,9 +623,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 626 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 628 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 Expand Up @@ -1507,6 +1509,14 @@
{
if (asProperty)
{
if (currentClass.Properties.FirstOrDefault(property => property.Name.Equals(name, StringComparison.OrdinalIgnoreCase)) is { } sameNameProperty)
{
if (sameNameProperty.Type.Name.Equals(type().Name, StringComparison.OrdinalIgnoreCase))
currentClass.RemoveChildElementByName(name); // type matches so just remove it for replacement
else
currentClass.RenameChildElement(name, $"{name}Escaped"); // type mismatch so just rename it
}

currentClass.AddProperty(new CodeProperty
{
Name = name,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,27 @@ public async Task KeepsCancellationParametersInRequestExecutors()
}
[Fact]
public async Task ReplacesExceptionPropertiesNames()
{
var exception = root.AddClass(new CodeClass
{
Name = "error403",
Kind = CodeClassKind.Model,
IsErrorDefinition = true,
}).First();
var propToAdd = exception.AddProperty(new CodeProperty
{
Name = "stacktrace",
Type = new CodeType
{
Name = "string"
}
}).First();
await ILanguageRefiner.Refine(new GenerationConfiguration { Language = GenerationLanguage.CSharp }, root);
Assert.Equal("stacktraceEscaped", propToAdd.Name, StringComparer.OrdinalIgnoreCase);
Assert.Equal("stacktrace", propToAdd.SerializationName, StringComparer.OrdinalIgnoreCase);
}
[Fact]
public async Task DoesNotRenamePrimaryErrorMessageIfMatchAlreadyExists()
{
var exception = root.AddClass(new CodeClass
{
Expand All @@ -461,9 +482,59 @@ public async Task ReplacesExceptionPropertiesNames()
Name = "string"
}
}).First();
Assert.False(exception.Properties.First().IsOfKind(CodePropertyKind.ErrorMessageOverride));// property is NOT message override
await ILanguageRefiner.Refine(new GenerationConfiguration { Language = GenerationLanguage.CSharp }, root);
Assert.Equal("messageEscaped", propToAdd.Name, StringComparer.OrdinalIgnoreCase);
Assert.Equal("message", propToAdd.SerializationName, StringComparer.OrdinalIgnoreCase);
Assert.Equal("message", propToAdd.Name, StringComparer.OrdinalIgnoreCase);// property remains
Assert.Single(exception.Properties); // no new properties added
Assert.True(exception.Properties.First().IsOfKind(CodePropertyKind.ErrorMessageOverride));// property is now message override
Assert.Equal("message", exception.Properties.First().Name, StringComparer.OrdinalIgnoreCase); // name is expected.
}
[Fact]
public async Task RenamesExceptionClassWithReservedPropertyName()
{
var exception = root.AddClass(new CodeClass
{
Name = "message",
Kind = CodeClassKind.Model,
IsErrorDefinition = true,
}).First();
var propToAdd = exception.AddProperty(new CodeProperty
{
Name = "message",
Type = new CodeType
{
Name = "string"
}
}).First();
await ILanguageRefiner.Refine(new GenerationConfiguration { Language = GenerationLanguage.CSharp }, root);
Assert.Equal("messageEscaped", exception.Name, StringComparer.OrdinalIgnoreCase);// class is renamed to avoid removing special overidden property
Assert.Equal("message", propToAdd.Name, StringComparer.OrdinalIgnoreCase); // property is unchanged
Assert.Single(exception.Properties); // no new properties added
Assert.Equal("message", exception.Properties.First().Name, StringComparer.OrdinalIgnoreCase);
}
[Fact]
public async Task RenamesExceptionClassWithReservedPropertyNameWhenPropertyIsInitiallyAbsent()
{
var exception = root.AddClass(new CodeClass
{
Name = "message",
Kind = CodeClassKind.Model,
IsErrorDefinition = true,
}).First();
var propToAdd = exception.AddProperty(new CodeProperty
{
Name = "something",
Type = new CodeType
{
Name = "string"
}
}).First();
await ILanguageRefiner.Refine(new GenerationConfiguration { Language = GenerationLanguage.CSharp }, root);
Assert.Equal("messageEscaped", exception.Name, StringComparer.OrdinalIgnoreCase);// class is renamed to avoid removing special overidden property
Assert.Equal("something", propToAdd.Name, StringComparer.OrdinalIgnoreCase); // existing property remains
Assert.Equal(2, exception.Properties.Count()); // initial property plus primary message
Assert.Equal("message", exception.Properties.ToArray()[0].Name, StringComparer.OrdinalIgnoreCase); // primary error message is present
Assert.Equal("something", exception.Properties.ToArray()[1].Name, StringComparer.OrdinalIgnoreCase);// existing property remains
}
[Fact]
public async Task DoesNotReplaceNonExceptionPropertiesNames()
Expand Down
Loading