Merge lp:~jtv/gwacl/pass-marker into lp:gwacl

[Jeroen T. Vermeulen]

Pass marker URI parameter to List Containers.

This is a crucial, but hard to test, part that I forgot in my original implementation.

Please excuse the crude URL composition. I don't think we currently have the flexibility to do it more nicely.

Jeroen

[Gavin Panella (allenap)]

Looks good.

[1]

     uri := interpolateURL(
         "http://___.blob.core.windows.net/?comp=list", context.Account)
+    if marker != "" {
+        uri += "&" + interpolateURL("marker=___", marker)
+    }

The choice of ___ was not arbitrary here. It let's us do:

    u, err := url.Parse("http://___.blob.core.windows.net/?comp=list")
    if err != nil { ... tiresome crap ... }
    q := u.Query()
    q.Set("marker", marker)
    u.RawQuery = q.Encode()
    uri := interpolateURL(u.String(), context.Account)

Or you could do it this way:

    uri := interpolateURL(
        "http://___.blob.core.windows.net/?comp=list", context.Account)
    u, err := url.Parse(uri)
    if err != nil { ... tiresome crap ... }
    q := u.Query()
    q.Set("marker", marker)
    u.RawQuery = q.Encode()
    uri = u.String()

net.url.URL can be a PITA, but it can sometimes be coerced into doing
the right thing.

[2]

+    query := transport.Exchanges[0].Request.URL.RawQuery
+    values, err := url.ParseQuery(query)

This is funny, because you could also do:

    query := transport.Exchanges[0].Request.URL.Query()

There's no error returned from Query(), unlike ParseQuery(). I wonder
where that error went? I mean, it's not possible that the error is
thrown away, is it? Not in Go I say with gusto, because developers are
constantly reminded of the need to deal correctly with errors. It's
not like the standard library could contain the following exact
implementation of Query() is it?

// Query parses RawQuery and returns the corresponding values.
func (u *URL) Query() Values {
        v, _ := ParseQuery(u.RawQuery)
        return v
}

I must be dreaming.

review: Approve

[Julian Edwards (julian-edwards)]

On Thursday 25 Apr 2013 16:30:32 you wrote:
>
> There's no error returned from Query(), unlike ParseQuery(). I wonder
> where that error went? I mean, it's not possible that the error is
> thrown away, is it? Not in Go I say with gusto, because developers are
> constantly reminded of the need to deal correctly with errors. It's
> not like the standard library could contain the following exact
> implementation of Query() is it?
>
> // Query parses RawQuery and returns the corresponding values.
> func (u *URL) Query() Values {
> v, _ := ParseQuery(u.RawQuery)
> return v
> }
>
> I must be dreaming.

Love it.

[Jeroen T. Vermeulen (jtv)]

Damn, I thought you were making that up! But it really is what the standard library does..

Quick, to the blogmobile!

[lp:~jtv/gwacl/pass-marker]

100. By Jeroen T. Vermeulen on 2013-04-26

Review change: use proper URL query handling, and make sure the marker can safely contain '___'.

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

Comment.

[Jeroen T. Vermeulen (jtv)]

I used the URL query handling, thanks. The first form you gave didn't look correct in the face of marker strings that happen to contain "___" so I used a version of the second form.

And I added a test for that nasty case, so we don't get bitten by it later. It may sound improbable, but spend some time with C++ link-name mangling...

=== modified file 'storage_base.go'
--- storage_base.go	2013-04-25 14:01:00 +0000
+++ storage_base.go	2013-04-26 04:00:40 +0000
@@ -273,8 +273,20 @@
 // "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)
+    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)
+        if err != nil {
+            return nil, "", 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

=== modified file 'storage_base_test.go'
--- storage_base_test.go	2013-04-25 09:54:06 +0000
+++ storage_base_test.go	2013-04-26 04:00:40 +0000
@@ -383,6 +383,81 @@
     c.Check(containers.Containers[1].Name, Equals, lastContainer)
 }
 
+func (suite *TestListContainers) TestGetListContainersBatchPassesMarker(c *C) {
+    transport := TestTransport2{}
+    transport.AddExchange(&http.Response{StatusCode: http.StatusOK, Body: Empty}, nil)
+    context := makeStorageContext()
+    context.client = &http.Client{Transport: &transport}
+
+    // Call getListContainersBatch.  This will fail because of the empty
+    // response, but no matter.  We only care about the request.
+    _, _, err := context.getListContainersBatch("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 *TestListContainers) TestGetListContainersBatchDoesNotPassEmptyMarker(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.getListContainersBatch("")
+    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))
+}
+
+func (suite *TestListContainers) TestGetListContainersBatchEscapesMarker(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.getListContainersBatch("x&y")
+    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{"x&y"})
+}
+
+// The marker string may safely contain the placeholder that we happen to use
+// for URL interpolation.
+func (suite *TestListContainers) TestGetListContainersBatchDoesNotInterpolateMarker(c *C) {
+    transport := TestTransport2{}
+    transport.AddExchange(&http.Response{StatusCode: http.StatusOK, Body: Empty}, nil)
+    context := makeStorageContext()
+    context.client = &http.Client{Transport: &transport}
+    marker := "x" + interpolationPlaceholder + "y"
+
+    // 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)
+    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{marker})
+}
+
 type TestListBlobs struct{}
 
 var _ = Suite(&TestListBlobs{})

=== modified file 'utils.go'
--- utils.go	2013-04-12 15:03:26 +0000
+++ utils.go	2013-04-26 04:00:40 +0000
@@ -23,19 +23,23 @@
     }
 }
 
-// interpolateURL replaces occurrences of ___ (three underscores) with the
+// We use ___ (three underscores) as a placeholder for variables that are to be
+// interpolated into a URL.
+const interpolationPlaceholder = "___"
+
+// interpolateURL replaces occurrences of the interpolation placeolder with the
 // components specified. Additionally, the components are percent-encoded
 // (using net.url.QueryEscape) before being interpolated.
 func interpolateURL(urlstring string, components ...string) string {
     components_count := len(components)
-    replacement_count := strings.Count(urlstring, "___")
+    replacement_count := strings.Count(urlstring, interpolationPlaceholder)
     if components_count != replacement_count {
         panic(fmt.Errorf("%d slot(s) but given %d replacement(s)",
             replacement_count, components_count))
     }
     for _, component := range components {
         component = url.QueryEscape(component)
-        urlstring = strings.Replace(urlstring, "___", component, 1)
+        urlstring = strings.Replace(urlstring, interpolationPlaceholder, component, 1)
     }
     return urlstring
 }