From e36b29a3d43bce5c1c044f7daf6e1db00b0a49e0 Mon Sep 17 00:00:00 2001 From: cojenco Date: Fri, 23 Jul 2021 13:02:11 -0700 Subject: [PATCH] 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 --- storage/integration_test.go | 30 ++++++++++++++ storage/reader.go | 27 ++++++++++++- storage/storage.go | 39 ------------------ storage/storage_test.go | 81 ++++++++++++++++++++++++------------- 4 files changed, 109 insertions(+), 68 deletions(-) diff --git a/storage/integration_test.go b/storage/integration_test.go index 7ea61e3c47e..82745f68252 100644 --- a/storage/integration_test.go +++ b/storage/integration_test.go @@ -744,6 +744,36 @@ func TestIntegration_ObjectsRangeReader(t *testing.T) { } } +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 94563c2afee..a992249846b 100644 --- a/storage/reader.go +++ b/storage/reader.go @@ -149,7 +149,14 @@ func (o *ObjectHandle) NewRangeReader(ctx context.Context, offset, length int64) 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 @@ func parseCRC32c(res *http.Response) (uint32, bool) { 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 c46d7c21c58..1b0610e380a 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -33,7 +33,6 @@ import ( "reflect" "regexp" "sort" - "strconv" "strings" "time" "unicode/utf8" @@ -1596,44 +1595,6 @@ func setConditionField(call reflect.Value, name string, value interface{}) bool 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 1cd4274cc5f..55d00e2252f 100644 --- a/storage/storage_test.go +++ b/storage/storage_test.go @@ -644,34 +644,6 @@ func TestCondition(t *testing.T) { }, "GET /buck/obj?generation=1234", }, - { - 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) @@ -734,6 +706,59 @@ func TestCondition(t *testing.T) { } } + 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") {