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

com.microsoft.kiota.serialization.ParseNodeFactoryRegistry throws RuntimeException if service returns status code 503 with unexpected content type #1683

Open
etcoyvindf opened this issue Nov 28, 2024 · 5 comments
Labels
enhancement New feature or request type:enhancement Enhancement request targeting an existing experience

Comments

@etcoyvindf
Copy link

Situation and background:
The Microsoft Graph API had some issues some days ago, where some calls to the API would return HTTP status code 503, with some html error message.

During this time, our use of the Graph API SDK, which in turn uses Kiota, lead to several application level crashes, because of uncaught RuntimeExceptions was propagated up the call stack from Kiota.

My observations, based on my understanding of the code:
The source of the exception, ParseNodeFactoryRegistry method getParseNode will attempt to find a parser for an expected content type.
In the case of Graph API, we expect JSON, so we only set up factories to handle that.

When the Graph API has an issue/downtime/whathaveyou, the method finds no factory to handle the response, and throws a RuntimeException directly.

Straight forward, I feel theres' two possible solutions.

  1. Make getParseNode throw ApiException, which is a bit safer to try/catch than a pure "catch all" RuntimeException
  2. Add a special case content type text/html factory that handles special error page responses, which might be in scope only for the graph api sdk.

With limited knowledge of how easy a custom factory is as an end user of MS Graph SDK, we feel forced to stop catching ApiException, and start catching RuntimeExceptions instead, hoping that no other runtime exceptions gets thrown and swallowed by our logic.

@github-project-automation github-project-automation bot moved this to Needs Triage 🔍 in Kiota Nov 28, 2024
@etcoyvindf
Copy link
Author

link to the method that throws the exception: https://github.com/microsoft/kiota-java/blob/0cd6b6d8a3263c6cc2d1d1fe6b2358c054da46e0/components/abstractions/src/main/java/com/microsoft/kiota/serialization/ParseNodeFactoryRegistry.java#L56C8-L57C97

Stacktrace from our log:

java.lang.RuntimeException: Content type text/html does not have a factory to be parsed
at com.microsoft.kiota.serialization.ParseNodeFactoryRegistry.getParseNode(ParseNodeFactoryRegistry.java:56)
at com.microsoft.kiota.http.OkHttpRequestAdapter.getRootParseNode(OkHttpRequestAdapter.java:582)
at com.microsoft.kiota.http.OkHttpRequestAdapter.throwIfFailedResponse(OkHttpRequestAdapter.java:649)
at com.microsoft.kiota.http.OkHttpRequestAdapter.send(OkHttpRequestAdapter.java:280)
at com.microsoft.graph.users.item.events.item.EventItemRequestBuilder.get(EventItemRequestBuilder.java:176)
at com.microsoft.graph.users.item.events.item.EventItemRequestBuilder.get(EventItemRequestBuilder.java:176)
.. internal code folllows here, graphClient.me().events().byEventId(id).get(rc -> addStandardOutlookRequestHeaders(rc.headers));

@baywet
Copy link
Member

baywet commented Nov 28, 2024

Hi @etcoyvindf
Thank you for using the SDK and for reaching out.

Could you please provide which endpoint and operation you are using? The service should NOT be replying with html pages and we'll want to report that to the service team. If you could also include client request ids and timestamps, it'd be great.

Since Api exception derives from runtime exception, it's unchecked, which means there would be no impact to the method prototype.

That class was originally designed to represent errors coming back from the service itself, and one could argue this is a client exception not being able to parse the result due to lack of support for a given format. What would you think about adding a "ClientException" similar to the API exception?

@baywet baywet added enhancement New feature or request status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close type:enhancement Enhancement request targeting an existing experience labels Nov 28, 2024
@baywet baywet moved this from Needs Triage 🔍 to Waits for author 🔁 in Kiota Nov 28, 2024
@etcoyvindf
Copy link
Author

Hey @baywet

The method (in graph sdk) we used was this graphClient.me().events().byEventId(id).get();

GET https://graph.microsoft.com/v1.0/me/events/{id}

Because of the coding of our application, we did not record the client request id. The error was recorded around 2024-11-25 10:30:04 UTC. At the time, MS365 had one open issue, Issue ID was MO941162. Internally we could confirm issues trying to access Exchange, both mail and calendars. Likewise our impacted customers experienced the same. X post about the issue linked here: https://x.com/MSFT365Status/status/1860973220662280356

If you have access to MS365 admin panel ,you can also look up the incident report details there. I'm not aware of where else this information is

From my point of view, this incident seems to also have caused the graph API to start responding with status 503 in html.

In other API integrations I've done, I would have checked this status and handled it before trying to parse the response and handled it as a "endpoint not available" situation.

Kiota with how it handles everything, I'm not sure where this all fits. Since runtime exceptions are unchecked, I would be very happy with it being ClientException or something similar. So that the situation can be "discovrered" and handled without this causing exceptions on application level.

Very very briefly, our use is inside a web application that adds events into peoples calendar, and the way we handle this today causes a 500 exception to appear for the user, since it happens on the request handler. Wether or not the way we implemented this is good or bad is of course up for debate, but not really "on topic".

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels Nov 28, 2024
@baywet
Copy link
Member

baywet commented Nov 28, 2024

Thank you for the additional information.

I think we're on the same page then.

Is this something you'd like to submit a pull request for provided some guidance?

(adding a new exception type, and updating this factory + the serialization writer one to use it)

@etcoyvindf
Copy link
Author

If I get around to PR it I might be able to. The task itself seems straigtht forward. At the earliest I'd be able to do it around newyear.

But if someone on the team already has an idea about how to do this, I would prefer it. I'm up to my shoulders in tasks as it is, and this would come on top if it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request type:enhancement Enhancement request targeting an existing experience
Projects
Status: Waits for author 🔁
Development

No branches or pull requests

2 participants