CommunityToolkit / Graph-Controls

Set of Helpers and Controls for Windows development using the Microsoft Graph.
https://docs.microsoft.com/en-us/windows/communitytoolkit/graph/overview
Other
155 stars 39 forks source link

Updated how Graph service exceptions are handling in controls #142

Closed shweaver-MSFT closed 3 years ago

shweaver-MSFT commented 3 years ago

PR Type

What kind of change does this PR introduce?

What is the current behavior?

When the GraphServiceClient fails to make a request for whatever reason and throws an exception, in most cases we eat the exception and move on. This makes it difficult to understand WHY a request has failed and whether the exception encountered is part of normal logical flow (e.g. 404 - Not Found), or actually unexpected.

What is the new behavior?

In the new behavior, Graph client extension methods no longer eat exceptions, so any GraphServiceClient exceptions will bubble up to the caller.

The controls have also been updated to handle any expected exceptions that may occur when using the GraphServiceClient to make requests, and rethrow any unexpected ones. This increases stability for controls like PersonView, which is frequently used to represent entities that do not support a profile photo, such as conference rooms.

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

I also tested the WithScopes method to see if issue #8 still exists, and it appears that incremental consent is not working at the moment. Perhaps there is some additional configuration required in MsalProvider to get it going, but I'm not sure what that is. WindowsProvider is based on WAM apis that do not support incremental consent quite yet, so it is not expected to work there.

So the current behavior is that WithScopes seemingly does nothing. It doesn't show a prompt for additional consent, and will throw a 403 when calling for resources without the pre-requisite consent.

Based on this, we should investigate how incremental consent is expected to work with MSAL for .NET and see if we can get that working.

ghost commented 3 years ago

Thanks shweaver-MSFT for opening a Pull Request! The reviewers will test the PR and highlight if there is any merge conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌