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

Allow fallback for unhandled non-success status codes #340

Closed
IanKemp opened this issue Aug 20, 2024 · 10 comments
Closed

Allow fallback for unhandled non-success status codes #340

IanKemp opened this issue Aug 20, 2024 · 10 comments
Labels
enhancement New feature or request status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close

Comments

@IanKemp
Copy link

IanKemp commented Aug 20, 2024

Is your feature request related to a problem? Please describe the problem.

Related to microsoft/kiota#3744 which was never satisfactorily addressed. The problem is pretty simple:

  • An endpoint's API specification says it supports 400 as a response, but it can actually return 404 as well
  • Kiota client generated against that endpoint
  • Generated client is invoked in such a way that causes the endpoint to return undocumented 404
  • Kiota throws an incredibly unhelpful The server returned an unexpected status code and no error factory is registered for this code exception

Now, I know the maintainers are gonna point me to this, but it doesn't cover a specific scenario: allowing me to see the returned 404 response so that I can either tell the API owner about it so that they fix their spec (because if you've ever worked with "enterprise" companies you know that they require evidence for everything), or fix up my local copy of their spec.

Using a DelegatingHandler to throw/log on any unsuccessful responses (HttpResponseMessage.IsSuccessStatusCode == false) isn't an option because in the given example, it will also throw/log for the 400 case - which it shouldn't, because that's covered by a generated error handler.

There is a Action<RequestConfiguration<T>> parameter that can be supplied to every Kiota-generated method, and it's possible to specify an IResponseHandler there that can maybe do what is requested here, but (a) this is on a per-method basis and not a global option (b) as noted here, that IResponseHandler isn't in addition to the default Kiota handling but a replacement of it; which rather defeats the point.

Client library/SDK language

None

Describe the solution you'd like

Any unsuccessful HTTP response (that is, one where HttpResponseMessage.IsSuccessStatusCode is false) will filter through that endpoint's explicit error mappings, exactly as currently.

However, in the case that no mapping is found for the returned status code, the library will attempt to resolve an implementation of the below interface:

public interface IUnsuccessfulResponseFallbackHandler
{
    Task HandleAsync(HttpResponseMessage response, CancellationToken cancellationToken = default);
}

If such an implementation is found, the problematic HttpResponseMessage is passed to this implementation's HandleAsync method. After this, Kiota will continue to behave as currently, i.e. throw The server returned an unexpected status code and no error factory is registered for this code.

In essence, this is inserting a step between the current "look up error mapping for this unsuccessful status code" and "throw if no matching error mapping is found". Because of the way that HttpClientRequestAdapter is currently designed, specifically the ThrowIfFailedResponse method, actually implementing this is not simple.

Additional context

  • This is a proposal/RFC, not a fully-fleshed-out "I have the code in a feature branch". I don't know the full implications of implementing this, I don't know what shape the contract should be (could we somehow just use a DelegatingHandler instead of a whole new interface? maybe? IDK), I don't know how this interface would be provided (a new IRequestOption implementation?), ...
  • The interface I've defined cannot affect the flow of response processing; this is by design to keep this proposal as simple as possible. (Sure, you can read to the end of the response content stream and not rewind it and thus cause Kiota to blow up internally when it tries to access that stream, but that's true of DelegatingHandlers anyway IIRC... if you want to shoot yourself in the foot, I ain't gonna stop you.)
@IanKemp IanKemp added the status:waiting-for-triage An issue that is yet to be reviewed or assigned label Aug 20, 2024
@baywet baywet transferred this issue from microsoft/kiota Aug 21, 2024
@baywet
Copy link
Member

baywet commented Aug 21, 2024

Hi @IanKemp
Thank you for using kiota and for reaching out.

I have transferred the issue to this repo since it most likely would not impact the generation.

I'll first try to understand the need here before defining the solution. Let me rephrase to make sure I understood things well and please correct me if I missed something:

The goal is to be able to access the response body on non-successful response status codes when no factory is registered or when parsing failed.

@baywet baywet added enhancement New feature or request status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close and removed status:waiting-for-triage An issue that is yet to be reviewed or assigned labels Aug 21, 2024
@baywet baywet moved this from Needs Triage 🔍 to Waits for author 🔁 in Kiota Aug 21, 2024
@github-project-automation github-project-automation bot moved this from Waits for author 🔁 to Done ✔️ in Kiota Aug 30, 2024
@kieronlanning
Copy link

Did anything ever happen with this?

@baywet baywet closed this as not planned Won't fix, can't repro, duplicate, stale Jan 8, 2025
@baywet
Copy link
Member

baywet commented Jan 8, 2025

@kieronlanning nothing. I re-closed the issue as not planned to clearly communicate that.
Let us know if you have any additional comments or questions.

@kieronlanning
Copy link

Thanks @baywet.

I have the same issue as the original author I believe: an upstream Open-API that I'm not in control of has undocumented response codes/ error conditions.

In this case it's returning an undocumented 400 - but I can't seem to get to the payload to understand what the problem is.

@baywet
Copy link
Member

baywet commented Jan 8, 2025

Since this issue was created, we added support for response body inspection.
See #482 and associated items.
Let us know if you have any additional comments or questions.

@kieronlanning
Copy link

kieronlanning commented Jan 8, 2025

Looking through the GH issue, took me to this page, where it details what support there is for body inspector.

It looks as though there is no support for C#, is that correct?

@baywet
Copy link
Member

baywet commented Jan 8, 2025

Thanks for the callout, with the break, we forgot to merge a pull request MicrosoftDocs/openapi-docs#140

@kieronlanning
Copy link

Ah-ah! Thanks for merging.

Will there be a new generator release soonish?

@baywet
Copy link
Member

baywet commented Jan 8, 2025

One is scheduled for tomorrow, but why would you expect a new release? The inspection handler is already available in the kiota dotnet dependencies.

@kieronlanning
Copy link

kieronlanning commented Jan 8, 2025

Oh, it just the documentation that was out of date not the generator itself. Understood.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close
Projects
Archived in project
Development

No branches or pull requests

3 participants