[core-http] de-serialize before checking internalError by jeremymeng · Pull Request #12150 · Azure/azure-sdk-for-js

jeremymeng · 2020-10-29T17:55:02Z

Currently we check parsedBody.error or parsedBody for code and
message to use in the RestError to be thrown. However, we do this
on the raw object from parsing XML.

In the example added to the test, without this fix, the parsedBody
from xml parsing is

{ Code: "ContainerAlreadyExists", Message: "The specified container already exists." }

but we are accessing the properties with lowercase code and message.

We should check on the object after de-serializing when we have error
object properly mapped already.

Currently we check `parsedBody.error` or `parsedBody` for `code` and `message` to use in the `RestError` to be thrown. However, we do this on the raw object from parsing XML. In the example added to the test, without this fix, the `parsedBody` from xml parsing is ```js { Code: "ContainerAlreadyExists", Message: "The specified container already exists." } ``` but we are accessing the properties with lowercase `code` and `message`. We should check on the object after de-serializing when we have error object properly mapped already.

the original XML response.

HarshaNalluru

Nice one!

HarshaNalluru · 2020-10-29T23:40:36Z

-    // If error response has a body, try to extract error code & message from it
-    // Then try to deserialize it using default body mapper
+    // If error response has a body, try to deserialize it using default body mapper.
+    // Then try to extract error code & message from it


ljian3377 · 2020-10-30T03:58:06Z

Well done. One remaining issue here is the err.code is still undefined. Reproduce with this case.
https://github.com/jeremymeng/azure-sdk-for-js/blob/5563bb80173184f84e3b8760f263f543b91ac125/sdk/storage/storage-blob/test/blockblobclient.spec.ts#L389

Looks like we are missing the mapping for Code. Do you know how to change this in the generated code?
https://github.com/jeremymeng/azure-sdk-for-js/blob/5563bb80173184f84e3b8760f263f543b91ac125/sdk/storage/storage-blob/src/generated/src/models/mappers.ts#L104

export const StorageError: coreHttp.CompositeMapper = {
  serializedName: "StorageError",
  type: {
    name: "Composite",
    className: "StorageError",
    modelProperties: {
      message: {
        xmlName: "Message",
        serializedName: "Message",
        type: {
          name: "String"
        }
      }
    }
  }
};

export const DataLakeStorageErrorError: coreHttp.CompositeMapper = {
  serializedName: "DataLakeStorageError_error",
  type: {
    name: "Composite",
    className: "DataLakeStorageErrorError",
    modelProperties: {
      code: {
        xmlName: "Code",
        serializedName: "Code",
        type: {
          name: "String"
        }
      },
      message: {
        xmlName: "Message",
        serializedName: "Message",
        type: {
          name: "String"
        }
      }
    }
  }
};

Need to fix this for all four packages. And I wonder what the DataLakeStorageErrorError in Blob and StorageErrorError in DataLake are for.

jeremymeng · 2020-10-30T19:28:48Z

One remaining issue here is the err.code

@ljian3377 yes, Storage side needs a swagger transformation to add the Code property.

Also note that this changes the Storage error message even though it's a bug fix. We might want to bump minor version of Storage packages.

joheredi

Looks good! Would you mind porting this fix to @azure/core-client?

jeremymeng · 2020-10-30T22:00:55Z

Would you mind porting this fix to @azure/core-client?

Done porting. Was waiting for feedback first before porting.

ljian3377 · 2020-10-31T14:48:48Z

One remaining issue here is the err.code

@ljian3377 yes, Storage side needs a swagger transformation to add the Code property.

Also note that this changes the Storage error message even though it's a bug fix. We might want to bump minor version of Storage packages.

@jeremymeng FYI, we are going to release the GA version of the previous beta in the November release, which bumped up the minor version.

ljian3377 · 2020-11-02T11:11:41Z

When will this be merged?

xirzec

The deserialization logic is very complex, so I'm not sure if my comments all make sense. 😅

xirzec · 2020-11-02T19:48:41Z

@@ -247,14 +243,26 @@ function handleErrorResponse(
          }
        }
        if (error.response) {


while reading this code, I'm not sure why this if is here, since error.response === parsedResponse and since we're already inside if (parsedResponse.parsedBody) presumably parsedResponse must be true.

This if check was added in last change by @sarangan12? I will remove

Checking on error.response removed.

xirzec · 2020-11-02T19:54:23Z

+      if (defaultBodyMapper) {
+        if (error.response) {


can't we combine these?

if (defaultBodyMapper && error.response) {

Checking on error.response removed.

xirzec · 2020-11-02T20:00:02Z

+          deserializedError = operationSpec.serializer.deserialize(
            defaultBodyMapper,
            valueToDeserialize,
            "error.response.parsedBody"


This is trying to deserialize valueToDeserialize.error.response.parsedBody ? Is there a reason we don't pass that property in as the valueToDeserialize instead?

I was trying to keep the current code/logic. It deserializes and assigns to error.response.parsedBody I was a little surprised that the code

const errorResponse: FullOperationResponse = error.response; errorResponse.parsedBody = operationSpec.serializer.deserialize(

also updates error.response.parsedBody

There is some extra handling of sequence type I think that's why valueToDeserialize is introduced.

This const is also introduced in last PR. @sarangan12 do you remember the reason why this change?

I guess it's to cast error.response back to FullOperationResponse so we can assign the parsedBody property. I updated this PR to remove the if check on error.response, and do as FulloperationResponse cast.

I feel like this could be cleaned up more, but I am too scared to suggest any specific changes this close to release, so I think I'm good with it.

- Cast `error.response` back to `FullOperationResponse` so we could assign it's `parsedBody` property

sadasant

Looks good!

python track2 configure (Azure#12150)

jeremymeng requested review from bterlson, chradek, daviwil and xirzec as code owners October 29, 2020 17:55

ghost added the Azure.Core label Oct 29, 2020

jeremymeng requested review from joheredi and sarangan12 October 29, 2020 17:55

Fix storage tests now that they have the correct message instead of

5563bb8

the original XML response.

jeremymeng requested review from HarshaNalluru, XiaoningLiu, jiacfan, ljian3377 and vinjiang as code owners October 29, 2020 19:29

HarshaNalluru approved these changes Oct 29, 2020

View reviewed changes

HarshaNalluru reviewed Oct 29, 2020

View reviewed changes

ljian3377 mentioned this pull request Oct 30, 2020

SDK error messages are in XML format #6927

Closed

6 tasks

joheredi approved these changes Oct 30, 2020

View reviewed changes

Port fix to core-client

62b46d2

jeremymeng requested a review from ramya-rao-a as a code owner October 30, 2020 22:00

ljian3377 mentioned this pull request Oct 31, 2020

Add the Code -> code mapping for StorageError #12211

Merged

sarangan12 approved these changes Nov 2, 2020

View reviewed changes

xirzec reviewed Nov 2, 2020

View reviewed changes

- Remove if (error.response) check as it is already known as non-empty

f155f51

- Cast `error.response` back to `FullOperationResponse` so we could assign it's `parsedBody` property

sadasant approved these changes Nov 2, 2020

View reviewed changes

xirzec approved these changes Nov 3, 2020

View reviewed changes

jeremymeng merged commit ba864c0 into Azure:master Nov 3, 2020

jeremymeng deleted the corehttp-deserialize-error-response-before-checking-innererror branch November 3, 2020 20:05

openapi-sdkautomation Bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Dec 29, 2020

CodeGen from PR 12150 in Azure/azure-rest-api-specs

25c0931

python track2 configure (Azure#12150)

jeremymeng mentioned this pull request Apr 9, 2021

[Storage] Error code and message should live as top level properties on the error object #5796

Closed

Conversation

jeremymeng commented Oct 29, 2020

Uh oh!

HarshaNalluru left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljian3377 commented Oct 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeremymeng commented Oct 30, 2020

Uh oh!

joheredi left a comment

Choose a reason for hiding this comment

Uh oh!

jeremymeng commented Oct 30, 2020

Uh oh!

ljian3377 commented Oct 31, 2020

Uh oh!

ljian3377 commented Nov 2, 2020

Uh oh!

xirzec left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadasant left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ljian3377 commented Oct 30, 2020 •

edited

Loading