Repository: microsoft/kiota (csharp)

Issue:
class to name conflict avoidance behaviour colliding with error default message behaviour in CSharp
### What are you generating using Kiota, clients or plugins?

API Client/SDK

### In what context or format are you using Kiota?

Windows executable

### Client library/SDK language

Csharp

### Describe the bug

I have a controller defined as follows:

~~~csharp
[ProducesResponseType(typeof(IEnumerable<Message1>), StatusCodes.Status400BadRequest)]
public class EnvironmentController(IEnvironmentInfoBL environmentInfoBL) : ControllerBase
~~~

With `Message1` defined as follows:

~~~csharp
public class Message1
{
    public string Something { get; set; }
}
~~~

The generated `Message1` model of kiota is as expected (I've omitted some of the lines for brevity):

~~~csharp
    #pragma warning disable CS1591
    public class Message1 : ApiException, IParsable
    #pragma warning restore CS1591
    {
        /// <summary>The primary error message.</summary>
        public override string Message { get => base.Message; }
        /// <summary>The something property</summary>
#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP3_1_OR_GREATER
#nullable enable
        public string? Something { get; set; }
#nullable restore
#else
        public string Something { get; set; }
#endif
   // etc.
    }
~~~

Now I just rename the class `Message` instead. I change nothing else, but just re-generate the C# code using the same kiota version. And here is what I get (again, some the lines that are identical are omitted for brevity):

~~~csharp
    #pragma warning disable CS1591
    public class Message : ApiException, IParsable
    #pragma warning restore CS1591
    {
        /// <summary>The primary error message.</summary>
        public override string MessageProp { get => base.Message; }
        /// <summary>The something property</summary>
#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP3_1_OR_GREATER
#nullable enable
        public string? Something { get; set; }
#nullable restore
#else
        public string Something { get; set; }
#endif
    }
~~~

This fails, because there is no `public override string MessageProp { get => base.Message; }`!

For the life of me, I can't fathom where this `MessageProp` property comes from. I don't even know where the summary comment of the property comes from, only that it doesn't occur in any of our code or internal frameworks.
It seems to be related to https://github.com/microsoft/kiota/blob/0254c190104cc94ae800afeb63ba1d9ea6582674/src/Kiota.Builder/Refiners/CommonLanguageRefiner.cs#L1504 but I don't know how.

I do not understand how renaming a type can have such an effect.


### Expected behavior

Except for the renaming, the generated code should be exactly the same

### How to reproduce

See the above description. 

### Open API description file

```json
{
  "x-generator": "NSwag v14.0.3.0 (NJsonSchema v11.0.0.0 (Newtonsoft.Json v13.0.0.0))",
  "openapi": "3.0.0",
  "info": {
    "title": "Facade contracts are only used by Guis and no perinity is provided.",
    "version": "1.0.0"
  },
  "paths": {
    "/Core/facade/Environment": {
      "get": {
        "tags": [
          "Environment"
        ],
        "summary": "Return details information about the application.",
        "operationId": "Environment_Get",
        "responses": {
          "400": {
            "description": "Error during the process of the request.",
            "content": {
              "application/json": {
                "schema": {
                  "type": "array",
                  "items": {
                    "$ref": "#/components/schemas/Message"
                  }
                }
              }
            }
          },
          "200": {
            "description": "Return the environment information.",
            "content": {
              "application/json": {
                "schema": {
                  "nullable": true,
                  "oneOf": [
                    {
                      "$ref": "#/components/schemas/EnvironmentInfo"
                    }
                  ]
                }
              }
            }
          }
        }
      }
    }
  },
  "components": {
    "schemas": {
      "Message": {
        "type": "object",
        "additionalProperties": false,
        "properties": {
          "something": {
            "type": "string"
          }
        }
      },
      "EnvironmentInfo": {
        "type": "object",
        "additionalProperties": false,
        "properties": {
          "name": {
            "type": "string",
            "nullable": true
          },
          "server": {
            "type": "string",
            "nullable": true
          },
          "databaseInfo": {
            "type": "array",
            "nullable": true,
            "items": {
              "$ref": "#/components/schemas/DatabaseInfo"
            }
          }
        }
      },
      "DatabaseInfo": {
        "type": "object",
        "additionalProperties": false,
        "properties": {
          "server": {
            "type": "string",
            "nullable": true
          },
          "database": {
            "type": "string",
            "nullable": true
          }
        }
      }
    }
  }
}
```

### Kiota Version

1.15.0+b535a94064cd8c14a022aaba42964467d5db525a

### Latest Kiota version known to work for scenario above?(Not required)

_No response_

### Known Workarounds

No realistic workarounds, we can't just start renaming stuff just because of a code generation anomaly. What's so special about `Message` that it cannot be used?

### Configuration

Windows 10, 64 bit, VS2022 ASP.NET Core 8.0

### Debug output

Debug doesn't seem to work.

### Other information

None

Pull request description:
Fixes https://github.com/microsoft/kiota/issues/4893

In summary, this PR

- Updates the `ReplaceReservedExceptionPropertyNames` method to also rename the error class if it is the same name as a reserved class to avoid collision with class name and property name.
- Updates the `AddPrimaryErrorMessage` to rename existing matching properties if they already exist in the error model to avoid information/property loss (otherwise we will fail to add the override and lead to invalid generation)
- Add tests to validate the scenarios.

Essentially, in a scenario where a `ErrorMessageOverride` property kind named `message` is added,
- if the error model class is also called `message`, the model class will be renamed to avoid class and property name collision.
- if a property with the same name already exists in the model, the pre-existing property is renamed to ensure the suitable override is added (if the type does not match).
- otherwise, normal behavior of adding the override property will be used.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 484ca91ea8..4e715171e6 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/)
 - 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
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<string, string> replacement, HashSet<Type>? codeElementExceptions = null, Func<CodeElement, bool>? 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,


Test patch:
diff --git a/tests/Kiota.Builder.Tests/Refiners/CSharpLanguageRefinerTests.cs b/tests/Kiota.Builder.Tests/Refiners/CSharpLanguageRefinerTests.cs
index 27b5091fd4..cb85b7cb0a 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,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()