Merge lp:~jtv/gwacl/batch-list-containers into lp:gwacl

[Jeroen T. Vermeulen]

Support batched results in ListContainers().

This is actually required for ListContainers() to function properly. I kept the existing tests, but removed a NextMarker in an existing test. That test value was inappropriate (it was an unbatched result, so should not have a NextMarker) and broke the test once I had batch support working. Plus of course I added a test for a batching call before actually implementing support.

The code I ended up with isn't as simple as I'd like. If this were python, I'd test it much more thoroughly.

You'll also note some small drive-by improvements and TODO comments. In the Makefile you'll see that gofmt with the -s (simplify) option also does the regular work of a gofmt without the -s.

In a next branch I'll be doing the exact same batching for the List Blobs call. Hopefully I'll be able to extract some shared code. These were the only two instances of batching that I found in the storage API insofar as we support it.

Jeroen

[Raphaël Badin (rvb)]

Looks good. I've got a few remarks but I'm not really familiar with that code so take them with a grain of salt.

[0]

[in ListContainers]
54 + firstBatch, marker, err := context.getListContainersBatch("")
55 if err != nil {
[...]
62 + return firstBatch, err
63 + }

Is it worth returning the firstBatch if we get an error here? Unless there is a good reason why we would want to do this (and then it's worth adding to the method's documentation), it would seem more logical to me to return "nil, err" in this case…

[1]

65 + batch, nextMarker, err := context.getListContainersBatch(marker)
66 + if err != nil {
67 + return firstBatch, err
68 + }

(let's forget [0] for a moment here) Why returning firstBatch here? To be more coherent with the first error return clause (the one which I talk about in [0]), I'd return the accumulated batches returned so far, *including the one we just got*. But maybe (see [0]) it's even more simple to simply return "nil, err".

[2]

77 + // Since we must have arrived at the last batch, where NextMarker is empty,
78 + // it would be misleading to report the original NextMarker.
79 + firstBatch.NextMarker = ""

I don't really understand the necessity for this (but I'm not really familiar with the context). From the looks of it, you're just introducing a small difference here, compared to what one will expect to get by reading the API documentation.

[3]

I understand the necessity for the nextMarker/marker trick given the way you've done things here… but how about a simpler implementation… something like this:
The method: http://paste.ubuntu.com/5600649/
The diff: http://paste.ubuntu.com/5600647/

review: Approve

[lp:~jtv/gwacl/batch-list-containers]

101. By Jeroen T. Vermeulen on 2013-04-25

Review suggestion: don't try to relay unreliable results in the error case.

[Jeroen T. Vermeulen (jtv)]

> Looks good. I've got a few remarks but I'm not really familiar with that code
> so take them with a grain of salt.

Thanks for the review. Yes, the code is not side we worked on in Brisbane... What I can tell you is it gets a bit ugly sometimes. :)

> [0]
>
> [in ListContainers]
> 54 + firstBatch, marker, err := context.getListContainersBatch("")
> 55 if err != nil {
> [...]
> 62 + return firstBatch, err
> 63 + }
>
> Is it worth returning the firstBatch if we get an error here? Unless there is
> a good reason why we would want to do this (and then it's worth adding to the
> method's documentation), it would seem more logical to me to return "nil, err"
> in this case…

You're right. I changed it.

I was reasoning that if I pass on whatever we get, I don't have to worry about any possibility of getting a useful result despite an error: if that ever happened we could just document whatever Azure does and our existing code would probably support it correctly. But that doesn't work very well in the presence of batched results: what would we do if we got an error code but a usable result *with* a NextMarker? Defensive coding could cope with any sensible scenario, but absent a good indication that there's any point, it's not the right thing to do.

> [1]
>
> 65 + batch, nextMarker, err :=
> context.getListContainersBatch(marker)
> 66 + if err != nil {
> 67 + return firstBatch, err
> 68 + }
>
> (let's forget [0] for a moment here) Why returning firstBatch here? To be
> more coherent with the first error return clause (the one which I talk about
> in [0]), I'd return the accumulated batches returned so far, *including the
> one we just got*. But maybe (see [0]) it's even more simple to simply return
> "nil, err".

I returned nil here as well.

> [2]
>
> 77 + // Since we must have arrived at the last batch, where NextMarker
> is empty,
> 78 + // it would be misleading to report the original NextMarker.
> 79 + firstBatch.NextMarker = ""
>
> I don't really understand the necessity for this (but I'm not really familiar
> with the context). From the looks of it, you're just introducing a small
> difference here, compared to what one will expect to get by reading the API
> documentation.

It's not so much a matter of necessity, as of avoiding a misrepresentation. If NextMarker is nonempty, that means the result is incomplete. Thus, for the complete result, it would be improper to keep NextMarker set to a nonempty value. The way I did it, a user who expects ListContainers to correspond exactly to one call to the List Containers API method will not be misled. It's just that the default batch size will be effectively infinite. (Except of course Go will break if you try to make an array of 2³¹ entries, but they've been hinting that they'll fix that at some point.)

There are different ways to argue this. The root cause is that ListContainers no longer corresponds exactly to an API call: it operates at a slightly higher level. And so ContainerEnumerationResults now serves two purposes that are no longer fully aligned:

1....

[lp:~jtv/gwacl/batch-list-containers]

102. By Jeroen T. Vermeulen on 2013-04-25

Streamline the ListContainers loop.

[Julian Edwards (julian-edwards)]

Did you check to see if the example still runs after this change?

[Jeroen T. Vermeulen (jtv)]

No, I didn't check... Couldn't, what with the consequences of that disk-deletion bug leaving my account "full." And indeed I left out a crucial bit, as corrected in my other branch.

=== modified file 'Makefile'
--- Makefile	2013-04-24 08:36:17 +0000
+++ Makefile	2013-04-25 14:03:27 +0000
@@ -11,9 +11,7 @@
 
 # Reformat the source files to match our layout standards.
 # This includes gofmt's "simplify" option to streamline the source code.
-# TODO: The -s run may actually also do the regular formatting.
 format:
-	./utilities/format
 	./utilities/format -s
 
 # Build the examples (we have no tests for them).

=== modified file 'storage_base.go'
--- storage_base.go	2013-04-22 16:00:31 +0000
+++ storage_base.go	2013-04-25 14:03:27 +0000
@@ -265,19 +265,61 @@
     return resp, nil
 }
 
-// Send a request to the storage service to list the containers in the
-// storage account.  error is non-nil if an error occurred.
+// getListContainersBatch calls the "List Containers" operation on the storage
+// API, and returns a single batch of results; its "next marker" for batching,
+// and an error code.
+// The marker argument should be empty for a new List Containers request.  for
+// subsequent calls to get additional batches of the same result, pass the
+// "next marker" returned by the previous call.
+// The "next marker" will be empty on the last batch.
+func (context *StorageContext) getListContainersBatch(marker string) (*ContainerEnumerationResults, string, error) {
+    uri := interpolateURL(
+        "http://___.blob.core.windows.net/?comp=list", context.Account)
+    req, err := http.NewRequest("GET", uri, nil)
+    if err != nil {
+        return nil, "", err
+    }
+    addStandardHeaders(req, context.Account, context.Key, "2012-02-12")
+    containers := ContainerEnumerationResults{}
+    _, err = context.send(req, &containers, http.StatusOK)
+    if err != nil {
+        return nil, "", err
+    }
+
+    // The response may contain a NextMarker field, to let us request a
+    // subsequent batch of results.  The XML parser won't trim whitespace out
+    // of the marker tag, so we do that here.
+    nextMarker := strings.TrimSpace(containers.NextMarker)
+    return &containers, nextMarker, nil
+}
+
+// ListContainers sends a request to the storage service to list the containers
+// in the storage account.  error is non-nil if an error occurred.
 func (context *StorageContext) ListContainers() (*ContainerEnumerationResults, error) {
-    uri := interpolateURL(
-        "http://___.blob.core.windows.net/?comp=list", context.Account)
-    req, err := http.NewRequest("GET", uri, nil)
-    if err != nil {
-        return nil, err
+    containers := make([]Container, 0)
+    var batch *ContainerEnumerationResults
+
+    // Request the initial result, using the empty marker.  Then, for as long
+    // as the result has a nonempty NextMarker, request the next batch using
+    // that marker.
+    for marker, nextMarker := "", "x"; nextMarker != ""; marker = nextMarker {
+        var err error
+        // Don't use := here or you'll shadow variables from the function's
+        // outer scopes.
+        batch, nextMarker, err = context.getListContainersBatch(marker)
+        if err != nil {
+            return nil, err
+        }
+        containers = append(containers, batch.Containers...)
     }
-    addStandardHeaders(req, context.Account, context.Key, "2012-02-12")
-    containers := &ContainerEnumerationResults{}
-    _, e := context.send(req, containers, http.StatusOK)
-    return containers, e
+
+    // There's more in a ContainerEnumerationResults than just the containers.
+    // Return the latest batch, but give it the full cumulative containers list
+    // instead of just the last batch.
+    // To the caller, this will look like they made one call to Azure's
+    // List Containers method, but batch size was unlimited.
+    batch.Containers = containers
+    return batch, nil
 }
 
 // Send a request to the storage service to list the blobs in a container.

=== modified file 'storage_base_test.go'
--- storage_base_test.go	2013-04-24 10:08:54 +0000
+++ storage_base_test.go	2013-04-25 14:03:27 +0000
@@ -295,13 +295,13 @@
               </Metadata>
             </Container>
           </Containers>
-          <NextMarker>next-marker-value</NextMarker>
+          <NextMarker/>
         </EnumerationResults>`
     context := makeStorageContext()
     response := &http.Response{
         Status:     fmt.Sprintf("%d", http.StatusOK),
         StatusCode: http.StatusOK,
-        Body:       ioutil.NopCloser(strings.NewReader(response_body)),
+        Body:       makeResponseBody(response_body),
     }
     transport := &TestTransport{Response: response}
     context.client = &http.Client{Transport: transport}
@@ -337,6 +337,52 @@
     c.Assert(err, NotNil)
 }
 
+// ListContainers combines multiple batches of output.
+func (suite *TestListContainers) TestBatchedResult(c *C) {
+    firstContainer := "container1"
+    lastContainer := "container2"
+    marker := "moreplease"
+    firstBatch := http.Response{
+        StatusCode: http.StatusOK,
+        Body: makeResponseBody(fmt.Sprintf(`
+            <EnumerationResults>
+              <Containers>
+                <Container>
+                  <Name>%s</Name>
+                  <URL>container-address</URL>
+                </Container>
+              </Containers>
+              <NextMarker>%s</NextMarker>
+            </EnumerationResults>
+        `, firstContainer, marker)),
+    }
+    lastBatch := http.Response{
+        StatusCode: http.StatusOK,
+        Body: makeResponseBody(fmt.Sprintf(`
+            <EnumerationResults>
+              <Containers>
+                <Container>
+                  <Name>%s</Name>
+                  <URL>container-address</URL>
+                </Container>
+              </Containers>
+            </EnumerationResults>
+        `, lastContainer)),
+    }
+    transport := TestTransport2{}
+    transport.AddExchange(&firstBatch, nil)
+    transport.AddExchange(&lastBatch, nil)
+    context := makeStorageContext()
+    context.client = &http.Client{Transport: &transport}
+
+    containers, err := context.ListContainers()
+    c.Assert(err, IsNil)
+
+    c.Check(len(containers.Containers), Equals, 2)
+    c.Check(containers.Containers[0].Name, Equals, firstContainer)
+    c.Check(containers.Containers[1].Name, Equals, lastContainer)
+}
+
 type TestListBlobs struct{}
 
 var _ = Suite(&TestListBlobs{})
@@ -392,7 +438,7 @@
     response := &http.Response{
         Status:     fmt.Sprintf("%d", http.StatusOK),
         StatusCode: http.StatusOK,
-        Body:       ioutil.NopCloser(strings.NewReader(response_body)),
+        Body:       makeResponseBody(response_body),
     }
     transport := &TestTransport{Response: response}
     context.client = &http.Client{Transport: transport}
@@ -523,7 +569,7 @@
     response := &http.Response{
         Status:     "102 Frotzed",
         StatusCode: 102,
-        Body:       ioutil.NopCloser(strings.NewReader("<Error><Code>Frotzed</Code><Message>failed to put blob</Message></Error>")),
+        Body:       makeResponseBody("<Error><Code>Frotzed</Code><Message>failed to put blob</Message></Error>"),
     }
     transport := &TestTransport{Response: response}
     context := makeStorageContext()
@@ -582,7 +628,7 @@
     response := &http.Response{
         Status:     "102 Frotzed",
         StatusCode: 102,
-        Body:       ioutil.NopCloser(strings.NewReader("<Error><Code>Frotzed</Code><Message>failed to put block</Message></Error>")),
+        Body:       makeResponseBody("<Error><Code>Frotzed</Code><Message>failed to put block</Message></Error>"),
     }
     transport := &TestTransport{Response: response}
     context := makeStorageContext()
@@ -645,7 +691,7 @@
     response := &http.Response{
         Status:     "102 Frotzed",
         StatusCode: 102,
-        Body:       ioutil.NopCloser(strings.NewReader("<Error><Code>Frotzed</Code><Message>failed to put blocklist</Message></Error>")),
+        Body:       makeResponseBody("<Error><Code>Frotzed</Code><Message>failed to put blocklist</Message></Error>"),
     }
     transport := &TestTransport{Response: response}
     context := makeStorageContext()
@@ -686,7 +732,7 @@
     response := &http.Response{
         Status:     fmt.Sprintf("%d", http.StatusOK),
         StatusCode: http.StatusOK,
-        Body:       ioutil.NopCloser(strings.NewReader(response_body)),
+        Body:       makeResponseBody(response_body),
     }
     transport := &TestTransport{Response: response}
     context.client = &http.Client{Transport: transport}
@@ -760,7 +806,7 @@
     response := &http.Response{
         Status:     "146 Frotzed",
         StatusCode: 146,
-        Body:       ioutil.NopCloser(strings.NewReader("<Error><Code>Frotzed</Code><Message>failed to delete blob</Message></Error>")),
+        Body:       makeResponseBody("<Error><Code>Frotzed</Code><Message>failed to delete blob</Message></Error>"),
     }
     transport := &TestTransport{Response: response}
     context := makeStorageContext()
@@ -782,7 +828,7 @@
     response := &http.Response{
         Status:     fmt.Sprintf("%d", http.StatusOK),
         StatusCode: http.StatusOK,
-        Body:       ioutil.NopCloser(strings.NewReader(response_body)),
+        Body:       makeResponseBody(response_body),
     }
     transport := &TestTransport{Response: response}
     context.client = &http.Client{Transport: transport}
@@ -818,7 +864,7 @@
     response := &http.Response{
         Status:     "246 Frotzed",
         StatusCode: 246,
-        Body:       ioutil.NopCloser(strings.NewReader("<Error><Code>Frotzed</Code><Message>failed to get blob</Message></Error>")),
+        Body:       makeResponseBody("<Error><Code>Frotzed</Code><Message>failed to get blob</Message></Error>"),
     }
     transport := &TestTransport{Response: response}
     context := makeStorageContext()

=== modified file 'testhelpers_http.go'
--- testhelpers_http.go	2013-04-24 10:08:54 +0000
+++ testhelpers_http.go	2013-04-25 14:03:27 +0000
@@ -7,35 +7,49 @@
 
 import (
     "fmt"
+    "io"
+    "io/ioutil"
     "net/http"
+    "strings"
 )
 
-// TestTransport is used as an http.Client.Transport for testing. The only
-// requirement is that it adhere to the http.RoundTripper interface.
+// TestTransport is used as an http.Client.Transport for testing.  It records
+// the latest request, and returns a predetermined Response and error.
+// TODO: Why does this need to be exported?
 type TestTransport struct {
     Request  *http.Request
     Response *http.Response
     Error    error
 }
 
+// TestTransport implements the http.RoundTripper interface.
+var _ http.RoundTripper = &TestTransport{}
+
 func (t *TestTransport) RoundTrip(req *http.Request) (resp *http.Response, err error) {
     t.Request = req
     return t.Response, t.Error
 }
 
+// TODO: Why does this need to be exported?
 type TestTransport2Exchange struct {
     Request  *http.Request
     Response *http.Response
     Error    error
 }
 
-// TestTransport2 is used as an http.Client.Transport for testing. The only
-// requirement is that it adhere to the http.RoundTripper interface.
+// TestTransport2 is used as an http.Client.Transport for testing.  It records
+// the sequence of requests, and returns a predetermined sequence of Responses
+// and errors.
+// TODO: Rename this to something remotely sensible.
+// TODO: Why does this need to be exported?
 type TestTransport2 struct {
     Exchanges     []*TestTransport2Exchange
     ExchangeCount int
 }
 
+// TestTransport2 implements the http.RoundTripper interface.
+var _ http.RoundTripper = &TestTransport2{}
+
 func (t *TestTransport2) AddExchange(response *http.Response, error error) {
     exchange := TestTransport2Exchange{Response: response, Error: error}
     t.Exchanges = append(t.Exchanges, &exchange)
@@ -56,3 +70,10 @@
         Body:       Empty,
     }
 }
+
+// makeResponseBody creates an http response body containing the given string.
+// Use this to initialize an http.Response.Body with a given string, without
+// having to care about the type details.
+func makeResponseBody(content string) io.ReadCloser {
+    return ioutil.NopCloser(strings.NewReader(content))
+}

=== modified file 'testhelpers_misc.go'
--- testhelpers_misc.go	2013-04-24 10:08:54 +0000
+++ testhelpers_misc.go	2013-04-25 14:03:27 +0000
@@ -4,11 +4,9 @@
 package gwacl
 
 import (
-    "bytes"
     "encoding/base64"
     "fmt"
     "io"
-    "io/ioutil"
 )
 
 // b64 is shorthand for base64-encoding a string.
@@ -17,7 +15,7 @@
 }
 
 // A Reader and ReadCloser that EOFs immediately.
-var Empty io.ReadCloser = ioutil.NopCloser(bytes.NewReader(nil))
+var Empty io.ReadCloser = makeResponseBody("")
 
 // BoolToString represents a boolean value as a string ("true" or "false").
 func BoolToString(v bool) string {