When REST API call error, it might fail to return response.body.error

[howieleung]

• Package Name: @azure/core-rest-pipeline
• Package Version: 1.17.1
• Operating system:
• [ ] nodejs
  • version:
• [ ] browser
  • name/version:
• [ ] typescript
  • version:
• Is the bug related to documentation in
  • [ ] README.md
  • [ ] source code documentation
  • [ ] SDK API docs on https://learn.microsoft.com

Describe the bug It looks like a missing policy to handle http error. Probably with certain error.
Case 1, client.path("xxx").get() just throw exceptions instead.
Case 2: client.path("xxx").post() return response but include response.body.message instead of response.body.error. So the following code in samples throw undefined:

  if (isUnexpected(response)) {
    throw response.body.error;
  }

To Reproduce Steps to reproduce the behavior: For case 1, try sample, sdk\healthdataaiservices\azure-health-deidentification\samples\v1-beta\javascript\listJobs.js with the example end point, "https://example.api.cac001.deid.azure.com". Yes, it is a bad end point, but still shouldn't throw exception

For case 2, try sample, sdk\ai\ai-inference-rest\samples\v1-beta\javascript\chatCompletions.js. Use end point, "https://llama3-1-xxx.westus.models.ai.azure.com"

Expected behavior A clear and concise description of what you expected to happen. Both cases should return response.body.error

Screenshots If applicable, add screenshots to help explain your problem.

Additional context Add any other context about the problem here.

[jeremymeng]

@joheredi for 1), should the RLC catch errors from core-rest-pipeline and present it in response in a unified way?

[xirzec]

We've never had the pipeline treat all possible errors (network / policies throwing /etc) as if they were uniformly rest responses from a service. I'm curious why you would want this?

Also if an error is thrown it shouldn't be assigned to response at all, you'd have to catch it, right?

[github-actions[bot]]

Hi @howieleung. Thank you for opening this issue and giving us the opportunity to assist. To help our team better understand your issue and the details of your scenario please provide a response to the question asked above or the information requested above. This will help us more accurately address your issue.

[howieleung]

We've never had the pipeline treat all possible errors (network / policies throwing /etc) as if they were uniformly rest responses from a service. I'm curious why you would want this?

Also if an error is thrown it shouldn't be assigned to response at all, you'd have to catch it, right?

Regarding for case 1, I agree that if the error doesn't come from server such as invalid domain, the method should not return a response back since it doesn't reach to the server to get back the response. Throwing exception sound right. But on the other hand, developers need to wrap a try catch to catch domain errors and network error, and at the same time, need to write code to handle response.body.error for 4xx or 5xx. This just make the error handling annoying. We could consider throwing exception for 4xx and 5xx also can centralize error handling in catch statements.

Anyway for case 2, it shouldn't return response.body.message. It should return response.body.error

[xirzec]

I think there are important distinctions between how the HttpClient, Pipeline, and Client handle various kinds of errors.

A HttpClient needs to implement a single method: makeRequest. The role of this method is to produce a PipelineResponse given a PipelineRequest. If anything goes wrong, such as the request being invalid or a platform issue arising, it should throw a RestError to indicate this. The service replying with a particular status code is not considered a failure at this level as that code could be expected (such as a 404 when checking if a resource already exists.)

The Pipeline itself is just a chain of PipelinePolicy objects that recursively call makeRequest in order until they reach the HttpClient. They may inspect the PipelineResponse or catch any RestError and take appropriate action (such as our retry policy re-issuing a request after a delay on certain HTTP codes or network errors.)

The service client finally gets to decide how to handle the call as a logical operation. This varies as the service may use 4xx or 5xx codes to return nominal responses with JSON objects. In our OpenAPI-based codegen, if we threw or not was dependent upon if the schema listed a particular response code in their specification.

You can see some of the logic here: https://github.com/Azure/azure-sdk-for-js/blob/f2c59d18c54261125e1daf950ac9a4ace6006403/sdk/core/core-client/src/serviceClient.ts#L206 though it's a bit tricky to reason about as the original throw happens in the deserializationPolicy: https://github.com/Azure/azure-sdk-for-js/blob/f2c59d18c54261125e1daf950ac9a4ace6006403/sdk/core/core-client/src/deserializationPolicy.ts#L226

I believe we use @error to do something similar for TypeSpec modular codegen.

For RLC libraries, we opted to stick close to how the pipeline handles it and return the response directly to the consumer: https://github.com/Azure/azure-sdk-for-js/blob/f2c59d18c54261125e1daf950ac9a4ace6006403/sdk/core/core-client-rest/src/sendRequest.ts#L53

This means the consumer is responsible for inspecting the response and determining if it is successful before trying to process it. To make this easier, we provide the isUnexpected helper to narrow to the shape of known response bodies. This also allows the consumer to decide if they wish to raise an error should a response be undescribed by the schema.

What you seem to be suggesting in this issue is a higher policy version of RLC that tries to normalize any thrown RestErrors along with service-generated errors into a standard response shape. Is that correct?

Given the wide-ranging shape of all possible error responses from a service, I'm uncertain this is something we could do generically for all of Azure. Furthermore, we may see errors from things like authentication libraries, proxy services, or any number of other participants involved in servicing HTTP calls.

/cc @joheredi

[github-actions[bot]]

Hi @howieleung. Thank you for opening this issue and giving us the opportunity to assist. To help our team better understand your issue and the details of your scenario please provide a response to the question asked above or the information requested above. This will help us more accurately address your issue.

[github-actions[bot]]

Hi @howieleung, we're sending this friendly reminder because we haven't heard back from you in 7 days. We need more information about this issue to help address it. Please be sure to give us your input. If we don't hear back from you within 14 days of this comment the issue will be automatically closed. Thank you!