Merge lp:~julian-edwards/gwacl/pageblob into lp:gwacl

[Julian Edwards]

Fix PutBlob by making sure it can supply x-ms-blob-type in the request.

The existing PutBlob implementation was half-finished. I think we originally started it with good intentions and then realised it wasn't needed.

This branch finishes the work by enabling the required x-ms-blob-type header. Some changes were needed in performRequest to support this.

We need to use PutBlob to initialise page blobs for the proposed user data solution.

[Jeroen T. Vermeulen (jtv)]

The capitalization of PutBlobRequest looks suspicious: the type is exported but its attributes are not. AFAICS its only use is in PutBlob, but how is anybody outside the module going to be able to create a PutBlobRequest to pass to PutBlob if they can't address its fields?

Is there any value to passing the address of a PutBlobRequest into PutBlob, as opposed to just giving it the struct itself? It just seems like extra weird syntax to live with: &PutBlockRequest{...} instead of just PutBlockRequest{...}. Then again, it's probably consistent with what we did in other places...

By the way, blob_type in Go style becomes blobType, and extra_headers becomes extraHeaders. Not pleasant, especially when it goes against vocal emphasis, but we'd better get used to it.

Once inside PutBlob, you return an error when an invalid blob type is selected. But this isn't variable data, so I'd say this was indicative of a static programming mistake. Might as well panic to teach them a lesson. And if that seems unfair, well, document the thing so they get a chance to do better!

The comment on TestPutBlob, about PutBlob returning Created on success, no longer belongs on top... It's now one of many things that the test looks at.

On TestPutPageBlob, the documenting comment incongruously starts with "Ensure that." Might as well drop that and just state the behaviour that the test verifies as fact.

[Gavin Panella (allenap)]

The blob_type/blobType stuff could be done with a pseudo-enum:

type BlobType int

const (
    PageBlob BlobType = iota
    BlockBlob
)

type PutBlobRequest struct {
    Container string
    BlobType  BlobType  // not sure if it'll like the repeated name.
    Filename  string
}

There's nothing stopping someone from using an int instead of one of
the enum values though, which I think goes to show that I don't have a
full mental model of Go's type system yet.

[lp:~julian-edwards/gwacl/pageblob]

112. By Julian Edwards on 2013-06-18

jtv's review comments addressed

113. By Julian Edwards on 2013-06-18

Add request param comments

114. By Julian Edwards on 2013-06-18

go fmt

[Julian Edwards (julian-edwards)]

On 18/06/13 18:35, Gavin Panella wrote:
> The blob_type/blobType stuff could be done with a pseudo-enum:
>
> type BlobType int
>
> const (
> PageBlob BlobType = iota
> BlockBlob
> )
>
> type PutBlobRequest struct {
> Container string
> BlobType BlobType // not sure if it'll like the repeated name.
> Filename string
> }
>
> There's nothing stopping someone from using an int instead of one of
> the enum values though, which I think goes to show that I don't have a
> full mental model of Go's type system yet.
>

Does this give us any advantage over an obvious string? Other than the
warm fuzzy feeling of knowing that you're embedding into the Pikeverse a
bit more.

[Julian Edwards (julian-edwards)]

On 18/06/13 18:06, Jeroen T. Vermeulen wrote:
> The capitalization of PutBlobRequest looks suspicious: the type is exported but its attributes are not. AFAICS its only use is in PutBlob, but how is anybody outside the module going to be able to create a PutBlobRequest to pass to PutBlob if they can't address its fields?

I forgot about the capitalisation exporting crap. Bugger. Why.... why
does it have to be like that.

>
> Is there any value to passing the address of a PutBlobRequest into PutBlob, as opposed to just giving it the struct itself? It just seems like extra weird syntax to live with: &PutBlockRequest{...} instead of just PutBlockRequest{...}. Then again, it's probably consistent with what we did in other places...

Exactly. I considered passing by value but it would be inconsistent,
and consistency always wins.

>
> By the way, blob_type in Go style becomes blobType, and extra_headers becomes extraHeaders. Not pleasant, especially when it goes against vocal emphasis, but we'd better get used to it.

Grrrrrr!

>
> Once inside PutBlob, you return an error when an invalid blob type is selected. But this isn't variable data, so I'd say this was indicative of a static programming mistake. Might as well panic to teach them a lesson. And if that seems unfair, well, document the thing so they get a chance to do better!

Fair enough!

>
> The comment on TestPutBlob, about PutBlob returning Created on success, no longer belongs on top... It's now one of many things that the test looks at.

Changed.

>
> On TestPutPageBlob, the documenting comment incongruously starts with "Ensure that." Might as well drop that and just state the behaviour that the test verifies as fact.
>

Ok.

Better now? :)

[Jeroen T. Vermeulen (jtv)]

Much better, thanks. (Apart from the capitalization, which is merely more Go-like).

review: Approve

[Julian Edwards (julian-edwards)]

Cheers.

If Gavin wants the enum thing it can be done as a separate branch.

[Julian Edwards (julian-edwards)]

The other option is to remove the check entirely and let the api callsite pass the exact value that Azure needs. I think Gavin took this route on one of his storage-related changes.

=== modified file 'storage_base.go'
--- storage_base.go	2013-06-11 16:31:49 +0000
+++ storage_base.go	2013-06-18 09:20:32 +0000
@@ -216,6 +216,7 @@
     URL            string       // Resource locator, e.g. "http://example.com/my/resource".
     Body           io.Reader    // Optional request body.
     APIVersion     string       // Expected Azure API version, e.g. "2012-02-12".
+    ExtraHeaders   http.Header  // Optional extra request headers.
     Result         Deserializer // Optional object to parse API response into.
     ExpectedStatus HTTPStatus   // Expected response status, e.g. http.StatusOK.
 }
@@ -250,6 +251,12 @@
     if err != nil {
         return nil, err
     }
+    // net/http has no way of adding headers en-masse, hence this abomination.
+    for header, values := range params.ExtraHeaders {
+        for _, value := range values {
+            req.Header.Add(header, value)
+        }
+    }
     addVersionHeader(req, params.APIVersion)
     addDateHeader(req)
     addContentHeaders(req)
@@ -408,18 +415,37 @@
     return nil
 }
 
+type PutBlobRequest struct {
+    Container string // Container name in the storage account
+    BlobType  string // Pass "page" or "block"
+    Filename  string // Filename for the new blob
+}
+
 // Send a request to create a space to upload a blob.  Note that this does not
 // do the uploading, it just makes an empty file.
-func (context *StorageContext) PutBlob(container, filename string) error {
+func (context *StorageContext) PutBlob(req *PutBlobRequest) error {
+    var blobType string
+    switch req.BlobType {
+    case "page":
+        blobType = "PageBlob"
+    case "block":
+        blobType = "BlockBlob"
+    default:
+        panic("block_type must be 'page' or 'block'")
+    }
+
+    extraHeaders := http.Header{}
+    extraHeaders.Add("x-ms-blob-type", blobType)
+
     _, err := context.performRequest(requestParams{
-        Method: "PUT",
-        // TODO: Add blobtype header.
-        URL:            context.getFileURL(container, filename),
+        Method:         "PUT",
+        URL:            context.getFileURL(req.Container, req.Filename),
         APIVersion:     "2012-02-12",
+        ExtraHeaders:   extraHeaders,
         ExpectedStatus: http.StatusCreated,
     })
     if err != nil {
-        return fmt.Errorf("failed to create blob %s: %v", filename, err)
+        return fmt.Errorf("failed to create blob %s: %v", req.Filename, err)
     }
     return nil
 }
 
=== modified file 'storage_base_test.go'
--- storage_base_test.go	2013-06-11 16:34:02 +0000
+++ storage_base_test.go	2013-06-18 09:20:32 +0000
@@ -671,8 +671,8 @@
 
 var _ = Suite(&TestPutBlob{})
 
-// The ListBlobs Storage API call returns 201 CREATED on success.
-func (suite *TestPutBlob) Test(c *C) {
+// Test basic PutBlob happy path functionality.
+func (suite *TestPutBlob) TestPutBlockBlob(c *C) {
     context := makeStorageContext()
     response := &http.Response{
         Status:     fmt.Sprintf("%d", http.StatusCreated),
@@ -680,10 +680,43 @@
     }
     transport := &TestTransport{Response: response}
     context.client = &http.Client{Transport: transport}
-    err := context.PutBlob("container", "blobname")
+    err := context.PutBlob(&PutBlobRequest{
+        Container: "container", BlobType: "block", Filename: "blobname"})
     c.Assert(err, IsNil)
+    // Ensure that container was set right.
     c.Check(transport.Request.URL.String(), Equals, context.getFileURL("container", "blobname"))
+    // Ensure that the Authorization header is set.
     c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
+    // The blob type should be a block.
+    c.Check(transport.Request.Header.Get("x-ms-blob-type"), Equals, "BlockBlob")
+}
+
+// PutBlob should set x-ms-blob-type to PageBlob for Page Blobs.
+func (suite *TestPutBlob) TestPutPageBlob(c *C) {
+    context := makeStorageContext()
+    response := &http.Response{
+        Status:     fmt.Sprintf("%d", http.StatusCreated),
+        StatusCode: http.StatusCreated,
+    }
+    transport := &TestTransport{Response: response}
+    context.client = &http.Client{Transport: transport}
+    err := context.PutBlob(&PutBlobRequest{
+        Container: "container", BlobType: "page", Filename: "blobname"})
+    c.Assert(err, IsNil)
+    c.Check(transport.Request.Header.Get("x-ms-blob-type"), Equals, "PageBlob")
+}
+
+// Passing a BlobType other than page or block results in a panic.
+func (suite *TestPutBlob) TestBlobType(c *C) {
+    defer func() {
+        err := recover()
+        c.Assert(err, Equals, "block_type must be 'page' or 'block'")
+    }()
+    context := makeStorageContext()
+    context.PutBlob(&PutBlobRequest{
+        Container: "container", BlobType: "invalid-blob-type",
+        Filename: "blobname"})
+    c.Assert("This should have panicked", Equals, "But it didn't.")
 }
 
 // Client-side errors from the HTTP client are propagated back to the caller.
@@ -692,7 +725,8 @@
     transport := &TestTransport{Error: error}
     context := makeStorageContext()
     context.client = &http.Client{Transport: transport}
-    err := context.PutBlob("container", "blobname")
+    err := context.PutBlob(&PutBlobRequest{
+        Container: "container", BlobType: "block", Filename: "blobname"})
     c.Assert(err, NotNil)
 }
 
@@ -706,7 +740,8 @@
     transport := &TestTransport{Response: response}
     context := makeStorageContext()
     context.client = &http.Client{Transport: transport}
-    err := context.PutBlob("container", "blobname")
+    err := context.PutBlob(&PutBlobRequest{
+        Container: "container", BlobType: "block", Filename: "blobname"})
     c.Assert(err, NotNil)
     c.Check(err, ErrorMatches, ".*102.*")
     c.Check(err, ErrorMatches, ".*Frotzed.*")