Merge lp:~jtv/gwacl/only-batch-list-blobs into lp:gwacl

[Jeroen T. Vermeulen]

Support batched responses from List Blobs.

This duplicates the logic from ListContainers(). It's basically my previous branch, but without the refactoring. The next step is to factor out the awful commonality.

Repeated jobs that I'd like to factor out include:
 * Interpolation of the URLs for storage accounts, containers, and files.
 * Adding of query parameters to a storage URL.
 * Stripping markers and adding them to URL queries.
 * Generation, signing, and send() of HTTP requests.

A bit harder to do without complicating things is the commonality between the batching loops in ListContainers() and ListBlobs().

I'd also like to remove the useless (and in this case, sort of misleading) Marker field from BlobsEnumerationResult.

Jeroen

[Julian Edwards (julian-edwards)]

s/for/For/ on line 102 of the diff, otherwise nothing controversial here, thanks.

review: Approve

[lp:~jtv/gwacl/only-batch-list-blobs]

105. By Jeroen T. Vermeulen on 2013-04-30

Review change. Failed to upper-case a letter in a comment. Cheap keyboard.

=== modified file 'storage_base.go'
--- storage_base.go	2013-04-26 03:53:37 +0000
+++ storage_base.go	2013-04-30 05:28:25 +0000
@@ -265,48 +265,53 @@
     return resp, nil
 }
 
+// addURLQueryParam adds a query parameter to a URL (and escapes as needed).
+func addURLQueryParam(originalURL, key, value string) (string, error) {
+    parsedURL, err := url.Parse(originalURL)
+    if err != nil {
+        // Not much we can add beyond that the URL doesn't parse.  Leave it
+        // to the caller to provide context.
+        return "", err
+    }
+    query := parsedURL.Query()
+    query.Add(key, value)
+    parsedURL.RawQuery = query.Encode()
+    return parsedURL.String(), nil
+}
+
 // 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.
+// API, and returns a single batch of results.
 // 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) {
+// NextMarker from the previous call's result.
+func (context *StorageContext) getListContainersBatch(marker string) (*ContainerEnumerationResults, error) {
+    var err error
     uri := interpolateURL("http://___.blob.core.windows.net/?comp=list", context.Account)
     if marker != "" {
         // Add the marker argument.  Do it after interpolation, in case the
         // marker string might contain the interpolation's placeholder
         // sequence.
-        parsedURL, err := url.Parse(uri)
+        uri, err = addURLQueryParam(uri, "marker", marker)
         if err != nil {
-            return nil, "", err
+            return nil, fmt.Errorf("malformed storage account URL '%s': %v", uri, err)
         }
-        query := parsedURL.Query()
-        query.Set("marker", marker)
-        parsedURL.RawQuery = query.Encode()
-        uri = parsedURL.String()
     }
     req, err := http.NewRequest("GET", uri, nil)
     if err != nil {
-        return nil, "", err
+        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
+        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
+    return &containers, 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.
+// ListContainers requests from the storage service a list of containers
+// in the storage account.
 func (context *StorageContext) ListContainers() (*ContainerEnumerationResults, error) {
     containers := make([]Container, 0)
     var batch *ContainerEnumerationResults
@@ -318,10 +323,14 @@
         var err error
         // Don't use := here or you'll shadow variables from the function's
         // outer scopes.
-        batch, nextMarker, err = context.getListContainersBatch(marker)
+        batch, err = context.getListContainersBatch(marker)
         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(batch.NextMarker)
         containers = append(containers, batch.Containers...)
     }
 
@@ -334,11 +343,25 @@
     return batch, nil
 }
 
-// Send a request to the storage service to list the blobs in a container.
-func (context *StorageContext) ListBlobs(container string) (*BlobEnumerationResults, error) {
+// getListBlobsBatch calls the "List Blobs" operation on the storage API, and
+// returns a single batch of results.
+// The marker argument should be empty for a new List Blobs request.  For
+// subsequent calls to get additional batches of the same result, pass the
+// NextMarker from the previous call's result.
+func (context *StorageContext) getListBlobsBatch(container, marker string) (*BlobEnumerationResults, error) {
+    var err error
     uri := interpolateURL(
         "http://___.blob.core.windows.net/___?restype=container&comp=list",
         context.Account, container)
+    if marker != "" {
+        // Add the marker argument.  Do it after interpolation, in case the
+        // marker string might contain the interpolation's placeholder
+        // sequence.
+        uri, err = addURLQueryParam(uri, "marker", marker)
+        if err != nil {
+            return nil, fmt.Errorf("malformed storage account URL '%s': %v", uri, err)
+        }
+    }
     req, err := http.NewRequest("GET", uri, nil)
     if err != nil {
         return nil, err
@@ -349,6 +372,39 @@
     return blob, err
 }
 
+// ListBlobs requests from the API a list of blobs in a container.
+func (context *StorageContext) ListBlobs(container string) (*BlobEnumerationResults, error) {
+    blobs := make([]Blob, 0)
+    var batch *BlobEnumerationResults
+
+    // 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.
+    // This loop is very similar to the one in ListContainers().
+    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, err = context.getListBlobsBatch(container, marker)
+        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(batch.NextMarker)
+        blobs = append(blobs, batch.Blobs...)
+    }
+
+    // There's more in a BlobsEnumerationResults than just the blobs.
+    // Return the latest batch, but give it the full cumulative blobs list
+    // instead of just the last batch.
+    // To the caller, this will look like they made one call to Azure's
+    // List Blobs method, but batch size was unlimited.
+    batch.Blobs = blobs
+    return batch, nil
+}
+
 // Send a request to the storage service to create a new container.  If the
 // request fails, error is non-nil.
 func (context *StorageContext) CreateContainer(container string) error {

=== modified file 'storage_base_test.go'
--- storage_base_test.go	2013-04-26 03:58:24 +0000
+++ storage_base_test.go	2013-04-30 05:28:25 +0000
@@ -266,6 +266,48 @@
     }
 }
 
+type TestAddURLQueryParam struct{}
+
+var _ = Suite(&TestAddURLQueryParam{})
+
+func (*TestAddURLQueryParam) TestAddURLQueryParamUsesBaseURL(c *C) {
+    baseURL := "http://example.com"
+
+    extendedURL, err := addURLQueryParam(baseURL, "key", "value")
+    c.Assert(err, IsNil)
+
+    parsedURL, err := url.Parse(extendedURL)
+    c.Assert(err, IsNil)
+    c.Check(parsedURL.Scheme, Equals, "http")
+    c.Check(parsedURL.Host, Equals, "example.com")
+}
+
+func (suite *TestAddURLQueryParam) TestEscapesParam(c *C) {
+    key := "key&key"
+    value := "value%value"
+
+    uri, err := addURLQueryParam("http://example.com", key, value)
+    c.Assert(err, IsNil)
+
+    parsedURL, err := url.Parse(uri)
+    c.Assert(err, IsNil)
+    c.Check(parsedURL.Query()[key], DeepEquals, []string{value})
+}
+
+// The parameter may safely contain the placeholder that we happen to use
+// for URL interpolation.
+func (suite *TestAddURLQueryParam) TestDoesNotInterpolateParam(c *C) {
+    key := "key" + interpolationPlaceholder + "key"
+    value := "value" + interpolationPlaceholder + "value"
+
+    uri, err := addURLQueryParam("http://example.com", key, value)
+    c.Assert(err, IsNil)
+
+    parsedURL, err := url.Parse(uri)
+    c.Assert(err, IsNil)
+    c.Check(parsedURL.Query()[key], DeepEquals, []string{value})
+}
+
 type TestListContainers struct{}
 
 var _ = Suite(&TestListContainers{})
@@ -391,7 +433,7 @@
 
     // Call getListContainersBatch.  This will fail because of the empty
     // response, but no matter.  We only care about the request.
-    _, _, err := context.getListContainersBatch("thismarkerhere")
+    _, err := context.getListContainersBatch("thismarkerhere")
     c.Assert(err, ErrorMatches, ".*Failed to deserialize data.*")
     c.Assert(transport.ExchangeCount, Equals, 1)
 
@@ -408,7 +450,7 @@
     context.client = &http.Client{Transport: &transport}
 
     // The error is OK.  We only care about the request.
-    _, _, err := context.getListContainersBatch("")
+    _, err := context.getListContainersBatch("")
     c.Assert(err, ErrorMatches, ".*Failed to deserialize data.*")
     c.Assert(transport.ExchangeCount, Equals, 1)
 
@@ -427,7 +469,7 @@
     context.client = &http.Client{Transport: &transport}
 
     // The error is OK.  We only care about the request.
-    _, _, err := context.getListContainersBatch("x&y")
+    _, err := context.getListContainersBatch("x&y")
     c.Assert(err, ErrorMatches, ".*Failed to deserialize data.*")
     c.Assert(transport.ExchangeCount, Equals, 1)
 
@@ -448,7 +490,7 @@
 
     // An error about the http response is fine.  What matters is that (1) we
     // don't fail while putting together the URL, and (2) we get the right URL.
-    _, _, err := context.getListContainersBatch(marker)
+    _, err := context.getListContainersBatch(marker)
     c.Assert(err, ErrorMatches, ".*Failed to deserialize data.*")
     c.Assert(transport.ExchangeCount, Equals, 1)
 
@@ -549,6 +591,87 @@
     c.Assert(err, NotNil)
 }
 
+// ListBlobs combines multiple batches of output.
+func (suite *TestListBlobs) TestBatchedResult(c *C) {
+    firstBlob := "blob1"
+    lastBlob := "blob2"
+    marker := "moreplease"
+    firstBatch := http.Response{
+        StatusCode: http.StatusOK,
+        Body: makeResponseBody(fmt.Sprintf(`
+            <EnumerationResults>
+              <Blobs>
+                <Blob>
+                  <Name>%s</Name>
+                </Blob>
+              </Blobs>
+              <NextMarker>%s</NextMarker>
+            </EnumerationResults>
+        `, firstBlob, marker)),
+    }
+    lastBatch := http.Response{
+        StatusCode: http.StatusOK,
+        Body: makeResponseBody(fmt.Sprintf(`
+            <EnumerationResults>
+              <Blobs>
+                <Blob>
+                  <Name>%s</Name>
+                </Blob>
+              </Blobs>
+            </EnumerationResults>
+        `, lastBlob)),
+    }
+    transport := TestTransport2{}
+    transport.AddExchange(&firstBatch, nil)
+    transport.AddExchange(&lastBatch, nil)
+    context := makeStorageContext()
+    context.client = &http.Client{Transport: &transport}
+
+    blobs, err := context.ListBlobs("mycontainer")
+    c.Assert(err, IsNil)
+
+    c.Check(len(blobs.Blobs), Equals, 2)
+    c.Check(blobs.Blobs[0].Name, Equals, firstBlob)
+    c.Check(blobs.Blobs[1].Name, Equals, lastBlob)
+}
+
+func (suite *TestListBlobs) TestGetListBlobsBatchPassesMarker(c *C) {
+    transport := TestTransport2{}
+    transport.AddExchange(&http.Response{StatusCode: http.StatusOK, Body: Empty}, nil)
+    context := makeStorageContext()
+    context.client = &http.Client{Transport: &transport}
+
+    // Call getListBlobsBatch.  This will fail because of the empty
+    // response, but no matter.  We only care about the request.
+    _, err := context.getListBlobsBatch("mycontainer", "thismarkerhere")
+    c.Assert(err, ErrorMatches, ".*Failed to deserialize data.*")
+    c.Assert(transport.ExchangeCount, Equals, 1)
+
+    query := transport.Exchanges[0].Request.URL.RawQuery
+    values, err := url.ParseQuery(query)
+    c.Assert(err, IsNil)
+    c.Check(values["marker"], DeepEquals, []string{"thismarkerhere"})
+}
+
+func (suite *TestListBlobs) TestGetListBlobsBatchDoesNotPassEmptyMarker(c *C) {
+    transport := TestTransport2{}
+    transport.AddExchange(&http.Response{StatusCode: http.StatusOK, Body: Empty}, nil)
+    context := makeStorageContext()
+    context.client = &http.Client{Transport: &transport}
+
+    // The error is OK.  We only care about the request.
+    _, err := context.getListBlobsBatch("mycontainer", "")
+    c.Assert(err, ErrorMatches, ".*Failed to deserialize data.*")
+    c.Assert(transport.ExchangeCount, Equals, 1)
+
+    query := transport.Exchanges[0].Request.URL.RawQuery
+    values, err := url.ParseQuery(query)
+    c.Assert(err, IsNil)
+    marker, present := values["marker"]
+    c.Check(present, Equals, false)
+    c.Check(marker, DeepEquals, []string(nil))
+}
+
 type TestCreateContainer struct{}
 
 var _ = Suite(&TestCreateContainer{})

=== modified file 'xmlobjects.go'
--- xmlobjects.go	2013-04-02 08:51:18 +0000
+++ xmlobjects.go	2013-04-30 05:28:25 +0000
@@ -447,6 +447,7 @@
     Delimiter     string   `xml:"Delimiter"`
     Blobs         []Blob   `xml:"Blobs>Blob"`
     BlobPrefixes  []string `xml:"Blobs>BlobPrefix>Name"`
+    NextMarker    string   `xml:"NextMarker"`
 }
 
 func (b *BlobEnumerationResults) Deserialize(data []byte) error {

=== modified file 'xmlobjects_test.go'
--- xmlobjects_test.go	2013-04-01 07:27:12 +0000
+++ xmlobjects_test.go	2013-04-30 05:28:25 +0000
@@ -383,6 +383,7 @@
     c.Check(r.Prefix, Equals, "prefix")
     c.Check(r.Marker, Equals, "marker")
     c.Check(r.Delimiter, Equals, "delimiter")
+    c.Check(r.NextMarker, Equals, "")
     b := r.Blobs[0]
     c.Check(b.Name, Equals, "blob-name")
     c.Check(b.Snapshot, Equals, "snapshot-date-time")