Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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
  • Loading branch information
cojenco committed Jul 23, 2021
1 parent dec7a67 commit e36b29a
Show file tree
Hide file tree
Showing 4 changed files with 109 additions and 68 deletions.
30 changes: 30 additions & 0 deletions storage/integration_test.go
Expand Up @@ -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()
Expand Down
27 changes: 26 additions & 1 deletion storage/reader.go
Expand Up @@ -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)
Expand Down Expand Up @@ -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.
Expand Down
39 changes: 0 additions & 39 deletions storage/storage.go
Expand Up @@ -33,7 +33,6 @@ import (
"reflect"
"regexp"
"sort"
"strconv"
"strings"
"time"
"unicode/utf8"
Expand Down Expand Up @@ -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 {
Expand Down
81 changes: 53 additions & 28 deletions storage/storage_test.go
Expand Up @@ -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)
Expand Down Expand Up @@ -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") {
Expand Down

0 comments on commit e36b29a

Please sign in to comment.