SubscriptionClient.subscriptions.get SubscriptionNotFound Error is in the wrong format

[natanel-ficher-cyera]

• Package Name: @azure/core-rest-pipeline": "1.19.0","@azure/arm-subscriptions": "5.1.0"
• Operating system: Mac
• nodejs
  • version: v18.20.4
• typescript
  • version: 4.8.4
• Is the bug related to documentation in
  • README.md
  • source code documentation
  • SDK API docs on https://learn.microsoft.com

Describe the bug
When you use SubscriptionClient.subscriptions.get(subscriptionId) on a subscription that does not exist, An error is thrown
the TS typing makes it seem like error.code === 'SubscriptionNotFound' and error.message = "The subscription '...' was not found."

but instead what you get is error.code === undefined and error.message = {error:"code": "SubscriptionNotFound", message: "The subscription '...' was not found."}, which goes against the Typing of message:string and means in TS you cannot check for 'SubscriptionNotFound' without TS shenanigans.

To Reproduce
Steps to reproduce the behavior:

1. call .get on a subscription that does not exist

example:
const client = new SubscriptionClient(CREDENTIALS) client.subscriptions.get(FAKE_ID)

Expected behavior
I would expect the returned error to have error.code === 'SubscriptionNotFound' and error.message = "The subscription '...' was not found."

[xirzec]

@joheredi @qiaozha looks like there might been an emitter issue here

[qiaozha]

@xirzec @azure/arm-subscription is a HLC library. This problem happens on how we handled the error response

azure-sdk-for-js/sdk/core/core-client/src/deserializationPolicy.ts

Line 255 in cc69e27

const error = new RestError(initialErrorMessage, {

in core-client. But I am not sure if we should fix it, as there maybe some other customers that are parsing the original error inside the message?

For modular and RLC, we have already fixed it in core client rest here https://github.com/Azure/azure-sdk-for-js/pull/27867/files and in codegen here Azure/autorest.typescript#1997

[xirzec]

@qiaozha Maybe I'm misunderstanding something here, but isn't the problem that the spec for this API doesn't have a default response shape?

https://github.com/Azure/azure-rest-api-specs/blob/f80f83ae1cbefd1bd8d512452353795a0e4efcdc/specification/subscription/resource-manager/Microsoft.Subscription/stable/2016-06-01/subscriptions.json#L119

Since that causes us to throw on this line:

azure-sdk-for-js/sdk/core/core-client/src/deserializationPolicy.ts

Line 264 in cc69e27

throw error;

But when the spec does have at least a default handler we set the code and message here:

azure-sdk-for-js/sdk/core/core-client/src/deserializationPolicy.ts

Line 295 in cc69e27

error.code = internalError.code;

Perhaps we could do something smarter here, like see if the parsedBody is a "standard" error shape, even if we don't have the original message? I do agree there is some risk of folks currently working around this by doing JSON.parse of the message though.

[jeremymeng]

Perhaps we could do something smarter here, like see if the parsedBody is a "standard" error shape, even if we don't have the original message? I do agree there is some risk of folks currently working around this by doing JSON.parse of the message though.

Yes we could try some heuristic in deserializationPolicy.ts to help the experience. Let me take a look into this.

[qiaozha]

@xirzec I see your point, I will contact the service team to add error response here.
But I realize the logic of handling error response is slightly different between Modular and HLC. in Modular, we parse the error code for unsuccessful status including expected error and unexpected error, but in HLC, we only parse the error code for expected error status. would it be a concern when we migrate to Modular from HLC?