Repository: Shopify/themekit
Pull request #827 (2020-10-14 19:33:23)

Run gofmt
~~Once this is in, I'd like to add something a check to CI to ensure that we don't merge misformatted files.~~ Done in here too.


Patch:
diff --git a/.travis.yml b/.travis.yml
index 62b1579..b294691 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -2,6 +2,7 @@ language: go
 go:
   - 1.12
 script:
+  - ./check_format.sh
   - env GO111MODULE=on make lint
   - env GO111MODULE=on go test -race -coverprofile=coverage.txt -covermode=atomic $(go list ./...)
 after_success:
diff --git a/check_format.sh b/check_format.sh
new file mode 100755
index 0000000..3bae9a3
--- /dev/null
+++ b/check_format.sh
@@ -0,0 +1,12 @@
+#!/usr/bin/env bash
+
+gofmt -s -w .
+
+# Ignore these when checking formatting
+git checkout cmd/static/generated-assets.go \
+  src/static/_testdata/static/generated-assets.go \
+
+if [ "$(git diff)" ]; then
+  echo "Bad formatting detected, please run gofmt"
+  exit 1
+fi
diff --git a/src/env/env.go b/src/env/env.go
index 55510dc..3b92be6 100644
--- a/src/env/env.go
+++ b/src/env/env.go
@@ -28,8 +28,7 @@ type Env struct {
 
 //Default is the default values for a environment
 var Default = Env{
-	Name:    "development",
-	Timeout: 30 * time.Second,
+	Name: "development",
 }
 
 func init() {
diff --git a/src/httpify/client.go b/src/httpify/client.go
index d2c9ef0..34b7e8b 100644
--- a/src/httpify/client.go
+++ b/src/httpify/client.go
@@ -2,11 +2,13 @@ package httpify
 
 import (
 	"bytes"
+	"context"
 	"crypto/tls"
 	"encoding/json"
 	"errors"
 	"fmt"
 	"io/ioutil"
+	"net"
 	"net/http"
 	"net/url"
 	"runtime"
@@ -19,9 +21,39 @@ import (
 )
 
 var (
-	errConnectionIssue = errors.New("DNS problem while connecting to Shopify, this indicates a problem with your internet connection")
+	// ErrConnectionIssue is an error that is thrown when a very specific error is
+	// returned from our http request that usually implies bad connections.
+	ErrConnectionIssue = errors.New("DNS problem while connecting to Shopify, this indicates a problem with your internet connection")
+	// ErrInvalidProxyURL is returned if a proxy url has been passed but is improperly formatted
+	ErrInvalidProxyURL = errors.New("invalid proxy URI")
+	netDialer          = &net.Dialer{
+		Timeout:   3 * time.Second,
+		KeepAlive: 1 * time.Second,
+	}
+	httpTransport = &http.Transport{
+		TLSClientConfig:       &tls.Config{InsecureSkipVerify: true},
+		IdleConnTimeout:       time.Second,
+		TLSHandshakeTimeout:   time.Second,
+		ExpectContinueTimeout: time.Second,
+		ResponseHeaderTimeout: time.Second,
+		MaxIdleConnsPerHost:   10,
+		DialContext: func(ctx context.Context, network, address string) (conn net.Conn, err error) {
+			if conn, err = netDialer.DialContext(ctx, network, address); err != nil {
+				return nil, err
+			}
+			deadline := time.Now().Add(5 * time.Second)
+			conn.SetReadDeadline(deadline)
+			return conn, conn.SetDeadline(deadline)
+		},
+	}
+	httpClient = &http.Client{
+		Transport: httpTransport,
+		Timeout:   30 * time.Second,
+	}
 )
 
+type proxyHandler func(*http.Request) (*url.URL, error)
+
 // Params allows for a better structured input into NewClient
 type Params struct {
 	Domain   string
@@ -36,7 +68,6 @@ type HTTPClient struct {
 	domain   string
 	password string
 	baseURL  *url.URL
-	client   *http.Client
 	limit    *ratelimiter.Limiter
 	maxRetry int
 }
@@ -49,16 +80,22 @@ func NewClient(params Params) (*HTTPClient, error) {
 		return nil, err
 	}
 
-	adapter, err := generateHTTPAdapter(params.Timeout, params.Proxy)
-	if err != nil {
-		return nil, err
+	if params.Timeout != 0 {
+		httpClient.Timeout = params.Timeout
+	}
+
+	if params.Proxy != "" {
+		parsedURL, err := url.ParseRequestURI(params.Proxy)
+		if err != nil {
+			return nil, ErrInvalidProxyURL
+		}
+		httpTransport.Proxy = http.ProxyURL(parsedURL)
 	}
 
 	return &HTTPClient{
 		domain:   params.Domain,
 		password: params.Password,
 		baseURL:  baseURL,
-		client:   adapter,
 		limit:    ratelimiter.New(params.Domain, 4),
 		maxRetry: 5,
 	}, nil
@@ -105,8 +142,7 @@ func (client *HTTPClient) do(method, path string, body interface{}, headers map[
 }
 
 func (client *HTTPClient) doWithRetry(req *http.Request, body interface{}) (*http.Response, error) {
-	attempt := 0
-	for {
+	for attempt := 0; attempt <= client.maxRetry; {
 		// reset the body when non-nil for every request (rewind)
 		if body != nil {
 			data, err := json.Marshal(body)
@@ -117,51 +153,22 @@ func (client *HTTPClient) doWithRetry(req *http.Request, body interface{}) (*htt
 		}
 
 		client.limit.Wait()
-		resp, err := client.client.Do(req)
+		resp, err := httpClient.Do(req)
 		if err == nil {
 			if resp.StatusCode >= 100 && resp.StatusCode <= 428 {
 				return resp, nil
 			} else if resp.StatusCode == http.StatusTooManyRequests {
 				after, _ := strconv.ParseFloat(resp.Header.Get("Retry-After"), 10)
-				client.limit.ResetAfter(time.Duration(after))
+				client.limit.ResetAfter(time.Duration(after) * time.Second)
 				continue
 			}
 		} else if strings.Contains(err.Error(), "no such host") {
-			return nil, errConnectionIssue
+			return nil, ErrConnectionIssue
 		}
-
 		attempt++
-		if attempt > client.maxRetry {
-			return resp, fmt.Errorf("request failed after %v retries with err: %v", client.maxRetry, err)
-		}
 		time.Sleep(time.Duration(attempt) * time.Second)
 	}
-}
-
-func generateHTTPAdapter(timeout time.Duration, proxyURL string) (*http.Client, error) {
-	adapter := &http.Client{Timeout: timeout}
-	if transport, err := generateClientTransport(proxyURL); err != nil {
-		return nil, err
-	} else if transport != nil {
-		adapter.Transport = transport
-	}
-	return adapter, nil
-}
-
-func generateClientTransport(proxyURL string) (*http.Transport, error) {
-	if proxyURL == "" {
-		return nil, nil
-	}
-
-	parsedURL, err := url.ParseRequestURI(proxyURL)
-	if err != nil {
-		return nil, fmt.Errorf("invalid proxy URI")
-	}
-
-	return &http.Transport{
-		Proxy:           http.ProxyURL(parsedURL),
-		TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
-	}, nil
+	return nil, fmt.Errorf("request failed after %v retries", client.maxRetry)
 }
 
 func parseBaseURL(domain string) (*url.URL, error) {
diff --git a/src/ratelimiter/rate_limiter.go b/src/ratelimiter/rate_limiter.go
index 1981fe0..1e29557 100644
--- a/src/ratelimiter/rate_limiter.go
+++ b/src/ratelimiter/rate_limiter.go
@@ -20,7 +20,7 @@ func New(domain string, reqPerSec int) *Limiter {
 		everySecond := rate.Every(time.Second / time.Duration(reqPerSec))
 		domainLimitMap[domain] = &Limiter{
 			perSecond: everySecond,
-			rate:      rate.NewLimiter(everySecond, 10),
+			rate:      rate.NewLimiter(everySecond, reqPerSec),
 		}
 	}
 	return domainLimitMap[domain]
diff --git a/src/release/release.go b/src/release/release.go
index 87359cc..9483b2d 100644
--- a/src/release/release.go
+++ b/src/release/release.go
@@ -30,7 +30,7 @@ var (
 		"windows-amd64": "theme.exe",
 	}
 	// ThemeKitVersion is the version build of the library
-	ThemeKitVersion, _ = version.NewVersion("1.1.2")
+	ThemeKitVersion, _ = version.NewVersion("1.1.3")
 )
 
 const (
diff --git a/src/shopify/theme_client.go b/src/shopify/theme_client.go
index e526f77..1f9b05a 100644
--- a/src/shopify/theme_client.go
+++ b/src/shopify/theme_client.go
@@ -142,7 +142,7 @@ func (c Client) GetShop() (Shop, error) {
 
 // Themes will return all the available themes on a domain.
 func (c Client) Themes() ([]Theme, error) {
-	resp, err := c.http.Get(APIPath + "themes.json", nil)
+	resp, err := c.http.Get(APIPath+"themes.json", nil)
 	if err != nil {
 		return []Theme{}, err
 	}
@@ -162,7 +162,7 @@ func (c *Client) CreateNewTheme(name string) (theme Theme, err error) {
 		return Theme{}, ErrThemeNameRequired
 	}
 
-	resp, err := c.http.Post(APIPath + "themes.json", map[string]interface{}{"theme": Theme{Name: name}}, nil)
+	resp, err := c.http.Post(APIPath+"themes.json", map[string]interface{}{"theme": Theme{Name: name}}, nil)
 	if err != nil {
 		return Theme{}, err
 	}
@@ -186,7 +186,7 @@ func (c Client) GetInfo() (Theme, error) {
 		return Theme{}, ErrInfoWithoutThemeID
 	}
 
-	resp, err := c.http.Get(fmt.Sprintf(APIPath + "themes/%s.json", c.themeID), nil)
+	resp, err := c.http.Get(fmt.Sprintf(APIPath+"themes/%s.json", c.themeID), nil)
 	if err != nil {
 		return Theme{}, err
 	} else if resp.StatusCode == 404 {
@@ -208,7 +208,7 @@ func (c Client) PublishTheme() error {
 	}
 
 	resp, err := c.http.Put(
-		fmt.Sprintf(APIPath + "themes/%s.json", c.themeID),
+		fmt.Sprintf(APIPath+"themes/%s.json", c.themeID),
 		map[string]Theme{"theme": {Role: "main"}},
 		nil,
 	)
@@ -347,7 +347,7 @@ func (c Client) DeleteAsset(asset Asset) error {
 func (c Client) assetPath(query map[string]string) string {
 	formatted := APIPath + "assets.json"
 	if c.themeID != "" {
-		formatted = fmt.Sprintf(APIPath + "themes/%s/assets.json", c.themeID)
+		formatted = fmt.Sprintf(APIPath+"themes/%s/assets.json", c.themeID)
 	}
 
 	if len(query) > 0 {


Test patch:
diff --git a/src/httpify/client_test.go b/src/httpify/client_test.go
index 032e7a4..d9a9cff 100644
--- a/src/httpify/client_test.go
+++ b/src/httpify/client_test.go
@@ -139,37 +139,41 @@ func TestClient_do(t *testing.T) {
 	assert.Nil(t, err)
 
 	_, err = client.do("POST", "/assets.json", body, nil)
-	assert.Contains(t, err.Error(), "request failed after 1 retries with err: ", server.URL)
+	assert.Contains(t, err.Error(), "request failed after 1 retries", server.URL)
 	server.Close()
 }
 
 func TestGenerateHTTPAdapter(t *testing.T) {
-	_, err := generateHTTPAdapter(time.Second, "#$#$^$%^##$")
-	if assert.NotNil(t, err) {
-		assert.EqualError(t, err, "invalid proxy URI")
-	}
+	NewClient(Params{
+		Domain:  "https://shop.myshopify.com",
+		Timeout: 60 * time.Second,
+	})
+	assert.Equal(t, httpClient.Timeout, 60*time.Second)
 
-	c, err := generateHTTPAdapter(time.Second, "http://localhost:3000")
-	assert.Nil(t, err)
-	assert.Equal(t, time.Second, c.Timeout)
-	assert.NotNil(t, c.Transport)
+	NewClient(Params{Domain: "https://shop.myshopify.com"})
+	assert.Equal(t, httpClient.Timeout, 60*time.Second)
 }
 
-func TestGenerateClientTransport(t *testing.T) {
+func TestProxyConfig(t *testing.T) {
 	testcases := []struct {
 		proxyURL, err string
-		expectNil     bool
 	}{
-		{proxyURL: "", expectNil: true},
-		{proxyURL: "http//localhost:3000", expectNil: true, err: "invalid proxy URI"},
-		{proxyURL: "http://127.0.0.1:8080", expectNil: false},
+		{proxyURL: ""},
+		{proxyURL: "http//localhost:3000", err: "invalid proxy URI"},
+		{proxyURL: "http://127.0.0.1:8080"},
 	}
 
 	for _, testcase := range testcases {
-		transport, err := generateClientTransport(testcase.proxyURL)
-		assert.Equal(t, transport == nil, testcase.expectNil)
-		if testcase.err == "" {
-			assert.Nil(t, err)
+		_, err := NewClient(Params{
+			Domain: "https://shop.myshopify.com",
+			Proxy:  testcase.proxyURL,
+		})
+		if testcase.err == "" && assert.Nil(t, err) {
+			if testcase.proxyURL == "" {
+				assert.Nil(t, httpTransport.Proxy)
+			} else {
+				assert.NotNil(t, httpTransport.Proxy)
+			}
 		} else if assert.NotNil(t, err) {
 			assert.Contains(t, err.Error(), testcase.err)
 		}
diff --git a/src/shopify/theme_client_test.go b/src/shopify/theme_client_test.go
index 865e25c..04a9c4e 100644
--- a/src/shopify/theme_client_test.go
+++ b/src/shopify/theme_client_test.go
@@ -94,7 +94,7 @@ func TestThemeClient_Themes(t *testing.T) {
 		client, _ := NewClient(&env.Env{})
 		client.http = m
 
-		expectation := m.On("Get", APIPath + "themes.json", NoHeaders)
+		expectation := m.On("Get", APIPath+"themes.json", NoHeaders)
 		if testcase.resperr != "" {
 			expectation.Return(nil, errors.New(testcase.resperr))
 		} else {
@@ -135,9 +135,9 @@ func TestThemeClient_CreateNewTheme(t *testing.T) {
 		query := map[string]interface{}{"theme": Theme{Name: testcase.in}}
 
 		if testcase.resp != "" {
-			m.On("Post", APIPath + "themes.json", query, NoHeaders).Return(jsonResponse(testcase.resp, 200), nil)
+			m.On("Post", APIPath+"themes.json", query, NoHeaders).Return(jsonResponse(testcase.resp, 200), nil)
 		} else if testcase.resperr != "" {
-			m.On("Post", APIPath + "themes.json", query, NoHeaders).Return(nil, errors.New(testcase.resperr))
+			m.On("Post", APIPath+"themes.json", query, NoHeaders).Return(nil, errors.New(testcase.resperr))
 		}
 
 		theme, err := client.CreateNewTheme(testcase.in)
@@ -173,7 +173,7 @@ func TestThemeClient_GetInfo(t *testing.T) {
 		client, _ := NewClient(&env.Env{ThemeID: testcase.themeID})
 		client.http = m
 
-		expectation := m.On("Get", fmt.Sprintf(APIPath + "themes/%s.json", testcase.themeID), NoHeaders)
+		expectation := m.On("Get", fmt.Sprintf(APIPath+"themes/%s.json", testcase.themeID), NoHeaders)
 		if testcase.resperr != "" {
 			expectation.Return(nil, errors.New(testcase.resperr))
 		} else {
@@ -216,7 +216,7 @@ func TestThemeClient_PublishTheme(t *testing.T) {
 
 		expectation := m.On(
 			"Put",
-			fmt.Sprintf(APIPath + "themes/%s.json", testcase.themeID),
+			fmt.Sprintf(APIPath+"themes/%s.json", testcase.themeID),
 			map[string]Theme{"theme": {Role: "main"}},
 			NoHeaders,
 		)
@@ -256,7 +256,7 @@ func TestThemeClient_GetAllAssets(t *testing.T) {
 		client, _ := NewClient(&env.Env{ThemeID: "123"})
 		client.http = m
 
-		expectation := m.On("Get", APIPath + "themes/123/assets.json?fields=key%2Cchecksum", NoHeaders)
+		expectation := m.On("Get", APIPath+"themes/123/assets.json?fields=key%2Cchecksum", NoHeaders)
 		if testcase.resperr != "" {
 			expectation.Return(nil, errors.New(testcase.resperr))
 		} else {
@@ -299,7 +299,7 @@ func TestThemeClient_GetAllAssets(t *testing.T) {
 		m := new(mocks.HttpAdapter)
 		client, _ := NewClient(&env.Env{ThemeID: "123", IgnoredFiles: testcase.ignore})
 		client.http = m
-		m.On("Get", APIPath + "themes/123/assets.json?fields=key%2Cchecksum", NoHeaders).Return(jsonResponse(testcase.input, 200), nil)
+		m.On("Get", APIPath+"themes/123/assets.json?fields=key%2Cchecksum", NoHeaders).Return(jsonResponse(testcase.input, 200), nil)
 		assets, err := client.GetAllAssets()
 		assert.Nil(t, err)
 		assert.Equal(t, testcase.expected, assets)
@@ -322,7 +322,7 @@ func TestThemeClient_GetAsset(t *testing.T) {
 		client, _ := NewClient(&env.Env{ThemeID: "123"})
 		client.http = m
 
-		expectation := m.On("Get", APIPath + "themes/123/assets.json?asset%5Bkey%5D=filename.txt", NoHeaders)
+		expectation := m.On("Get", APIPath+"themes/123/assets.json?asset%5Bkey%5D=filename.txt", NoHeaders)
 		if testcase.resperr != "" {
 			expectation.Return(nil, errors.New(testcase.resperr))
 		} else if testcase.code != 0 {
@@ -358,7 +358,7 @@ func TestThemeClient_UpdateAsset(t *testing.T) {
 		client, _ := NewClient(&env.Env{ThemeID: "123"})
 		client.http = m
 
-		expectation := m.On("Put", APIPath + "themes/123/assets.json", map[string]Asset{"asset": {Key: "filename.txt"}}, map[string]string{})
+		expectation := m.On("Put", APIPath+"themes/123/assets.json", map[string]Asset{"asset": {Key: "filename.txt"}}, map[string]string{})
 		if testcase.resperr != "" {
 			expectation.Return(nil, errors.New(testcase.resperr))
 		} else if testcase.code != 0 {
@@ -400,13 +400,13 @@ func TestThemeClient_UpdateAsset(t *testing.T) {
 
 	m.On(
 		"Delete",
-		APIPath + "themes/123/assets.json?asset%5Bkey%5D=filename.txt.liquid",
+		APIPath+"themes/123/assets.json?asset%5Bkey%5D=filename.txt.liquid",
 		NoHeaders,
 	).Return(jsonResponse("{}", 200), nil)
 
 	m.On(
 		"Put",
-		APIPath + "themes/123/assets.json",
+		APIPath+"themes/123/assets.json",
 		map[string]Asset{"asset": asset},
 		map[string]string{},
 	).Return(jsonResponse(`{"asset":{"key":"assets/hello.txt"}}`, 200), nil)
@@ -431,7 +431,7 @@ func TestThemeClient_DeleteAsset(t *testing.T) {
 		client, _ := NewClient(&env.Env{ThemeID: "123"})
 		client.http = m
 
-		expectation := m.On("Delete", APIPath + "themes/123/assets.json?asset%5Bkey%5D=filename.txt", NoHeaders)
+		expectation := m.On("Delete", APIPath+"themes/123/assets.json?asset%5Bkey%5D=filename.txt", NoHeaders)
 		if testcase.resperr != "" {
 			expectation.Return(nil, errors.New(testcase.resperr))
 		} else {