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

Beta and official SDK should share the same ODataError type #631

Open
olivermue opened this issue Mar 9, 2023 · 2 comments
Open

Beta and official SDK should share the same ODataError type #631

olivermue opened this issue Mar 9, 2023 · 2 comments
Labels
Question: SDK Not actionable. Question related to the SDK. Request: enhancement New feature or request

Comments

@olivermue
Copy link

In our current solution we mix the usage of the official and beta Graph API, cause we use a few features that are currently only available within the beta. Within v4 this mix was really cumbersome due to the shared namespace (to get this to run you need to use package aliases and extern alias). So switching to an individual namespace for the beta package really improves the usage of it.

While upgrading our solution to the v5 SDK I see that the ODataError type exists in both SDKs, which leads to some more complicated codes at a few places, cause we have to catch both exception types to ensure everything works as expected.

It would be great if the ODataError could be moved to the core library just like it had been done for the PageIterator to make the parallel usage of both libraries even more simple.

@ghost ghost added the Needs: Triage label Mar 9, 2023
@andrueastman
Copy link
Member

Thanks for raising this @olivermue

The ODataError is a generated type that has a common base of type ApiException. Any chance catching the base type is helpful? Also, is there any chance you could share an example of the current usage to help us understand it better?

@andrueastman andrueastman added Question: SDK Not actionable. Question related to the SDK. Needs: Author Feedback and removed Needs: Triage labels Mar 9, 2023
@olivermue
Copy link
Author

The drawback of catching ApiException would be that we can't easily access the error details from the MainError Error property. At several places we are able to handle specific error cases like Request_ResourceNotFound or we search for specific substrings within ex.Error.Message like "originated within an external service" or "Permission being assigned already exists on the object".

As already stated we try to use the v1.0 API as much as possible, but a few places (especially DeviceManagement) still has some endpoints only available via the beta API. While we move from 4.x to 5.x I'm going to check at which places we can move from beta to v1.0 to make life easier, cause a lot of endpoints made it into v1.0 since we implemented them by using beta (e.g. deleted users, user password resets, education assignments).

I think the most part will boil down to be v1.0 and a few places maybe will be beta exclusive. The only really messy part will still be device management a.k.a. Intune, cause here we really mix up v1.0 and beta calls (by the way one reason for that could be that the official Intune admin center under https://endpoint.microsoft.com/ heavenly dependent on beta and mixes v1.0 into it. Just open it, check the network tab of your browser while clicking around. It is really funny that the beta endpoint doesn't get any customer support, but a Microsoft globally available product depends so much on this interface).

While further refactoring our code, my guess would be that really only a handful places do these mixes and we have to do such messy stuff like:

try
{
    myServiceWrapper.DoSomething();
}
catch (Microsoft.Graph.Models.ODataErrors.ODataError) when (ex.Error.Code == "FooBar")
{
    // Handle FooBar error
}
catch (Microsoft.Graph.Beta.Models.ODataErrors.ODataError) when (ex.Error.Code == "FooBar")
{
    // Handle FooBar error
}

The thing that probably could happen more often, if you have added both NuGet Packages to your project (or to any of your child or grand child projects) and then adding the wrong using namespace to your code like this:

public async Task DoSomething()
{
    try
    {
        var user = await serviceClient.Users[userId].GetAsync();
        await ChangeSomething(user);
    }
    catch(ODataError ex) when (ex.Error.Code == "Request_ResourceNotFound")
    {
        // User doesn't exist. We ignore the change.
    }
}

This given code is absolutely okay and looks fine, but if your header of the file looks like this:

using Microsoft.Graph;
using Microsoft.Graph.Models;
using Microsoft.Graph.Beta.Models.ODataErrors;

You are in trouble, because you catch the wrong type of exception. This can easily happen, especially when you add the ODataError the first time to a file within Visual Studio it will show up the available using statements in alphabetically order, which makes the beta namespace first and that is the potentially biggest problem:
IntelliSense suggestions to pick using namespace

Hope, these thoughts help you to better understand the problems arised while upgrading to v5 SDK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question: SDK Not actionable. Question related to the SDK. Request: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants