Merge lp:~jtv/gwacl/require-storage-location into lp:gwacl

[Jeroen T. Vermeulen]

Make StorageContext.AzureEndpoint required.

Unfortunately, making this required is a runtime-incompatible API change. That is, client code that fails to provide it will still compile but it will panic at run time.

There was a lot of collateral damage, as expected. In particular, two storage tests relied on some arbitrary hard-coded names (not even mentioned in the tests themselves!) being identical between various helper functions. Luckily it wasn't hard to clean this up a bit. The tests themselves are now the only source of those arbitrary strings.

The test-helper factory for storage contexts sets an "unguessable" AzureEndpoint by default, so that we don't accidentally rely on such implicit connections here. Tests that need a known string, have to set one. As per the normal policy I used example.com, except of course in the tests for the functions that actually generate those strings. Those are already acceptance-tested against the real-world URLs, although it probably won't show up in the diff here.

Jeroen

[Julian Edwards (julian-edwards)]

Looks great! Good testing as always from you.

8 - Account: MakeRandomString(10),
9 - Key: base64.StdEncoding.EncodeToString(MakeRandomByteSlice(10)),
10 + Account: MakeRandomString(10),
11 + Key: base64.StdEncoding.EncodeToString(MakeRandomByteSlice(10)),
12 + AzureEndpoint: APIEndpoint("http://" + MakeRandomString(5) + ".example.com/"),

I'd like to just take a moment to bitch at gofmt for changing lines completely unrelated to the diff at hand. >:(

44 + if context.AzureEndpoint == APIEndpoint("") {
45 + panic(errors.New("no Azure blob storage endpoint specified"))

Can you mention the parameter by name please, it will save a confused developer from hunting down this line of source.

review: Approve

[lp:~jtv/gwacl/require-storage-location]

218. By Jeroen T. Vermeulen on 2013-08-07

Name AzureEndpoint in panic text.

[Jeroen T. Vermeulen (jtv)]

Collateral damage is a recurring problem in Go.

=== modified file 'helpers_http_test.go'
--- helpers_http_test.go	2013-07-08 10:03:48 +0000
+++ helpers_http_test.go	2013-08-07 02:15:36 +0000
@@ -50,8 +50,9 @@
 // random base64-encoded key.
 func makeStorageContext(transport http.RoundTripper) *StorageContext {
     context := &StorageContext{
-        Account: MakeRandomString(10),
-        Key:     base64.StdEncoding.EncodeToString(MakeRandomByteSlice(10)),
+        Account:       MakeRandomString(10),
+        Key:           base64.StdEncoding.EncodeToString(MakeRandomByteSlice(10)),
+        AzureEndpoint: APIEndpoint("http://" + MakeRandomString(5) + ".example.com/"),
     }
     context.client = &http.Client{Transport: transport}
     return context

=== modified file 'storage_base.go'
--- storage_base.go	2013-08-06 10:15:28 +0000
+++ storage_base.go	2013-08-07 02:15:36 +0000
@@ -210,12 +210,7 @@
     Key string
 
     // AzureEndpoint specifies a base service endpoint URL for the Azure APIs.
-    // If this is not set, it will default to the international endpoint which
-    // will not work in mainland China.
-    //
-    // Try to set this if at all possible.  Use GetEndpoint() to obtain the
-    // endpoint associated with a given service location, e.g. "West US" or
-    // "North Europe" or "East China".
+    // This field is required.
     AzureEndpoint APIEndpoint
 
     client *http.Client
@@ -334,20 +329,16 @@
 // getAccountURL returns the base URL for the context's storage account.
 // (The result ends in a slash.)
 func (context *StorageContext) getAccountURL() string {
-    endpoint := context.AzureEndpoint
-    if endpoint == "" {
-        // No API endpoint specified.  Default to the international one.
-        // This will not work for mainland China.
-        // TODO: Make this required, and panic() instead.
-        endpoint = GetEndpoint("West US")
+    if context.AzureEndpoint == APIEndpoint("") {
+        panic(errors.New("no AzureEndpoint specified in gwacl.StorageContext"))
     }
-    return endpoint.BlobStorageAPI(context.Account)
+    return context.AzureEndpoint.BlobStorageAPI(context.Account)
 }
 
 // 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)
+    return strings.TrimRight(context.getAccountURL(), "/") + "/" + url.QueryEscape(container)
 }
 
 // GetFileURL returns the URL for a given file in the given container.

=== modified file 'storage_base_test.go'
--- storage_base_test.go	2013-08-06 04:53:38 +0000
+++ storage_base_test.go	2013-08-07 02:15:36 +0000
@@ -6,6 +6,7 @@
 import (
     "bytes"
     "encoding/base64"
+    "errors"
     "fmt"
     "io/ioutil"
     . "launchpad.net/gocheck"
@@ -309,33 +310,50 @@
         "http://"+url.QueryEscape(account)+".blob.example.com")
 }
 
-func (*TestStorageContext) TestGetAccountURLDefaultsToInternationalEndpoint(c *C) {
+func (*TestStorageContext) TestGetAccountURLRequiresEndpoint(c *C) {
     context := StorageContext{Account: "myaccount"}
     c.Check(
-        context.getAccountURL(),
-        Equals,
-        "https://myaccount.blob.core.windows.net/")
+        context.getAccountURL,
+        Panics,
+        errors.New("no AzureEndpoint specified in gwacl.StorageContext"))
 }
 
-func (suite *TestStorageContext) TestGetContainerURL(c *C) {
+func (suite *TestStorageContext) TestGetContainerURLAddsContainer(c *C) {
     account := makeNastyURLUnfriendlyString()
     container := makeNastyURLUnfriendlyString()
-    context := StorageContext{Account: account}
+    context := StorageContext{
+        Account:       account,
+        AzureEndpoint: "http://example.com/",
+    }
     c.Check(
         context.getContainerURL(container),
         Equals,
-        "https://"+url.QueryEscape(account)+".blob.core.windows.net/"+url.QueryEscape(container))
+        "http://"+url.QueryEscape(account)+".blob.example.com/"+url.QueryEscape(container))
+}
+
+func (suite *TestStorageContext) TestGetContainerURLAddsSlashIfNeeded(c *C) {
+    context := StorageContext{
+        Account:       "account",
+        AzureEndpoint: "http://example.com",
+    }
+    c.Check(
+        context.getContainerURL("container"),
+        Equals,
+        "http://account.blob.example.com/container")
 }
 
 func (suite *TestStorageContext) TestGetFileURL(c *C) {
     account := makeNastyURLUnfriendlyString()
     container := makeNastyURLUnfriendlyString()
     file := makeNastyURLUnfriendlyString()
-    context := StorageContext{Account: account}
+    context := StorageContext{
+        Account:       account,
+        AzureEndpoint: "http://example.com/",
+    }
     c.Check(
         context.GetFileURL(container, file),
         Equals,
-        "https://"+url.QueryEscape(account)+".blob.core.windows.net/"+url.QueryEscape(container)+"/"+url.QueryEscape(file))
+        "http://"+url.QueryEscape(account)+".blob.example.com/"+url.QueryEscape(container)+"/"+url.QueryEscape(file))
 }
 
 func (suite *TestStorageContext) TestGetSignedFileURL(c *C) {
@@ -343,7 +361,11 @@
     container := "container"
     file := "/a/file"
     key := base64.StdEncoding.EncodeToString([]byte("dummykey"))
-    context := StorageContext{Account: account, Key: key}
+    context := StorageContext{
+        Account:       account,
+        Key:           key,
+        AzureEndpoint: "http://example.com/",
+    }
     expires := time.Now()
 
     signedURL := context.GetAnonymousFileURL(container, file, expires)
@@ -408,11 +430,12 @@
     response := makeHttpResponse(http.StatusOK, responseBody)
     transport := &TestTransport{Response: response}
     context := makeStorageContext(transport)
+    context.AzureEndpoint = "http://example.com/"
     request := &ListContainersRequest{Marker: ""}
     results, err := context.ListContainers(request)
     c.Assert(err, IsNil)
     c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
-        "https://%s.blob.core.windows.net/?comp=list", context.Account))
+        "http://%s.blob.example.com/?comp=list", context.Account))
     c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
     c.Assert(results, NotNil)
     c.Assert(results.Containers[0].Name, Equals, "name-value")
@@ -666,11 +689,12 @@
     response := makeHttpResponse(http.StatusCreated, "")
     transport := &TestTransport{Response: response}
     context := makeStorageContext(transport)
+    context.AzureEndpoint = "http://example.com/"
     containerName := MakeRandomString(10)
     err := context.CreateContainer(containerName)
     c.Assert(err, IsNil)
     c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
-        "https://%s.blob.core.windows.net/%s?restype=container",
+        "http://%s.blob.example.com/%s?restype=container",
         context.Account, containerName))
     c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
 }
@@ -720,11 +744,12 @@
     response := makeHttpResponse(http.StatusAccepted, "")
     transport := &TestTransport{Response: response}
     context := makeStorageContext(transport)
+    context.AzureEndpoint = "http://example.com/"
     containerName := MakeRandomString(10)
     err := context.DeleteContainer(containerName)
     c.Assert(err, IsNil)
     c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
-        "https://%s.blob.core.windows.net/%s?restype=container",
+        "http://%s.blob.example.com/%s?restype=container",
         context.Account, containerName))
     c.Check(transport.Request.Method, Equals, "DELETE")
     c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
@@ -786,11 +811,12 @@
 
     transport := &TestTransport{Response: response}
     context := makeStorageContext(transport)
+    context.AzureEndpoint = "http://example.com/"
     containerName := MakeRandomString(10)
     props, err := context.GetContainerProperties(containerName)
     c.Assert(err, IsNil)
     c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
-        "https://%s.blob.core.windows.net/%s?restype=container",
+        "http://%s.blob.example.com/%s?restype=container",
         context.Account, containerName))
     c.Check(transport.Request.Method, Equals, "GET")
     c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
@@ -1099,6 +1125,7 @@
     response := makeHttpResponse(http.StatusCreated, "")
     transport := &TestTransport{Response: response}
     context := makeStorageContext(transport)
+    context.AzureEndpoint = "http://example.com/"
     blocklist := &BlockList{}
     blocklist.Add(BlockListLatest, "b1")
     blocklist.Add(BlockListLatest, "b2")
@@ -1107,7 +1134,7 @@
 
     c.Check(transport.Request.Method, Equals, "PUT")
     c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
-        "https://%s.blob.core.windows.net/container/blobname?comp=blocklist",
+        "http://%s.blob.example.com/container/blobname?comp=blocklist",
         context.Account))
     c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
 
@@ -1340,6 +1367,7 @@
     response := makeHttpResponse(http.StatusOK, "")
     transport := &TestTransport{Response: response}
     context := makeStorageContext(transport)
+    context.AzureEndpoint = "http://example.com/"
     err := context.SetContainerACL(&SetContainerACLRequest{
         Container: "mycontainer", Access: "container"})
 
@@ -1347,7 +1375,7 @@
     c.Check(transport.Request.Method, Equals, "PUT")
     c.Check(transport.Request.URL.String(), Matches,
         fmt.Sprintf(
-            "https://%s.blob.core.windows.net/mycontainer?.*", context.Account))
+            "http://%s.blob.example.com/mycontainer?.*", context.Account))
     c.Check(transport.Request.URL.Query(), DeepEquals, url.Values{
         "comp":    {"acl"},
         "restype": {"container"},

=== modified file 'storage_test.go'
--- storage_test.go	2013-08-05 12:01:32 +0000
+++ storage_test.go	2013-08-07 02:15:36 +0000
@@ -26,13 +26,14 @@
     // UploadBlockBlob then sends the list of blocks with PutBlockList.
     transport.AddExchange(makeFakeCreatedResponse(), nil)
     // Upload a random blob of data.
-    data := uploadRandomBlob(c, context, 10)
+    data := uploadRandomBlob(c, context, 10, "MyContainer", "MyFile")
     // There were two exchanges.
     c.Assert(transport.ExchangeCount, Equals, 2)
     // The first request is a Put Block with the block data.
-    assertBlockSent(c, context, data, b64("000000000000000000000000000000"), transport.Exchanges[0])
+    fileURL := context.GetFileURL("MyContainer", "MyFile")
+    assertBlockSent(c, context, data, b64("000000000000000000000000000000"), transport.Exchanges[0], fileURL)
     // The second request is Put Block List to commit the block above.
-    assertBlockListSent(c, context, []string{b64("000000000000000000000000000000")}, transport.Exchanges[1])
+    assertBlockListSent(c, context, []string{b64("000000000000000000000000000000")}, transport.Exchanges[1], fileURL)
 }
 
 func (suite *testUploadBlockBlob) TestLargeFile(c *C) {
@@ -44,28 +45,29 @@
     // UploadBlockBlob then sends the list of blocks with PutBlockList.
     transport.AddExchange(makeFakeCreatedResponse(), nil)
     // Upload a large random blob of data.
-    data := uploadRandomBlob(c, context, 1348*1024)
+    data := uploadRandomBlob(c, context, 1348*1024, "MyContainer", "MyFile")
     // There were three exchanges.
     c.Assert(transport.ExchangeCount, Equals, 3)
     // The first two requests are Put Block with chunks of the block data. The
     // weird looking block IDs are base64 encodings of the strings "0" and "1".
-    assertBlockSent(c, context, data[:1024*1024], b64("000000000000000000000000000000"), transport.Exchanges[0])
-    assertBlockSent(c, context, data[1024*1024:], b64("000000000000000000000000000001"), transport.Exchanges[1])
+    fileURL := context.GetFileURL("MyContainer", "MyFile")
+    assertBlockSent(c, context, data[:1024*1024], b64("000000000000000000000000000000"), transport.Exchanges[0], fileURL)
+    assertBlockSent(c, context, data[1024*1024:], b64("000000000000000000000000000001"), transport.Exchanges[1], fileURL)
     // The second request is Put Block List to commit the block above.
-    assertBlockListSent(c, context, []string{b64("000000000000000000000000000000"), b64("000000000000000000000000000001")}, transport.Exchanges[2])
+    assertBlockListSent(c, context, []string{b64("000000000000000000000000000000"), b64("000000000000000000000000000001")}, transport.Exchanges[2], fileURL)
 }
 
-func uploadRandomBlob(c *C, context *StorageContext, size int) []byte {
+func uploadRandomBlob(c *C, context *StorageContext, size int, container, filename string) []byte {
     data := MakeRandomByteSlice(size)
     err := context.UploadBlockBlob(
-        "MyContainer", "MyFile", bytes.NewReader(data))
+        container, filename, bytes.NewReader(data))
     c.Assert(err, IsNil)
     return data
 }
 
 func assertBlockSent(
-    c *C, context *StorageContext, data []byte, blockID string, exchange *MockingTransportExchange) {
-    c.Check(exchange.Request.URL.String(), Matches, context.GetFileURL("MyContainer", "MyFile")+"?.*")
+    c *C, context *StorageContext, data []byte, blockID string, exchange *MockingTransportExchange, fileURL string) {
+    c.Check(exchange.Request.URL.String(), Matches, fileURL+"?.*")
     c.Check(exchange.Request.URL.Query(), DeepEquals, url.Values{
         "comp":    {"block"},
         "blockid": {blockID},
@@ -85,10 +87,11 @@
 }
 
 func assertBlockListSent(
-    c *C, context *StorageContext, blockIDs []string, exchange *MockingTransportExchange) {
-    c.Check(exchange.Request.URL.String(), Equals, fmt.Sprintf(
-        "https://%s.blob.core.windows.net/MyContainer/MyFile"+
-            "?comp=blocklist", context.Account))
+    c *C, context *StorageContext, blockIDs []string, exchange *MockingTransportExchange, fileURL string) {
+    c.Check(exchange.Request.URL.String(), Matches, fileURL+"?.*")
+    c.Check(exchange.Request.URL.Query(), DeepEquals, url.Values{
+        "comp": {"blocklist"},
+    })
     body, err := ioutil.ReadAll(exchange.Request.Body)
     c.Check(err, IsNil)
     expected := "<BlockList>\n"
@@ -272,10 +275,11 @@
     }
     transport := &TestTransport{Response: response}
     context := makeStorageContext(transport)
+    context.AzureEndpoint = "http://example.com/"
     results, err := context.ListAllContainers()
     c.Assert(err, IsNil)
     c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
-        "https://%s.blob.core.windows.net/?comp=list", context.Account))
+        "http://%s.blob.example.com/?comp=list", context.Account))
     c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
     c.Assert(results, NotNil)
     c.Assert(results.Containers[0].Name, Equals, "name-value")

=== modified file 'testing_test.go'
--- testing_test.go	2013-06-27 13:58:43 +0000
+++ testing_test.go	2013-08-07 02:15:36 +0000
@@ -19,6 +19,7 @@
     transport := &TestTransport{Error: error}
     client := &http.Client{Transport: transport}
     context := NewTestStorageContext(client)
+    context.AzureEndpoint = "http://example.com/"
     request := &ListContainersRequest{Marker: ""}
     _, err := context.ListContainers(request)
     c.Check(err, ErrorMatches, ".*"+errorMessage+".*")