Merge lp:~jtv/gwacl/storage-urls into lp:gwacl

[Jeroen T. Vermeulen]

Factor out some repeated composition of storage URLs.

This increases the testing code a bit, but simplifies run-of-the-mill code nicely and eliminates some error-prone repetition. I also moved the addURLQueryParam() helper into utils.go, where it probably belongs now. Actually addURLQueryParam() is now a simple wrapper around addURLQueryParams() — I didn't bother testing them separately given how thin the wrapping is. Maybe I should drop the singular one now.

Much of the added complication in tests is an absolutely revolting property of url.URL: it does not maintain a consistent sorting of URL parameters. It's arbitrary. So the tests that happily assumed a fixed ordering, now that we're setting them "properly" through the standard library, start failing randomly. Different subset every time. The tests need to do looser text matching on URLs, and separate matching on the query.

With this change, interpolateURL() is barely used any more — just two cases, with one interpolation argument each. And I'm not even sure the one in x509Session.composeURL() is really a good idea. So unless we find new use for interpolateURL(), it might be worth dropping.

Jeroen

[Gavin Panella (allenap)]

I think addURLQueryParams() is good.

However, I don't think get{Account,Container,File}URL and the
resulting changes to storage_base.go are very useful. Despite the lack
of precedent, the syntax used with interpolateURL() is quite obvious
and readable, though I am surely biased. There's more code now, it
obfuscates what's happening, and I suspect YAGNI too.

I think most of the benefit of this branch could be obtained with:

  const STORAGE_URL_TEMPLATE = "http://___.blob.core.windows.net"

  s/"http://___.blob.core.windows.net"/STORAGE_URL_TEMPLATE + "/g

I'm torn. On the one hand I could well be biased because I introduced
interpolateURL(). On the other hand, I have little love for any Go
code, mine or otherwise. I vote approve, but please don't land this
unless you're really sure that it improves gwacl, regardless of any
other factor.

review: Approve

[Jeroen T. Vermeulen (jtv)]

Hypothetically, if get{Account,Container,File}URL each constructed their URL from scratch, repeating part of the URL between the three but not between call sites, would that assuage your concerns?

[Gavin Panella (allenap)]

> Hypothetically, if get{Account,Container,File}URL each constructed
> their URL from scratch, repeating part of the URL between the three
> but not between call sites, would that assuage your concerns?

Bizarrely enough, even though I like addURLQueryParam(s), it's its use
that adds the most noise. There's the map[string][]string{...}
construction and then the err != nil bits, both of which are almost
entirely noise, and distracting from the intention of the code.

There are two changes I might make:

1. Don't use a map[noise noise noise}, create a new struct to hold
   query parameters and pass them using a variadic arg. This might
   also help with the ordering issue you mentioned.

   -- or --

   Use a url.Values instead of a map[noise}. It won't help with the
   ordering though, if this is where the problem lies.

2. Panic instead of returning an error? I'm not saying anything new,
   but returning errors means loads of err != nil noise, and panicking
   is awkward to deal with because defer/recover is about as pleasant
   as rubbing fire ants into one's squishy bits. The choice is not
   clear.

   However, if the URL can't be parsed in the situations we have in
   gwacl *at this point in time* (as you say, the URL is tightly
   controlled), I would say that's a programmatic error rather than a
   runtime error, and so panicking seems appropriate.

Something like:

  type queryParam struct {
      name string
      value string
  }

  func addURLQueryParams(
      originalURL string, params ...queryParam) string {
      parsedURL, err := url.Parse(originalURL)
      if err != nil {
          panic(err)
      }
      query := parsedURL.Query()
      for param := range params {
          query.Add(param.name, param.value)
      }
      parsedURL.RawQuery = query.Encode()
      return parsedURL.String()
  }

  url := "http://example.com"
  addURLQueryParams(
      url, queryParam{"fred", "foo"}, queryParam("fred", "bar"})
  c.Assert(url, Equals, "http://example.com?fred=foo&fred=bar")

[Jeroen T. Vermeulen (jtv)]

About interpolateURL, I think fmt.Sprintf("http://%s.foo/bar//", url.QueryEscape(account)) isn't so bad for just a few cases in the codebase. Not particularly hard to read.

[Jeroen T. Vermeulen (jtv)]

> 1. Don't use a map[noise noise noise}, create a new struct to hold
> query parameters and pass them using a variadic arg. This might
> also help with the ordering issue you mentioned.

Good one. I'll do that. It also unifies the two functions.

> 2. Panic instead of returning an error? I'm not saying anything new,
> but returning errors means loads of err != nil noise, and panicking
> is awkward to deal with because defer/recover is about as pleasant
> as rubbing fire ants into one's squishy bits. The choice is not
> clear.
>
> However, if the URL can't be parsed in the situations we have in
> gwacl *at this point in time* (as you say, the URL is tightly
> controlled), I would say that's a programmatic error rather than a
> runtime error, and so panicking seems appropriate.

I toyed with the idea, but didn't have the courage to do it. But if I have an accomplice, that makes me feel safer.

Attempts to write clean code feel so futile in Go... In any other language, if I see the same 4 or 5 lines repeated all over the place, that's intolerable so I extract a function. But in Go, calling the function is 4 or 5 lines of code. Maybe 6 if it means I have to pre-define a variable to avoid shadowing. Or I can compress it a bit if I don't mind making the code harder to read — but I do.

Jeroen

[Jeroen T. Vermeulen (jtv)]

Okay, I removed interpolateURL and that simplified a few things (like over-interpolation bugs we had to watch out for). I also whittled down the redundancy in the get*URL() tests. The branch now removes about as many lines as it adds. Since I'm more or less manning the fort alone this week, I'm going to land this and then we can argue over any remaining details later.

[Julian Edwards (julian-edwards)]

On 29/04/13 22:57, Gavin Panella wrote:
> Review: Approve
>
> I think addURLQueryParams() is good.
>
> However, I don't think get{Account,Container,File}URL and the
> resulting changes to storage_base.go are very useful. Despite the lack
> of precedent, the syntax used with interpolateURL() is quite obvious
> and readable, though I am surely biased. There's more code now, it
> obfuscates what's happening, and I suspect YAGNI too.
>
> I think most of the benefit of this branch could be obtained with:
>
> const STORAGE_URL_TEMPLATE = "http://___.blob.core.windows.net"
>
> s/"http://___.blob.core.windows.net"/STORAGE_URL_TEMPLATE + "/g
>
> I'm torn. On the one hand I could well be biased because I introduced
> interpolateURL(). On the other hand, I have little love for any Go
> code, mine or otherwise. I vote approve, but please don't land this
> unless you're really sure that it improves gwacl, regardless of any
> other factor.
>

FWIW, that URL needs to be configurable at some point. There is a
development simulator service which we may want to use if anyone dares
run up Windows.

[Julian Edwards (julian-edwards)]

No proposals found for merge of https://code.launchpad.net/~jtv/gwacl/only-batch-list-blobs into https://code.launchpad.net/~gwacl-hackers/gwacl/trunk.

[Julian Edwards (julian-edwards)]

No proposals found for merge of https://code.launchpad.net/~jtv/gwacl/only-batch-list-blobs into https://code.launchpad.net/~gwacl-hackers/gwacl/trunk.

[Jeroen T. Vermeulen (jtv)]

This has already been approved in a previous incarnation of the MP, but I'm resubmitting it because Tarmac is refusing to land it. The previous merge proposal had a prerequisite branch, which has already landed.

review: Approve

=== modified file 'storage_base.go'
--- storage_base.go	2013-04-30 08:58:11 +0000
+++ storage_base.go	2013-04-30 09:36:29 +0000
@@ -265,18 +265,23 @@
     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
+// getAccountURL returns the base URL for the context's storage account.
+// (The result ends in a slash.)
+func (context *StorageContext) getAccountURL() string {
+    escapedAccount := url.QueryEscape(context.Account)
+    return fmt.Sprintf("http://%s.blob.core.windows.net/", escapedAccount)
+}
+
+// getContainerURL returns the URL for a given storage container.
+// (The result does not end in a slash.)
+func (context *StorageContext) getContainerURL(container string) string {
+    return context.getAccountURL() + url.QueryEscape(container)
+}
+
+// getFileURL returns the URL for a given file in the given container.
+// (The result does not end in a slash.)
+func (context *StorageContext) getFileURL(container, filename string) string {
+    return context.getContainerURL(container) + "/" + url.QueryEscape(filename)
 }
 
 // getListContainersBatch calls the "List Containers" operation on the storage
@@ -285,16 +290,10 @@
 // subsequent calls to get additional batches of the same result, pass the
 // 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)
+    uri := context.getAccountURL()
+    uri = addURLQueryParams(uri, "comp", "list")
     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)
-        }
+        uri = addURLQueryParams(uri, "marker", marker)
     }
     req, err := http.NewRequest("GET", uri, nil)
     if err != nil {
@@ -349,18 +348,13 @@
 // 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)
+    uri := context.getContainerURL(container)
+    uri = addURLQueryParams(
+        uri,
+        "restype", "container",
+        "comp", "list")
     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)
-        }
+        uri = addURLQueryParams(uri, "marker", marker)
     }
     req, err := http.NewRequest("GET", uri, nil)
     if err != nil {
@@ -408,9 +402,8 @@
 // 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 {
-    uri := interpolateURL(
-        "http://___.blob.core.windows.net/___?restype=container",
-        context.Account, container)
+    uri := context.getContainerURL(container)
+    uri = addURLQueryParams(uri, "restype", "container")
     req, err := http.NewRequest("PUT", uri, nil)
     if err != nil {
         return err
@@ -427,9 +420,7 @@
 // do the uploading, it just makes an empty file.
 func (context *StorageContext) PutBlob(container, filename string) error {
     // TODO: Add blobtype header.
-    uri := interpolateURL(
-        "http://___.blob.core.windows.net/___/___",
-        context.Account, container, filename)
+    uri := context.getFileURL(container, filename)
     req, err := http.NewRequest("PUT", uri, nil)
     if err != nil {
         return err
@@ -445,9 +436,12 @@
 // Send a request to fetch the list of blocks that have been uploaded as part
 // of a block blob.
 func (context *StorageContext) GetBlockList(container, filename string) (*GetBlockList, error) {
-    uri := interpolateURL(
-        "http://___.blob.core.windows.net/___/___?comp=blocklist&blocklisttype=all",
-        context.Account, container, filename)
+    uri := context.getFileURL(container, filename)
+    uri = addURLQueryParams(
+        uri,
+        "comp", "blocklist",
+        "blocklisttype", "all")
+
     req, err := http.NewRequest("GET", uri, nil)
     if err != nil {
         return nil, err
@@ -466,9 +460,11 @@
 // data block to upload.
 func (context *StorageContext) PutBlock(container, filename, id string, data io.Reader) error {
     base64_id := base64.StdEncoding.EncodeToString([]byte(id))
-    uri := interpolateURL(
-        "http://___.blob.core.windows.net/___/___?comp=block&blockid=___",
-        context.Account, container, filename, base64_id)
+    uri := context.getFileURL(container, filename)
+    uri = addURLQueryParams(
+        uri,
+        "comp", "block",
+        "blockid", base64_id)
     req, err := http.NewRequest("PUT", uri, data)
     if err != nil {
         return err
@@ -486,9 +482,8 @@
 
 // Send a request to piece together blocks into a list that specifies a blob.
 func (context *StorageContext) PutBlockList(container, filename string, blocklist *BlockList) error {
-    uri := interpolateURL(
-        "http://___.blob.core.windows.net/___/___?comp=blocklist",
-        context.Account, container, filename)
+    uri := context.getFileURL(container, filename)
+    uri = addURLQueryParams(uri, "comp", "blocklist")
     data, err := blocklist.Serialize()
     if err != nil {
         return err
@@ -511,9 +506,7 @@
 
 // Delete the specified blob from the given container.
 func (context *StorageContext) DeleteBlob(container, filename string) error {
-    uri := interpolateURL(
-        "http://___.blob.core.windows.net/___/___",
-        context.Account, container, filename)
+    uri := context.getFileURL(container, filename)
     req, err := http.NewRequest("DELETE", uri, nil)
     if err != nil {
         return err
@@ -532,9 +525,7 @@
 
 // Get the specified blob from the given container.
 func (context *StorageContext) GetBlob(container, filename string) (io.ReadCloser, error) {
-    uri := interpolateURL(
-        "http://___.blob.core.windows.net/___/___",
-        context.Account, container, filename)
+    uri := context.getFileURL(container, filename)
     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-29 08:06:24 +0000
+++ storage_base_test.go	2013-04-30 09:36:29 +0000
@@ -246,6 +246,42 @@
 
 var _ = Suite(&TestStorageContext{})
 
+// makeNastyURLUnfriendlyString returns a string that really needs escaping
+// before it can be included in a URL.
+func makeNastyURLUnfriendlyString() string {
+    return MakeRandomString(3) + "?&" + MakeRandomString(3) + "$%"
+}
+
+func (suite *TestStorageContext) TestGetAccountURL(c *C) {
+    account := makeNastyURLUnfriendlyString()
+    context := StorageContext{Account: account}
+    c.Check(
+        context.getAccountURL(),
+        Equals,
+        "http://"+url.QueryEscape(account)+".blob.core.windows.net/")
+}
+
+func (suite *TestStorageContext) TestGetContainerURL(c *C) {
+    account := makeNastyURLUnfriendlyString()
+    container := makeNastyURLUnfriendlyString()
+    context := StorageContext{Account: account}
+    c.Check(
+        context.getContainerURL(container),
+        Equals,
+        "http://"+url.QueryEscape(account)+".blob.core.windows.net/"+url.QueryEscape(container))
+}
+
+func (suite *TestStorageContext) TestGetFileURL(c *C) {
+    account := makeNastyURLUnfriendlyString()
+    container := makeNastyURLUnfriendlyString()
+    file := makeNastyURLUnfriendlyString()
+    context := StorageContext{Account: account}
+    c.Check(
+        context.getFileURL(container, file),
+        Equals,
+        "http://"+url.QueryEscape(account)+".blob.core.windows.net/"+url.QueryEscape(container)+"/"+url.QueryEscape(file))
+}
+
 func (suite *TestStorageContext) TestGetClientReturnsDefaultClient(c *C) {
     context := &StorageContext{client: nil}
     c.Assert(context.getClient(), Equals, http.DefaultClient)
@@ -266,48 +302,6 @@
     }
 }
 
-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{})
@@ -479,27 +473,6 @@
     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{})
@@ -561,9 +534,11 @@
     context.client = &http.Client{Transport: transport}
     results, err := context.ListBlobs("container")
     c.Assert(err, IsNil)
-    c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
-        "http://%s.blob.core.windows.net/container?restype=container&comp=list",
-        context.Account))
+    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)
 }
@@ -746,9 +721,7 @@
     context.client = &http.Client{Transport: transport}
     err := context.PutBlob("container", "blobname")
     c.Assert(err, IsNil)
-    c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
-        "http://%s.blob.core.windows.net/container/blobname",
-        context.Account))
+    c.Check(transport.Request.URL.String(), Equals, context.getFileURL("container", "blobname"))
     c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
 }
 
@@ -799,10 +772,11 @@
 
     // The blockid should have been base64 encoded and url escaped.
     base64_id := base64.StdEncoding.EncodeToString([]byte(blockid))
-    escaped_id := url.QueryEscape(base64_id)
-    c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
-        "http://%s.blob.core.windows.net/container/blobname?comp=block&blockid=%s",
-        context.Account, escaped_id))
+    c.Check(transport.Request.URL.String(), Matches, context.getFileURL("container", "blobname")+"?.*")
+    c.Check(transport.Request.URL.Query(), DeepEquals, url.Values{
+        "comp":    {"block"},
+        "blockid": {base64_id},
+    })
     c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
 
     data, err := ioutil.ReadAll(transport.Request.Body)
@@ -936,9 +910,11 @@
     context.client = &http.Client{Transport: transport}
     results, err := context.GetBlockList("container", "myfilename")
     c.Assert(err, IsNil)
-    c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
-        "http://%s.blob.core.windows.net/container/myfilename?comp=blocklist&blocklisttype=all",
-        context.Account))
+    c.Check(transport.Request.URL.String(), Matches, context.getFileURL("container", "myfilename")+"?.*")
+    c.Check(transport.Request.URL.Query(), DeepEquals, url.Values{
+        "comp":          {"blocklist"},
+        "blocklisttype": {"all"},
+    })
     c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
     c.Assert(results, NotNil)
 }
@@ -982,9 +958,7 @@
     c.Assert(err, IsNil)
 
     c.Check(transport.Request.Method, Equals, "DELETE")
-    c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
-        "http://%s.blob.core.windows.net/container/blobname",
-        context.Account))
+    c.Check(transport.Request.URL.String(), Equals, context.getFileURL("container", "blobname"))
     c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
     c.Check(transport.Request.Body, IsNil)
 }
@@ -1036,9 +1010,7 @@
     defer reader.Close()
 
     c.Check(transport.Request.Method, Equals, "GET")
-    c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
-        "http://%s.blob.core.windows.net/container/blobname",
-        context.Account))
+    c.Check(transport.Request.URL.String(), Equals, context.getFileURL("container", "blobname"))
     c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
 
     data, err := ioutil.ReadAll(reader)

=== modified file 'storage_test.go'
--- storage_test.go	2013-04-24 10:08:54 +0000
+++ storage_test.go	2013-04-30 09:36:29 +0000
@@ -67,10 +67,11 @@
 
 func assertBlockSent(
     c *C, context *StorageContext, data []byte, blockID string, exchange *TestTransport2Exchange) {
-    c.Check(exchange.Request.URL.String(), Equals, fmt.Sprintf(
-        "http://%s.blob.core.windows.net/MyContainer/MyFile"+
-            "?comp=block&blockid=%s",
-        context.Account, url.QueryEscape(blockID)))
+    c.Check(exchange.Request.URL.String(), Matches, context.getFileURL("MyContainer", "MyFile")+"?.*")
+    c.Check(exchange.Request.URL.Query(), DeepEquals, url.Values{
+        "comp":    {"block"},
+        "blockid": {blockID},
+    })
     body, err := ioutil.ReadAll(exchange.Request.Body)
     c.Check(err, IsNil)
     c.Check(body, DeepEquals, data)

=== modified file 'utils.go'
--- utils.go	2013-04-26 03:53:37 +0000
+++ utils.go	2013-04-30 09:36:29 +0000
@@ -6,7 +6,6 @@
 import (
     "fmt"
     "net/url"
-    "strings"
 )
 
 // checkPathComponents checks that none of the passed components contains any
@@ -23,23 +22,25 @@
     }
 }
 
-// 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, 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, interpolationPlaceholder, component, 1)
-    }
-    return urlstring
+// addURLQueryParams adds query parameters to a URL (and escapes as needed).
+// Parameters are URL, [key, value, [key, value, [...]]].
+// The original URL must be correct, i.e. it should parse without error.
+func addURLQueryParams(originalURL string, params ...string) string {
+    if len(params)%2 != 0 {
+        panic(fmt.Errorf("got %d parameter argument(s), instead of matched key/value pairs", len(params)))
+    }
+    parsedURL, err := url.Parse(originalURL)
+    if err != nil {
+        panic(err)
+    }
+    query := parsedURL.Query()
+
+    for index := 0; index < len(params); index += 2 {
+        key := params[index]
+        value := params[index+1]
+        query.Add(key, value)
+    }
+
+    parsedURL.RawQuery = query.Encode()
+    return parsedURL.String()
 }

=== modified file 'utils_test.go'
--- utils_test.go	2013-04-12 15:03:26 +0000
+++ utils_test.go	2013-04-30 09:36:29 +0000
@@ -5,7 +5,7 @@
 
 import (
     . "launchpad.net/gocheck"
-    "regexp"
+    "net/url"
 )
 
 type UtilsSuite struct{}
@@ -25,29 +25,63 @@
         PanicMatches, "'[.][.]' is not allowed")
 }
 
-func (suite *UtilsSuite) TestInterpolateURL(c *C) {
-    // Interpolation with nothing is fine.
-    c.Check(
-        interpolateURL("http://example.com"),
-        Equals, "http://example.com")
-    // Interpolation with left-over arguments causes a panic.
-    c.Check(
-        func() { interpolateURL("http://example.com", "fred") }, PanicMatches,
-        regexp.QuoteMeta("0 slot(s) but given 1 replacement(s)"))
-    // Three underscores denotes where replacement should happen.
-    c.Check(
-        interpolateURL("http://___.example.com", "fred"),
-        Equals, "http://fred.example.com")
-    // Multiple replacements are fine.
-    c.Check(
-        interpolateURL("http://___.example.com/___", "fred", "bob"),
-        Equals, "http://fred.example.com/bob")
-    // If there are too few replacements it panics.
-    c.Check(
-        func() { interpolateURL("http://example.com/___") }, PanicMatches,
-        regexp.QuoteMeta("1 slot(s) but given 0 replacement(s)"))
-    // Replacements are percent-encoded.
-    c.Check(
-        interpolateURL("http://example.com/___", "foo^bar/baz"),
-        Equals, "http://example.com/foo%5Ebar%2Fbaz")
+type TestAddURLQueryParams struct{}
+
+var _ = Suite(&TestAddURLQueryParams{})
+
+func (*TestAddURLQueryParams) TestUsesBaseURL(c *C) {
+    baseURL := "http://example.com"
+
+    extendedURL := addURLQueryParams(baseURL, "key", "value")
+
+    parsedURL, err := url.Parse(extendedURL)
+    c.Assert(err, IsNil)
+    c.Check(parsedURL.Scheme, Equals, "http")
+    c.Check(parsedURL.Host, Equals, "example.com")
+}
+
+func (suite *TestAddURLQueryParams) TestEscapesParams(c *C) {
+    key := "key&key"
+    value := "value%value"
+
+    uri := addURLQueryParams("http://example.com", key, value)
+
+    parsedURL, err := url.Parse(uri)
+    c.Assert(err, IsNil)
+    c.Check(parsedURL.Query()[key], DeepEquals, []string{value})
+}
+
+func (suite *TestAddURLQueryParams) TestAddsToExistingParams(c *C) {
+    uri := addURLQueryParams("http://example.com?a=one", "b", "two")
+
+    parsedURL, err := url.Parse(uri)
+    c.Assert(err, IsNil)
+    c.Check(parsedURL.Query(), DeepEquals, url.Values{
+        "a": {"one"},
+        "b": {"two"},
+    })
+}
+
+func (suite *TestAddURLQueryParams) TestAppendsRepeatedParams(c *C) {
+    uri := addURLQueryParams("http://example.com?foo=bar", "foo", "bar")
+    c.Check(uri, Equals, "http://example.com?foo=bar&foo=bar")
+}
+
+func (suite *TestAddURLQueryParams) TestAddsMultipleParams(c *C) {
+    uri := addURLQueryParams("http://example.com", "one", "un", "two", "deux")
+    parsedURL, err := url.Parse(uri)
+    c.Assert(err, IsNil)
+    c.Check(parsedURL.Query(), DeepEquals, url.Values{
+        "one": {"un"},
+        "two": {"deux"},
+    })
+}
+
+func (suite *TestAddURLQueryParams) TestRejectsOddNumberOfParams(c *C) {
+    defer func() {
+        err := recover()
+        c.Check(err, ErrorMatches, ".*got 1 parameter.*")
+    }()
+    addURLQueryParams("http://example.com", "key")
+    c.Assert("This should have panicked", Equals, "But it didn't.")
 }

=== modified file 'x509session.go'
--- x509session.go	2013-04-12 09:26:52 +0000
+++ x509session.go	2013-04-30 09:36:29 +0000
@@ -34,14 +34,12 @@
     if err != nil {
         panic(err)
     }
-    path = fmt.Sprintf("___/%s", path)
-    path_url, err := url.Parse(path)
+    escapedID := url.QueryEscape(session.subscriptionId)
+    path_url, err := url.Parse(escapedID + "/" + path)
     if err != nil {
         panic(err)
     }
-    return interpolateURL(
-        azure_url.ResolveReference(path_url).String(),
-        session.subscriptionId)
+    return azure_url.ResolveReference(path_url).String()
 }
 
 // _X509Dispatcher is the function used to dispatch requests.  We call the