fix(storage): revise Reader to send XML preconditions (#4479)
This updates `Reader` to send precondition request headers to fit the XML API used for downloads.
- adds a new function `setConditionsHeaders()` that parses `Conditions` and sets the corresponding precondition headers
- revises `NewRangerReader` to call `setConditionsHeaders()`
- if an object version is specified via `generation`, include `generation` as query string parameters
- updates tests
Fixes #4470
diff --git a/storage/integration_test.go b/storage/integration_test.go
index 7ea61e3..82745f6 100644
--- a/storage/integration_test.go
+++ b/storage/integration_test.go
@@ -744,6 +744,36 @@
}
}
+func TestIntegration_ConditionalDownload(t *testing.T) {
+ ctx := context.Background()
+ client := testConfig(ctx, t)
+ defer client.Close()
+ h := testHelper{t}
+
+ o := client.Bucket(bucketName).Object("condread")
+ defer o.Delete(ctx)
+
+ wc := o.NewWriter(ctx)
+ wc.ContentType = "text/plain"
+ h.mustWrite(wc, []byte("foo"))
+
+ gen := wc.Attrs().Generation
+ metaGen := wc.Attrs().Metageneration
+
+ if _, err := o.Generation(gen + 1).NewReader(ctx); err == nil {
+ t.Fatalf("Unexpected successful download with nonexistent Generation")
+ }
+ if _, err := o.If(Conditions{MetagenerationMatch: metaGen + 1}).NewReader(ctx); err == nil {
+ t.Fatalf("Unexpected successful download with failed preconditions IfMetaGenerationMatch")
+ }
+ if _, err := o.If(Conditions{GenerationMatch: gen + 1}).NewReader(ctx); err == nil {
+ t.Fatalf("Unexpected successful download with failed preconditions IfGenerationMatch")
+ }
+ if _, err := o.If(Conditions{GenerationMatch: gen}).NewReader(ctx); err != nil {
+ t.Fatalf("Download failed: %v", err)
+ }
+}
+
func TestIntegration_Objects(t *testing.T) {
// TODO(jba): Use subtests (Go 1.7).
ctx := context.Background()
diff --git a/storage/reader.go b/storage/reader.go
index 94563c2..a992249 100644
--- a/storage/reader.go
+++ b/storage/reader.go
@@ -149,7 +149,14 @@
req.Header.Set("Range", fmt.Sprintf("bytes=%d-%d", start, offset+length-1))
}
// We wait to assign conditions here because the generation number can change in between reopen() runs.
- req.URL.RawQuery = conditionsQuery(gen, o.conds)
+ if err := setConditionsHeaders(req.Header, o.conds); err != nil {
+ return nil, err
+ }
+ // If an object generation is specified, include generation as query string parameters.
+ if gen >= 0 {
+ req.URL.RawQuery = fmt.Sprintf("generation=%d", gen)
+ }
+
var res *http.Response
err = runWithRetry(ctx, func() error {
res, err = o.c.hc.Do(req)
@@ -324,6 +331,24 @@
return 0, false
}
+// setConditionsHeaders sets precondition request headers for downloads
+// using the XML API. It assumes that the conditions have been validated.
+func setConditionsHeaders(headers http.Header, conds *Conditions) error {
+ if conds == nil {
+ return nil
+ }
+ if conds.MetagenerationMatch != 0 {
+ headers.Set("x-goog-if-metageneration-match", fmt.Sprint(conds.MetagenerationMatch))
+ }
+ switch {
+ case conds.GenerationMatch != 0:
+ headers.Set("x-goog-if-generation-match", fmt.Sprint(conds.GenerationMatch))
+ case conds.DoesNotExist:
+ headers.Set("x-goog-if-generation-match", "0")
+ }
+ return nil
+}
+
var emptyBody = ioutil.NopCloser(strings.NewReader(""))
// Reader reads a Cloud Storage object.
diff --git a/storage/storage.go b/storage/storage.go
index c46d7c2..1b0610e 100644
--- a/storage/storage.go
+++ b/storage/storage.go
@@ -33,7 +33,6 @@
"reflect"
"regexp"
"sort"
- "strconv"
"strings"
"time"
"unicode/utf8"
@@ -1596,44 +1595,6 @@
return true
}
-// conditionsQuery returns the generation and conditions as a URL query
-// string suitable for URL.RawQuery. It assumes that the conditions
-// have been validated.
-func conditionsQuery(gen int64, conds *Conditions) string {
- // URL escapes are elided because integer strings are URL-safe.
- var buf []byte
-
- appendParam := func(s string, n int64) {
- if len(buf) > 0 {
- buf = append(buf, '&')
- }
- buf = append(buf, s...)
- buf = strconv.AppendInt(buf, n, 10)
- }
-
- if gen >= 0 {
- appendParam("generation=", gen)
- }
- if conds == nil {
- return string(buf)
- }
- switch {
- case conds.GenerationMatch != 0:
- appendParam("ifGenerationMatch=", conds.GenerationMatch)
- case conds.GenerationNotMatch != 0:
- appendParam("ifGenerationNotMatch=", conds.GenerationNotMatch)
- case conds.DoesNotExist:
- appendParam("ifGenerationMatch=", 0)
- }
- switch {
- case conds.MetagenerationMatch != 0:
- appendParam("ifMetagenerationMatch=", conds.MetagenerationMatch)
- case conds.MetagenerationNotMatch != 0:
- appendParam("ifMetagenerationNotMatch=", conds.MetagenerationNotMatch)
- }
- return string(buf)
-}
-
// composeSourceObj wraps a *raw.ComposeRequestSourceObjects, but adds the methods
// that modifyCall searches for by name.
type composeSourceObj struct {
diff --git a/storage/storage_test.go b/storage/storage_test.go
index 1cd4274..55d00e2 100644
--- a/storage/storage_test.go
+++ b/storage/storage_test.go
@@ -646,34 +646,6 @@
},
{
func() error {
- _, err := obj.If(Conditions{GenerationMatch: 1234}).NewReader(ctx)
- return err
- },
- "GET /buck/obj?ifGenerationMatch=1234",
- },
- {
- func() error {
- _, err := obj.If(Conditions{GenerationNotMatch: 1234}).NewReader(ctx)
- return err
- },
- "GET /buck/obj?ifGenerationNotMatch=1234",
- },
- {
- func() error {
- _, err := obj.If(Conditions{MetagenerationMatch: 1234}).NewReader(ctx)
- return err
- },
- "GET /buck/obj?ifMetagenerationMatch=1234",
- },
- {
- func() error {
- _, err := obj.If(Conditions{MetagenerationNotMatch: 1234}).NewReader(ctx)
- return err
- },
- "GET /buck/obj?ifMetagenerationNotMatch=1234",
- },
- {
- func() error {
_, err := obj.If(Conditions{MetagenerationNotMatch: 1234}).Attrs(ctx)
return err
},
@@ -734,6 +706,59 @@
}
}
+ readerTests := []struct {
+ fn func() error
+ want string
+ }{
+ {
+ func() error {
+ _, err := obj.If(Conditions{GenerationMatch: 1234}).NewReader(ctx)
+ return err
+ },
+ "x-goog-if-generation-match: 1234, x-goog-if-metageneration-match: ",
+ },
+ {
+ func() error {
+ _, err := obj.If(Conditions{MetagenerationMatch: 5}).NewReader(ctx)
+ return err
+ },
+ "x-goog-if-generation-match: , x-goog-if-metageneration-match: 5",
+ },
+ {
+ func() error {
+ _, err := obj.If(Conditions{GenerationMatch: 1234, MetagenerationMatch: 5}).NewReader(ctx)
+ return err
+ },
+ "x-goog-if-generation-match: 1234, x-goog-if-metageneration-match: 5",
+ },
+ }
+
+ for i, tt := range readerTests {
+ if err := tt.fn(); err != nil && err != io.EOF {
+ t.Error(err)
+ continue
+ }
+
+ select {
+ case r := <-gotReq:
+ generationConds := r.Header.Get("x-goog-if-generation-match")
+ metagenerationConds := r.Header.Get("x-goog-if-metageneration-match")
+ got := fmt.Sprintf(
+ "x-goog-if-generation-match: %s, x-goog-if-metageneration-match: %s",
+ generationConds,
+ metagenerationConds,
+ )
+ if got != tt.want {
+ t.Errorf("%d. RequestHeaders = %q; want %q", i, got, tt.want)
+ }
+ case <-time.After(5 * time.Second):
+ t.Fatalf("%d. timeout", i)
+ }
+ if err != nil {
+ t.Fatal(err)
+ }
+ }
+
// Test an error, too:
err = obj.Generation(1234).NewWriter(ctx).Close()
if err == nil || !strings.Contains(err.Error(), "NewWriter: generation not supported") {
