Raise appropriate exception when error_body is dict | Fix exception raised during handling of another exception

[tasherif-msft]

This resolves #18129, Azure/azure-cli#16217 and #15956

This PR will improve error handling in the following ways:

• The error traceback will be more thorough showing the error code, message, additional info consistently
• No more error occurred during handling of the above exception
• No more bug related to JSON responses
• All errors thrown should be consistent in their properties

[cbare]

Hi Tamer,

Thanks for having a look at this bug. I hope you don't mind, but I went down a bit of rabbit-hole on this one.

What I imagine is happening is that Azure's endpoints return either XML or JSON depending on the Accept header in the request, and that applies to error responses as well as 200s.

So, the error_body we get in this method could be the result of deserializing an XML response into xml.etree.ElementTree objects or a JSON response into a dict. Here's some toy code to play around with.

import xml.etree.ElementTree as ET
import json

xml_txt = """
<?xml version="1.0"?>
<error>
  <code>1234</code>
  <message>Flaming disaster!!</message>
  <asdf>quux</asdf>
</error>
""".strip()

err1 = ET.fromstring(xml_txt)

json_txt = """
{
    "error": {
        "code": "InvalidQueryParameterValue",
        "message": "Value for one of the query parameters specified in the request URI is invalid...",
        "xyz": "blah"
    }
}
"""
err2 = json.loads(json_txt)

We could tweak the existing code to handle either scenario, plus have a half-way decent fall-back if some other type, like plain text, comes back.

# try out the XML case
error_body = err1

# try out the dict case
# error_body = err2

additional_data = {}
if isinstance(error_body, ET.Element):
    for child in error_body:
        if child.tag.lower() == 'code':
            error_code = child.text
        elif child.tag.lower() == 'message':
            error_message = child.text
        else:
            additional_data[child.tag] = child.text
elif isinstance(error_body, dict):
    for key, value in error_body['error'].items():
        if key.lower() == 'code':
            error_code = value

        elif key.lower() == 'message':
            error_message = value
        else:
            additional_data[key] = value
else:
   #  _LOGGER.warning(f'Unexpected return type {type(error_body)} from ContentDecodePolicy.deserialize_from_http_generics.')
    error_message = str(error_body)

print(f'code: {error_code}')
print(f'message: {error_message}')
print(f'additional data: {additional_data}')

The code above is repetitive and still has some problems, like what if our message doesn't have "code" and "message" fields? Then error_code and error_message are undefined.

One idea would be to coerce the one case into the other:

if isinstance(error_body, ET.Element):
    error_dict = {
        child.tag.lower():child.text
        for child in error_body
    }
elif isinstance(error_body, dict):
    error_dict = error_body.get('error', {})
else:
    # _LOGGER.warning(f'Unexpected return type {type(error_body)} from ContentDecodePolicy.deserialize_from_http_generics.')
    error_dict = {'message': str(error_body)}

error_code = error_dict.get('code')
error_message = error_dict.get('message')
additional_data = {k:v for k,v in error_dict.items() if k not in {'code', 'message'}}

print(f'code: {error_code}')
print(f'message: {error_message}')
print(f'additional data: {additional_data}')

I hope this helps,
Cheers!

[tasherif-msft]

Hi @cbare I've spent some time to dig deeper into our error handling.
As you mentioned in most cases we are deserializing the xml responses. In some cases we will not have a response body that contains error codes/messages and instead we will just fetch them from the response headers. Which means this check will be avoided (since the error code and message would just be set here).

This is also why I don't think there is a case where we would encounter an undefined error code/message since the three cases (error in headers and not body, error in xml body, error in JSON body) should cover everything.

I do like your second recommendation, it's very concise. I will make some few tweaks to account for the case I mentioned earlier (error code and message in headers + empty body)

Thank you very much @cbare :)

[azure-pipelines]

No pipelines are associated with this pull request.

[tasherif-msft]

/azp run python - storage - ci

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[tasherif-msft]

/azp run python - storage - ci

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[tasherif-msft]

/azp run python - storage - ci

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[tasherif-msft]

/azp run python - storage - ci

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).