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

[core-client] Check response body for error details even without default body mapper #33151

Merged
merged 2 commits into from
Mar 5, 2025

Conversation

jeremymeng
Copy link
Member

Currently, when we receive an error response and there is no default body mapper, we immediately throw a
RestError without further inspecting the response body. It's ideal for operation specifications to define a
default mapper for unexpected responses. However, some services did not implement this leading to issues like
#33035 where the error message contains a JSON string of the
error object, and the error code is undefined.

This PR modifies the behavior to look into parsedBody if it has an 'error' property that contains both
'code' and 'message' properties, improving the developer experience in this scenario.

…ult body mapper

Currently, when we receive an error response and there is no default body mapper, we immediately throw a
RestError without further inspecting the response body. It's ideal for operation specifications to define a
default mapper for unexpected responses. However, some services did not implement this leading to issues like
Azure#33035 where the error message contains a JSON string of the
error object, and the error code is undefined.

This PR modifies the behavior to look into parsedBody if it has an 'error' property that contains both
'code' and 'message' properties, improving the developer experience in this scenario.
@jeremymeng jeremymeng requested a review from a team as a code owner February 21, 2025 00:37
@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@xirzec xirzec requested review from timovv and deyaaeldeen February 24, 2025 19:55
Copy link
Member

@deyaaeldeen deyaaeldeen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a tangent, I recommend checking out the discussion in the email thread "Error parsing in Azure-Core" in May 2023.

This fix addresses the immediate customer ask. I don't have data on how many of our supported libraries still use this client but if it is significant enough, I wonder if we want to generalize the fix based on the discussion in the thread.

Also, the spec says that the x-ms-error-code header should have the error code, I wonder if we should look at that header if we can't find the code in the body.

@jeremymeng
Copy link
Member Author

As a tangent, I recommend checking out the discussion in the email thread "Error parsing in Azure-Core" in May 2023.

Thanks for the point! I rechecked. The email thread was about supporting the two format of response body, one with the error wrapping object and the other without, which we already support in JS, but currently we return earlier before that supporting code because there's no matching operation spec.

This fix addresses the immediate customer ask. I don't have data on how many of our supported libraries still use this client but if it is significant enough, I wonder if we want to generalize the fix based on the discussion in the thread.

We may not need this fix for now if the service can update their specs to include the default response mapper.

@deyaaeldeen
Copy link
Member

Also, related to this, I just received a 401 response from the Azure Foundry service that looks like this:

{
    statusCode: 401,
    message: 'Unauthorized. Access token is missing, invalid, audience is incorrect (https://cognitiveservices.azure.com), or have expired.'   
}

This is problematic because not only it doesn't conform to the Azure error response design, it also doesn't have a code at all. To make things worse, it also doesn't have an x-ms-error-code header.

@jeremymeng
Copy link
Member Author

This is problematic because not only it doesn't conform to the Azure error response design, it also doesn't have a code at all. To make things worse, it also doesn't have an x-ms-error-code header.

Was the error from Azure Foundry, or from Microsoft Graph/Entra which may have a different design?

@deyaaeldeen
Copy link
Member

deyaaeldeen commented Feb 25, 2025

Was the error from Azure Foundry, or from Microsoft Graph/Entra which may have a different design?

@jeremymeng, it was an Azure Foundry API (https://.services.ai.azure.com/models/chat/completions?api-version=2024-05-01-preview).

@jeremymeng jeremymeng merged commit 9640825 into Azure:main Mar 5, 2025
24 checks passed
@jeremymeng jeremymeng deleted the core/error-heuristics branch March 5, 2025 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants