Merge lp:~rvb/gwacl/delete-404 into lp:gwacl

[Raphaël Badin]

Deleting a non-existant blob does not return an error.

[lp:~rvb/gwacl/delete-404]

126. By Raphaël Badin on 2013-06-21

Tiny consistency fix.

[Gavin Panella (allenap)]

review: Approve

[Jeroen T. Vermeulen (jtv)]

Looks good, and of course HTTP DELETE is meant to be idempotent.

The comment in TestServerError is very welcome for review, but possibly a bit superfluous as a part of the code. If you do keep it, two general notes about the comment:

1. I'd avoid the preamble "note that" in comments. It can liven up longer texts, but is redundant in a code comment. In this case I'd say it calls undue attention to something of relatively little importance.

2. Language easily contorts around the passive voice. "X is done by Y" is usually worse than "Y does X."

review: Approve

[lp:~rvb/gwacl/delete-404]

127. By Raphaël Badin on 2013-06-21

Improve comment.

[Raphaël Badin (rvb)]

 > 1. I'd avoid the preamble "note that" in comments. It can liven up longer
> texts, but is redundant in a code comment. In this case I'd say it calls
> undue attention to something of relatively little importance.

All right, this is not the first you've mentioned this.

> 2. Language easily contorts around the passive voice. "X is done by Y" is
> usually worse than "Y does X."

Well, you really seem to have something against the passive voice :).

[Julian Edwards (julian-edwards)]

s/existant/existent/

Surprised the other spelling police didn't notice this.

=== modified file 'storage_base.go'
--- storage_base.go	2013-06-21 11:27:17 +0000
+++ storage_base.go	2013-06-21 15:08:26 +0000
@@ -567,7 +567,8 @@
     return nil
 }
 
-// Delete the specified blob from the given container.
+// Delete the specified blob from the given container.  Deleting a non-existant
+// blob will return without an error.
 func (context *StorageContext) DeleteBlob(container, filename string) error {
     _, err := context.performRequest(requestParams{
         Method:         "DELETE",
@@ -575,9 +576,12 @@
         APIVersion:     "2012-02-12",
         ExpectedStatus: http.StatusAccepted,
     })
-    // TODO Handle a 404 with an <Error>BlobNotFound response body silently.
-    // Now this is easy to fix with the method IsNotFoundError().
     if err != nil {
+        // If the error is an Azure 404 error, return silently: the blob does
+        // not exist.
+        if IsNotFoundError(err) {
+            return nil
+        }
         msg := fmt.Sprintf("failed to delete blob %s: ", filename)
         return extendError(err, msg)
     }

=== modified file 'storage_base_test.go'
--- storage_base_test.go	2013-06-21 11:27:17 +0000
+++ storage_base_test.go	2013-06-21 15:08:26 +0000
@@ -1010,12 +1010,22 @@
 // Azure HTTP errors (for instance 404 responses) are propagated back to
 // the caller as ServerError objects.
 func (suite *TestDeleteBlob) TestServerError(c *C) {
-    response := makeHttpResponse(http.StatusNotFound, "not found")
+    // We're not using http.StatusNotFound for the test here because
+    // 404 errors are handled in a special way by DeleteBlob().  See the test
+    // TestDeleteNotExistantBlobDoesNotFail.
+    response := makeHttpResponse(http.StatusMethodNotAllowed, "not allowed")
     context := makeStorageContext(&TestTransport{Response: response})
     err := context.DeleteBlob("container", "blobname")
     serverError, ok := err.(*ServerError)
     c.Check(ok, Equals, true)
-    c.Check(serverError.HTTPStatus.StatusCode(), Equals, http.StatusNotFound)
+    c.Check(serverError.HTTPStatus.StatusCode(), Equals, http.StatusMethodNotAllowed)
+}
+
+func (suite *TestDeleteBlob) TestDeleteNotExistantBlobDoesNotFail(c *C) {
+    response := makeHttpResponse(http.StatusNotFound, "not found")
+    context := makeStorageContext(&TestTransport{Response: response})
+    err := context.DeleteBlob("container", "blobname")
+    c.Assert(err, IsNil)
 }
 
 // Server-side errors are propagated back to the caller.