fix(storage): revise Reader to send XML preconditions

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()

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.

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 {

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") {