class to name conflict avoidance behaviour colliding with error default message behaviour in CSharp

[vvdb-architecture]

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:

[ProducesResponseType(typeof(IEnumerable<Message1>), StatusCodes.Status400BadRequest)]
public class EnvironmentController(IEnvironmentInfoBL environmentInfoBL) : ControllerBase

With Message1 defined as follows:

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):

    #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):

    #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

kiota/src/Kiota.Builder/Refiners/CommonLanguageRefiner.cs

Line 1504 in 0254c19

internal static void AddPrimaryErrorMessage(CodeElement currentElement, string name, Func<CodeType> type, bool asProperty = false)

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

{
  "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

[baywet]

Hi @vvdb-architecture
Thanks for using kiota and for reaching out.
This will happen if a property name is the same as the class name it belongs to. This is done because otherwise, it confuses the C# compiler.

kiota/src/Kiota.Builder/Refiners/CSharpRefiner.cs

Line 126 in b5ca324

sameNameProperty.Name = $"{sameNameProperty.Name}Prop";

Let us know if you have further questions.

[vvdb-architecture]

Hi @vvdb-architecture Thanks for using kiota and for reaching out. This will happen if a property name is the same as the class name it belongs to. This is done because otherwise, it confuses the C# compiler.

kiota/src/Kiota.Builder/Refiners/CSharpRefiner.cs

Line 126 in b5ca324

sameNameProperty.Name = $"{sameNameProperty.Name}Prop";

Let us know if you have further questions.

I first didn't t understand your answer. If you look at the definition:

"Message": {
        "type": "object",
        "additionalProperties": false,
        "properties": {
          "something": {
            "type": "string"
          }
        }

I don't know where that Message property is.

But then in the code generation it seems that you derive this class from ApiException, which in turn derives from Exception, which contains a Message property. Aha!

But then, why does kiota generates an override? In fact, that's why I filled an issue: the generated code just doesn't compile.

  /// <summary>The primary error message.</summary>
        public override string MessageProp { get => base.Message; }

This compilation fails because there is nothing to override.
Either you override the property with the same name (if the property is virtual), or you define a property with a different name.
One could argue that in this case, there is no ambiguity. But I don't really know: it's a bit difficult to find out what the right thing to do is here, or I would consider a PR. But not now: I need advice from an expert.

On a somewhat related matter, when looking at the code you referenced, the mechanism looks a bit fragile. If both a Message and a MessageProp exist in the original definition, you'll end up with MessageProp and MessagePropProp which have MessageProp in common... there be dragons!

[andrueastman]

Types that are used as error definitions end up with the override when the AddPrimaryErrorMessage method is called. This is to ensure the exception thrown in error scenarios also have the message property populated.

kiota/src/Kiota.Builder/Refiners/CommonLanguageRefiner.cs

Line 1504 in b5ca324

internal static void AddPrimaryErrorMessage(CodeElement currentElement, string name, Func<CodeType> type, bool asProperty = false)

Since the intent is to override the message property from the Exception class so as to enable meaningful exception messages, I don't think we should rename the property. Maybe the class should be renamed here.

This could be done by potentially updating the filter here to rename the errorDefinition className as well when its names match one of the reserved words for errors properties.

kiota/src/Kiota.Builder/Refiners/CommonLanguageRefiner.cs

Line 297 in b5ca324

static x => ((x is CodeProperty prop && prop.IsOfKind(CodePropertyKind.Custom)) || x is CodeMethod) && x.Parent is CodeClass parent && parent.IsOfKind(CodeClassKind.Model) && parent.IsErrorDefinition

Any chance you'd be willing to try that out and submit a PR?

[vvdb-architecture]

Isn't this problem caused by the fact that the model type inherits from ApiException? It really doesn't have to.
Why not remove the ApiException inheritance from the model type, and define an ApiException<T> instead, which would hold the reference to the model instance instead. That would avoid the is ApiException test in methods like ThrowIfFailedResponse and keep the model "clean" of possibly conflicting inheritance.

Granted, it would also be a problem for those clients who presuppose that some models inherit from ApiException, so probably the change at this point would be impossible.

[baywet]

@vvdb-architecture how would you throw an instance of something that doesn't inherit from exception? (which is the parent of ApiException and where the message property is ultimately defined)

[vvdb-architecture]

Well, of course I don't know the code very well, but if you look at https://github.com/microsoft/kiota-http-dotnet/blob/6c1e799f63a26b9209e7153d7f1eb2d2c730e9f6/src/HttpClientRequestAdapter.cs#L381 you see this:

   if(result is not Exception ex)
                throw new ApiException($"The server returned an unexpected status code and the error registered for this code failed to deserialize: {statusCodeAsString}")
                {
                    ResponseStatusCode = statusCodeAsInt,
                    ResponseHeaders = responseHeadersDictionary
                };
            if(result is ApiException apiEx)
            {
                apiEx.ResponseStatusCode = statusCodeAsInt;
                apiEx.ResponseHeaders = responseHeadersDictionary;
            }

The result comes from the errorMapping, which in my case contains the Message.
This tells me we don't need to inherit from ApiException. But if we don't we lose the information in result: that's because there is no ApiException<T> or something like that that can hold the result for us.
This forces us to inherit from ApiException. There may be other constraints that forces that kind of inheritance, but I don't know the kiota code very well.
If ignorance is bliss, I would just say to go for composition instead of inheritance, but I fear ignorance isn't bliss in this case.

[vvdb-architecture]

Go at the time didn't support generic types (some other languages have limitations), and we wanted a consistent solution.

It's strange that you bring "Go" into this discussion. What has C# code generation to do with Go, TypeScript, ....: surely there is no rule that says you are limited to the lowest common denominator of the implementable patterns in all the supported languages? Everything starts with the single OpenAPI spec and you don't program in Go like you program in C#, TypeScript, Java, ...

I don't think we're going to change that now, unless we get overwhelming feedback people don't like it since it'd be a source breaking change.

That's fine with me. Something still needs to be done for the non-compileable code generation that override nonexistent methods. I'm out of my depth here, and I'll leave this up to someone with a better understanding than I.
Any chance you'd be willing to try straightening this out and submit a PR :-)?

[baywet]

Go: it's a delicate balance between inter-language consistency, and taking advantages of what the platform supports to offer a better experience. Leveraging platform specificities is one of the main reasons why TypeScript composed types support is still not available (we're getting there), which itself is one of the only remaining reasons why TypeScript is not stable yet. Those specificities have a higher cost.

To be clear, we're talking about this fix and nothing else at this point, correct? (i.e. we want the property name to remain Message and not MessageProp). I'm not forgetting about anything else?

[vvdb-architecture]

To be clear, we're talking about
#4893 (comment)
and nothing else at this point, correct? (i.e. we want the property name to remain Message and not MessageProp). I'm not forgetting about anything else?

If the name of the property is identical to the name of its enclosing type(as is the case here, because Kiota forces the model to inherit from ApiException and for some unknown reason forces the generation of a public property for Message), you need to rename it.

So, respecting the current mechanics of the Kiota code, in this case you need to rename the Message property to MessageProp and remove the override keyword since the property definition is not an override. Another solution would be to rename the enclosing type, but that would be strange: it's not the model definition's fault that Kiota insists to inherit from ApiException, causing the Message property to come into existence.

I wonder if you need to generate a public property MessageProp at all... after all, it just exposes the underlying Exception.Message property under a different name (i.e. it just calls base.Message). Why is the added value of redeclaring this in the model anyway?

And for the life of me, I still am unable to understand the motivation for all this. I'm still unable to find code evidence for @andrueastman's statement that this mechanism returns "a meaningful error message that is returned from the API.".

It's also very puzzling that the renaming is inconsistent in Kiota. For example, if Message were called MyMessage and defined as follows:

public class MyMessage
{
    public string Message { get; set; }
}

... then the appropriate schema section in the above OpenApi spec would become:

      "MyMessage": {
        "type": "object",
        "additionalProperties": false,
        "properties": {
          "message": {
            "type": "string"
          }
        }

... and because of the insistence of generating the model class inheriting from ApiException, the code generated by Kiota becomes (omitting some lines for brevity):

    #pragma warning disable CS1591
    public class MyMessage : ApiException, IParsable
    #pragma warning restore CS1591
    {
        /// <summary>The message property</summary>
#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP3_1_OR_GREATER
#nullable enable
        public string? MessageEscaped { get; set; }
#nullable restore
#else
        public string MessageEscaped { get; set; }
#endif
    }

I was expecting to see MessageProp, but now it has become MessageEscaped. And there is no public property that redefines Exception.Message in contrast to the case when the model type was called Message. This last thing makes sense: there is no point in defining a public property that just returns the value of an underlying property under a different name: there is no ambiguity in either case, neither in MyMessage nor in Message.

I do not understand this design. This is why any PR should be done by someone smarter than me. I would be glad if the generated code could just compile correctly so that I can really start investigating if this is a good replacement for NSwag.

[baywet]

Why is the added value of redeclaring this in the model anyway?

The fact that it calls the base property is just a placeholder. The idea is you would decorate your description with the right extension so kiota knows where to get the message value from in response payload. This way when the service returns an error an exception is thrown and the message you get in the exception is the main message from the service.

I think there are two possibly three mechanisms interacting with each other and adding to the confusion of the situation here:

• Whenever a described property name conflicts with a structural property from kiota it gets the "escaped" suffix is automatically added. This is valid for all models that kiota generates and for all languages.
• In C# whenever a property name is identical to its parent type name it gets the specific "prop" appended automatically.
• In C# error types have the name message reserved for a structural property this is where all the override and based call behavior comes from.

Assumptions probably have been made in the error mechanisms which is why we override behavior is not consistent when the property has been renamed to something else to avoid collisions. Thank you for your continuous investigation of the problem. I think you're close to getting to a solution. Let us know if you have further questions.

[vvdb-architecture]

. The idea is you would decorate your description with the right extension so kiota knows where to get the message value from in response payload.

I've spend time reading https://learn.microsoft.com/en-us/openapi/kiota/errors, #3744 and #3066 and could find no reference about how to "decorate your description with the right extension".

I think you're close to getting to a solution.

I think not. The generated code does not compile. Your reactivity is much better than with NSwag (that's not very difficult) but I am in exactly the same place as when I started this issue, with a lot more questions and incomprehension. I understand you made some design decisions that are irreversible. That's OK, but I shudder to think about my incomprehension if/when Kiota will generate something that actually compiles correctly.

I do appreciate your answers. But I didn't expect to invest so much time for such a simple issue. For me, this stops here. Kiota isn't the product we are looking for, even if we had time to invest in its development.

[baywet]

Understood.
Thanks for all the research done here. Re-opening so we can fix the bug at a later.

[darrelmiller]

I do appreciate your answers. But I didn't expect to invest so much time for such a simple issue. For me, this stops here. Kiota isn't the product we are looking for, even if we had time to invest in its development.

We appreciate your input here. Unfortunately translating HTTP failure modes into programing language exceptions in a way that is both maintainable across many languages and feels natural in each of those languages isn't a simple issue.

My gut reaction to this issue is that inheriting from ApiException does feel wrong to me. But I have not been involved in the implementation details of this area, so I am not intimately familiar with all the challenges. Even without generics, I feel like I would rather catch an APIException class that has Content property on it that I need to cast. e.g. something like

catch(ApiException exception) {
  var message = exception.Content as Message;
  ...
}