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

Proposed by Jeroen T. Vermeulen on 2013-08-06
Status: Merged
Approved by: Jeroen T. Vermeulen on 2013-08-07
Approved revision: 218
Merged at revision: 215
Proposed branch: lp:~jtv/gwacl/require-storage-location
Merge into: lp:gwacl
Diff against target: 334 lines (+73/-48)
5 files modified
helpers_http_test.go (+3/-2)
storage_base.go (+5/-14)
storage_base_test.go (+44/-16)
storage_test.go (+20/-16)
testing_test.go (+1/-0)
To merge this branch: bzr merge lp:~jtv/gwacl/require-storage-location
Reviewer Review Type Date Requested Status
Julian Edwards (community) 2013-08-06 Approve on 2013-08-07
Review via email: mp+178721@code.launchpad.net

Commit message

Make StorageContext.AzureEndpoint required.

Description of the change

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

To post a comment you must log in.
Julian Edwards (julian-edwards) wrote :

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
218. By Jeroen T. Vermeulen on 2013-08-07

Name AzureEndpoint in panic text.

Jeroen T. Vermeulen (jtv) wrote :

Collateral damage is a recurring problem in Go.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'helpers_http_test.go'
2--- helpers_http_test.go 2013-07-08 10:03:48 +0000
3+++ helpers_http_test.go 2013-08-07 02:15:36 +0000
4@@ -50,8 +50,9 @@
5 // random base64-encoded key.
6 func makeStorageContext(transport http.RoundTripper) *StorageContext {
7 context := &StorageContext{
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/"),
13 }
14 context.client = &http.Client{Transport: transport}
15 return context
16
17=== modified file 'storage_base.go'
18--- storage_base.go 2013-08-06 10:15:28 +0000
19+++ storage_base.go 2013-08-07 02:15:36 +0000
20@@ -210,12 +210,7 @@
21 Key string
22
23 // AzureEndpoint specifies a base service endpoint URL for the Azure APIs.
24- // If this is not set, it will default to the international endpoint which
25- // will not work in mainland China.
26- //
27- // Try to set this if at all possible. Use GetEndpoint() to obtain the
28- // endpoint associated with a given service location, e.g. "West US" or
29- // "North Europe" or "East China".
30+ // This field is required.
31 AzureEndpoint APIEndpoint
32
33 client *http.Client
34@@ -334,20 +329,16 @@
35 // getAccountURL returns the base URL for the context's storage account.
36 // (The result ends in a slash.)
37 func (context *StorageContext) getAccountURL() string {
38- endpoint := context.AzureEndpoint
39- if endpoint == "" {
40- // No API endpoint specified. Default to the international one.
41- // This will not work for mainland China.
42- // TODO: Make this required, and panic() instead.
43- endpoint = GetEndpoint("West US")
44+ if context.AzureEndpoint == APIEndpoint("") {
45+ panic(errors.New("no AzureEndpoint specified in gwacl.StorageContext"))
46 }
47- return endpoint.BlobStorageAPI(context.Account)
48+ return context.AzureEndpoint.BlobStorageAPI(context.Account)
49 }
50
51 // getContainerURL returns the URL for a given storage container.
52 // (The result does not end in a slash.)
53 func (context *StorageContext) getContainerURL(container string) string {
54- return context.getAccountURL() + url.QueryEscape(container)
55+ return strings.TrimRight(context.getAccountURL(), "/") + "/" + url.QueryEscape(container)
56 }
57
58 // GetFileURL returns the URL for a given file in the given container.
59
60=== modified file 'storage_base_test.go'
61--- storage_base_test.go 2013-08-06 04:53:38 +0000
62+++ storage_base_test.go 2013-08-07 02:15:36 +0000
63@@ -6,6 +6,7 @@
64 import (
65 "bytes"
66 "encoding/base64"
67+ "errors"
68 "fmt"
69 "io/ioutil"
70 . "launchpad.net/gocheck"
71@@ -309,33 +310,50 @@
72 "http://"+url.QueryEscape(account)+".blob.example.com")
73 }
74
75-func (*TestStorageContext) TestGetAccountURLDefaultsToInternationalEndpoint(c *C) {
76+func (*TestStorageContext) TestGetAccountURLRequiresEndpoint(c *C) {
77 context := StorageContext{Account: "myaccount"}
78 c.Check(
79- context.getAccountURL(),
80- Equals,
81- "https://myaccount.blob.core.windows.net/")
82+ context.getAccountURL,
83+ Panics,
84+ errors.New("no AzureEndpoint specified in gwacl.StorageContext"))
85 }
86
87-func (suite *TestStorageContext) TestGetContainerURL(c *C) {
88+func (suite *TestStorageContext) TestGetContainerURLAddsContainer(c *C) {
89 account := makeNastyURLUnfriendlyString()
90 container := makeNastyURLUnfriendlyString()
91- context := StorageContext{Account: account}
92+ context := StorageContext{
93+ Account: account,
94+ AzureEndpoint: "http://example.com/",
95+ }
96 c.Check(
97 context.getContainerURL(container),
98 Equals,
99- "https://"+url.QueryEscape(account)+".blob.core.windows.net/"+url.QueryEscape(container))
100+ "http://"+url.QueryEscape(account)+".blob.example.com/"+url.QueryEscape(container))
101+}
102+
103+func (suite *TestStorageContext) TestGetContainerURLAddsSlashIfNeeded(c *C) {
104+ context := StorageContext{
105+ Account: "account",
106+ AzureEndpoint: "http://example.com",
107+ }
108+ c.Check(
109+ context.getContainerURL("container"),
110+ Equals,
111+ "http://account.blob.example.com/container")
112 }
113
114 func (suite *TestStorageContext) TestGetFileURL(c *C) {
115 account := makeNastyURLUnfriendlyString()
116 container := makeNastyURLUnfriendlyString()
117 file := makeNastyURLUnfriendlyString()
118- context := StorageContext{Account: account}
119+ context := StorageContext{
120+ Account: account,
121+ AzureEndpoint: "http://example.com/",
122+ }
123 c.Check(
124 context.GetFileURL(container, file),
125 Equals,
126- "https://"+url.QueryEscape(account)+".blob.core.windows.net/"+url.QueryEscape(container)+"/"+url.QueryEscape(file))
127+ "http://"+url.QueryEscape(account)+".blob.example.com/"+url.QueryEscape(container)+"/"+url.QueryEscape(file))
128 }
129
130 func (suite *TestStorageContext) TestGetSignedFileURL(c *C) {
131@@ -343,7 +361,11 @@
132 container := "container"
133 file := "/a/file"
134 key := base64.StdEncoding.EncodeToString([]byte("dummykey"))
135- context := StorageContext{Account: account, Key: key}
136+ context := StorageContext{
137+ Account: account,
138+ Key: key,
139+ AzureEndpoint: "http://example.com/",
140+ }
141 expires := time.Now()
142
143 signedURL := context.GetAnonymousFileURL(container, file, expires)
144@@ -408,11 +430,12 @@
145 response := makeHttpResponse(http.StatusOK, responseBody)
146 transport := &TestTransport{Response: response}
147 context := makeStorageContext(transport)
148+ context.AzureEndpoint = "http://example.com/"
149 request := &ListContainersRequest{Marker: ""}
150 results, err := context.ListContainers(request)
151 c.Assert(err, IsNil)
152 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
153- "https://%s.blob.core.windows.net/?comp=list", context.Account))
154+ "http://%s.blob.example.com/?comp=list", context.Account))
155 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
156 c.Assert(results, NotNil)
157 c.Assert(results.Containers[0].Name, Equals, "name-value")
158@@ -666,11 +689,12 @@
159 response := makeHttpResponse(http.StatusCreated, "")
160 transport := &TestTransport{Response: response}
161 context := makeStorageContext(transport)
162+ context.AzureEndpoint = "http://example.com/"
163 containerName := MakeRandomString(10)
164 err := context.CreateContainer(containerName)
165 c.Assert(err, IsNil)
166 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
167- "https://%s.blob.core.windows.net/%s?restype=container",
168+ "http://%s.blob.example.com/%s?restype=container",
169 context.Account, containerName))
170 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
171 }
172@@ -720,11 +744,12 @@
173 response := makeHttpResponse(http.StatusAccepted, "")
174 transport := &TestTransport{Response: response}
175 context := makeStorageContext(transport)
176+ context.AzureEndpoint = "http://example.com/"
177 containerName := MakeRandomString(10)
178 err := context.DeleteContainer(containerName)
179 c.Assert(err, IsNil)
180 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
181- "https://%s.blob.core.windows.net/%s?restype=container",
182+ "http://%s.blob.example.com/%s?restype=container",
183 context.Account, containerName))
184 c.Check(transport.Request.Method, Equals, "DELETE")
185 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
186@@ -786,11 +811,12 @@
187
188 transport := &TestTransport{Response: response}
189 context := makeStorageContext(transport)
190+ context.AzureEndpoint = "http://example.com/"
191 containerName := MakeRandomString(10)
192 props, err := context.GetContainerProperties(containerName)
193 c.Assert(err, IsNil)
194 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
195- "https://%s.blob.core.windows.net/%s?restype=container",
196+ "http://%s.blob.example.com/%s?restype=container",
197 context.Account, containerName))
198 c.Check(transport.Request.Method, Equals, "GET")
199 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
200@@ -1099,6 +1125,7 @@
201 response := makeHttpResponse(http.StatusCreated, "")
202 transport := &TestTransport{Response: response}
203 context := makeStorageContext(transport)
204+ context.AzureEndpoint = "http://example.com/"
205 blocklist := &BlockList{}
206 blocklist.Add(BlockListLatest, "b1")
207 blocklist.Add(BlockListLatest, "b2")
208@@ -1107,7 +1134,7 @@
209
210 c.Check(transport.Request.Method, Equals, "PUT")
211 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
212- "https://%s.blob.core.windows.net/container/blobname?comp=blocklist",
213+ "http://%s.blob.example.com/container/blobname?comp=blocklist",
214 context.Account))
215 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
216
217@@ -1340,6 +1367,7 @@
218 response := makeHttpResponse(http.StatusOK, "")
219 transport := &TestTransport{Response: response}
220 context := makeStorageContext(transport)
221+ context.AzureEndpoint = "http://example.com/"
222 err := context.SetContainerACL(&SetContainerACLRequest{
223 Container: "mycontainer", Access: "container"})
224
225@@ -1347,7 +1375,7 @@
226 c.Check(transport.Request.Method, Equals, "PUT")
227 c.Check(transport.Request.URL.String(), Matches,
228 fmt.Sprintf(
229- "https://%s.blob.core.windows.net/mycontainer?.*", context.Account))
230+ "http://%s.blob.example.com/mycontainer?.*", context.Account))
231 c.Check(transport.Request.URL.Query(), DeepEquals, url.Values{
232 "comp": {"acl"},
233 "restype": {"container"},
234
235=== modified file 'storage_test.go'
236--- storage_test.go 2013-08-05 12:01:32 +0000
237+++ storage_test.go 2013-08-07 02:15:36 +0000
238@@ -26,13 +26,14 @@
239 // UploadBlockBlob then sends the list of blocks with PutBlockList.
240 transport.AddExchange(makeFakeCreatedResponse(), nil)
241 // Upload a random blob of data.
242- data := uploadRandomBlob(c, context, 10)
243+ data := uploadRandomBlob(c, context, 10, "MyContainer", "MyFile")
244 // There were two exchanges.
245 c.Assert(transport.ExchangeCount, Equals, 2)
246 // The first request is a Put Block with the block data.
247- assertBlockSent(c, context, data, b64("000000000000000000000000000000"), transport.Exchanges[0])
248+ fileURL := context.GetFileURL("MyContainer", "MyFile")
249+ assertBlockSent(c, context, data, b64("000000000000000000000000000000"), transport.Exchanges[0], fileURL)
250 // The second request is Put Block List to commit the block above.
251- assertBlockListSent(c, context, []string{b64("000000000000000000000000000000")}, transport.Exchanges[1])
252+ assertBlockListSent(c, context, []string{b64("000000000000000000000000000000")}, transport.Exchanges[1], fileURL)
253 }
254
255 func (suite *testUploadBlockBlob) TestLargeFile(c *C) {
256@@ -44,28 +45,29 @@
257 // UploadBlockBlob then sends the list of blocks with PutBlockList.
258 transport.AddExchange(makeFakeCreatedResponse(), nil)
259 // Upload a large random blob of data.
260- data := uploadRandomBlob(c, context, 1348*1024)
261+ data := uploadRandomBlob(c, context, 1348*1024, "MyContainer", "MyFile")
262 // There were three exchanges.
263 c.Assert(transport.ExchangeCount, Equals, 3)
264 // The first two requests are Put Block with chunks of the block data. The
265 // weird looking block IDs are base64 encodings of the strings "0" and "1".
266- assertBlockSent(c, context, data[:1024*1024], b64("000000000000000000000000000000"), transport.Exchanges[0])
267- assertBlockSent(c, context, data[1024*1024:], b64("000000000000000000000000000001"), transport.Exchanges[1])
268+ fileURL := context.GetFileURL("MyContainer", "MyFile")
269+ assertBlockSent(c, context, data[:1024*1024], b64("000000000000000000000000000000"), transport.Exchanges[0], fileURL)
270+ assertBlockSent(c, context, data[1024*1024:], b64("000000000000000000000000000001"), transport.Exchanges[1], fileURL)
271 // The second request is Put Block List to commit the block above.
272- assertBlockListSent(c, context, []string{b64("000000000000000000000000000000"), b64("000000000000000000000000000001")}, transport.Exchanges[2])
273+ assertBlockListSent(c, context, []string{b64("000000000000000000000000000000"), b64("000000000000000000000000000001")}, transport.Exchanges[2], fileURL)
274 }
275
276-func uploadRandomBlob(c *C, context *StorageContext, size int) []byte {
277+func uploadRandomBlob(c *C, context *StorageContext, size int, container, filename string) []byte {
278 data := MakeRandomByteSlice(size)
279 err := context.UploadBlockBlob(
280- "MyContainer", "MyFile", bytes.NewReader(data))
281+ container, filename, bytes.NewReader(data))
282 c.Assert(err, IsNil)
283 return data
284 }
285
286 func assertBlockSent(
287- c *C, context *StorageContext, data []byte, blockID string, exchange *MockingTransportExchange) {
288- c.Check(exchange.Request.URL.String(), Matches, context.GetFileURL("MyContainer", "MyFile")+"?.*")
289+ c *C, context *StorageContext, data []byte, blockID string, exchange *MockingTransportExchange, fileURL string) {
290+ c.Check(exchange.Request.URL.String(), Matches, fileURL+"?.*")
291 c.Check(exchange.Request.URL.Query(), DeepEquals, url.Values{
292 "comp": {"block"},
293 "blockid": {blockID},
294@@ -85,10 +87,11 @@
295 }
296
297 func assertBlockListSent(
298- c *C, context *StorageContext, blockIDs []string, exchange *MockingTransportExchange) {
299- c.Check(exchange.Request.URL.String(), Equals, fmt.Sprintf(
300- "https://%s.blob.core.windows.net/MyContainer/MyFile"+
301- "?comp=blocklist", context.Account))
302+ c *C, context *StorageContext, blockIDs []string, exchange *MockingTransportExchange, fileURL string) {
303+ c.Check(exchange.Request.URL.String(), Matches, fileURL+"?.*")
304+ c.Check(exchange.Request.URL.Query(), DeepEquals, url.Values{
305+ "comp": {"blocklist"},
306+ })
307 body, err := ioutil.ReadAll(exchange.Request.Body)
308 c.Check(err, IsNil)
309 expected := "<BlockList>\n"
310@@ -272,10 +275,11 @@
311 }
312 transport := &TestTransport{Response: response}
313 context := makeStorageContext(transport)
314+ context.AzureEndpoint = "http://example.com/"
315 results, err := context.ListAllContainers()
316 c.Assert(err, IsNil)
317 c.Check(transport.Request.URL.String(), Equals, fmt.Sprintf(
318- "https://%s.blob.core.windows.net/?comp=list", context.Account))
319+ "http://%s.blob.example.com/?comp=list", context.Account))
320 c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
321 c.Assert(results, NotNil)
322 c.Assert(results.Containers[0].Name, Equals, "name-value")
323
324=== modified file 'testing_test.go'
325--- testing_test.go 2013-06-27 13:58:43 +0000
326+++ testing_test.go 2013-08-07 02:15:36 +0000
327@@ -19,6 +19,7 @@
328 transport := &TestTransport{Error: error}
329 client := &http.Client{Transport: transport}
330 context := NewTestStorageContext(client)
331+ context.AzureEndpoint = "http://example.com/"
332 request := &ListContainersRequest{Marker: ""}
333 _, err := context.ListContainers(request)
334 c.Check(err, ErrorMatches, ".*"+errorMessage+".*")

Subscribers

People subscribed via source and target branches