Azure-sdk-for-go: Handling of API Errors in the SDK

[tombuildsstuff]

:wave:

I've been meaning to raise this one for a while.. in this case I'm using the SQL Databases SDK - however this also applies to CosmosDB Accounts and I believe Container Service (amongst other services) too.

When making the following request using the SQL Databases SDK with an invalid configuration:

PUT /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/acctest_rg_5909366699526987910/providers/Microsoft.Sql/servers/acctestsqlserver5909366699526987910/databases/acctestdb5909366699526987910?api-version=2014-04-01 HTTP/1.1
Host: management.azure.com
User-Agent: HashiCorp-Terraform-v0.10.0-dev
Content-Length: 179
Content-Type: application/json
Accept-Encoding: gzip

{
    "tags": {},
    "location": "eastus",
    "properties": {
        "collation": "SQL_Latin1_General_CP1_CI_AS",
        "createMode": "Default",
        "edition": "DataWarehouse",
        "requestedServiceObjectiveName": "System"
    }
}

which returns the response from the API:

HTTP/1.1 400 Bad Request
Content-Length: 323
Cache-Control: no-store, no-cache
Content-Type: application/json
Dataserviceversion: 3.0;
Date: Tue, 29 Aug 2017 23:18:28 GMT
Preference-Applied: return-content
Server: Microsoft-HTTPAPI/2.0
Strict-Transport-Security: max-age=31536000; includeSubDomains
X-Content-Type-Options: nosniff
X-Ms-Correlation-Request-Id: b6059a2e-6c97-4f31-aa5f-37689644e58a
X-Ms-Ratelimit-Remaining-Subscription-Writes: 1197
X-Ms-Request-Id: 936005d0-13ee-4142-8039-43b972c2df7c
X-Ms-Routing-Request-Id: UKWEST:20170829T231829Z:b6059a2e-6c97-4f31-aa5f-37689644e58a

{
    "code": "40808",
    "message": "The edition \u0027DataWarehouse\u0027 does not support the service objective \u0027System\u0027.",
    "target": null,
    "details": [{
        "code": "40808",
        "message": "The edition \u0027DataWarehouse\u0027 does not support the service objective \u0027System\u0027.",
        "target": null,
        "severity": "16"
    }],
    "innererror": []
}

When checking the error response from the SDK for this call, the error returned is:

sql.DatabasesClient#CreateOrUpdate: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="Unknown" Message="Unknown service error"

Whilst it's possible to deserialise the response in each case and parse out the error message; it feels like something the SDK should be doing. As such would it make sense for the SDK to automatically check for and parse an error response if it's not a successful status code and append that to the error message (which in it's current form isn't particularly helpful)? In addition - it seems odd that the code is Unknown when BadRequest is being returned from the API?

@marstr @mcardosos what do you think? :)

Thanks!

[tombuildsstuff]

Just to add another example to this, I believe we’ve also seeing this from the CosmosDB SDK (but I don’t have a sample to hand)

[tombuildsstuff]

Just found another example for this - when updating an App Service (which, coincidently I think the Swagger's broken for - but I'll file a separate issue on the Rest API Specs repository about that 😄)

Response:

{
    "Code": "BadRequest",
    "Message": "ttlInSeconds object is not present in the request body.",
    "Target": null,
    "Details": [{
        "Message": "ttlInSeconds object is not present in the request body."
    }, {
        "Code": "BadRequest"
    }, {
        "ErrorEntity": {
            "ExtendedCode": "01008",
            "MessageTemplate": "{0} object is not present in the request body.",
            "Parameters": ["ttlInSeconds"],
            "Code": "BadRequest",
            "Message": "ttlInSeconds object is not present in the request body."
        }
    }],
    "Innererror": null
}

Error:

web.AppsClient#CreateOrUpdate: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="Unknown" Message="Unknown service error"

Hope that helps :)

[tombuildsstuff]

hey @jhendrixMSFT

Just checking in - we're still seeing this in SDK 11.1 and AutoRest 9.4.1 for the Container Service SDK (issue) - is there a rough timeline for when this will be fixed? :)

Thanks!

[jhendrixMSFT]

Hello @tombuildsstuff sorry this has languished a bit. I will try to dig into this in the next few days, if it ends up being an easy fix we can roll it into v12.

[jhendrixMSFT]

OK I looked into this and using your example for SQL this is fixed in the upcoming v12 SDK. Here's the output I get in a test program I wrote.

panic: sql.DatabasesClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="40808" Message="The edition 'DataWarehouse' does not support the service objective 'System'." Details=[{"code":"40808","message":"The edition 'DataWarehouse' does not support the service objective 'System'.","severity":"16","target":null}]

goroutine 1 [running]:
main.main()
        D:/work/src/playground/issue748/issue.go:46 +0x7b3
exit status 2

The output is a little wonky due to how we wrap exceptions (something I'd like to eventually clean up) but you can actually get the underlying issue which is good.

[tombuildsstuff]

@jhendrixMSFT that's great news - thanks for looking into this :)

[jhendrixMSFT]

This should be resolved in v12, please re-open if you still experience this issue.
https://github.com/Azure/azure-sdk-for-go/releases/tag/v12.0.0-beta

[pixelicous]

@jhendrixMSFT @tombuildsstuff i am still seeing this issue when trying to update an appservice, exactly like tom showed above!

[sjwaight]

I am seeing this error when attempting to update an existing App Service Plan, but the scenario seems to arise when you have multiple ways of updating the App Service using Terraform and each has its state file.

[tombuildsstuff]

@sjwaight @pixelicous just to let you know that the App Service errors are related to a bug in the App Service API's (which is being tracked here) - and not related to the SDK / this issue (which has been resolved).

[pixelicous]

@tombuildsstuff thanks mate.. ill take a look

[marstr]

Howdy folks, seeing as we've confirmed this issue as resolved and it's still triggering response, I'm going to lock the conversation. If you believe you're running into a related issue, and don't find the guidance you need in the issue @tombuildsstuff linked above, please open a new issue in this repository.