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

Add a way to get response body for error responses #5167

Closed
marcinjahn opened this issue Feb 13, 2024 · 21 comments
Closed

Add a way to get response body for error responses #5167

marcinjahn opened this issue Feb 13, 2024 · 21 comments
Assignees
Labels
area:http Focused on functional module http Csharp Pull requests that update .net code enhancement New feature or request Go Java Python TypeScript Pull requests that update Javascript code
Milestone

Comments

@marcinjahn
Copy link
Contributor

marcinjahn commented Feb 13, 2024

I generated a client with Kiota, based on an OpenAPI spec, which does not specify error codes, hence errorMapping is empty in Kiota-generated client. The ApiException that Kiota throws contains just the status code and response headers. Is there any way to read the actual response body (as string) without modifying the Kiota-generated client code?

@baywet baywet added the enhancement New feature or request label Feb 13, 2024
@baywet
Copy link
Member

baywet commented Feb 13, 2024

Hi @marcinjahn ,
Thanks for using kiota and for reaching out.
This is something we've been talking about a couple of times internally and waiting to see if there was customer demand for it.
On one hand we'd prefer the API descriptions to be improved so a model for the error is generated and the API consumer gets a better experience.
We're also concerned about the performance aspects, especially if the consumer doesn't dispose of the body.
On the other hand, we acknowledge that API producers and consumers are more often than not two different people, and that it's hard for consumers to funnel feedback to the producers.
Maybe what we could do is add a content type and content fields to the API exception type.
And only populate then if we were not able to deserialize to a generated type.
I'd like @sebastienlevert our PM to weight in on the experience aspect. He is on vacations for the next few weeks though.
In the meantime, you might be able to work this around by implementing your custom handler.

@marcinjahn
Copy link
Contributor Author

Hi @baywet, exception containing the content would pretty much solve my issue.
Do you mean implementing a custom DelegatingHandler, or something else, Kiota-specific?

@baywet
Copy link
Member

baywet commented Feb 13, 2024

yes that what I meant

@sebastienlevert
Copy link
Contributor

While I like the suggestion, it would not be an experience that would be seamless across all exceptions. Could we use the work we are doing around UntypedJson to support the body of the exception?

@baywet
Copy link
Member

baywet commented Feb 27, 2024

We could potentially decide to wrap the response into untyped json constructs when we don't have a mapping, and attach that to API error. Of course that assumes the response is JSON

@andrueastman
Copy link
Member

Transferring issue as part of microsoft/kiota-dotnet#238

@andrueastman andrueastman transferred this issue from microsoft/kiota-http-dotnet Jul 9, 2024
@andrueastman andrueastman added the area:http Focused on functional module http label Jul 9, 2024
@andrueastman
Copy link
Member

Moving this to main Repo to track this accross all languages for now.

@github-project-automation github-project-automation bot moved this to Needs Triage 🔍 in Kiota Aug 15, 2024
@andrueastman andrueastman transferred this issue from microsoft/kiota-dotnet Aug 15, 2024
@andrueastman andrueastman moved this from Needs Triage 🔍 to Proposed 💡 in Kiota Aug 15, 2024
@andrueastman andrueastman added Csharp Pull requests that update .net code TypeScript Pull requests that update Javascript code Python Go Java labels Aug 15, 2024
@andrueastman andrueastman moved this from Proposed 💡 to In Design 🎨 in Kiota Aug 15, 2024
@andrueastman andrueastman moved this from In Design 🎨 to Proposed 💡 in Kiota Aug 15, 2024
@baywet baywet added this to the Backlog milestone Aug 15, 2024
@ntauth
Copy link

ntauth commented Nov 7, 2024

Following, this would be an essential feature, not all OpenAPI specs come with proper error mappings unfortunately

@merphidos
Copy link

This would solve an important pain point for us. +1.

@marcinjahn marcinjahn changed the title Is there any way to get response body when status code is unsuccessful? Add a way to get response body for error responses Nov 11, 2024
@marcinjahn
Copy link
Contributor Author

I updated the title of this issue to reflect a bit better that this is a request for a new feature

@baywet
Copy link
Member

baywet commented Nov 11, 2024

Thank you for the additional information.

In that scenario, are you looking to get the response body only at development time? (while writing and testing the code) or also in production? (app has been deployed for a while, and you don't want to have to re-deploy to enable observability of the body)

@ntauth
Copy link

ntauth commented Nov 11, 2024 via email

@marcinjahn
Copy link
Contributor Author

marcinjahn commented Nov 11, 2024

From my perspective, I don't really understand what you mean by "only at development time". My idea was to be able to have some API that would allow you to read the body of error responses.
Some example would be: let's say I have generated the client (using kiota) for some Web API that has some defined contract for error responses. The response body contains some domain-specific error codes. When I get HTTP error, I want to be able to retrieve the error code from the body of the response in order to properly react to it.
Still, it assumes that the OpenAPI spec of the Web API does not have proper types for HTTP errors (let's assume I have no control over that, and that's just what I get).

@baywet
Copy link
Member

baywet commented Nov 11, 2024

The reason why we haven't enabled observing response (or request) bodies "in production" is because it's really easy for anybody to shoot themselves in the foot and start logging data in production.
Logging data in production can be terrible for security reasons, it also will be detrimental to performance, and increase application hosting costs.

By "only at development time" what I meant is that code could be behind conditional compilation, and only be available with debug builds (for languages that support it)

Observing all request/response bodies

That being said, we have received that request multiple times since the release of kiota. And we could design a similar middleware handler to the HeadersInspectionHandler, name it something like "DangerousBodyInspectionHandler".
This would work in pair with an option and be used like that.

var observationOption = new DangerousBodyInspectionOption();
var result = await client.Foo.Bar.GetAsync(r => r.Options.Add(observationOption));
observationOption.ResponseBody;
// would be a stream the application would be responsible for disposing

This design allows to only copy/observe bodies when requested, lowering IOs.

Observing only error responses

The alternative only for errors is to add a field that would provide access to the response body stream. That field would only be populated if the response was not deserialized to a specific generated type. Which means the cost of errors is higher (memory, IOs), and poses limitations when wanting to observe successful responses or request bodies.

Let us know what you think about the two designs.

@marcinjahn
Copy link
Contributor Author

  • I like the design of having this as an option, which will potentially give better performance to those who don't need that
  • the naming like DangerousBodyInspectionHandler is a bit exaggerated I think. It's not like people never had a way to read response bodies. I think it's up to the specific application to decide if this is dangerous or not. I see you assume the logging use-case, which probably makes sense, but it's not all of the use cases. For other use cases the response body might just be used in control flow of the app to decide what to do next
  • I think it makes sense to enable the responses "observability" only for errors, since in most cases I guess that would be the only reason to enable that. If it allows for better performance in non-error scenarios, even better.

@baywet
Copy link
Member

baywet commented Nov 11, 2024

Thank you for the additional information.

@sebastienlevert what do you think of the handler design? What if it didn't have the Dangerous part in the name?

@ntauth
Copy link

ntauth commented Nov 20, 2024

Is there any temporary workaround we can use to access the response bodies while a proper fix is worked on?

@baywet
Copy link
Member

baywet commented Nov 25, 2024

The only viable option at this point is to implement your own http client middleware handler.

@sebastienlevert
Copy link
Contributor

I like the option of the BodyInspectionOptions, the same way we're doing it for Headers. I don't see this as an issue and this would provide the required capability, while still being an opt-in. I don't think the Dangerous is required. Let's just make sure we don't alter the original response.

Having a field on all models with the body content seems overkill and will be more confusing than anything.

@baywet
Copy link
Member

baywet commented Nov 27, 2024

Thanks for the input. Created microsoftgraph/msgraph-sdk-design#116 in our design repo. Once it gets merged, I'll create the individual issues for the http libraries, and close this main issue.

@baywet
Copy link
Member

baywet commented Dec 2, 2024

The design PR has been merged. I also created a bunch of implementation issues (list below) that I recommend anyone following this issue to go subscribe to. Closing to clear confusion. @marcinjahn is this something you'd like to submit a pull request for provided some guidance? If so please jump on the issue for the language you're interested in.

Here is the list of issues:

@baywet baywet closed this as not planned Won't fix, can't repro, duplicate, stale Dec 2, 2024
@github-project-automation github-project-automation bot moved this from Proposed 💡 to Done ✔️ in Kiota Dec 2, 2024
@baywet baywet self-assigned this Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:http Focused on functional module http Csharp Pull requests that update .net code enhancement New feature or request Go Java Python TypeScript Pull requests that update Javascript code
Projects
Status: Done ✔️
Development

No branches or pull requests

6 participants