feat: use 500 as default status code since it's an unhandled error

[teofilomonteiro]

Summary

If an unhandled error boils up, the statusCode by default is -1.

Motivation

Explain the motivation for making this change. What existing problem does the pull request solve?

Test plan

Demonstrate the code is solid. Example: The exact commands you ran and their output.

Closing issues

Fixes #

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the CONTRIBUTING document.
• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[sebastienlevert]

Thanks for the contribution @teofilomonteiro. In this case, the error you are getting back is probably a client error rather than a server side error. Assigning it to 500 by default wouldn't true in this case as there was an error in sending the request, not in receiving the response.

Can you expand on the scenario and how it happens in your app? Thanks!

[teofilomonteiro]

@sebastienlevert An example I can give you is when your credentials are incorrect. When you make a request the error thrown comes with a status code -1. This is not the correct behaviour.

Also if you are using a framework like nestjs if you throw an error with status code -1, you break the app. Also sending a -1 is strange. Maybe 500 is better since it was an internal error.

[sebastienlevert]

So you are saying that sending an invalid token / without a token results in a -1 in the Graph Response? When calling Graph I'm receiving a 401 from the server with an internal error.

{"error":{"code":"InvalidAuthenticationToken","message":"Access token is empty.","innerError":{"date":"2023-07-24T14:06:51","request-id":"989061cc-3dd5-418b-b250-1a41d973f9d5","client-request-id":"989061cc-3dd5-418b-b250-1a41d973f9d5"}}}

[sebastienlevert]

If you share a small repro, we'd love to investigate using this. Based on the logic in the Error handler, I don't think we would be missing a status coming from the server for instance.

[teofilomonteiro]

Thanks @sebastienlevert! It's a super easy setup.

import { ClientSecretCredential } from '@azure/identity'; import { Client } from '@microsoft/microsoft-graph-client'; import { TokenCredentialAuthenticationProvider } from '@microsoft/microsoft-graph-client/authProviders/azureTokenCredentials'; async function main() { const tenantId = 'valid_tenant_id'; const clientId = 'valid_client_id'; const credential = new ClientSecretCredential(tenantId, clientId, 'wrong_client_secret'); const authProvider = new TokenCredentialAuthenticationProvider(credential, { scopes: ['.default'], }); const client = Client.initWithMiddleware({ debugLogging: false, authProvider, }); await client.api('/domains').get(); } main();

This should replicate the issue 👍 In the end it will throw the error but we will have statusCode -1

[sebastienlevert]

So I was able to run and replicate the issue. That being said, I don't think the issue reside in the default value of the status code, but rather in some of the ways we handle auth errors internally in the library. @koros, please have a look at this as we should return a 400 based on this scenario.

I'll go and create an issue and we'll take it from there! Thanks for the contribution, but this bug should be handled where the issue happens and not use the status code as a way to "normalize" errors of this kind. Thanks!