Merge lp:~jtv/gwacl/factor-storage-requests into lp:gwacl

Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Jeroen T. Vermeulen
Approved revision: 111
Merged at revision: 103
Proposed branch: lp:~jtv/gwacl/factor-storage-requests
Merge into: lp:gwacl
Diff against target: 416 lines (+149/-117)
2 files modified
storage_base.go (+147/-116)
storage_base_test.go (+2/-1)
To merge this branch: bzr merge lp:~jtv/gwacl/factor-storage-requests
Reviewer Review Type Date Requested Status
Julian Edwards (community) Approve
Gavin Panella Approve
Review via email: mp+161617@code.launchpad.net

Commit message

Factor out "perform HTTP request" method in StorageContext.

Description of the change

This combines a few tightly-related refactorings in the storage-API code (mostly in separate commits):

 * Move signRequest() into StorageAPI.

 * Move the signRequest() call out of addStandardheaders().

 * Fix up a few bits of dead code where call sites tried to check for unexpected HTTP statuses — after send() would already have returned an error.

 * Factor out a performRequest() method, with a parameter-object struct, to encapsulate the repeated code to perform HTTP requests.

The parameter-object initializer makes the calls to performRequest() rather verbose, but I think it's better than passing lots of positional arguments, especially when several are of the same type and easily confused. Personally I find named arguments preferable to positional ones once you have more than four of them. It would be possible to move some of the required fields into the call as positional parameters, but it would become either an arbitrary selection, or if we move all required fields, we'd be back to the long parameter list. In which case the parameter object would just make things more irregular so we'd want to get rid of it, and we'd have truly ridiculously long argument lists.

Some small changes in addition to strict refactoring:

 * Document that you can't change a request further after signing — I lost some time to that trap in Brisbane, since signing was hidden inside addStandardHeaders().

 * Added some more detail to error messages.

To help keep future development smooth I also added some sanity-checking to the parameter object. No, it's not technically needed, but it helped make sure that I didn't make any of the obvious mistakes during refactoring — without having to test the same things again for every call. We can take it out now if we want, but I figure it's an easy way to find out about mistakes sooner.

Jeroen

To post a comment you must log in.
110. By Jeroen T. Vermeulen

Removed a bit of Java-like silliness.

Revision history for this message
Gavin Panella (allenap) wrote :

Looks good.

[1]

+func (params *requestParams) Check() {

This isn't used anywhere.

[2]

+func (context *StorageContext) performRequest(params requestParams) ...

So, does this mean that a copy of requestParams gets passed in? Does
that mean that copies of params' fields are made too? Body is an
io.Reader, so does that mean it's always a pointer/reference type, and
so only the pointer is copied? Same with Deserializer.

[3]

Is it worth keeping send() distinct from performRequest()?

review: Approve
Revision history for this message
Julian Edwards (julian-edwards) wrote :

Very nice change, thanks Jeroen.

75 + methods := map[string]bool{"GET": true, "PUT": true, "POST": true, "DELETE": true}
76 + if _, ok := methods[params.Method]; !ok {

This could possibly be the best 2 lines of Go I've ever seen :)

81 +// performRequest issues an HTTP request to Azure.
82 +func (context *StorageContext) performRequest(params requestParams) (*http.Response, error) {

Make sure you take a pointer for params here, we don't want to copy them. Been doing Python lately? :)
Also you forgot to call Check().

On another note, I wish there were a way of linking the doc for params into the doc for performRequest. Param descriptions are nowhere near the function :( This is a huge problem with godoc (I believe I contributed that bit to Gripes).

141 + _, err := context.performRequest(requestParams{
142 + Method: "GET",
143 + URL: uri,
144 + APIVersion: "2012-02-12",
145 + Result: &containers,
146 + ExpectedStatus: http.StatusOK,
147 + })

You're right - for long lists of params this style is hugely better than positional args. I really miss named parameters in Go :(

BTW I have an idea for error handling in the library. I will email the team about it so we don't cloud the MP here.

cheers

review: Approve
111. By Jeroen T. Vermeulen

Review change: forgot to call requestParams.Check().

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :
Download full text (4.0 KiB)

> [1]
>
> +func (params *requestParams) Check() {
>
> This isn't used anywhere.

Thanks for spotting that. I added the one call that was supposed to be there, right at the top of performRequest().

> [2]
>
> +func (context *StorageContext) performRequest(params requestParams) ...
>
> So, does this mean that a copy of requestParams gets passed in? Does
> that mean that copies of params' fields are made too? Body is an
> io.Reader, so does that mean it's always a pointer/reference type, and
> so only the pointer is copied? Same with Deserializer.

That's right. The io.Reader and Deserializer types are interfaces that wrap pointers, so we get to court disaster with Go's fragile type system; there isn't much we can do about that.

The requestParams object, on the other hand, is passed by value. I feel this is appropriate for an object that doesn't go through a series of stateful changes.

That may sound strange. You may think "but passing a pointer is more efficient, right?" But even if it's not particularly relevant when speaking http[s] across the internet, that's not necessarily the case, for fairly complicated reasons. I'll try to explain these not because they matter much here, but because they may put a new perspective on other situations you come across in the future.

To begin with, a smart compiler can detect that the function does not modify the parameter, and so pass it by reference if it that seems more efficient. Or it may pass parts of the object in registers, instead of writing to memory and possibly pushing data out of cache in the process. The trade-off for how to pass the parameter could shift depending on architecture, targeted CPU model, estimated cache pressure, prevalent optimization goals, changes in the compiler, profiling feedback... More than you'd care to tweak your code for every time.

The key thing is that the compiler has to know exactly what goes on with the object. If you pass an object by value, it's easy to see that no changes to the object will "flow back" to the caller. That may also allow the compiler to prove that the caller won't modify the object either, and this gives it the freedom to pass by reference in more situations. Similarly, it's hard to registerize (parts of) the object if there may be pointers to the object through which it may be modified, or through which it should be possible to observe changes. Whenever the compiler is not sure about these things (or when it can't be bothered to find out), it does the safe thing which is also the inefficient thing — with pass-by-reference, that means treating every potentially mutable item as location in memory instead of as a value that can be optimized in all sorts of ways.

Usually it's easy to see whether there may be other references to an object, but this "aliasing problem" as it's called is not generally computable.

So be kind to your compiler. Don't micro-manage it. Trust it, and give it the information it needs to do its job well. You do that by minimizing the scope of what "can" happen to an item. Copy by value where you can. In languages that have it, avoid pointer arithmetic. Try to avoid creating pointers to objects as ...

Read more...

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

> 75 + methods := map[string]bool{"GET": true, "PUT": true, "POST":
> true, "DELETE": true}
> 76 + if _, ok := methods[params.Method]; !ok {
>
> This could possibly be the best 2 lines of Go I've ever seen :)

Thank you!

It means “if params.Method not in {"GET", "PUT", "POST", "DELETE"}:”

Really goes to show how Go was designed to put the fun back in programming. Expressing this complex problem reasonably well kept me amused for a while.

Oh, you wanted fun while *reading* the code? Too bad. Put on some music or something.

> 81 +// performRequest issues an HTTP request to Azure.
> 82 +func (context *StorageContext) performRequest(params requestParams)
> (*http.Response, error) {
>
> Make sure you take a pointer for params here, we don't want to copy them.
> Been doing Python lately? :)
> Also you forgot to call Check().

Yes, I forgot to call Check(), but too late! Gavin spotted that in his review 9—10 hours earlier. Sometimes you do need to refresh merge proposals in your browser. :)

The copying is deliberate, harmless, and beneficial. See my explanation to Gavin above.

.

> On another note, I wish there were a way of linking the doc for params into
> the doc for performRequest. Param descriptions are nowhere near the function
> :( This is a huge problem with godoc (I believe I contributed that bit to
> Gripes).

Actually, I don't think I have that particular point there. Might be worth having a separate section about godoc, if you care to provide the material. I haven't built up much experience with godoc myself, because I haven't seen any use for it yet. From my point of view it's just been a roundabout way of reading a function's comment without seeing its body.

Revision history for this message
Julian Edwards (julian-edwards) wrote :

FWIW I think you are giving Go's compiler far too much benefit. Why risk it
not getting optimised when passing a pointer will definitely not copy
anything. And who's to say it won't optimise that, too?

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :
Download full text (4.9 KiB)

> FWIW I think you are giving Go's compiler far too much benefit. Why risk it
> not getting optimised when passing a pointer will definitely not copy
> anything. And who's to say it won't optimise that, too?

How much experience do you have with compiler back-ends? Compilers improve over time, and absent a very direct immediate need you're better off letting them optimize your code better in the future, than trying to clamp down on every risk of something not getting optimized now — and interfering with both future maintenance and future optimization. This is no different from when some people kept using "goto" because loops in the new structured-programming style were slower. Soon the compilers were tiling, fusing, fissioning, strip-mining, unrolling, and parallellizing the structured loops to stupendous effect... but skipping the parts with goto in them because those were icky and potentially hard to figure out.

Also, if you study compiler optimization you will find that passing a pointer definitely can cause excessive copying, and other costs, because of the aliasing problem. It just doesn't fit into a naïve, 1970s mental model of how compiled code works. Go may be in many ways a 1970s language, but much of this is now standard fare even for a young compiler. Plus there's a gcc front-end for Go, and gcc definitely has mature optimizations.

Where do the costs come from? Seemingly everywhere. Every time the compiler wants to access a variable, it needs to re-read that variable from memory — unless it can rule out aliasing, in which case it can registerize. Reloading the variable on demand every time is a form of copying, without the efficiencies of a full object copy. And if the access changes the variable, the same goes for writing the new value back to memory afterwards. The register allocator gets less freedom to make optimal choices. Trivial operations can't be optimized away if they put an object through intermediate states that may be observed through aliasing. The scheduler is much more constrained, which also eats away at the benefits you get from inlining. Escape analysis is less successful, so more objects need to be allocated on the heap that could have been allocated on the stack and/or registerized. More objects on the heap means more work for the garbage collector, more lock contention in the memory allocator, less cache locality.

You ask who's to say that the compiler won't optimise away pass-by-reference. Nothing obscures the compiler's view of what happens to an object quite like passing its address around. That's what usually creates aliases. It's harder for a compiler (and as I said, technically incomputable) to prove that aliasing is harmless in a given case, than it is to optimize away an object copy in cases where there is no aliasing in the first place. The two are not symmetrical.

Maybe it's best if I give you an example. Imagine you're the compiler. Consider this pseudocode. (If it looks silly, imagine it being the outcome of earlier optimization stages such as inlining. Imagine the computations being more interesting. Imagine much of this happening in a performance-critical loop. Any...

Read more...

Revision history for this message
Julian Edwards (julian-edwards) wrote :

So what you're saying is that Go is missing const pointer args ;)

Revision history for this message
Julian Edwards (julian-edwards) wrote :

OK I'll stop trolling now!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'storage_base.go'
2--- storage_base.go 2013-04-30 09:51:53 +0000
3+++ storage_base.go 2013-05-01 05:20:37 +0000
4@@ -16,6 +16,7 @@
5 "crypto/hmac"
6 "crypto/sha256"
7 "encoding/base64"
8+ "errors"
9 "fmt"
10 "io"
11 "io/ioutil"
12@@ -40,12 +41,6 @@
13 "Range",
14 }
15
16-// Add the Authorization: header to a Request.
17-func signRequest(req *http.Request, account_name, account_key string) {
18- header := composeAuthHeader(req, account_name, account_key)
19- req.Header.Set("Authorization", header)
20-}
21-
22 // Calculate the value required for an Authorization header.
23 func composeAuthHeader(req *http.Request, account_name, account_key string) string {
24 signable := composeStringToSign(req, account_name)
25@@ -184,12 +179,12 @@
26 req.Header.Set("Date", now)
27 }
28
29-// Convenience function to add mandatory headers required in every request.
30-func addStandardHeaders(req *http.Request, account, key, ms_version string) {
31- addVersionHeader(req, ms_version)
32- addDateHeader(req)
33- addContentHeaders(req)
34- signRequest(req, account, key)
35+// signRequest adds the Authorization: header to a Request.
36+// Don't make any further changes to the request before sending it, or the
37+// signature will not be valid.
38+func (context *StorageContext) signRequest(req *http.Request) {
39+ header := composeAuthHeader(req, context.Account, context.Key)
40+ req.Header.Set("Authorization", header)
41 }
42
43 // StorageContext keeps track of the mandatory parameters required to send a
44@@ -215,6 +210,53 @@
45 Deserialize([]byte) error
46 }
47
48+// requestParams is a Parameter Object for performRequest().
49+type requestParams struct {
50+ Method string // HTTP method, e.g. "GET" or "PUT".
51+ URL string // Resource locator, e.g. "http://example.com/my/resource".
52+ Body io.Reader // Optional request body.
53+ APIVersion string // Expected Azure API version, e.g. "2012-02-12".
54+ Result Deserializer // Optional object to parse API response into.
55+ ExpectedStatus HTTPStatus // Expected response status, e.g. http.StatusOK.
56+}
57+
58+// Check performs a basic sanity check on the request. This will only catch
59+// a few superficial problems that you can spot at compile time, to save a
60+// debugging cycle for the most basic mistakes.
61+func (params *requestParams) Check() {
62+ const panicPrefix = "invalid request: "
63+ if params.Method == "" {
64+ panic(errors.New(panicPrefix + "HTTP method not specified"))
65+ }
66+ if params.URL == "" {
67+ panic(errors.New(panicPrefix + "URL not specified"))
68+ }
69+ if params.APIVersion == "" {
70+ panic(errors.New(panicPrefix + "API version not specified"))
71+ }
72+ if params.ExpectedStatus == 0 {
73+ panic(errors.New(panicPrefix + "expected HTTP status not specified"))
74+ }
75+ methods := map[string]bool{"GET": true, "PUT": true, "POST": true, "DELETE": true}
76+ if _, ok := methods[params.Method]; !ok {
77+ panic(fmt.Errorf(panicPrefix+"unsupported HTTP method '%s'", params.Method))
78+ }
79+}
80+
81+// performRequest issues an HTTP request to Azure.
82+func (context *StorageContext) performRequest(params requestParams) (*http.Response, error) {
83+ params.Check()
84+ req, err := http.NewRequest(params.Method, params.URL, params.Body)
85+ if err != nil {
86+ return nil, err
87+ }
88+ addVersionHeader(req, params.APIVersion)
89+ addDateHeader(req)
90+ addContentHeaders(req)
91+ context.signRequest(req)
92+ return context.send(req, params.Result, params.ExpectedStatus)
93+}
94+
95 // Send a request to the storage service and process the response.
96 // The "res" parameter is typically an XML struct that will deserialize the
97 // raw XML into the struct data. The http Response object is returned.
98@@ -224,7 +266,7 @@
99 // is an HTTPError, the request response is also returned. In other error
100 // cases, the returned response may be the one received from the server or
101 // it may be nil.
102-func (context *StorageContext) send(req *http.Request, res Deserializer, expected_status int) (*http.Response, error) {
103+func (context *StorageContext) send(req *http.Request, res Deserializer, expected_status HTTPStatus) (*http.Response, error) {
104 client := context.getClient()
105 resp, err := client.Do(req)
106 if err != nil {
107@@ -233,7 +275,7 @@
108
109 var data []byte
110
111- if resp.StatusCode != expected_status {
112+ if resp.StatusCode != int(expected_status) {
113 if resp.Body != nil {
114 data, err = ioutil.ReadAll(resp.Body)
115 if err != nil {
116@@ -250,7 +292,7 @@
117 return resp, nil
118 }
119
120- // TODO: deserialize response headers into the "res" object.
121+ // TODO: Also deserialize response headers into the "res" object.
122 data, err = ioutil.ReadAll(resp.Body)
123 if err != nil {
124 msg := fmt.Errorf("failed to read response data: %s", err)
125@@ -290,22 +332,21 @@
126 // subsequent calls to get additional batches of the same result, pass the
127 // NextMarker from the previous call's result.
128 func (context *StorageContext) getListContainersBatch(marker string) (*ContainerEnumerationResults, error) {
129- uri := context.getAccountURL()
130- uri = addURLQueryParams(uri, "comp", "list")
131+ uri := addURLQueryParams(context.getAccountURL(), "comp", "list")
132 if marker != "" {
133 uri = addURLQueryParams(uri, "marker", marker)
134 }
135- req, err := http.NewRequest("GET", uri, nil)
136- if err != nil {
137- return nil, err
138- }
139- addStandardHeaders(req, context.Account, context.Key, "2012-02-12")
140 containers := ContainerEnumerationResults{}
141- _, err = context.send(req, &containers, http.StatusOK)
142+ _, err := context.performRequest(requestParams{
143+ Method: "GET",
144+ URL: uri,
145+ APIVersion: "2012-02-12",
146+ Result: &containers,
147+ ExpectedStatus: http.StatusOK,
148+ })
149 if err != nil {
150- return nil, err
151+ return nil, fmt.Errorf("request for containers list failed: %v", err)
152 }
153-
154 return &containers, nil
155 }
156
157@@ -348,22 +389,25 @@
158 // subsequent calls to get additional batches of the same result, pass the
159 // NextMarker from the previous call's result.
160 func (context *StorageContext) getListBlobsBatch(container, marker string) (*BlobEnumerationResults, error) {
161- uri := context.getContainerURL(container)
162- uri = addURLQueryParams(
163- uri,
164+ uri := addURLQueryParams(
165+ context.getContainerURL(container),
166 "restype", "container",
167 "comp", "list")
168 if marker != "" {
169 uri = addURLQueryParams(uri, "marker", marker)
170 }
171- req, err := http.NewRequest("GET", uri, nil)
172+ blobs := BlobEnumerationResults{}
173+ _, err := context.performRequest(requestParams{
174+ Method: "GET",
175+ URL: uri,
176+ APIVersion: "2012-02-12",
177+ Result: &blobs,
178+ ExpectedStatus: http.StatusOK,
179+ })
180 if err != nil {
181- return nil, err
182+ return nil, fmt.Errorf("request for blobs list failed: %v", err)
183 }
184- addStandardHeaders(req, context.Account, context.Key, "2012-02-12")
185- blob := &BlobEnumerationResults{}
186- _, err = context.send(req, blob, http.StatusOK)
187- return blob, err
188+ return &blobs, err
189 }
190
191 // ListBlobs requests from the API a list of blobs in a container.
192@@ -402,16 +446,17 @@
193 // Send a request to the storage service to create a new container. If the
194 // request fails, error is non-nil.
195 func (context *StorageContext) CreateContainer(container string) error {
196- uri := context.getContainerURL(container)
197- uri = addURLQueryParams(uri, "restype", "container")
198- req, err := http.NewRequest("PUT", uri, nil)
199+ uri := addURLQueryParams(
200+ context.getContainerURL(container),
201+ "restype", "container")
202+ _, err := context.performRequest(requestParams{
203+ Method: "PUT",
204+ URL: uri,
205+ APIVersion: "2012-02-12",
206+ ExpectedStatus: http.StatusCreated,
207+ })
208 if err != nil {
209- return err
210- }
211- addStandardHeaders(req, context.Account, context.Key, "2012-02-12")
212- _, errmsg := context.send(req, nil, http.StatusCreated)
213- if errmsg != nil {
214- return errmsg
215+ return fmt.Errorf("failed to create container %s: %v", container, err)
216 }
217 return nil
218 }
219@@ -419,16 +464,15 @@
220 // Send a request to create a space to upload a blob. Note that this does not
221 // do the uploading, it just makes an empty file.
222 func (context *StorageContext) PutBlob(container, filename string) error {
223- // TODO: Add blobtype header.
224- uri := context.getFileURL(container, filename)
225- req, err := http.NewRequest("PUT", uri, nil)
226+ _, err := context.performRequest(requestParams{
227+ Method: "PUT",
228+ // TODO: Add blobtype header.
229+ URL: context.getFileURL(container, filename),
230+ APIVersion: "2012-02-12",
231+ ExpectedStatus: http.StatusCreated,
232+ })
233 if err != nil {
234- return err
235- }
236- addStandardHeaders(req, context.Account, context.Key, "2012-02-12")
237- _, errmsg := context.send(req, nil, http.StatusCreated)
238- if errmsg != nil {
239- return errmsg
240+ return fmt.Errorf("failed to create blob %s: %v", filename, err)
241 }
242 return nil
243 }
244@@ -436,23 +480,21 @@
245 // Send a request to fetch the list of blocks that have been uploaded as part
246 // of a block blob.
247 func (context *StorageContext) GetBlockList(container, filename string) (*GetBlockList, error) {
248- uri := context.getFileURL(container, filename)
249- uri = addURLQueryParams(
250- uri,
251+ uri := addURLQueryParams(
252+ context.getFileURL(container, filename),
253 "comp", "blocklist",
254 "blocklisttype", "all")
255-
256- req, err := http.NewRequest("GET", uri, nil)
257- if err != nil {
258- return nil, err
259- }
260- addStandardHeaders(req, context.Account, context.Key, "2012-02-12")
261 bl := GetBlockList{}
262- _, errmsg := context.send(req, &bl, http.StatusOK)
263- if errmsg != nil {
264- return nil, errmsg
265+ _, err := context.performRequest(requestParams{
266+ Method: "GET",
267+ URL: uri,
268+ APIVersion: "2012-02-12",
269+ Result: &bl,
270+ ExpectedStatus: http.StatusOK,
271+ })
272+ if err != nil {
273+ return nil, fmt.Errorf("request for block list in file %s failed: %v", filename, err)
274 }
275-
276 return &bl, nil
277 }
278
279@@ -460,83 +502,72 @@
280 // data block to upload.
281 func (context *StorageContext) PutBlock(container, filename, id string, data io.Reader) error {
282 base64_id := base64.StdEncoding.EncodeToString([]byte(id))
283- uri := context.getFileURL(container, filename)
284- uri = addURLQueryParams(
285- uri,
286+ uri := addURLQueryParams(
287+ context.getFileURL(container, filename),
288 "comp", "block",
289 "blockid", base64_id)
290- req, err := http.NewRequest("PUT", uri, data)
291- if err != nil {
292- return err
293- }
294- addStandardHeaders(req, context.Account, context.Key, "2012-02-12")
295- resp, err := context.send(req, nil, http.StatusCreated)
296- if err != nil {
297- return err
298- }
299- if resp.StatusCode != http.StatusCreated {
300- return fmt.Errorf("failed to put block: %s", resp.Status)
301+ _, err := context.performRequest(requestParams{
302+ Method: "PUT",
303+ URL: uri,
304+ Body: data,
305+ APIVersion: "2012-02-12",
306+ ExpectedStatus: http.StatusCreated,
307+ })
308+ if err != nil {
309+ return fmt.Errorf("failed to put block %s for file %s: %v", id, filename, err)
310 }
311 return nil
312 }
313
314 // Send a request to piece together blocks into a list that specifies a blob.
315 func (context *StorageContext) PutBlockList(container, filename string, blocklist *BlockList) error {
316- uri := context.getFileURL(container, filename)
317- uri = addURLQueryParams(uri, "comp", "blocklist")
318+ uri := addURLQueryParams(
319+ context.getFileURL(container, filename),
320+ "comp", "blocklist")
321 data, err := blocklist.Serialize()
322 if err != nil {
323 return err
324 }
325 data_reader := bytes.NewReader(data)
326- req, err := http.NewRequest("PUT", uri, data_reader)
327- if err != nil {
328- return err
329- }
330- addStandardHeaders(req, context.Account, context.Key, "2012-02-12")
331- resp, err := context.send(req, nil, http.StatusCreated)
332- if err != nil {
333- return err
334- }
335- if resp.StatusCode != http.StatusCreated {
336- return fmt.Errorf("failed to put blocklist: %s", resp.Status)
337+
338+ _, err = context.performRequest(requestParams{
339+ Method: "PUT",
340+ URL: uri,
341+ Body: data_reader,
342+ APIVersion: "2012-02-12",
343+ ExpectedStatus: http.StatusCreated,
344+ })
345+ if err != nil {
346+ return fmt.Errorf("failed to put blocklist for file %s: %v", filename, err)
347 }
348 return nil
349 }
350
351 // Delete the specified blob from the given container.
352 func (context *StorageContext) DeleteBlob(container, filename string) error {
353- uri := context.getFileURL(container, filename)
354- req, err := http.NewRequest("DELETE", uri, nil)
355- if err != nil {
356- return err
357- }
358- addStandardHeaders(req, context.Account, context.Key, "2012-02-12")
359- resp, err := context.send(req, nil, http.StatusAccepted)
360+ _, err := context.performRequest(requestParams{
361+ Method: "DELETE",
362+ URL: context.getFileURL(container, filename),
363+ APIVersion: "2012-02-12",
364+ ExpectedStatus: http.StatusAccepted,
365+ })
366 // TODO Handle a 404 with an <Error>BlobNotFound response body silently.
367 if err != nil {
368- return err
369- }
370- if resp.StatusCode != http.StatusAccepted {
371- return fmt.Errorf("failed to delete blob: %s", resp.Status)
372+ return fmt.Errorf("failed to delete blob %s: %v", filename, err)
373 }
374 return nil
375 }
376
377 // Get the specified blob from the given container.
378 func (context *StorageContext) GetBlob(container, filename string) (io.ReadCloser, error) {
379- uri := context.getFileURL(container, filename)
380- req, err := http.NewRequest("GET", uri, nil)
381+ response, err := context.performRequest(requestParams{
382+ Method: "GET",
383+ URL: context.getFileURL(container, filename),
384+ APIVersion: "2012-02-12",
385+ ExpectedStatus: http.StatusOK,
386+ })
387 if err != nil {
388- return nil, err
389- }
390- addStandardHeaders(req, context.Account, context.Key, "2012-02-12")
391- resp, errmsg := context.send(req, nil, http.StatusOK)
392- if errmsg != nil {
393- return nil, errmsg
394- }
395- if resp.StatusCode != http.StatusOK {
396- return nil, fmt.Errorf("failed to get blob: %s", resp.Status)
397- }
398- return resp.Body, nil
399+ return nil, fmt.Errorf("failed to get blob %s: %v", filename, err)
400+ }
401+ return response.Body, nil
402 }
403
404=== modified file 'storage_base_test.go'
405--- storage_base_test.go 2013-04-30 08:42:44 +0000
406+++ storage_base_test.go 2013-05-01 05:20:37 +0000
407@@ -180,7 +180,8 @@
408 c.Assert(req.Header.Get("Authorization"), Equals, "")
409
410 key := base64.StdEncoding.EncodeToString([]byte("dummykey"))
411- signRequest(req, "myname", key)
412+ context := StorageContext{client: nil, Account: "myname", Key: key}
413+ context.signRequest(req)
414
415 expected := "SharedKey myname:Xf9hWQ99mM0IyEOL6rNeAUdTQlixVqiYnt2TpLCCpY0="
416 c.Assert(req.Header.Get("Authorization"), Equals, expected)

Subscribers

People subscribed via source and target branches