From 86fd8932e8e91a0707ae67a1c6008af9aed8f704 Mon Sep 17 00:00:00 2001 From: Andrew Omondi Date: Wed, 3 Jul 2024 12:11:07 +0300 Subject: [PATCH 1/3] Fixes property name disambiguation --- CHANGELOG.md | 1 + .../Refiners/CommonLanguageRefiner.cs | 12 ++++- .../Refiners/CSharpLanguageRefinerTests.cs | 49 ++++++++++++++++++- 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2136947087..05a29f6889 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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/) ## [1.15.0] - 2024-06-06 diff --git a/src/Kiota.Builder/Refiners/CommonLanguageRefiner.cs b/src/Kiota.Builder/Refiners/CommonLanguageRefiner.cs index b4073dc385..3bff4b8ff3 100644 --- a/src/Kiota.Builder/Refiners/CommonLanguageRefiner.cs +++ b/src/Kiota.Builder/Refiners/CommonLanguageRefiner.cs @@ -294,7 +294,9 @@ protected static void ReplaceReservedExceptionPropertyNames(CodeElement current, 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 replacement, HashSet? codeElementExceptions = null, Func? shouldReplaceCallback = null) @@ -1507,6 +1509,14 @@ internal static void AddPrimaryErrorMessage(CodeElement currentElement, string n { 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, diff --git a/tests/Kiota.Builder.Tests/Refiners/CSharpLanguageRefinerTests.cs b/tests/Kiota.Builder.Tests/Refiners/CSharpLanguageRefinerTests.cs index 27b5091fd4..9fcff4b818 100644 --- a/tests/Kiota.Builder.Tests/Refiners/CSharpLanguageRefinerTests.cs +++ b/tests/Kiota.Builder.Tests/Refiners/CSharpLanguageRefinerTests.cs @@ -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 { @@ -461,9 +482,33 @@ 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); + Assert.True(exception.Properties.First().IsOfKind(CodePropertyKind.ErrorMessageOverride));// property is now message override + Assert.Equal("Message", exception.Properties.First().Name); + } + [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 + Assert.Equal("message", propToAdd.Name, StringComparer.OrdinalIgnoreCase); } [Fact] public async Task DoesNotReplaceNonExceptionPropertiesNames() From 5931f0bf46f36f404ac95ffd3894d1811e4d5d84 Mon Sep 17 00:00:00 2001 From: Andrew Omondi Date: Wed, 3 Jul 2024 12:39:57 +0300 Subject: [PATCH 2/3] Adds more tests --- .../Refiners/CSharpLanguageRefinerTests.cs | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/Kiota.Builder.Tests/Refiners/CSharpLanguageRefinerTests.cs b/tests/Kiota.Builder.Tests/Refiners/CSharpLanguageRefinerTests.cs index 9fcff4b818..1d783cb158 100644 --- a/tests/Kiota.Builder.Tests/Refiners/CSharpLanguageRefinerTests.cs +++ b/tests/Kiota.Builder.Tests/Refiners/CSharpLanguageRefinerTests.cs @@ -509,6 +509,32 @@ public async Task RenamesExceptionClassWithReservedPropertyName() await ILanguageRefiner.Refine(new GenerationConfiguration { Language = GenerationLanguage.CSharp }, root); Assert.Equal("messageEscaped", exception.Name, StringComparer.OrdinalIgnoreCase);// class is renamed Assert.Equal("message", propToAdd.Name, StringComparer.OrdinalIgnoreCase); + Assert.Single(exception.Properties); + 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 + Assert.Equal("something", propToAdd.Name, StringComparer.OrdinalIgnoreCase); + Assert.Equal(2, exception.Properties.Count()); // initial property plus primary message + Assert.Equal("Message", exception.Properties.ToArray()[0].Name); + Assert.Equal("Something", exception.Properties.ToArray()[1].Name); } [Fact] public async Task DoesNotReplaceNonExceptionPropertiesNames() From 447f582f222db6d0208ef7bbfb334c3513529577 Mon Sep 17 00:00:00 2001 From: Andrew Omondi Date: Wed, 3 Jul 2024 12:45:11 +0300 Subject: [PATCH 3/3] Cleanup comments --- .../Refiners/CSharpLanguageRefinerTests.cs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/Kiota.Builder.Tests/Refiners/CSharpLanguageRefinerTests.cs b/tests/Kiota.Builder.Tests/Refiners/CSharpLanguageRefinerTests.cs index 1d783cb158..cb85b7cb0a 100644 --- a/tests/Kiota.Builder.Tests/Refiners/CSharpLanguageRefinerTests.cs +++ b/tests/Kiota.Builder.Tests/Refiners/CSharpLanguageRefinerTests.cs @@ -485,9 +485,9 @@ public async Task DoesNotRenamePrimaryErrorMessageIfMatchAlreadyExists() Assert.False(exception.Properties.First().IsOfKind(CodePropertyKind.ErrorMessageOverride));// property is NOT message override await ILanguageRefiner.Refine(new GenerationConfiguration { Language = GenerationLanguage.CSharp }, root); Assert.Equal("message", propToAdd.Name, StringComparer.OrdinalIgnoreCase);// property remains - Assert.Single(exception.Properties); + 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); + Assert.Equal("message", exception.Properties.First().Name, StringComparer.OrdinalIgnoreCase); // name is expected. } [Fact] public async Task RenamesExceptionClassWithReservedPropertyName() @@ -507,9 +507,9 @@ public async Task RenamesExceptionClassWithReservedPropertyName() } }).First(); await ILanguageRefiner.Refine(new GenerationConfiguration { Language = GenerationLanguage.CSharp }, root); - Assert.Equal("messageEscaped", exception.Name, StringComparer.OrdinalIgnoreCase);// class is renamed - Assert.Equal("message", propToAdd.Name, StringComparer.OrdinalIgnoreCase); - Assert.Single(exception.Properties); + 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] @@ -530,11 +530,11 @@ public async Task RenamesExceptionClassWithReservedPropertyNameWhenPropertyIsIni } }).First(); await ILanguageRefiner.Refine(new GenerationConfiguration { Language = GenerationLanguage.CSharp }, root); - Assert.Equal("messageEscaped", exception.Name, StringComparer.OrdinalIgnoreCase);// class is renamed - Assert.Equal("something", propToAdd.Name, StringComparer.OrdinalIgnoreCase); + 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); - Assert.Equal("Something", exception.Properties.ToArray()[1].Name); + 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()