Follow-up to gzip encoding of request body (#343)

* Add semver to go.mod

* Bring back tests for sourcegraphVersionCheck

* Extract version check into public function

* Move check for gzip compression to campaigns service

* Fix unmarshaling of sourcegraph version

* Add CHANGELOG

CHANGELOG.md

@@ -23,6 +23,7 @@ All notable changes to `src-cli` are documented in this file.
 - `src campaign [preview|apply]` now check whether `git` and `docker` are available before trying to execute a campaign spec's steps. [#326](https://github.com/sourcegraph/src-cli/pull/326)
 - The progress bar displayed by `src campaign [preview|apply]` has been extended by status bars that show which steps are currently being executed for each repository. [#338](https://github.com/sourcegraph/src-cli/pull/338)
 - `src campaign [preview|apply]` now shows a warning when no changeset specs have been created.
+- Requests sent to Sourcegraph by the `src campaign` commands now use gzip compression for the body when talking to Sourcegraph 3.21.0 and later. [#336](https://github.com/sourcegraph/src-cli/pull/336) and [#343](https://github.com/sourcegraph/src-cli/pull/343)
 
 ### Fixed
 

cmd/src/campaigns_apply.go

@@ -65,6 +65,10 @@ Examples:
 			Client:           cfg.apiClient(flags.api, flagSet.Output()),
 		})
 
+		if err := svc.DetermineFeatureFlags(ctx); err != nil {
+			return err
+		}
+
 		if err := doApply(ctx, out, svc, flags); err != nil {
 			printExecutionError(out, err)
 			return &exitCodeError{nil, 1}

cmd/src/campaigns_preview.go

@@ -42,6 +42,10 @@ Examples:
 			Client:           cfg.apiClient(flags.api, flagSet.Output()),
 		})
 
+		if err := svc.DetermineFeatureFlags(ctx); err != nil {
+			return err
+		}
+
 		_, url, err := campaignsExecute(ctx, out, svc, flags)
 		if err != nil {
 			printExecutionError(out, err)

cmd/src/campaigns_repositories.go

@@ -49,6 +49,11 @@ Examples:
 		client := cfg.apiClient(apiFlags, flagSet.Output())
 
 		svc := campaigns.NewService(&campaigns.ServiceOpts{Client: client})
+
+		if err := svc.DetermineFeatureFlags(ctx); err != nil {
+			return err
+		}
+
 		spec, _, err := svc.ParseCampaignSpec(specFile)
 		if err != nil {
 			return errors.Wrap(err, "parsing campaign spec")

cmd/src/campaigns_validate.go

@@ -39,6 +39,7 @@ Examples:
 		defer specFile.Close()
 
 		svc := campaigns.NewService(&campaigns.ServiceOpts{})
+
 		spec, _, err := svc.ParseCampaignSpec(specFile)
 		if err != nil {
 			return errors.Wrap(err, "parsing campaign spec")

cmd/src/config_get.go

@@ -66,7 +66,9 @@ Examples:
 			ViewerSettings  *SettingsCascade
 			SettingsSubject *SettingsSubject
 		}
-		if ok, err := client.NewRequest(query, queryVars).Do(context.Background(), &result); err != nil || !ok {
+
+		ok, err := client.NewRequest(query, queryVars).Do(context.Background(), &result)
+		if err != nil || !ok {
 			return err
 		}
 

cmd/src/config_list.go

@@ -73,7 +73,9 @@ Examples:
 			ViewerSettings  *SettingsCascade
 			SettingsSubject *SettingsSubject
 		}
-		if ok, err := client.NewRequest(query, queryVars).Do(context.Background(), &result); err != nil || !ok {
+
+		ok, err := client.NewRequest(query, queryVars).Do(context.Background(), &result)
+		if err != nil || !ok {
 			return err
 		}
 

go.mod

@@ -3,6 +3,7 @@ module github.com/sourcegraph/src-cli
 go 1.13
 
 require (
+	github.com/Masterminds/semver v1.5.0
 	github.com/dustin/go-humanize v1.0.0
 	github.com/efritz/pentimento v0.0.0-20190429011147-ade47d831101
 	github.com/google/go-cmp v0.5.2

go.sum

@@ -1,3 +1,5 @@
+github.com/Masterminds/semver v1.5.0 h1:H65muMkzWKEuNDnfl9d70GUjFniHKHRbFPGBuZ3QEww=
+github.com/Masterminds/semver v1.5.0/go.mod h1:MB6lktGJrhw8PrUyiEoblNEGEQ+RzHPF078ddwwvV3Y=
 github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=

internal/api/api.go

@@ -10,12 +10,10 @@ import (
 	"io/ioutil"
 	"net/http"
 	"os"
-	"regexp"
 	"strings"
 
-	"github.com/Masterminds/semver"
 	"github.com/hashicorp/go-multierror"
-	"github.com/jig/teereadcloser"
+	ioaux "github.com/jig/teereadcloser"
 	"github.com/kballard/go-shellquote"
 	"github.com/mattn/go-isatty"
 	"github.com/pkg/errors"
@@ -31,6 +29,13 @@ type Client interface {
 	// NewRequest creates a GraphQL request.
 	NewRequest(query string, vars map[string]interface{}) Request
 
+	// NewGzippedRequest creates a GraphQL request with gzip compression turned on.
+	NewGzippedRequest(query string, vars map[string]interface{}) Request
+
+	// NewGzippedQuery is a convenience wrapper around NewQuery with gzip
+	// compression turned on.
+	NewGzippedQuery(query string) Request
+
 	// NewHTTPRequest creates an http.Request for the Sourcegraph API.
 	//
 	// path is joined against the API route. For example on Sourcegraph.com this
@@ -56,15 +61,15 @@ type Request interface {
 
 // client is the internal concrete type implementing Client.
 type client struct {
-	opts         ClientOpts
-	supportsGzip *bool
+	opts ClientOpts
 }
 
 // request is the internal concrete type implementing Request.
 type request struct {
 	client *client
 	query  string
 	vars   map[string]interface{}
+	gzip   bool
 }
 
 // ClientOpts encapsulates the options given to NewClient.
@@ -116,36 +121,25 @@ func (c *client) NewRequest(query string, vars map[string]interface{}) Request {
 	}
 }
 
-func (c *client) NewHTTPRequest(ctx context.Context, method, p string, body io.Reader) (*http.Request, error) {
-	if c.supportsGzip == nil {
-		// set to false, unless we have a new enough version
-		supportsGzip := false
-		c.supportsGzip = &supportsGzip
-
-		version, err := c.getSourcegraphVersion(ctx)
-
-		// ignore errors; we only care if the version is sufficently new
-		if err == nil {
-			supportsGzip, err = sourcegraphVersionCheck(version, ">= 3.21.0", "2020-10-12")
-			if err == nil {
-				c.supportsGzip = &supportsGzip
-			}
-		}
+func (c *client) NewGzippedRequest(query string, vars map[string]interface{}) Request {
+	return &request{
+		client: c,
+		query:  query,
+		vars:   vars,
+		gzip:   true,
 	}
+}
 
-	if *c.supportsGzip && body != nil {
-		body = codeintelutils.Gzip(body)
-	}
+func (c *client) NewGzippedQuery(query string) Request {
+	return c.NewGzippedRequest(query, nil)
+}
 
+func (c *client) NewHTTPRequest(ctx context.Context, method, p string, body io.Reader) (*http.Request, error) {
 	req, err := c.createHTTPRequest(ctx, method, p, body)
 	if err != nil {
 		return nil, err
 	}
 
-	if *c.supportsGzip {
-		req.Header.Set("Content-Encoding", "gzip")
-	}
-
 	return req, nil
 }
 
@@ -201,12 +195,21 @@ func (r *request) do(ctx context.Context, result interface{}) (bool, error) {
 		return false, err
 	}
 
+	var bufBody io.Reader = bytes.NewBuffer(reqBody)
+	if r.gzip {
+		bufBody = codeintelutils.Gzip(bufBody)
+	}
+
 	// Create the HTTP request.
-	req, err := r.client.NewHTTPRequest(ctx, "POST", ".api/graphql", bytes.NewBuffer(reqBody))
+	req, err := r.client.NewHTTPRequest(ctx, "POST", ".api/graphql", bufBody)
 	if err != nil {
 		return false, err
 	}
 
+	if r.gzip {
+		req.Header.Set("Content-Encoding", "gzip")
+	}
+
 	// Perform the request.
 	resp, err := http.DefaultClient.Do(req)
 	if err != nil {
@@ -303,80 +306,3 @@ func (r *request) curlCmd() (string, error) {
 	s += fmt.Sprintf("   %s", shellquote.Join(r.client.opts.Endpoint+"/.api/graphql"))
 	return s, nil
 }
-
-const sourcegraphVersionQuery = `query SourcegraphVersion {
-	site {
-	  productVersion
-	}
-  }
-  `
-
-func (c *client) getSourcegraphVersion(ctx context.Context) (string, error) {
-	var sourcegraphVersion struct {
-		Data struct {
-			Site struct {
-				ProductVersion string
-			}
-		}
-	}
-
-	// Create the JSON object.
-	reqBody, err := json.Marshal(map[string]interface{}{
-		"query": sourcegraphVersionQuery,
-	})
-	if err != nil {
-		return "", err
-	}
-
-	// Create the HTTP request.
-	req, err := c.createHTTPRequest(ctx, "POST", ".api/graphql", bytes.NewBuffer(reqBody))
-	if err != nil {
-		return "", err
-	}
-
-	// Perform the request.
-	resp, err := http.DefaultClient.Do(req)
-	if err != nil {
-		return "", err
-	}
-	defer resp.Body.Close()
-
-	respBytes, err := ioutil.ReadAll(resp.Body)
-	if err != nil {
-		return "", err
-	}
-
-	if resp.StatusCode != http.StatusOK {
-		return "", fmt.Errorf("checking sourcegraph backend version; got status code %d", resp.StatusCode)
-	}
-
-	err = json.Unmarshal(respBytes, &sourcegraphVersion)
-	if err != nil {
-		return "", err
-	}
-
-	return sourcegraphVersion.Data.Site.ProductVersion, err
-}
-
-func sourcegraphVersionCheck(version, constraint, minDate string) (bool, error) {
-	if version == "dev" || version == "0.0.0+dev" {
-		return true, nil
-	}
-
-	buildDate := regexp.MustCompile(`^\d+_(\d{4}-\d{2}-\d{2})_[a-z0-9]{7}$`)
-	matches := buildDate.FindStringSubmatch(version)
-	if len(matches) > 1 {
-		return matches[1] >= minDate, nil
-	}
-
-	c, err := semver.NewConstraint(constraint)
-	if err != nil {
-		return false, nil
-	}
-
-	v, err := semver.NewVersion(version)
-	if err != nil {
-		return false, err
-	}
-	return c.Check(v), nil
-}

internal/api/version_check.go

@@ -0,0 +1,30 @@
+package api
+
+import (
+	"regexp"
+
+	"github.com/Masterminds/semver"
+)
+
+func CheckSourcegraphVersion(version, constraint, minDate string) (bool, error) {
+	if version == "dev" || version == "0.0.0+dev" {
+		return true, nil
+	}
+
+	buildDate := regexp.MustCompile(`^\d+_(\d{4}-\d{2}-\d{2})_[a-z0-9]{7}$`)
+	matches := buildDate.FindStringSubmatch(version)
+	if len(matches) > 1 {
+		return matches[1] >= minDate, nil
+	}
+
+	c, err := semver.NewConstraint(constraint)
+	if err != nil {
+		return false, nil
+	}
+
+	v, err := semver.NewVersion(version)
+	if err != nil {
+		return false, err
+	}
+	return c.Check(v), nil
+}

internal/api/version_check_test.go

@@ -0,0 +1,74 @@
+package api
+
+import "testing"
+
+func TestCheckSourcegraphVersion(t *testing.T) {
+	for _, tc := range []struct {
+		currentVersion, constraint, minDate string
+		expected                            bool
+	}{
+		{
+			currentVersion: "3.12.6",
+			constraint:     ">= 3.12.6",
+			minDate:        "2020-01-19",
+			expected:       true,
+		},
+		{
+			currentVersion: "3.12.6-rc.1",
+			constraint:     ">= 3.12.6-0",
+			minDate:        "2020-01-19",
+			expected:       true,
+		},
+		{
+			currentVersion: "3.12.6",
+			constraint:     ">= 3.13",
+			minDate:        "2020-01-19",
+			expected:       false,
+		},
+		{
+			currentVersion: "3.13.0",
+			constraint:     ">= 3.13",
+			minDate:        "2020-01-19",
+			expected:       true,
+		},
+		{
+			currentVersion: "dev",
+			constraint:     ">= 3.13",
+			minDate:        "2020-01-19",
+			expected:       true,
+		},
+		{
+			currentVersion: "0.0.0+dev",
+			constraint:     ">= 3.13",
+			minDate:        "2020-01-19",
+			expected:       true,
+		},
+		{
+			currentVersion: "54959_2020-01-29_9258595",
+			minDate:        "2020-01-19",
+			constraint:     ">= 999.13",
+			expected:       true,
+		},
+		{
+			currentVersion: "54959_2020-01-29_9258595",
+			minDate:        "2020-01-30",
+			constraint:     ">= 999.13",
+			expected:       false,
+		},
+		{
+			currentVersion: "54959_2020-01-29_9258595",
+			minDate:        "2020-01-29",
+			constraint:     ">= 0.0",
+			expected:       true,
+		},
+	} {
+		actual, err := CheckSourcegraphVersion(tc.currentVersion, tc.constraint, tc.minDate)
+		if err != nil {
+			t.Errorf("err: %s", err)
+		}
+
+		if actual != tc.expected {
+			t.Errorf("wrong result. want=%t, got=%t (version=%q, constraint=%q)", tc.expected, actual, tc.currentVersion, tc.constraint)
+		}
+	}
+}