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

Take exception message from OpenAPI document #4349

Open
bkoelman opened this issue Mar 16, 2024 · 16 comments
Open

Take exception message from OpenAPI document #4349

bkoelman opened this issue Mar 16, 2024 · 16 comments
Labels
enhancement New feature or request generator Issues or improvements relater to generation capabilities. help wanted Issue caused by core project dependency modules or library
Milestone

Comments

@bkoelman
Copy link

Today, when a non-successful status code is returned (which is described in the OpenAPI document), an exception is thrown with an unhelpful generic message:

Exception of type 'MyProject.Models.ErrorResponseDocument' was thrown.

In contrast, NSwag takes the message from the OpenAPI document.

For comparison, here are my test assertions from Kiota:

var exception = (await action.Should().ThrowExactlyAsync<ErrorResponseDocument>()).Which;
exception.ResponseStatusCode.Should().Be((int)HttpStatusCode.Conflict);
exception.Message.Should().Be($"Exception of type '{typeof(ErrorResponseDocument).FullName}' was thrown.");
exception.Errors.ShouldHaveCount(1);

Compared with those from NSwag:

var exception = (await action.Should().ThrowExactlyAsync<ApiException<ErrorResponseDocument>>()).Which;
exception.StatusCode.Should().Be((int)HttpStatusCode.Conflict);
exception.Message.Should().Be("HTTP 409: The request body contains conflicting information or another resource with the same ID already exists.");
exception.Result.Errors.ShouldHaveCount(1);

Here's the OpenAPI fragment the code was generated against:

"responses": {
  "400": {
    "description": "The query string is invalid or the request body is missing or malformed.",
    "content": {
      "application/vnd.api+json": {
        "schema": {
          "$ref": "#/components/schemas/errorResponseDocument"
        }
      }
    }
  },
  "409": {
    "description": "The request body contains conflicting information or another resource with the same ID already exists.",
    "content": {
      "application/vnd.api+json": {
        "schema": {
          "$ref": "#/components/schemas/errorResponseDocument"
        }
      }
    }
  },
  "422": {
    "description": "Validation of the request body failed.",
    "content": {
      "application/vnd.api+json": {
        "schema": {
          "$ref": "#/components/schemas/errorResponseDocument"
        }
      }
    }
  }
}

And, for reference, our shared ApiException class we point NSwag to, where we format the exception message by prefixing it with the HTTP status code (note: the constructor signatures are prescribed by NSwag):

// We cannot rely on generating ApiException as soon as we are generating multiple clients, see https://github.com/RicoSuter/NSwag/issues/2839#issuecomment-776647377.
// Instead, we configure NSwag to point to the exception below in the generated code.

namespace JsonApiDotNetCore.OpenApi.Client.NSwag;

public class ApiException(string message, int statusCode, string? response, IReadOnlyDictionary<string, IEnumerable<string>> headers, Exception? innerException)
    : Exception($"HTTP {statusCode}: {message}", innerException)
{
    public int StatusCode { get; } = statusCode;
    public virtual string? Response { get; } = string.IsNullOrEmpty(response) ? null : response;
    public IReadOnlyDictionary<string, IEnumerable<string>> Headers { get; } = headers;
}

public sealed class ApiException<TResult>(
    string message, int statusCode, string? response, IReadOnlyDictionary<string, IEnumerable<string>> headers, TResult result, Exception? innerException)
    : ApiException(message, statusCode, response, headers, innerException)
{
    public TResult Result { get; } = result;
    public override string Response => $"The response body is unavailable. Use the {nameof(Result)} property instead.";
}

So, to summarize, would it be possible for Kiota to use the text from the OpenAPI document as the Exception.Message value of the generated exception class? It should be a parameter (like in NSwag), because the same exception type could be used for multiple status codes (each with a different message).

@github-project-automation github-project-automation bot moved this to Todo in Kiota Mar 16, 2024
@baywet baywet added enhancement New feature or request help wanted Issue caused by core project dependency modules or library generator Issues or improvements relater to generation capabilities. labels Mar 18, 2024
@baywet baywet added this to the Backlog milestone Mar 18, 2024
@baywet
Copy link
Member

baywet commented Mar 18, 2024

Thanks for the suggestion @bkoelman!

To add some context to that: kiota was designed to get the exception message from the response payload.
But in some cases the response payload is missing, not documented to have a primary error message, or cannot be deserialized (not schema, content type not supported).
In those cases, it could default the message to what's in the description.
We already have the infrastructure to model that, the description message could be modeled as a code constant.

Is this something you'd be willing to submit a pull request for?

@bkoelman
Copy link
Author

Perhaps in the future, but not anytime soon.

It would also be helpful to have an out-of-the-box default implicit handler for HTTP 500, if that's not in the OpenAPI document. It should read the first few kb of the response body and use that for the exception message, if unable to match the component schema or the content type is incompatible.

The reason is that it feels odd to add 500 as an expected outcome for every endpoint. And even if I did, I can't guarantee the response structure. This would mean a world of difference in complex systems that just log exception.ToString() as a fallback. Nswag does that by default.

@baywet
Copy link
Member

baywet commented Mar 18, 2024

This second suggestion could have serious performance impacts in some scenarios. I don't think we'll implement that.

As for the default handler, you can use code XXX (as opposed to 503 or 5XX) to do that today already.
We're also thinking about implementing overlays so you don't have to maintain a copy of the description

@bkoelman
Copy link
Author

Can you elaborate on the performance impact? I would have thought reading the first few kb avoids that.

@bkoelman
Copy link
Author

bkoelman commented Mar 19, 2024

Well, I tried adding a 5XX response, but that's a complete disaster:

{
  "responses": {
    // ....
    "400": {
      "description": "The query string is invalid.",
      "content": {
        "application/vnd.api+json": {
          "schema": {
            "$ref": "#/components/schemas/errorResponseDocument"
          }
        }
      }
    },
    "5XX": {
      "description": "Unexpected error",
      "content": {
        "text/plain": {}
      }
    }
  }
}

This makes kiota generate string? as the method return type (so for all status codes, including success). Same when I replace 5XX with default.

Then I tried adding the content type and component schema:

{
  "responses": {
    // ....
    "400": {
      "description": "The query string is invalid.",
      "content": {
        "application/vnd.api+json": {
          "schema": {
            "$ref": "#/components/schemas/errorResponseDocument"
          }
        }
      }
    },
    "5XX": {
      "description": "Unexpected error",
      "content": {
        "application/vnd.api+json": {
          "schema": {
            "$ref": "#/components/schemas/errorResponseDocument"
          }
        }
      }
    }
  }
}

Which does work properly. But changing "5XX" to "default" generates error mappings for "4XX" and "5XX", which is wrong. It should catch any other status code, not just errors.

But this is all just for fun. I'm not going to add content-type and schema for "something unexpected happened", because, like I said, I can't guarantee any format or structure of the response body in such cases. And I'm not even going to add "5XX" or "default" at all, because the OpenAPI spec tells me it shouldn't be done:

Note that an API specification does not necessarily need to cover all possible HTTP response codes, since they may not be known in advance. However, it is expected to cover successful responses and any known errors. By “known errors” we mean, for example, a 404 Not Found response for an operation that returns a resource by ID, or a 400 Bad Request response in case of invalid operation parameters.


At this point, I'm very disappointed by kiota. Not only have I come to realize it's still very immature, but more importantly, critical blockers are not being addressed. So far, I've only built simplistic demos (basically what a developer advocate does, nowhere near real-world usage) and already found 8 problems, for which I had to implement ugly workarounds. Users are already expressing concerns about kiota support while we haven't even released anything. Four months ago, when I wrote:

So I thought I'd give Kiota a try. Its code looks clean, is documented and the project is active. Using query strings was just the first issue I ran into, and I doubt it will be the only one. I'm curious if we can work together and find ways to address them. If all goes well, I'd be happy to drop support for NSwag in favor of Kiota, but it's too early for such a decision.

Absolutely.

I had something else in mind than you guys asking me to create a PR all the time. No? Then leave it lingering on the backlog forever. One time, I even got an issue assigned, before getting the chance to respond, pretty brutal. I'm not responsible for fixing your bugs and addressing its deficiencies.

When I was still studying, I worked part-time at a supermarket. I remember the full-time employees repeatedly getting frustrated by the impossible floor plans they'd get from the head quarters. Clearly, those "people at the office" had never actually visited a supermarket. I've built webbased enterprise systems for 20 years using .NET, but switched to the tech sector (building libraries like kiota) a few years ago. Likewise, I find it stunning how little "tech-sector" developers know or care about building actual user-facing systems. Seeing that from the inside, I realized there are plenty of examples within Microsoft too. Where teams are building things primarily for themselves, not what customers ask for. This got worse when Microsoft open-sourced projects and gave teams more freedom. Looking at the open/closed kiota issues, there's clear user feedback: core docs/samples are missing, versioning is a mess, lack of csproj integration (inner loop), handling unstructured data, logging, error handling, warnings we can't fix (so we'll just suppress all), extensibility, nullability (by the way, the top-voted issue). The theoretical approach to "just ensure your OpenAPI document is right and everything is perfectly modeled" shows lack of real-world experience. We can't always change it, nor do we want to maintain a "diff layer" that compensates for its imperfections, just to satisfy rigid kiota. What we need is easy ways to break out when the world isn't perfect. If that means creating an opportunity for people to do something wrong sometimes, or take a little performance hit sometimes, that's a sound tradeoff.

@baywet
Copy link
Member

baywet commented Mar 19, 2024

Can you elaborate on the performance impact? I would have thought reading the first few kb avoids that.

The first few kbs is a very arbitrary definition which will lead to people wanting the full thing. And if we provide the full thing by default, a large response could lead to memory pressure, network IOs and even a herd stampede in distributed systems when things are starting to fail.

This makes kiota generate string? as the method return type (so for all status codes, including success). Same when I replace 5XX with default.

I'm not sure how you got a string as a return type as a consequence of changing the error type. A schema like this one will be ignore as it's empty though.

Which does work properly. But changing "5XX" to "default" generates error mappings for "4XX" and "5XX", which is wrong. It should catch any other status code, not just errors.

Default was a design mistake on the specification side. Although it can be handy to save time to the description author, it's useless to most other audiences: is this an error, is this successful, what expectations does the service have on the client behaviour, etc... We made the opiniated choice that default means "error" as it's the broader usage we've seen out there, knowing perfectly that it wouldn't suit everybody. Our guidance is: don't use default. But overall yes, document very specific error codes that are known in advance, and if the service has a generic structure for errors, use the code class mapping (4XX, 5XX) as a shorthand.

On the last 3 paragraphs, feedback taken. We received similar feedback recently at an internal event. Looping in @sebastienlevert and @maisarissi our PMs to highlight this as further evidence for prioritization of the work.

@darrelmiller
Copy link
Member

@bkoelman I agree that having to add an explicit 5XX for every operation is a pain and shouldn't be necessary. But also, if you don't then you can't provide a custom error message. OpenAPI v3.x doesn't support defining responses at the global scope. We have proposed that for moonwalk though https://github.com/OAI/sig-moonwalk?tab=readme-ov-file#responses. Also, there is a proposal for the Overlays specification https://github.com/OAI/Overlay-Specification/pull/17/files?short_path=2df9558# that makes this easier. We are currently planning to add Overlay support to Kiota.

I'm sorry that you are disappointed with your experience. We appreciate your support to date. Fully supporting JSON:API would be awesome for Kiota, however, because it is a mature and opinionated protocol, I'm not surprised that we are running into scenarios that we don't currently support. We do not see much usage of JSON:API in our regular community because of the overlap with OData.

The effort to make this a fully OSS project does not come from Microsoft the organization, but from a few Microsoft employees that wanted an OpenAPI generator that fully supported the OpenAPI specification across a broad range of languages. We recognize that there is lots more work to do, but we have other responsibilities as well, beyond contributing to this project, and so it may feel like progress is slow sometimes. However, we are working to ensure that this project is a long term effort that Microsoft will continue to fund and so that we can reach the level of quality we all aspire too across a broad range of languages.

@bkoelman
Copy link
Author

Can you elaborate on the performance impact? I would have thought reading the first few kb avoids that.

The first few kbs is a very arbitrary definition which will lead to people wanting the full thing. And if we provide the full thing by default, a large response could lead to memory pressure, network IOs and even a herd stampede in distributed systems when things are starting to fail.

You're just making my point. Rejecting to solve a practical limitation, for theoretical reasons, which makes developers hate using kiota. Obviously, the team would never agree to fetch the full response, and for good reasons. But nobody is asking for that. So I looked it up: NSwag takes the first 512 bytes. It was implemented 10 years ago and worked well since. Why ignore proven solutions and reinvent the wheel all the time? I find it deeply concerning that I even need to explain this. So, just start with 512 bytes. In the unlikely case you get massive pushback, consider bumping that to 1 kb.

Here's a practical example: I'm debugging a complex solution with many services and layers, then hit an unhandled exception in the IDE. Right now this gives me useless information:

Exception of type 'MyProject.Models.ErrorResponseDocument' was thrown.

So now I need to guess where to add breakpoints (assuming it's easily reproducible). Or dive into log files, correlate requests, etc. While it should just be as simple as this:

> HTTP 500: Unexpected error. First 512 bytes of response body:
> 
> System.InvalidOperationException: The binary operator GreaterThan is not defined for the types 'System.String' and 'System.String'.
   at System.Linq.Expressions.Expression.GetUserDefinedBinaryOperatorOrThrow(ExpressionType binaryType, String name, Expression left, Expression right, Boolean liftToNull)
   at System.Linq.Expressions.Expression.GreaterThan(Expression left, Expression right, Boolean liftToNull, MethodInfo method)
   at JsonApiDotNetCore.Queries.QueryableBuilding.WhereClauseBuilder.VisitComp

This makes kiota generate string? as the method return type (so for all status codes, including success). Same when I replace 5XX with default.

I'm not sure how you got a string as a return type as a consequence of changing the error type. A schema like this one will be ignore as it's empty though.

Well, just try it. This is what I observed.

Which does work properly. But changing "5XX" to "default" generates error mappings for "4XX" and "5XX", which is wrong. It should catch any other status code, not just errors.

Default was a design mistake on the specification side. Although it can be handy to save time to the description author, it's useless to most other audiences: is this an error, is this successful, what expectations does the service have on the client behaviour, etc... We made the opiniated choice that default means "error" as it's the broader usage we've seen out there, knowing perfectly that it wouldn't suit everybody. Our guidance is: don't use default. But overall yes, document very specific error codes that are known in advance, and if the service has a generic structure for errors, use the code class mapping (4XX, 5XX) as a shorthand.

Then why did you suggest it? I didn't even know it was possible until you told me to:

As for the default handler, you can use code XXX (as opposed to 503 or 5XX) to do that today already.

@bkoelman
Copy link
Author

@bkoelman I agree that having to add an explicit 5XX for every operation is a pain and shouldn't be necessary. But also, if you don't then you can't provide a custom error message.

I'm fine with that. If the status appears in the OpenAPI document, kiota should utilize its information. Otherwise, it should provide best-effort information to help understand what's happened (see my previous comment).

OpenAPI v3.x doesn't support defining responses at the global scope. We have proposed that for moonwalk though https://github.com/OAI/sig-moonwalk?tab=readme-ov-file#responses. Also, there is a proposal for the Overlays specification https://github.com/OAI/Overlay-Specification/pull/17/files?short_path=2df9558# that makes this easier. We are currently planning to add Overlay support to Kiota.

Aw, that's a terrible idea. And then we need to implement overlays all the time, to compensate for kiota's inflexibility? This reminds me of XSLT and web.config transforms, which were impossible to debug. We need smart tools, not more layers of abstraction.

I'm sorry that you are disappointed with your experience. We appreciate your support to date. Fully supporting JSON:API would be awesome for Kiota, however, because it is a mature and opinionated protocol, I'm not surprised that we are running into scenarios that we don't currently support. We do not see much usage of JSON:API in our regular community because of the overlap with OData.

That's an unfair framing of the issues I've submitted. Except for the one about query strings, they are all general problems that anyone runs into pretty quickly.

The effort to make this a fully OSS project does not come from Microsoft the organization, but from a few Microsoft employees that wanted an OpenAPI generator that fully supported the OpenAPI specification across a broad range of languages. We recognize that there is lots more work to do, but we have other responsibilities as well, beyond contributing to this project, and so it may feel like progress is slow sometimes. However, we are working to ensure that this project is a long term effort that Microsoft will continue to fund and so that we can reach the level of quality we all aspire too across a broad range of languages.

It's not my place to judge what the team spends time on. What bothers me is continuously setting unfair expectations, resulting in disappointed users, losing trust, and damaging reputation. Keep doing this and the project will die soon.

  • As I understand it, the team's primary responsibility is the Microsoft Graph API. One of the current objectives is to build kiota, a tool that works great with the Microsoft Graph API, to address internal maintainability issues. Making it support other REST APIs is a stretch goal at best, if time permits. But that's unlikely to happen because the team is highly understaffed. Just honestly express that at all the places where outside developers are introduced to the project, so they won't expect the team to help serve their needs. When I asked if we could work together, the response should have been: "We can approve/reject proposals, but you'll have to do all the work yourself because we're currently not interested in serving the needs of outside developers", and I would have moved on, perhaps giving it another try in a few years. Now I feel deceived and that I wasted my time.
  • If the product isn't ready for widespread use, don't release a stable version. Don't say that support for some languages is stable. And don't have it advocated at conferences as a production-ready tool. Put a big red comment at the top of README.md: "This is experimental software, built by a few Microsoft employees. Use at your own risk."
  • If the team isn't ready to deal with a community, it shouldn't have been open-sourced in the first place. It should not have been put in the Microsoft GitHub organization unless it's officially backed and supported. Various members of the roslyn team provide tools under their personal account, which sets the right expectations: built by someone extremely experienced on the topic, but it may be abandoned at any time.

I found a few other quirks and oddities in kiota, but I don't see the point of taking the time to report them anymore. Because I know what the response will be, leaving me in the cold anyway. Let's not forget there's a small group of early adopters, willing to spend time on improving kiota. Every created issue is read many times by others, without ever responding. Once they turn their backs on you, it's over.

Because NSwag was recently abandoned, now is the window of opportunity to invest heavily in kiota. Primarily as a tool with excellent support for .NET and TypeScript (and awesome IDE integrations, which Microsoft is loved for). We hardly care about the other languages. OpenAPI originated in the .NET ecosystem, that's where most of its users are. Explaining why progress is so slow helps me understand, but does not motivate me to invest in kiota. Pretty sad.

@baywet
Copy link
Member

baywet commented Mar 20, 2024

Thanks for your invaluable input, these (painful) verbatims help shape internal decisions.

When I asked if we could work together, the response should have been: "We can approve/reject proposals, but you'll have to do all the work yourself because we're currently not interested in serving the needs of outside developers"

I don't think this is a fair representation of the situation for multiple reasons:

  • situations change, since you've originally reached out, we've been re-org'd 3 times. Not that it should be your concern, but it helps explain some of the impact here.
  • when we set out to build "yet another REST API code generator", I personally made a point to developing it as an open source project, and in such a way that it's a tool that any API consumer can have a great experience with. You couldn't imagine how many prior internal only initiatives lived and died because they didn't follow this path. You couldn't imagine how many Graph specific scenarios would have been much easier to implement if we have taken Graph specific shortcuts, and we had lots of influent people advocating to do so internally.
  • we have a successful track record of collaborating with the community to at least take the feedback in, and at best ensure their contributions get merged in swiftly.

Yes, our staffing challenges show up through multiple aspects (lack of documentation, some issues are not being addressed as fast as we'd like given other priorities, etc...), and this is one of the growing pains we've been trying to address with mitigated success.

I found a few other quirks and oddities in kiota, but I don't see the point of taking the time to report them anymore

Please do if you can find the energy, being the devil's advocate here, even if we (Microsoft) didn't do anything with those issues in the short term, it'd be a central point for others to tack on, and let us know this is something that needs attention, helping prioritize work in the mid-term.

Every created issue is read many times by others, without ever responding

If you can find issues where we haven't responded at all, please ping me personally on those. Throughout the staffing issues, we've made a point to ensuring every issue at least gets an initial reply and triaged. If a few items fell through the cracks, reminding us about those is always a net benefit for everyone.

Because NSwag was recently abandoned, now is the window of opportunity to invest heavily in kiota

This, and other generators being abandoned over the last 18 months, I believe proves the point that delivering a great code generation experience takes backing from an important industry player with vested interests in the API space. Which is what we're doing. These efforts represent, in my opinion, too much work to be supported by the community only (which is why we didn't go with any personal account to address your earlier comment). And the only sustainable alternative to those two models are paid for, generally closed source, venture capital, startup, kind of initiatives. Which I don't believe would put the community in a better place.

I know those kinds of conversations can feel tense sometimes due to the lossy nature of aync/text-only media. We value your general feedback, and our replies are here to provide good-hearted context. ❤️

@vvdb-architecture
Copy link
Contributor

Because NSwag was recently abandoned, now is the window of opportunity to invest heavily in kiota. Primarily as a tool with excellent support for .NET and TypeScript (and awesome IDE integrations, which Microsoft is loved for). We hardly care about the other languages. OpenAPI originated in the .NET ecosystem, that's where most of its users are. Explaining why progress is so slow helps me understand, but does not motivate me to invest in kiota. Pretty sad.

We are also desperate to find a good alternative to NSwag because the ability to generate code from OpenAPI is crucially important. We can't live without this functionality, and we suspect few people are. Moving from NSwag 13 to 14 was (and still is) painful.

There were internal discussions at my company about the best way to proceed given our limited resources. There is no problem in contributing to a project in our limited way, even in monetary terms. But we also encountered problems that lead to an incomprehension of the design decisions made by the Kiota team. It's funny that it was also regarding error handling. See #4893: don't let the title of the issue fool you, it was a different title and they changed it.

So yes, I feel your pain. There is something wrong with the design of Kiota, at least as far as exception handling is concerned. They talk about "using the right extensions" but without pointing to any realistic example. I'm glad it's not just me who are encountering problems even for trivial APIs.

@baywet
Copy link
Member

baywet commented Jul 5, 2024

Let's try to re-focus the discussion here on the technical hurdle. I'm first going to try to state the problem as accurately as possible, and then suggest a solution. If we can come to an agreement here, we'll be more than happy to receive a pull request.

Kiota is able to get a "developer friendly" error message from the payload only if an extension has been added to the description to let kiota know which property to use. While this works, it's not the simplest experience since it requires the developer to know about this extension and amend the OpenAPI description if the API producer didn't already do it. It'd be simpler if, while maintaining the current behavior, kiota fell back to additional defaults when the extension is absent from the description.

These defaults could come from:

  1. the first string property we find in an object schema
  2. the description/summary on the schema/response

An alternative to the defaults mechanism would be to support overlays and have API consumers create "a patch document" to insert the extension where required.

Does that sound like an accurate description of the problem? what do you think about the suggested solutions?

@vvdb-architecture
Copy link
Contributor

Thanks @baywet. Before I can reply and to make sure there is no misunderstanding, can you please clarify what you mean by "an extension added to the description"? The word "extension" is overloaded and I am unable to find documentation with the definition of the word "extension" in the context of this discussion. Thank you.

@darrelmiller
Copy link
Member

darrelmiller commented Jul 5, 2024

@bkoelman

OpenAPI originated in the .NET ecosystem, that's where most of its users are.

Just for historical correctness, OpenAPI, formerly Swagger, came from the Java ecosystem. Developers at Wordnik created the first Swagger tooling. This is one of the reasons SwaggerCodeGen is written in Java.

We hardly care about the other languages.

The Kiota team do care deeply about the other languages. This does give us less time to focus on any one specific language. This is a trade off, for sure.

@vvdb-architecture The term extension here is referring to OpenAPI specification extensions. The specific one being referred to is documented here, https://github.com/microsoft/OpenAPI/blob/main/extensions/x-ms-primary-error-message.md

@vvdb-architecture
Copy link
Contributor

vvdb-architecture commented Jul 6, 2024

the term extension here is referring to OpenAPI specification extensions.

Thank you. That clears it up.

So...

Respectfully, from our point of view (and I write "our" since I speak not only for myself), there are 3 design flaws here, the third being the direct consequence of the other two.

Flaw 1:

Kiota is able to get a "developer friendly" error message from the payload (#4349 (comment))

I'm a developer. "We" are a group of about 100 .NET developers at our company.
I think I can say with some confidence that having a "developer friendly" message is irrelevant. It's the least of our worries.
Developer care about the type of the exception and the content of any payload that may be sent with that exception.
Any friendly messages are for any exception propagated to the user/client that will end up in the UI if at all relevant and possible (in a multi-language environment, issuing a friendly message in the appropriate language has its own challenges).
Any other stuff is either dealt with or logged via structured logging.
But "friendly developer messages"? No, we don' care.
Having a design that forces you to use an extension to accomplish something we're not interested in is flawed.

Flaw 2:

...only if an extension has been added to the description to let Kiota know which property to use (#4349 (comment))

The flaw here is that it is assumed that the callers of the API are at all able to ask the team of the API to adjust their OpenAPI specification for this.
Next to .NET, we also have Java and other tech platform teams exposing APIs. I don't think asking them to modify their spec with an extension just because we want to use a tool that requires it to do something is going to fly.
Even asking other .NET teams to do so will be met with blank stares. No tool should force you to do that. So we won't add extensions to OpenAPI specs for something we don't care about.

If that was all there was, it would end here: we're not interested in friendly developer messages, nor in extensions. So we can just ignore it.
Alas, the design of Kiota in view of the above flaws automatically generate a third flaw which we can't ignore:

Flaw 3: inheriting from ApiException

I totally agree with @darrelmiller: forcing a model to inherit from ApiException is the fundamental flaw in the design.
There is no good reason to do this, and it forces the code generation to work around potential conflicts of property names. The correction of #4893 may cause the code to compile, but the design flaw remains.

The problem of course is that even if you're not interested in friendly developer messages, your models will still inherit from ApiException if they are used in an error return.

A model can be used either as a legitimate return payload, or as a payload from an error status code. I'm not saying this is good design, but it's allowed. Making models inherit from Exception (directly or indirectly) just because they are used in an error return is (in our opinion) wrong and just plain confusing.

At the risk of repeating myself, a better design (which is an improvement over even NSwag) would be to define a generic ApiException:

public class ApiException<TModel>: ApiException
{
    public TModel Model { get; }

    public ApiException(TModel model)
    {
        Model = model;
    }

    public ApiException(TModel model, string message)
    : base(message)
    {
        Model = model;
    }

    public ApiException(TModel model, string message, Exception innerException)
    : base(message, innerException)
    {
        Model = model;
    }
}

And somewhere (I would make it a static method in the ApiException class) we would have:

public class ApiException
{
    // existing implementation here //


    public static ApiException CreateException(object model)
    {
        ArgumentNullException.ThrowIfNull(model);
        var type = typeof(ApiException<>).MakeGenericType(model.GetType());
        return (ApiException)Activator.CreateInstance(type, model)!;
    }

    public static ApiException CreateException(object model, string message)
    {
        ArgumentNullException.ThrowIfNull(model);
        var type = typeof(ApiException<>).MakeGenericType(model.GetType());
        return (ApiException)Activator.CreateInstance(type, model, message)!;
    }
}

Then the code in https://github.com/microsoft/kiota-http-dotnet/blob/6c1e799f63a26b9209e7153d7f1eb2d2c730e9f6/src/HttpClientRequestAdapter.cs#L381 can be rewritten to look like this:

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

This would return a correctly model-typed exception containing a (hopefully meaningful) model instance returned from the API. That information (i.e. the result in the code) would otherwise be lost.

There are two extra thoughts (and yes, I'm repeating myself again):

  • Activator.CreateInstance doesn't exactly have stellar performance, but this is used for the exceptional flow, so I think performance is OK in this scenario. We can avoid metaprogramming by introducing an extra interface, but let's not go there for now.
  • API controllers in .NET support Problem Details, it would be interesting to think about how this "fits" with this design.

Is this the best design possible? No, of course not.
But any design that doesn't force your models to inherit from anything outside of their own OpenAPI definition is, in my view, an improvement. No more special code that renames properties behind your back.

C# developers want the best C# code generation. Java developers want the best Java code generation. Go developers want the best Go code generation. Nobody cares about the "other" development languages or tech platforms except its own. OpenAPI is the common spec. If the intermediate abstraction from which the code is generated isn't able to do that for all languages, then it's not rich/good enough.

And no, I won't submit a PR. This is a design problem, not a code problem.

@bkoelman
Copy link
Author

bkoelman commented Jul 9, 2024

I think I can say with some confidence that having a "developer friendly" message is irrelevant. It's the least of our worries.
Developer care about the type of the exception and the content of any payload that may be sent with that exception.

Totally agree. We need access to the technical bits. We'll translate it to a localized UI message, based on what the user tried to do and how he/she should proceed.

The flaw here is that it is assumed that the callers of the API are at all able to ask the team of the API to adjust their OpenAPI specification for this.

Yes, this is usually not feasible. It's brought up frequently, a more pragmatic approach would help. We shouldn't fall off the cliff when the world isn't perfect.

At the risk of repeating myself, a better design (which is an improvement over even NSwag) would be to define a generic ApiException:

As far as I'm aware, this is already how NSwag works (and I'm happy with it). ApiException<TModel> derives from ApiException, which provides access to the status code, headers, and (the first few KBs of) the payload, which is great for log correlation. It doesn't need reflection, just generates the appropriate construction code:

 if (status_ == 400)
 {
     var objectResponse_ = await ReadObjectResponseAsync<ErrorResponseDocument>(response_, headers_, cancellationToken).ConfigureAwait(false);
     if (objectResponse_.Object == null)
     {
         throw new ApiException("Response was null which was not expected.", status_, objectResponse_.Text, headers_, null);
     }
     throw new ApiException<ErrorResponseDocument>("The query string is invalid.", status_, objectResponse_.Text, headers_, objectResponse_.Object, null);
 }
 else
 {
     var responseData_ = response_.Content == null ? null : await response_.Content.ReadAsStringAsync().ConfigureAwait(false);
     throw new ApiException("The HTTP status code of the response was not expected (" + status_ + ").", status_, responseData_, headers_, null);
 }

In the snippet above, the text "The query string is invalid." originates from the description in the OpenAPI document and is mapped to the Exception.Message property. If there is no mapping (the else clause), you'll get (the first few KBs of) the payload, which is great to log an HTTP 500 that contains HTML.

So yes, I feel your pain. There is something wrong with the design of Kiota, at least as far as exception handling is concerned.
... But I didn't expect to invest so much time for such a simple issue.

Thanks. I've hit many odd designs in Kiota over the past 6 months. When you bring it up, it either gets dismissed without motivation, or a long thread starts (based on incomprehension or theoretical dilemmas), without being heard. When you're at the point of giving up, the issue is closed as completed. A very frustrating experience indeed.
I don't think there's bad intent, just a lack of understanding what app developers typically need (combined with stubbornness), and not knowing how to foster a community. Surely kiota has a long way to go, but code can be refactored and redesigned, so that would just be a matter of time. It's the attitude (and the continuous misunderstandings) that make me give up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request generator Issues or improvements relater to generation capabilities. help wanted Issue caused by core project dependency modules or library
Projects
Status: Todo 📃
Development

No branches or pull requests

4 participants