Merge lp:~jtv/gwacl/http-error into lp:gwacl

[Jeroen T. Vermeulen]

Harmonize "error based on http response" types.

This code replaces our existing Error and ServerError structs with one interface: HTTPError. It's an "error" but with an added Status() for the HTTP status code.

Underneath, there are two implementations: ServerError represents any HTTP error response. AzureError is an error message from Azure itself, with more details embedded as XML. For now we don't have any need to distinguish between them, so Error() and Status() are the only methods we need. A single factory selects and produces the right implementation, so all kinds of HTTP errors are automatically handled wherever we use the new types. We get better Azure errors in the management API as a free part of the bargain.

You may wonder how HTTPStatus works. It's a type based on int, but it adds a Status() method. And so both ServerError and AzureError implement their Status() methods simply by embedding an HTTPStatus. Effectively it's a member whose name matches its type and whose methods get added to those of the surrounding struct.

There are a few more places I can clean up now, because send() now returns an "error" instead of a "*Error" — so there's no more risk of accidentally converting a nil "*Error" to a non-nil "error." But this branch touches a lot of code (mostly unavoidable changes that just came along) so let's stop it from growing even further!

Jeroen

[lp:~jtv/gwacl/http-error]

67. By Jeroen T. Vermeulen on 2013-03-27

Merge trunk, resolve conflict.

68. By Jeroen T. Vermeulen on 2013-03-27

Add missing error check that messed up my real-world tests.

[Raphaël Badin (rvb)]

Looks good, a very nice improvement!

[0]

28 +// HTTPError is an extended version of the standard "error" interface. It
29 +// adds an HTTP status code.
30 +type HTTPError interface {
31 + error
32 + Status() int
33 +}

For clarity, I'd be in favor of renaming Status() into StatusCode().

[1]

253 + c.Check(err, ErrorMatches, ".*102.*")
254 + c.Check(err, ErrorMatches, ".*Frotzed.*")

and

265 + c.Check(err, ErrorMatches, ".*102.*")
266 + c.Check(err, ErrorMatches, ".*Frotzed.*")

and

277 + c.Check(err, ErrorMatches, ".*102.*")
278 + c.Check(err, ErrorMatches, ".*Frotzed.*")

and

289 + c.Check(err, ErrorMatches, ".*146.*")
290 + c.Check(err, ErrorMatches, ".*Frotzed.*")

and

300 + c.Check(err, ErrorMatches, ".*246.*")
301 + c.Check(err, ErrorMatches, ".*Frotzed.*")

I think you should fold these into on statement like this:
c.Check(err, ErrorMatches, ".*102 Frotzed.*")

because the status of the response is "102 Frotzed":

    response := &http.Response{
        Status: "102 Frotzed",
        StatusCode: 102,
        Body: ioutil.NopCloser(strings.NewReader("<Error><Code>Frotzed</Code><Message>failed to put blob</Message></Error>")),
    }

review: Approve

[Julian Edwards (julian-edwards)]

On Wednesday 27 Mar 2013 10:27:19 you wrote:
> Review: Approve
>
> Looks good, a very nice improvement!
>
> [0]
>
> 28 +// HTTPError is an extended version of the standard "error" interface.
> It 29 +// adds an HTTP status code.
> 30 +type HTTPError interface {
> 31 + error
> 32 + Status() int
> 33 +}
>
> For clarity, I'd be in favor of renaming Status() into StatusCode().
>
> [1]
>
> 253 + c.Check(err, ErrorMatches, ".*102.*")
> 254 + c.Check(err, ErrorMatches, ".*Frotzed.*")
>
> and
>
> 265 + c.Check(err, ErrorMatches, ".*102.*")
> 266 + c.Check(err, ErrorMatches, ".*Frotzed.*")
>
> and
>
> 277 + c.Check(err, ErrorMatches, ".*102.*")
> 278 + c.Check(err, ErrorMatches, ".*Frotzed.*")
>
> and
>
> 289 + c.Check(err, ErrorMatches, ".*146.*")
> 290 + c.Check(err, ErrorMatches, ".*Frotzed.*")
>
> and
>
> 300 + c.Check(err, ErrorMatches, ".*246.*")
> 301 + c.Check(err, ErrorMatches, ".*Frotzed.*")
>
> I think you should fold these into on statement like this:
> c.Check(err, ErrorMatches, ".*102 Frotzed.*")
>
> because the status of the response is "102 Frotzed":
>
> response := &http.Response{
> Status: "102 Frotzed",
> StatusCode: 102,
> Body:
> ioutil.NopCloser(strings.NewReader("<Error><Code>Frotzed</Code><Message>fai
> led to put blob</Message></Error>")), }

Mmmmm. The actual error should be what's in <Message>. The "102 Frotzed" is
largely irrelevant.

[lp:~jtv/gwacl/http-error]

69. By Jeroen T. Vermeulen on 2013-03-27

Review suggestion: renamed Status() to StatusCode().

[Jeroen T. Vermeulen (jtv)]

> 28 +// HTTPError is an extended version of the standard "error"
> interface. It
> 29 +// adds an HTTP status code.
> 30 +type HTTPError interface {
> 31 + error
> 32 + Status() int
> 33 +}
>
> For clarity, I'd be in favor of renaming Status() into StatusCode().

Done.

> 253 + c.Check(err, ErrorMatches, ".*102.*")
> 254 + c.Check(err, ErrorMatches, ".*Frotzed.*")

> I think you should fold these into on statement like this:
> c.Check(err, ErrorMatches, ".*102 Frotzed.*")
>
> because the status of the response is "102 Frotzed":

That particular status string does not enter into the picture, because http.Response doesn't. The HTTPError factory had to work with both the http package and go-curl.

So instead I use the numerical status code, and add the Go standard library's name for that status code. I don't want all these tests dependent on details of how the error string is composed — there are separate tests for that!

=== modified file 'example/live_example_managementapi.go'
--- example/live_example_managementapi.go	2013-03-22 07:01:27 +0000
+++ example/live_example_managementapi.go	2013-03-27 16:36:22 +0000
@@ -130,7 +130,8 @@
     role := gwacl.NewRole("ExtraSmall", roleName, []gwacl.LinuxProvisioningConfiguration{*configurationSet}, OSVirtualHardDisk)
     machineName := makeRandomIdentifier("gwaclmachine", 20)
     deployment := gwacl.NewDeploymentForCreateVMDeployment(machineName, "Staging", machineName, []gwacl.Role{*role}, "")
-    api.AddDeployment(deployment, hostServiceName)
+    err = api.AddDeployment(deployment, hostServiceName)
+    checkError(err)
     fmt.Println("Done adding VM deployment\n")
 
     // Wait for the deployment to be created.

=== added file 'httperror.go'
--- httperror.go	1970-01-01 00:00:00 +0000
+++ httperror.go	2013-03-27 16:36:22 +0000
@@ -0,0 +1,77 @@
+package gwacl
+
+import (
+    "encoding/xml"
+    "errors"
+    "fmt"
+    "net/http"
+)
+
+// HTTPError is an extended version of the standard "error" interface.  It
+// adds an HTTP status code.
+type HTTPError interface {
+    error
+    StatusCode() int
+}
+
+// HTTPStatus is an HTTP status code.
+type HTTPStatus int
+
+// Status returns the HTTP status code as an int.
+func (s HTTPStatus) StatusCode() int {
+    return int(s)
+}
+
+// AzureError is an HTTPError returned by the Azure API.  It contains an
+// error message, and an Azure-defined error code.
+type AzureError struct {
+    error      `xml:"-"`
+    HTTPStatus `xml:"-"`
+    Code       string `xml:"Code"`
+    Message    string `xml:"Message"`
+}
+
+// *AzureError implements HTTPError.
+var _ HTTPError = &AzureError{}
+
+func (e *AzureError) Error() string {
+    description := e.error.Error()
+    status := e.StatusCode()
+    name := http.StatusText(status)
+    return fmt.Sprintf("%s: %s - %s (http code %d: %s)", description, e.Code, e.Message, status, name)
+}
+
+// ServerError is a generic HTTPError, without any further helpful information
+// from the server that we can count on.
+type ServerError struct {
+    error
+    HTTPStatus
+}
+
+// *ServerError implements HTTPError.
+var _ HTTPError = &ServerError{}
+
+func (e *ServerError) Error() string {
+    description := e.error.Error()
+    status := e.StatusCode()
+    name := http.StatusText(status)
+    return fmt.Sprintf("%s (%d: %s)", description, status, name)
+}
+
+// NewHTTPError returns the appropriate HTTPError implementation for a given
+// HTTP response.
+// It takes a status code and response body, rather than just a standard
+// http.Response object, because it must work with go-curl as well as Go's
+// standard http package.
+func newHTTPError(status int, body []byte, description string) HTTPError {
+    httpStatus := HTTPStatus(status)
+    baseErr := errors.New(description)
+    azureError := AzureError{error: baseErr, HTTPStatus: httpStatus}
+    err := xml.Unmarshal(body, &azureError)
+    if err != nil {
+        // It's OK if the response body wasn't actually XML...  That just means
+        // it wasn't a proper AzureError.  We have another error type for that.
+        return &ServerError{error: baseErr, HTTPStatus: httpStatus}
+    }
+    return &azureError
+}

=== added file 'httperror_test.go'
--- httperror_test.go	1970-01-01 00:00:00 +0000
+++ httperror_test.go	2013-03-27 16:36:22 +0000
@@ -0,0 +1,68 @@
+package gwacl
+
+import (
+    "errors"
+    "fmt"
+    . "launchpad.net/gocheck"
+)
+
+type httpErrorSuite struct{}
+
+var _ = Suite(&httpErrorSuite{})
+
+func (suite *httpErrorSuite) TestNewHTTPErrorParsesAzureError(c *C) {
+    description := "upload failed"
+    status := 415
+    code := "CannotUpload"
+    message := "Unknown data format"
+    xml := fmt.Sprintf(`<Error>
+        <Code>%s</Code>
+        <Message>%s</Message>
+    </Error>`,
+        code, message)
+
+    httpErr := newHTTPError(status, []byte(xml), description)
+
+    azureErr, ok := httpErr.(*AzureError)
+    c.Assert(ok, Equals, true)
+    c.Check(azureErr.StatusCode(), Equals, status)
+    c.Check(azureErr.Code, Equals, code)
+    c.Check(azureErr.Message, Equals, message)
+    c.Check(httpErr, ErrorMatches, ".*"+description+".*")
+    c.Check(httpErr, ErrorMatches, ".*415: Unsupported Media Type.*")
+}
+
+func (suite *httpErrorSuite) TestNewHTTPErrorResortsToServerError(c *C) {
+    description := "could not talk to server"
+    status := 505
+
+    httpErr := newHTTPError(status, []byte{}, description)
+
+    _, ok := httpErr.(*ServerError)
+    c.Assert(ok, Equals, true)
+    c.Check(httpErr.StatusCode(), Equals, status)
+    c.Check(httpErr, ErrorMatches, ".*505: HTTP Version Not Supported.*")
+    c.Check(httpErr, ErrorMatches, ".*"+description+".*")
+}
+
+func (suite *httpErrorSuite) TestAzureErrorComposesError(c *C) {
+    description := "something failed"
+    status := 410
+    httpErr := AzureError{
+        error:      errors.New(description),
+        HTTPStatus: HTTPStatus(status),
+        Code:       "MissingError",
+        Message:    "Your object has disappeared",
+    }
+    c.Check(httpErr.Error(), Equals, "something failed: MissingError - Your object has disappeared (http code 410: Gone)")
+}
+
+func (suite *httpErrorSuite) TestServerErrorComposesError(c *C) {
+    description := "something failed"
+    status := 501
+    httpErr := ServerError{
+        error:      errors.New(description),
+        HTTPStatus: HTTPStatus(status),
+    }
+    c.Check(httpErr.Error(), Equals, "something failed (501: Not Implemented)")
+}

=== modified file 'storage.go'
--- storage.go	2013-03-27 03:47:21 +0000
+++ storage.go	2013-03-27 16:36:22 +0000
@@ -218,40 +218,30 @@
 // Send a request to the storage service and process the response.
 // The "res" parameter is typically an XML struct that will deserialize the
 // raw XML into the struct data.  The http Response object is returned.
-// If the response's HTTP status code is not the same as "expected_status" then
-// Error will be non-nil and the received status will be set in Error.status.
-// If any error occurrs and if more specific error data is available from
-// Azure, it is returned in Error.Code/Error.Message.  Error.status will always
-// contain the HTTP response code if a response is received.  If the request
-// itself failed, Error.Code is the empty string and a textual failure reason
-// will be in Error.Message.
+//
+// If the response's HTTP status code is not the same as "expected_status"
+// then an HTTPError will be returned as the error.  When the returned error
+// is an HTTPError, the request response is also returned.  In other error
+// cases, the returned response may be the one received from the server or
+// it may be nil.
 func (context *StorageContext) send(req *http.Request, res Deserializer, expected_status int) (*http.Response, error) {
     client := context.getClient()
     resp, err := client.Do(req)
     if err != nil {
-        return nil, &Error{
-            status: 0, Code: "", Message: err.Error()}
+        return nil, err
     }
 
-    errmsg := &Error{}
-    errmsg.status = resp.StatusCode
     var data []byte
 
     if resp.StatusCode != expected_status {
         if resp.Body != nil {
             data, err = ioutil.ReadAll(resp.Body)
             if err != nil {
-                errmsg.Message = fmt.Sprintf(
-                    "Failed to read response data: %s", err)
-                return resp, errmsg
+                return resp, err
             }
         }
-        err := errmsg.Deserialize(data)
-        if err != nil {
-            errmsg.Message = fmt.Sprintf(
-                "request failed with status %d", resp.StatusCode)
-        }
-        return resp, errmsg
+        msg := newHTTPError(resp.StatusCode, data, "Azure request failed")
+        return resp, msg
     }
 
     // If the caller didn't supply an object to deserialize the message into
@@ -263,14 +253,13 @@
     // TODO: deserialize response headers into the "res" object.
     data, err = ioutil.ReadAll(resp.Body)
     if err != nil {
-        errmsg.Message = fmt.Sprintf(
-            "Failed to read response data: %s", err)
-        return resp, errmsg
+        msg := fmt.Errorf("failed to read response data: %s", err)
+        return resp, msg
     }
     err = res.Deserialize(data)
     if err != nil {
-        errmsg.Message = fmt.Sprintf("Failed to deserialize data: %s", err)
-        return resp, errmsg
+        msg := fmt.Errorf("Failed to deserialize data: %s", err)
+        return resp, msg
     }
 
     return resp, nil

=== modified file 'storage_test.go'
--- storage_test.go	2013-03-26 06:58:54 +0000
+++ storage_test.go	2013-03-27 16:36:22 +0000
@@ -542,7 +542,10 @@
     context := makeStorageContext()
     context.client = &http.Client{Transport: transport}
     err := context.PutBlob("container", "blobname")
-    c.Assert(err.Error(), Equals, "102: Frotzed, failed to put blob")
+    c.Assert(err, NotNil)
+    c.Check(err, ErrorMatches, ".*102.*")
+    c.Check(err, ErrorMatches, ".*Frotzed.*")
+    c.Check(err, ErrorMatches, ".*failed to put blob.*")
 }
 
 type TestPutBlock struct{}
@@ -599,7 +602,10 @@
     context.client = &http.Client{Transport: transport}
     data_reader := bytes.NewReader(MakeRandomByteSlice(10))
     err := context.PutBlock("container", "blobname", "blockid", data_reader)
-    c.Assert(err.Error(), Equals, "102: Frotzed, failed to put block")
+    c.Assert(err, NotNil)
+    c.Check(err, ErrorMatches, ".*102.*")
+    c.Check(err, ErrorMatches, ".*Frotzed.*")
+    c.Check(err, ErrorMatches, ".*failed to put block.*")
 }
 
 type TestPutBlockList struct{}
@@ -659,7 +665,10 @@
     context.client = &http.Client{Transport: transport}
     blocklist := &BlockList{}
     err := context.PutBlockList("container", "blobname", blocklist)
-    c.Assert(err.Error(), Equals, "102: Frotzed, failed to put blocklist")
+    c.Assert(err, NotNil)
+    c.Check(err, ErrorMatches, ".*102.*")
+    c.Check(err, ErrorMatches, ".*Frotzed.*")
+    c.Check(err, ErrorMatches, ".*failed to put blocklist.*")
 }
 
 type TestGetBlockList struct{}
@@ -770,7 +779,10 @@
     context := makeStorageContext()
     context.client = &http.Client{Transport: transport}
     err := context.DeleteBlob("container", "blobname")
-    c.Assert(err.Error(), Equals, "146: Frotzed, failed to delete blob")
+    c.Assert(err, NotNil)
+    c.Check(err, ErrorMatches, ".*146.*")
+    c.Check(err, ErrorMatches, ".*Frotzed.*")
+    c.Check(err, ErrorMatches, ".*failed to delete blob.*")
 }
 
 type TestGetBlob struct{}
@@ -827,5 +839,7 @@
     reader, err := context.GetBlob("container", "blobname")
     c.Check(reader, IsNil)
     c.Assert(err, NotNil)
-    c.Assert(err.Error(), Equals, "246: Frotzed, failed to get blob")
+    c.Check(err, ErrorMatches, ".*246.*")
+    c.Check(err, ErrorMatches, ".*Frotzed.*")
+    c.Check(err, ErrorMatches, ".*failed to get blob.*")
 }

=== modified file 'x509session.go'
--- x509session.go	2013-03-26 06:50:50 +0000
+++ x509session.go	2013-03-27 16:36:22 +0000
@@ -4,7 +4,6 @@
 package gwacl
 
 import (
-    "fmt"
     "net/http"
 )
 
@@ -31,22 +30,11 @@
 // fakes.
 var _X509Dispatcher = performX509CurlRequest
 
-// ServerError is an error implementation representing an http error response
-// from the server.  In addition to being an "error," it also contains the
-// http status code.
-type ServerError struct {
-    error
-    StatusCode int
-}
-
-var _ error = &ServerError{}
-
 // getServerError returns a ServerError matching the given server response
 // status, or nil if the server response indicates success.
-func (session *x509Session) getServerError(status int, description string) error {
+func (session *x509Session) getServerError(status int, body []byte, description string) error {
     if status < http.StatusOK || status >= http.StatusMultipleChoices {
-        failure := fmt.Errorf("%s (http status code %d)", description, status)
-        return &ServerError{error: failure, StatusCode: status}
+        return newHTTPError(status, body, description)
     }
     return nil
 }
@@ -62,7 +50,7 @@
     if err != nil {
         return nil, err
     }
-    err = session.getServerError(response.StatusCode, "GET request failed")
+    err = session.getServerError(response.StatusCode, response.Body, "GET request failed")
     return response.Body, err
 }
 
@@ -77,7 +65,7 @@
     if err != nil {
         return nil, err
     }
-    err = session.getServerError(response.StatusCode, "POST request failed")
+    err = session.getServerError(response.StatusCode, response.Body, "POST request failed")
     return response.Body, err
 }
 
@@ -89,7 +77,7 @@
     if err != nil {
         return err
     }
-    return session.getServerError(response.StatusCode, "DELETE request failed")
+    return session.getServerError(response.StatusCode, response.Body, "DELETE request failed")
 }
 
 // put performs a PUT request to the Azure management API.
@@ -100,5 +88,5 @@
     if err != nil {
         return err
     }
-    return session.getServerError(response.StatusCode, "POST request failed")
+    return session.getServerError(response.StatusCode, response.Body, "POST request failed")
 }

=== modified file 'x509session_test.go'
--- x509session_test.go	2013-03-26 10:23:51 +0000
+++ x509session_test.go	2013-03-27 16:36:22 +0000
@@ -145,12 +145,12 @@
     session, err := newX509Session("subscriptionid", "azure.pem")
     c.Assert(err, IsNil)
 
-    err = session.getServerError(status, msg)
+    err = session.getServerError(status, []byte{}, msg)
     c.Assert(err, NotNil)
 
     c.Check(err, ErrorMatches, ".*"+msg+".*")
     serverError := err.(*ServerError)
-    c.Check(serverError.StatusCode, Equals, status)
+    c.Check(serverError.StatusCode(), Equals, status)
 }
 
 func (suite *x509SessionSuite) TestGetServerErrorLikes20x(c *C) {
@@ -162,7 +162,7 @@
     c.Assert(err, IsNil)
 
     for _, status := range goodCodes {
-        c.Check(session.getServerError(status, ""), IsNil)
+        c.Check(session.getServerError(status, []byte{}, ""), IsNil)
     }
 }
 
@@ -180,7 +180,7 @@
     c.Assert(err, IsNil)
 
     for _, status := range badCodes {
-        c.Check(session.getServerError(status, ""), NotNil)
+        c.Check(session.getServerError(status, []byte{}, ""), NotNil)
     }
 }
 
@@ -232,7 +232,7 @@
     c.Assert(err, NotNil)
 
     serverError := err.(*ServerError)
-    c.Check(serverError.StatusCode, Equals, fixedResponse.StatusCode)
+    c.Check(serverError.StatusCode(), Equals, fixedResponse.StatusCode)
     c.Check(body, DeepEquals, fixedResponse.Body)
 }
 
@@ -288,7 +288,7 @@
     c.Assert(err, NotNil)
 
     serverError := err.(*ServerError)
-    c.Check(serverError.StatusCode, Equals, fixedResponse.StatusCode)
+    c.Check(serverError.StatusCode(), Equals, fixedResponse.StatusCode)
     c.Check(body, DeepEquals, fixedResponse.Body)
 }
 

=== modified file 'xmlobjects.go'
--- xmlobjects.go	2013-03-26 01:02:04 +0000
+++ xmlobjects.go	2013-03-27 16:36:22 +0000
@@ -29,23 +29,6 @@
 }
 
 //
-// Error messages
-//
-type Error struct {
-    status  int    // HTTP status returned.
-    Code    string `xml:"Code"`
-    Message string `xml:"Message"`
-}
-
-func (e *Error) Error() string {
-    return fmt.Sprintf("%d: %s, %s", e.status, e.Code, e.Message)
-}
-
-func (e *Error) Deserialize(data []byte) error {
-    return xml.Unmarshal(data, e)
-}
-
-//
 // LinuxProvisioningConfiguration bits
 //
 

=== modified file 'xmlobjects_test.go'
--- xmlobjects_test.go	2013-03-26 07:00:46 +0000
+++ xmlobjects_test.go	2013-03-27 16:36:22 +0000
@@ -19,24 +19,6 @@
 // Tests for Marshallers
 //
 
-func (suite *xmlSuite) TestErrorDeserialize(c *C) {
-    input := `
-        <Error>
-            <Code>InvalidMusicVolume</Code>
-            <Message>Van Halen played too quietly</Message>
-        </Error>`
-
-    observed := &Error{}
-    err := observed.Deserialize([]byte(input))
-    c.Assert(err, IsNil)
-
-    expected := &Error{
-        Code:    "InvalidMusicVolume",
-        Message: "Van Halen played too quietly",
-    }
-    c.Check(expected, DeepEquals, observed)
-}
-
 func (suite *xmlSuite) TestLinuxProvisioningConfiguration(c *C) {
     config := makeLinuxProvisioningConfiguration()