Merge lp:~allenap/gwacl/list-files-by-prefix into lp:gwacl

[Gavin Panella]

List blobs matching a prefix.

This branch's mission - and it's chosen to accept it - is to add
prefix support to the List Blobs API call via GWACL, where `prefix` is
defined as:

  Optional. Filters the results to return only blobs whose names begin
  with the specified prefix.

As far as I am aware, it's not possible to have "proper" optional
arguments with Go, so I've created the ListBlobsRequest struct to
provide this behaviour. It'll also make it easy to add support for
maxresults, include, timeout, etc. down the road.

However, first I had some refactoring to do: the List Blobs API call
was implemented in private getListBlobsBatch function, and ListBlobs
was a wrapper that was repeatedly calling getListBlobsBatch to get all
blobs. I think the layering is good, but masking the foundational API
call is limiting, and premature.

To address this I've:

- moved ListBlobs from storage_base.go to storage.go and renamed it
  ListAllBlobs.

- getListBlobsBatch is now ListBlobs.

- Both functions take the new ListBlobsRequest struct as their only
  argument.

A similar problem exists in ListContainers and getListContainersBatch,
but that can be addressed in a separate branch if the approach taken
here is acceptable.

I've also updated the storage example so that a prefix can be passed
in, and verified that it works.

[Julian Edwards (julian-edwards)]

(You gotta wonder why Go doesn't have default args :/)

The first thing that popped into my head was, do you think that we should put mandatory parameters as separate args to the ListAllBlobs function, and reserve ListBlobsRequest for optional params only? It seems as though you don't check for the mandatory params anyway, but it would obviate that param checking and is more explicit.

Second thing: the tests nearly all have the same set-up code for the fake transport. I think we need a factored test helper for this as all the setup detracts from the test code's intention. I don't think it should be done in this branch though, a follow-up one is better.

Also - I am wondering if we need some factory methods to create all these huge blobs of XML we're passing around? It's not entirely clear what we need though.

Everything else looks pretty good to me, thanks for all the extra test cases!

review: Approve

[Gavin Panella (allenap)]

> (You gotta wonder why Go doesn't have default args :/)
>
> The first thing that popped into my head was, do you think that we should put
> mandatory parameters as separate args to the ListAllBlobs function, and
> reserve ListBlobsRequest for optional params only?  It seems as though you
> don't check for the mandatory params anyway, but it would obviate that param
> checking and is more explicit.

Now that we have a struct to represent the request, I think it should
be the place that it's represented. We could add a Validate() method
to the struct to check always-required parameters or something, but
I'm more inclined to just throw at Azure whatever is passed in, and
let Azure do the checking. Let's make the fewest assumptions about
Azure possible. It's less work too.

>
> Second thing: the tests nearly all have the same set-up code for the fake
> transport.  I think we need a factored test helper for this as all the setup
> detracts from the test code's intention.  I don't think it should be done in
> this branch though, a follow-up one is better.

Yeah, it's ugly. We can extract some of that stuff, but it may come
with a loss of clarity. I've seen enough of this code for now. Given
time I may come back to it and see a good way to clean this up; the
same goes for any of us.

>
> Also - I am wondering if we need some factory methods to create all these huge
> blobs of XML we're passing around? It's not entirely clear what we need
> though.

Yeah :-/ Same goes as for the set-up.

>
> Everything else looks pretty good to me, thanks for all the extra test cases!

Thanks for the review!

[Julian Edwards (julian-edwards)]

On Tuesday 11 Jun 2013 13:30:38 you wrote:
> > (You gotta wonder why Go doesn't have default args :/)
> >
> > The first thing that popped into my head was, do you think that we should
> > put mandatory parameters as separate args to the ListAllBlobs function,
> > and reserve ListBlobsRequest for optional params only? It seems as
> > though you don't check for the mandatory params anyway, but it would
> > obviate that param checking and is more explicit.
>
> Now that we have a struct to represent the request, I think it should
> be the place that it's represented. We could add a Validate() method
> to the struct to check always-required parameters or something, but
> I'm more inclined to just throw at Azure whatever is passed in, and
> let Azure do the checking. Let's make the fewest assumptions about
> Azure possible. It's less work too.

My only worry with that is that it break the "fail as early as possible"
paradigm. The later you fail, the harder it is to work out the source of the
problem. Especially so with Go.

Please seriously consider adding some validation. It's tied to the API
version used anyway so it's not going to suddenly break at the other end.

>
> > Second thing: the tests nearly all have the same set-up code for the fake
> > transport. I think we need a factored test helper for this as all the
> > setup detracts from the test code's intention. I don't think it should
> > be done in this branch though, a follow-up one is better.
>
> Yeah, it's ugly. We can extract some of that stuff, but it may come
> with a loss of clarity.

I don't think clarity can get much worse from what it is now :)

> I've seen enough of this code for now. Given
> time I may come back to it and see a good way to clean this up; the
> same goes for any of us.

Ok, please file a tech debt bug.

>
> > Also - I am wondering if we need some factory methods to create all these
> > huge blobs of XML we're passing around? It's not entirely clear what we
> > need though.
>
> Yeah :-/ Same goes as for the set-up.
>
> > Everything else looks pretty good to me, thanks for all the extra test
> > cases!
> Thanks for the review!

I'd like to say it was a pleasure looking at all this Go. I'd really like to.

:^)

=== modified file 'example/storage/run.go'
--- example/storage/run.go	2013-04-18 15:03:42 +0000
+++ example/storage/run.go	2013-06-10 15:57:40 +0000
@@ -21,6 +21,7 @@
 var key string
 var filename string
 var container string
+var prefix string
 
 func checkContainerAndFilename() error {
     if container == "" || filename == "" {
@@ -38,6 +39,7 @@
     flag.StringVar(&key, "key", "", "A valid storage account key (base64 encoded)")
     flag.StringVar(&container, "container", "", "Name of the container to use")
     flag.StringVar(&filename, "file", "", "File containing blob to upload/download")
+    flag.StringVar(&prefix, "prefix", "", "Prefix to match when listing blobs")
     flag.Parse()
 
     if account == "" || key == "" {
@@ -102,7 +104,9 @@
 }
 
 func list(storage gwacl.StorageContext) {
-    res, err := storage.ListBlobs(container)
+    request := &gwacl.ListBlobsRequest{
+        Container: container, Prefix: prefix}
+    res, err := storage.ListAllBlobs(request)
     if err != nil {
         dumpError(err)
         return
 
=== modified file 'storage.go'
--- storage.go	2013-04-22 16:17:23 +0000
+++ storage.go	2013-06-10 15:57:40 +0000
@@ -11,6 +11,7 @@
     "io"
     . "launchpad.net/gwacl/logging"
     "strconv"
+    "strings"
 )
 
 // UploadBlockBlob uses PutBlock and PutBlockList API operations to upload
@@ -45,3 +46,37 @@
     Debugf("Committing %d blocks.\n", len(blockList.Items))
     return context.PutBlockList(container, filename, blockList)
 }
+
+// ListAllBlobs requests from the API a list of blobs in a container.
+func (context *StorageContext) ListAllBlobs(request *ListBlobsRequest) (*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.
+        request.Marker = marker
+        batch, err = context.ListBlobs(request)
+        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
+}
 
=== modified file 'storage_base.go'
--- storage_base.go	2013-05-01 05:16:08 +0000
+++ storage_base.go	2013-06-10 15:57:40 +0000
@@ -383,18 +383,27 @@
     return batch, nil
 }
 
-// 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
+type ListBlobsRequest struct {
+    Container string
+    Marker    string
+    Prefix    string
+}
+
+// ListBlobs calls the "List Blobs" operation on the storage API, and returns
+// a single batch of results.
+// The request.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) {
+func (context *StorageContext) ListBlobs(request *ListBlobsRequest) (*BlobEnumerationResults, error) {
     uri := addURLQueryParams(
-        context.getContainerURL(container),
+        context.getContainerURL(request.Container),
         "restype", "container",
         "comp", "list")
-    if marker != "" {
-        uri = addURLQueryParams(uri, "marker", marker)
+    if request.Marker != "" {
+        uri = addURLQueryParams(uri, "marker", request.Marker)
+    }
+    if request.Prefix != "" {
+        uri = addURLQueryParams(uri, "prefix", request.Prefix)
     }
     blobs := BlobEnumerationResults{}
     _, err := context.performRequest(requestParams{
@@ -410,39 +419,6 @@
     return &blobs, 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-30 14:48:05 +0000
+++ storage_base_test.go	2013-06-10 15:57:40 +0000
@@ -533,7 +533,8 @@
     }
     transport := &TestTransport{Response: response}
     context.client = &http.Client{Transport: transport}
-    results, err := context.ListBlobs("container")
+    request := &ListBlobsRequest{Container: "container"}
+    results, err := context.ListBlobs(request)
     c.Assert(err, IsNil)
     c.Check(transport.Request.URL.String(), Matches, context.getContainerURL("container")+"?.*")
     c.Check(transport.Request.URL.Query(), DeepEquals, url.Values{
@@ -550,7 +551,8 @@
     transport := &TestTransport{Error: error}
     context := makeStorageContext()
     context.client = &http.Client{Transport: transport}
-    _, err := context.ListBlobs("container")
+    request := &ListBlobsRequest{Container: "container"}
+    _, err := context.ListBlobs(request)
     c.Assert(err, NotNil)
 }
 
@@ -563,63 +565,21 @@
     transport := &TestTransport{Response: response}
     context := makeStorageContext()
     context.client = &http.Client{Transport: transport}
-    _, err := context.ListBlobs("container")
+    request := &ListBlobsRequest{Container: "container"}
+    _, err := context.ListBlobs(request)
     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) {
+func (suite *TestListBlobs) TestListBlobsPassesMarker(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
+    // Call ListBlobs.  This will fail because of the empty
     // response, but no matter.  We only care about the request.
-    _, err := context.getListBlobsBatch("mycontainer", "thismarkerhere")
+    request := &ListBlobsRequest{Container: "mycontainer", Marker: "thismarkerhere"}
+    _, err := context.ListBlobs(request)
     c.Assert(err, ErrorMatches, ".*Failed to deserialize data.*")
     c.Assert(transport.ExchangeCount, Equals, 1)
 
@@ -629,14 +589,15 @@
     c.Check(values["marker"], DeepEquals, []string{"thismarkerhere"})
 }
 
-func (suite *TestListBlobs) TestGetListBlobsBatchDoesNotPassEmptyMarker(c *C) {
+func (suite *TestListBlobs) TestListBlobsDoesNotPassEmptyMarker(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", "")
+    request := &ListBlobsRequest{Container: "mycontainer"}
+    _, err := context.ListBlobs(request)
     c.Assert(err, ErrorMatches, ".*Failed to deserialize data.*")
     c.Assert(transport.ExchangeCount, Equals, 1)
 
@@ -648,6 +609,45 @@
     c.Check(marker, DeepEquals, []string(nil))
 }
 
+func (suite *TestListBlobs) TestListBlobsPassesPrefix(c *C) {
+    transport := TestTransport2{}
+    transport.AddExchange(&http.Response{StatusCode: http.StatusOK, Body: Empty}, nil)
+    context := makeStorageContext()
+    context.client = &http.Client{Transport: &transport}
+
+    // Call ListBlobs.  This will fail because of the empty
+    // response, but no matter.  We only care about the request.
+    request := &ListBlobsRequest{Container: "mycontainer", Prefix: "thisprefixhere"}
+    _, err := context.ListBlobs(request)
+    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["prefix"], DeepEquals, []string{"thisprefixhere"})
+}
+
+func (suite *TestListBlobs) TestListBlobsDoesNotPassEmptyPrefix(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.
+    request := &ListBlobsRequest{Container: "mycontainer"}
+    _, err := context.ListBlobs(request)
+    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)
+    prefix, present := values["prefix"]
+    c.Check(present, Equals, false)
+    c.Check(prefix, DeepEquals, []string(nil))
+}
+
 type TestCreateContainer struct{}
 
 var _ = Suite(&TestCreateContainer{})
 
=== modified file 'storage_test.go'
--- storage_test.go	2013-04-29 11:52:04 +0000
+++ storage_test.go	2013-06-10 15:57:40 +0000
@@ -91,3 +91,144 @@
     expected += "</BlockList>"
     c.Check(string(body), Equals, expected)
 }
+
+type testListAllBlobs struct{}
+
+var _ = Suite(&testListAllBlobs{})
+
+// The ListAllBlobs Storage API call returns a BlobEnumerationResults struct
+// on success.
+func (suite *testListAllBlobs) Test(c *C) {
+    response_body := `
+        <?xml version="1.0" encoding="utf-8"?>
+        <EnumerationResults ContainerName="http://myaccount.blob.core.windows.net/mycontainer">
+          <Prefix>prefix</Prefix>
+          <Marker>marker</Marker>
+          <MaxResults>maxresults</MaxResults>
+          <Delimiter>delimiter</Delimiter>
+          <Blobs>
+            <Blob>
+              <Name>blob-name</Name>
+              <Snapshot>snapshot-date-time</Snapshot>
+              <Url>blob-address</Url>
+              <Properties>
+                <Last-Modified>last-modified</Last-Modified>
+                <Etag>etag</Etag>
+                <Content-Length>size-in-bytes</Content-Length>
+                <Content-Type>blob-content-type</Content-Type>
+                <Content-Encoding />
+                <Content-Language />
+                <Content-MD5 />
+                <Cache-Control />
+                <x-ms-blob-sequence-number>sequence-number</x-ms-blob-sequence-number>
+                <BlobType>blobtype</BlobType>
+                <LeaseStatus>leasestatus</LeaseStatus>
+                <LeaseState>leasestate</LeaseState>
+                <LeaseDuration>leasesduration</LeaseDuration>
+                <CopyId>id</CopyId>
+                <CopyStatus>copystatus</CopyStatus>
+                <CopySource>copysource</CopySource>
+                <CopyProgress>copyprogress</CopyProgress>
+                <CopyCompletionTime>copycompletiontime</CopyCompletionTime>
+                <CopyStatusDescription>copydesc</CopyStatusDescription>
+              </Properties>
+              <Metadata>
+                <MetaName1>metadataname1</MetaName1>
+                <MetaName2>metadataname2</MetaName2>
+              </Metadata>
+            </Blob>
+            <BlobPrefix>
+              <Name>blob-prefix</Name>
+            </BlobPrefix>
+          </Blobs>
+          <NextMarker />
+        </EnumerationResults>`
+    context := makeStorageContext()
+    response := &http.Response{
+        Status:     fmt.Sprintf("%d", http.StatusOK),
+        StatusCode: http.StatusOK,
+        Body:       makeResponseBody(response_body),
+    }
+    transport := &TestTransport{Response: response}
+    context.client = &http.Client{Transport: transport}
+    request := &ListBlobsRequest{Container: "container"}
+    results, err := context.ListAllBlobs(request)
+    c.Assert(err, IsNil)
+    c.Check(transport.Request.URL.String(), Matches, context.getContainerURL("container")+"?.*")
+    c.Check(transport.Request.URL.Query(), DeepEquals, url.Values{
+        "restype": {"container"},
+        "comp":    {"list"},
+    })
+    c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
+    c.Assert(results, NotNil)
+}
+
+// Client-side errors from the HTTP client are propagated back to the caller.
+func (suite *testListAllBlobs) TestError(c *C) {
+    error := fmt.Errorf("canned-error")
+    transport := &TestTransport{Error: error}
+    context := makeStorageContext()
+    context.client = &http.Client{Transport: transport}
+    request := &ListBlobsRequest{Container: "container"}
+    _, err := context.ListAllBlobs(request)
+    c.Assert(err, NotNil)
+}
+
+// Server-side errors are propagated back to the caller.
+func (suite *testListAllBlobs) TestErrorResponse(c *C) {
+    response := &http.Response{
+        Status:     fmt.Sprintf("%d", http.StatusNotFound),
+        StatusCode: http.StatusNotFound,
+    }
+    transport := &TestTransport{Response: response}
+    context := makeStorageContext()
+    context.client = &http.Client{Transport: transport}
+    request := &ListBlobsRequest{Container: "container"}
+    _, err := context.ListAllBlobs(request)
+    c.Assert(err, NotNil)
+}
+
+// ListAllBlobs combines multiple batches of output.
+func (suite *testListAllBlobs) 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}
+
+    request := &ListBlobsRequest{Container: "mycontainer"}
+    blobs, err := context.ListAllBlobs(request)
+    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)
+}