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

[Julian Edwards]

Add the PutPage storage API call

Add the PutPage storage API call.

There's nothing particularly noteworthy here, other than that a lot of the tests are taking on a familiar sheen. My next branch will be to refactor a lot of the test setup into better factory helpers, in particular we need a factory for the storage context with a fake transport. The one that was added the other day isn't helpful and also overlaps with the non-generic one already in storage_base_test.go.

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

118. By Julian Edwards on 2013-06-19

Add reference to API page on web

[Raphaël Badin (rvb)]

Looks good.

[0]

8 +type PutPageRequest struct {
9 + Container string // Container name in the storage account
10 + Filename string // The blob's file name
11 + StartRange int // Must be modulo 512, or an error is returned.
12 + EndRange int // Must be (modulo 512)-1, or an error is returned.
13 + Data io.Reader // The data to upload to the page.
14 +}
15 +
16 +// Send a request to add a range of data into a page blob.
17 +// See http://msdn.microsoft.com/en-us/library/windowsazure/ee691975.aspx
18 +func (context *StorageContext) PutPage(req *PutPageRequest) error {

As discussed this morning, I think this is a good idea to have all our API method take structures as opposed to passing parameters. But I wonder if the container should not be excluded from PutPageRequest, and the signature of the method be something like: "PutPage(container string, req *PutPageRequest)"; this way the PutPageRequest will only capture things related to that request and could potentially be reused across different containers. What do you think?

[1]

// Check the range is set.

I know one ayatollah who would say that you don't need to say "Check" but me, I'm a good lad and I'm happy there is a comment at all ;).

[2]

Maybe this is something that belongs to a follow-up branch but I'd be happy to see this working in the field and thus I'd recommend calling this from example/storage/run.go.

[3]

106 +func (suite *TestPutPage) TestErrorResponse(c *C) {
107 + response := &http.Response{
108 + Status: "102 Frotzed",
109 + StatusCode: 102,
110 + Body: makeResponseBody("<Error><Code>Frotzed</Code><Message>failed to put blob</Message></Error>"),
111 + }
112 + transport := &TestTransport{Response: response}
113 + context := makeStorageContext()
114 + context.client = &http.Client{Transport: transport}
115 + err := context.PutPage(&PutPageRequest{
116 + Container: "container", Filename: "filename", StartRange: 0,
117 + EndRange: 512, Data: nil})
118 + c.Assert(err, NotNil)
119 + c.Check(err, ErrorMatches, ".*102.*")
120 + c.Check(err, ErrorMatches, ".*Frotzed.*")
121 + c.Check(err, ErrorMatches, ".*failed to put blob.*")

This is really a detail but I'd advocate in favor of defining variables with the error status, the error body, etc and do the checks at the end of the methods against these variables instead of duplicating things like "failed to put blob".

review: Approve

[Julian Edwards (julian-edwards)]

On 19/06/13 19:08, Raphaël Badin wrote:
> Review: Approve
>
> Looks good.

Thank you for reviewing.

>
> [0]
>
> 8 +type PutPageRequest struct {
> 9 + Container string // Container name in the storage account
> 10 + Filename string // The blob's file name
> 11 + StartRange int // Must be modulo 512, or an error is returned.
> 12 + EndRange int // Must be (modulo 512)-1, or an error is returned.
> 13 + Data io.Reader // The data to upload to the page.
> 14 +}
> 15 +
> 16 +// Send a request to add a range of data into a page blob.
> 17 +// See http://msdn.microsoft.com/en-us/library/windowsazure/ee691975.aspx
> 18 +func (context *StorageContext) PutPage(req *PutPageRequest) error {
>
> As discussed this morning, I think this is a good idea to have all our API method take structures as opposed to passing parameters. But I wonder if the container should not be excluded from PutPageRequest, and the signature of the method be something like: "PutPage(container string, req *PutPageRequest)"; this way the PutPageRequest will only capture things related to that request and could potentially be reused across different containers. What do you think?

I used to think this but Gavin talked me out of it here:
https://code.launchpad.net/~allenap/gwacl/list-files-by-prefix/+merge/168479

>
> [1]
>
> // Check the range is set.
>
> I know one ayatollah who would say that you don't need to say "Check" but me, I'm a good lad and I'm happy there is a comment at all ;).

ayatollah - lol

>
> [2]
>
> Maybe this is something that belongs to a follow-up branch but I'd be happy to see this working in the field and thus I'd recommend calling this from example/storage/run.go.

Yes, totally, that is my plan. I just didn't want to mix it in here.

>
> [3]
>
> 106 +func (suite *TestPutPage) TestErrorResponse(c *C) {
> 107 + response := &http.Response{
> 108 + Status: "102 Frotzed",
> 109 + StatusCode: 102,
> 110 + Body: makeResponseBody("<Error><Code>Frotzed</Code><Message>failed to put blob</Message></Error>"),
> 111 + }
> 112 + transport := &TestTransport{Response: response}
> 113 + context := makeStorageContext()
> 114 + context.client = &http.Client{Transport: transport}
> 115 + err := context.PutPage(&PutPageRequest{
> 116 + Container: "container", Filename: "filename", StartRange: 0,
> 117 + EndRange: 512, Data: nil})
> 118 + c.Assert(err, NotNil)
> 119 + c.Check(err, ErrorMatches, ".*102.*")
> 120 + c.Check(err, ErrorMatches, ".*Frotzed.*")
> 121 + c.Check(err, ErrorMatches, ".*failed to put blob.*")
>
> This is really a detail but I'd advocate in favor of defining variables with the error status, the error body, etc and do the checks at the end of the methods against these variables instead of duplicating things like "failed to put blob".

This is a copy and paste of some other code. I would like to refactor
all of this in one fell swoop in a separate branch if you don't mind.

Cheers.

=== modified file 'storage_base.go'
--- storage_base.go	2013-06-18 09:18:06 +0000
+++ storage_base.go	2013-06-19 06:58:27 +0000
@@ -450,6 +450,41 @@
     return nil
 }
 
+type PutPageRequest struct {
+    Container  string    // Container name in the storage account
+    Filename   string    // The blob's file name
+    StartRange int       // Must be modulo 512, or an error is returned.
+    EndRange   int       // Must be (modulo 512)-1, or an error is returned.
+    Data       io.Reader // The data to upload to the page.
+}
+
+// Send a request to add a range of data into a page blob.
+// See http://msdn.microsoft.com/en-us/library/windowsazure/ee691975.aspx
+func (context *StorageContext) PutPage(req *PutPageRequest) error {
+    uri := addURLQueryParams(
+        context.getFileURL(req.Container, req.Filename),
+        "comp", "page")
+
+    extraHeaders := http.Header{}
+
+    rangeData := fmt.Sprintf("bytes=%d-%d", req.StartRange, req.EndRange)
+    extraHeaders.Add("x-ms-range", rangeData)
+    extraHeaders.Add("x-ms-page-write", "update")
+
+    _, err := context.performRequest(requestParams{
+        Method:         "PUT",
+        URL:            uri,
+        Body:           req.Data,
+        APIVersion:     "2012-02-12",
+        ExtraHeaders:   extraHeaders,
+        ExpectedStatus: http.StatusCreated,
+    })
+    if err != nil {
+        return fmt.Errorf("failed to put page for file %s: %v", req.Filename, err)
+    }
+    return nil
+}
+
 // Send a request to fetch the list of blocks that have been uploaded as part
 // of a block blob.
 func (context *StorageContext) GetBlockList(container, filename string) (*GetBlockList, error) {

=== modified file 'storage_base_test.go'
--- storage_base_test.go	2013-06-18 09:18:06 +0000
+++ storage_base_test.go	2013-06-19 06:58:27 +0000
@@ -667,6 +667,76 @@
     c.Assert(err, NotNil)
 }
 
+type TestPutPage struct{}
+
+var _ = Suite(&TestPutPage{})
+
+// Basic happy path testing.
+func (suite *TestPutPage) TestHappyPath(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}
+    randomData := MakeRandomByteSlice(10)
+    dataReader := bytes.NewReader(randomData)
+
+    err := context.PutPage(&PutPageRequest{
+        Container: "container", Filename: "filename", StartRange: 0,
+        EndRange: 512, Data: dataReader})
+    c.Assert(err, IsNil)
+
+    // Ensure that container was set right.
+    c.Check(transport.Request.URL.String(), Matches, context.getFileURL("container", "filename")+"?.*")
+    // Ensure that the Authorization header is set.
+    c.Check(transport.Request.Header.Get("Authorization"), Not(Equals), "")
+    // Check the range is set.
+    c.Check(transport.Request.Header.Get("x-ms-range"), Equals, "bytes=0-512")
+    // Check special page write header.
+    c.Check(transport.Request.Header.Get("x-ms-page-write"), Equals, "update")
+    // "?comp=page" should be part of the URL.
+    c.Check(transport.Request.URL.Query(), DeepEquals, url.Values{
+        "comp": {"page"},
+    })
+    // Check the data payload.
+    data, err := ioutil.ReadAll(transport.Request.Body)
+    c.Assert(err, IsNil)
+    c.Check(data, DeepEquals, randomData)
+}
+
+// Client-side errors from the HTTP client are propagated back to the caller.
+func (suite *TestPutPage) TestError(c *C) {
+    cannedError := fmt.Errorf("canned-error")
+    transport := &TestTransport{Error: cannedError}
+    context := makeStorageContext()
+    context.client = &http.Client{Transport: transport}
+    err := context.PutPage(&PutPageRequest{
+        Container: "container", Filename: "filename", StartRange: 0,
+        EndRange: 512, Data: nil})
+    c.Assert(err, NotNil)
+}
+
+// Server-side errors are propagated back to the caller.
+func (suite *TestPutPage) TestErrorResponse(c *C) {
+    response := &http.Response{
+        Status:     "102 Frotzed",
+        StatusCode: 102,
+        Body:       makeResponseBody("<Error><Code>Frotzed</Code><Message>failed to put blob</Message></Error>"),
+    }
+    transport := &TestTransport{Response: response}
+    context := makeStorageContext()
+    context.client = &http.Client{Transport: transport}
+    err := context.PutPage(&PutPageRequest{
+        Container: "container", Filename: "filename", StartRange: 0,
+        EndRange: 512, Data: nil})
+    c.Assert(err, NotNil)
+    c.Check(err, ErrorMatches, ".*102.*")
+    c.Check(err, ErrorMatches, ".*Frotzed.*")
+    c.Check(err, ErrorMatches, ".*failed to put blob.*")
+}
+
 type TestPutBlob struct{}
 
 var _ = Suite(&TestPutBlob{})