Merge lp:~jtv/gwacl/poll-if-needed into lp:gwacl

[Jeroen T. Vermeulen]

Support both synchronous and asynchronous operations in blockUntilCompleted().

With this change in place, we'll be able to call blockUntilCompleted() for every request that might possibly be asynchronous — without caring whether it actually is.

This will make us more robust with regard to changes in which operations are asynchronous, or inconsistencies between documentation and the real Azure service. All POST/PUT/DELETE requests can simply return blockUntilCompleted(response) and they will work regardless of how Azure chooses to process them.

Jeroen

[Julian Edwards (julian-edwards)]

Excellent change, thanks. TestBlockUntilCompletedSucceedsOnSyncSuccess was really hard to read and understand but I don't have any constructive things to say about it, maybe this is just How It Has To Be with Go...

49 +func (suite *httpErrorSuite) TestNewAzureErrorFromOperation(c *C) {

Again I think consts are not useful (you may disagree) but use DeepEquals with a single struct check here.

91 +// blockUntilCompleted blocks if necessary. It does not return until the
92 +// requested operation is finished (whether successfully or unsuccessfully).
93 +// Pass in the response from the request that you want to block on. If the
94 +// response indicates that the request finished synchronously (or failed in
95 +// whatever way) then this will not block. However, if the response indicates
96 +// that the Azure service is handling the operation asynchronously, it will
97 +// poll until completion.

I found this hard to read/parse. Can I suggest a replacement:

// blockUntilCompleted blocks as necessary. It does not return until the
// requested operation has finished (whether successfully or unsuccessfully).
// The passed 'response' argument is checked and the following actions will
// occur:
// * response is synchronous: return immediately
// * response is a failure: return immediately
// * response is asynchronous: poll until the operation completes, then return

118 + case http.StatusOK, http.StatusCreated, http.StatusNoContent:

Are you sure there's no other successful statuses? In the storage stuff we checked to see if it was 200-299. Honestly, I'm not sure which is better though!

189 + c.Check(err, NotNil)

Use Assert not Check here, there's not much point continuing if this bit fails.

review: Approve

[lp:~jtv/gwacl/poll-if-needed]

84. By Jeroen T. Vermeulen on 2013-04-02

Review changes.

[Jeroen T. Vermeulen (jtv)]

> Excellent change, thanks. TestBlockUntilCompletedSucceedsOnSyncSuccess was
> really hard to read and understand but I don't have any constructive things to
> say about it, maybe this is just How It Has To Be with Go...

And that was one of the shortest tests! I made some changes and also applied them to the longer equivalent, TestBlockUntilCompletedReturnsHTTPErrorOnSyncFailure. Should produce better error output, too.

> 49 +func (suite *httpErrorSuite) TestNewAzureErrorFromOperation(c *C) {
>
> Again I think consts are not useful (you may disagree) but use DeepEquals with
> a single struct check here.

Turns out I can't do that. There's an “error” in there which contains a pointer to an error that has been created on the fly. DeepEquals will compare the pointers instead of what they point to, and fail.

> 91 +// blockUntilCompleted blocks if necessary. It does not return until
> the
> 92 +// requested operation is finished (whether successfully or
> unsuccessfully).
> 93 +// Pass in the response from the request that you want to block on.
> If the
> 94 +// response indicates that the request finished synchronously (or
> failed in
> 95 +// whatever way) then this will not block. However, if the response
> indicates
> 96 +// that the Azure service is handling the operation asynchronously,
> it will
> 97 +// poll until completion.
>
> I found this hard to read/parse. Can I suggest a replacement:

Rewritten. I avoided enumerating the cases in quite that detail, but I think it's easier to read now.

> 118 + case http.StatusOK, http.StatusCreated, http.StatusNoContent:
>
> Are you sure there's no other successful statuses? In the storage stuff we
> checked to see if it was 200-299. Honestly, I'm not sure which is better
> though!

On the contrary: I know there are other successful statuses. One of them is the very “Accepted” which tells us that an operation is asynchronous. Another one is “Partial Response.” The important thing is that these require special handling that we do not have, and so it's actually better to return an error than to proceed with possibly invalid data.

I went by RFC 2616: http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html

An API might also return “Found” after creating an object on request, but that's a redirect and I don't think the client would bother exposing it to us as a separate response. Or if it does, all our existing code would expose it as an error as well. It'd be better to fail and know than to risk misinterpreting failures.

> 189 + c.Check(err, NotNil)
>
> Use Assert not Check here, there's not much point continuing if this bit
> fails.

In this case, there is. A failure here shows that one particular status is not being handled correctly. It's just that you get two failures for every nil, and the failures don't state what status code triggers the problem.

This example shows that the c.Check()/c.Assert() distinction in gocheck, even though it addresses a real problem, is not all that helpful. I rewrote the checks using a map of expected results.

[lp:~jtv/gwacl/poll-if-needed]

85. By Jeroen T. Vermeulen on 2013-04-02

Review change: rewrote function documentation.

86. By Jeroen T. Vermeulen on 2013-04-02

Review change: some small test simplifications, and clearer test output.

=== modified file 'httperror.go'
--- httperror.go	2013-03-27 16:34:04 +0000
+++ httperror.go	2013-04-02 07:42:21 +0000
@@ -58,7 +58,7 @@
     return fmt.Sprintf("%s (%d: %s)", description, status, name)
 }
 
-// NewHTTPError returns the appropriate HTTPError implementation for a given
+// 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
@@ -75,3 +75,18 @@
     }
     return &azureError
 }
+
+// newAzureErrorFromOperation composes an HTTPError based on an Operation
+// struct, i.e. the result of an asynchronous operation.
+func newAzureErrorFromOperation(outcome *Operation) *AzureError {
+    if outcome.Status != FailedOperationStatus {
+        msg := fmt.Errorf("interpreting Azure %s as an asynchronous failure", outcome.Status)
+        panic(msg)
+    }
+    return &AzureError{
+        error:      errors.New("asynchronous operation failed"),
+        HTTPStatus: HTTPStatus(outcome.HttpStatusCode),
+        Code:       outcome.ErrorCode,
+        Message:    outcome.ErrorMessage,
+    }
+}

=== modified file 'httperror_test.go'
--- httperror_test.go	2013-03-27 16:34:04 +0000
+++ httperror_test.go	2013-04-02 07:42:21 +0000
@@ -4,6 +4,7 @@
     "errors"
     "fmt"
     . "launchpad.net/gocheck"
+    "net/http"
 )
 
 type httpErrorSuite struct{}
@@ -66,3 +67,30 @@
     }
     c.Check(httpErr.Error(), Equals, "something failed (501: Not Implemented)")
 }
+
+func (suite *httpErrorSuite) TestNewAzureErrorFromOperation(c *C) {
+    status := http.StatusConflict
+    code := MakeRandomString(7)
+    message := MakeRandomString(20)
+    // Test body copied from Azure documentation, mutatis mutandi.
+    body := fmt.Sprintf(`
+        <?xml version="1.0" encoding="utf-8"?>
+          <Operation xmlns="http://schemas.microsoft.com/windowsazure">
+            <ID>%s</ID>
+            <Status>Failed</Status>
+            <HttpStatusCode>%d</HttpStatusCode>
+            <Error>
+              <Code>%s</Code>
+              <Message>%s</Message>
+            </Error>
+          </Operation>
+        `,
+        MakeRandomString(5), status, code, message)
+    operation := Operation{}
+    operation.Deserialize([]byte(body))
+
+    err := newAzureErrorFromOperation(&operation)
+    c.Check(err.HTTPStatus, Equals, HTTPStatus(status))
+    c.Check(err.Code, Equals, code)
+    c.Check(err.Message, Equals, message)
+}

=== modified file 'managementapi.go'
--- managementapi.go	2013-04-02 03:44:55 +0000
+++ managementapi.go	2013-04-02 07:42:21 +0000
@@ -41,26 +41,39 @@
 // Set this to 0 to prevent polling from happening.
 var pollerInterval = 10 * time.Second
 
-// blockUntilCompleted blocks until the operation referenced in the headers of the given response
-// is completed.  It returns an error if the given response does not contain
-// a reference to an asychronous operation, if the response from the server
-// cannot be parsed, or if the asynchronous operation fails.
-// Internally, it polls the server at regular intervals until the operation is completed.
+// blockUntilCompleted blocks and polls for completion of an Azure operation.
+// The "response" parameter is the result of the request that started the
+// operation.  If the response says that the operation is running
+// asynchronously, this function will block and poll until the operation is
+// finished.  On the other hand, if the response was a failure or a synchronous
+// result, the function returns immediately.
 func (api *ManagementAPI) blockUntilCompleted(response *x509Response) error {
-    if pollerInterval != 0 {
-        operationID, err := getOperationID(response)
-        if err != nil {
-            return err
-        }
-        poller := newOperationPoller(api, operationID)
-        operation, err := performOperationPolling(poller, pollerInterval)
-        if err != nil {
-            return err
-        }
-        if operation.Status != SucceededOperationStatus {
-            err := fmt.Errorf("asychronous operation failed (status: %v)", operation.Status)
-            return err
-        }
+    switch response.StatusCode {
+    case http.StatusAccepted:
+        // Asynchronous.  Fall out of the switch and start blocking.
+    case http.StatusOK, http.StatusCreated, http.StatusNoContent:
+        // Simple success.  Sometimes it happens; enjoy it.
+        return nil
+    default:
+        // Request failed, synchronously.
+        return newHTTPError(response.StatusCode, response.Body, "request failed")
+    }
+
+    if pollerInterval == 0 {
+        // Polling has been disabled for test purposes.  Return immediately.
+        return nil
+    }
+    operationID, err := getOperationID(response)
+    if err != nil {
+        return fmt.Errorf("could not interpret asynchronous response: %v", err)
+    }
+    poller := newOperationPoller(api, operationID)
+    operation, err := performOperationPolling(poller, pollerInterval)
+    if err != nil {
+        return err
+    }
+    if operation.Status != SucceededOperationStatus {
+        return newAzureErrorFromOperation(operation)
     }
     return nil
 }

=== modified file 'managementapi_test.go'
--- managementapi_test.go	2013-04-02 03:44:55 +0000
+++ managementapi_test.go	2013-04-02 07:42:21 +0000
@@ -5,9 +5,11 @@
 
 import (
     "encoding/xml"
+    "errors"
     "fmt"
     . "launchpad.net/gocheck"
     "net/http"
+    "strings"
     "time"
 )
 
@@ -56,7 +58,77 @@
     c.Check(returnedOperationID, Equals, operationID)
 }
 
-func (suite *managementAPIUtilitiesSuite) TestBlockUntilCompletedErrorsIfOperationFails(c *C) {
+func (suite *managementAPIUtilitiesSuite) TestBlockUntilCompletedSucceedsOnSyncSuccess(c *C) {
+    // Set of expected error returns for success statuses.
+    // (They all map to nil.  It makes it easier further down to report
+    // unexpected errors in a helpful way).
+    expectedErrors := map[int]error{
+        http.StatusOK:        nil,
+        http.StatusCreated:   nil,
+        http.StatusNoContent: nil,
+    }
+    api := makeAPI(c)
+
+    receivedErrors := make(map[int]error)
+    for status := range expectedErrors {
+        err := api.blockUntilCompleted(&x509Response{StatusCode: status})
+        receivedErrors[status] = err
+    }
+
+    c.Check(receivedErrors, DeepEquals, expectedErrors)
+}
+
+func (suite *managementAPIUtilitiesSuite) TestBlockUntilCompletedReturnsHTTPErrorOnSyncFailure(c *C) {
+    // Set of failure statuses, and whether they're supposed to return
+    // HTTPError.
+    // (They all map to true.  It makes it easier further down to report
+    // failures in a helpful way).
+    expectedErrors := map[int]bool{
+        http.StatusBadRequest:          true,
+        http.StatusUnauthorized:        true,
+        http.StatusForbidden:           true,
+        http.StatusNotFound:            true,
+        http.StatusConflict:            true,
+        http.StatusInternalServerError: true,
+        http.StatusNotImplemented:      true,
+    }
+    api := makeAPI(c)
+
+    receivedErrors := make(map[int]bool)
+    for status := range expectedErrors {
+        err := api.blockUntilCompleted(&x509Response{StatusCode: status})
+        _, ok := err.(HTTPError)
+        receivedErrors[status] = ok
+    }
+
+    c.Check(receivedErrors, DeepEquals, expectedErrors)
+}
+
+func (suite *managementAPIUtilitiesSuite) TestBlockUntilCompletedPollsOnAsyncOperation(c *C) {
+    const operationID = "async-operation-id"
+    // For this test, we can't leave polling disabled.  Instead, we set it to
+    // sleep as briefly as possible.
+    pollerInterval = time.Nanosecond
+    // We set the dispatcher up for failure, and prove that
+    // blockUntilCompleted() makes a polling request.
+    failure := errors.New("Simulated failure")
+    rigFailingDispatcher(failure)
+    requests := make([]*x509Request, 0)
+    rigRecordingDispatcher(&requests)
+    api := makeAPI(c)
+    accepted := makeX509ResponseWithOperationHeader(operationID)
+
+    err := api.blockUntilCompleted(accepted)
+
+    c.Check(err, Equals, failure)
+    c.Assert(len(requests), Equals, 1)
+    requestURL := requests[0].URL
+    urlParts := strings.Split(requestURL, "/")
+    polledOperationID := urlParts[len(urlParts)-1]
+    c.Check(polledOperationID, Equals, operationID)
+}
+
+func (suite *managementAPIUtilitiesSuite) TestBlockUntilCompletedErrorsIfAsyncOperationFails(c *C) {
     response := DispatcherResponse{
         response: &x509Response{
             Body:       []byte(fmt.Sprintf(operationXMLTemplate, "Failed")),
@@ -70,10 +142,10 @@
     api := makeAPI(c)
 
     err := api.blockUntilCompleted(operationResponse)
-    c.Check(err, ErrorMatches, ".*asychronous operation failed.*")
+    c.Check(err, ErrorMatches, ".*asynchronous operation failed.*")
 }
 
-func (suite *managementAPIUtilitiesSuite) TestBlockUntilCompletedErrorsIfInvalidXML(c *C) {
+func (suite *managementAPIUtilitiesSuite) TestBlockUntilCompletedErrorsOnInvalidXML(c *C) {
     response := DispatcherResponse{
         response: &x509Response{
             Body:       []byte(">invalidXML<"),
@@ -90,7 +162,7 @@
     c.Check(err, FitsTypeOf, new(xml.SyntaxError))
 }
 
-func (suite *managementAPIUtilitiesSuite) TestBlockUntilCompletedErrorsIfFailsToPoll(c *C) {
+func (suite *managementAPIUtilitiesSuite) TestBlockUntilCompletedErrorsIfPollingFails(c *C) {
     response := DispatcherResponse{
         response:    &x509Response{},
         errorObject: fmt.Errorf("unexpected error")}
@@ -104,7 +176,7 @@
     c.Check(err, ErrorMatches, ".*unexpected error.*")
 }
 
-func (suite *managementAPIUtilitiesSuite) TestBlockUntilCompletedSucceedsIfOperationSucceeds(c *C) {
+func (suite *managementAPIUtilitiesSuite) TestBlockUntilCompletedSucceedsIfAsyncOperationSucceeds(c *C) {
     response := DispatcherResponse{
         response: &x509Response{
             Body:       []byte(fmt.Sprintf(operationXMLTemplate, "Succeeded")),

=== modified file 'poller.go'
--- poller.go	2013-03-29 08:45:30 +0000
+++ poller.go	2013-04-02 07:42:21 +0000
@@ -4,7 +4,6 @@
 package gwacl
 
 import (
-    "encoding/xml"
     "time"
 )
 
@@ -53,7 +52,7 @@
         return nil, err
     }
     operation := Operation{}
-    err = xml.Unmarshal(response.Body, &operation)
+    err = operation.Deserialize(response.Body)
     return &operation, err
 }
 
@@ -90,7 +89,7 @@
     // server cannot be reached.
     if response.StatusCode >= 200 && response.StatusCode < 300 {
         operation := Operation{}
-        err := xml.Unmarshal(response.Body, &operation)
+        err := operation.Deserialize(response.Body)
         if err != nil {
             return false, err
         }

=== modified file 'xmlobjects.go'
--- xmlobjects.go	2013-04-01 07:27:12 +0000
+++ xmlobjects.go	2013-04-02 07:42:21 +0000
@@ -528,6 +528,7 @@
 const (
     InProgressOperationStatus = "InProgress"
     SucceededOperationStatus  = "Succeeded"
+    FailedOperationStatus     = "Failed"
 )
 
 type Operation struct {
@@ -537,3 +538,7 @@
     ErrorCode      string `xml:"Error>Code"`
     ErrorMessage   string `xml:"Error>Message"`
 }
+
+func (o *Operation) Deserialize(data []byte) error {
+    return xml.Unmarshal(data, o)
+}