Repository: Shopify/themekit
Pull request #840 (2020-11-19 16:09:10)

Adding more feedback while unmarshalling responses
Related to #631

### Why is this PR needed

Right now, while unmarshalling responses, if the response is not JSON, then themekit will return an unhelpful error of `malformed response` While themekit does not really know what happened, and we have no way of knowing, we can add some more information to help analyze the issue.

### What this PR does

To the main `unmarshalResponse` method that the theme client uses, this adds
- http response status codes to the error messages. This should give us more insight into what the actual response is (500+ status codes)
- flushes the response body to a tempfile, so that it can be included in issue debugging. 
- outputs a path to the tempfile in the error message if flushing was successful.

### Warn Checklist
- [ ] This changes the interface and requires a Major/Minor version change.
- [ ] ~I have :tophat:'d these changes by using the commands I changed by hand.~ it's hard to produce this error.
- [ ] I have added a dependancy to the project.
- [x] I have considered any potential impact on [node-themekit](https://github.com/Shopify/node-themekit)


andyw8: Would it help to also include the Request ID in the error output?

Patch:
diff --git a/changelog.txt b/changelog.txt
index 191b9d1..03333f3 100644
--- a/changelog.txt
+++ b/changelog.txt
@@ -8,6 +8,7 @@ configuration to the published theme
 - Added --hidepb flag to hide preview bar (#829)
 - Improved request throttling and retrying, to eliminate ABS interactions and
 hanging operations (#839)
+- Adding more feedback when having issues unmarshalling responses (#840)
 
 v1.1.2 (Sep 29, 2020)
 =====================
diff --git a/src/shopify/theme_client.go b/src/shopify/theme_client.go
index 1f9b05a..193fbeb 100644
--- a/src/shopify/theme_client.go
+++ b/src/shopify/theme_client.go
@@ -1,11 +1,8 @@
 package shopify
 
 import (
-	"encoding/json"
 	"errors"
 	"fmt"
-	"io"
-	"io/ioutil"
 	"net/http"
 	"net/url"
 	"sort"
@@ -24,8 +21,6 @@ var (
 	ErrCriticalFile = errors.New("this file is critical and removing it would cause your theme to become non-functional")
 	// ErrNotPartOfTheme will be returned when trying to alter a filepath that does not exist in the theme
 	ErrNotPartOfTheme = errors.New("this file is not part of your theme")
-	// ErrMalformedResponse will be returned if we could not unmarshal the response from shopify
-	ErrMalformedResponse = errors.New("received a malformed response from shopify, this usually indicates a problem with your connection")
 	// ErrZipPathRequired is returned if a source path was not provided to create a new theme
 	ErrZipPathRequired = errors.New("theme zip path is required")
 	// ErrInfoWithoutThemeID will be returned if GetInfo is called without a theme ID
@@ -78,10 +73,6 @@ type assetsResponse struct {
 	Assets []Asset `json:"assets"`
 }
 
-type reqErr struct {
-	Errors string `json:"errors"`
-}
-
 type httpAdapter interface {
 	Get(string, map[string]string) (*http.Response, error)
 	Post(string, interface{}, map[string]string) (*http.Response, error)
@@ -133,7 +124,7 @@ func (c Client) GetShop() (Shop, error) {
 	}
 
 	var shop Shop
-	if err := unmarshalResponse(resp.Body, &shop); err != nil {
+	if err := unmarshalResponse(resp, &shop); err != nil {
 		return Shop{}, err
 	}
 
@@ -148,7 +139,7 @@ func (c Client) Themes() ([]Theme, error) {
 	}
 
 	var r themesResponse
-	if err := unmarshalResponse(resp.Body, &r); err != nil {
+	if err := unmarshalResponse(resp, &r); err != nil {
 		return []Theme{}, err
 	}
 
@@ -168,7 +159,7 @@ func (c *Client) CreateNewTheme(name string) (theme Theme, err error) {
 	}
 
 	var r themeResponse
-	if err = unmarshalResponse(resp.Body, &r); err != nil {
+	if err = unmarshalResponse(resp, &r); err != nil {
 		return Theme{}, err
 	}
 
@@ -194,7 +185,7 @@ func (c Client) GetInfo() (Theme, error) {
 	}
 
 	var r themeResponse
-	if err := unmarshalResponse(resp.Body, &r); err != nil {
+	if err := unmarshalResponse(resp, &r); err != nil {
 		return Theme{}, err
 	}
 
@@ -219,7 +210,7 @@ func (c Client) PublishTheme() error {
 	}
 
 	var r themeResponse
-	if err = unmarshalResponse(resp.Body, &r); err != nil {
+	if err = unmarshalResponse(resp, &r); err != nil {
 		return err
 	}
 
@@ -243,7 +234,7 @@ func (c Client) GetAllAssets() ([]Asset, error) {
 	}
 
 	var r assetsResponse
-	if err := unmarshalResponse(resp.Body, &r); err != nil {
+	if err := unmarshalResponse(resp, &r); err != nil {
 		return []Asset{}, err
 	}
 
@@ -268,7 +259,7 @@ func (c Client) GetAsset(filename string) (Asset, error) {
 	}
 
 	var r assetResponse
-	if err := unmarshalResponse(resp.Body, &r); err != nil {
+	if err := unmarshalResponse(resp, &r); err != nil {
 		return Asset{}, err
 	}
 
@@ -298,7 +289,7 @@ func (c Client) UpdateAsset(asset Asset, lastKnownChecksum string) error {
 	}
 
 	var r assetResponse
-	if err := unmarshalResponse(resp.Body, &r); err != nil {
+	if err := unmarshalResponse(resp, &r); err != nil {
 		return err
 	}
 
@@ -333,7 +324,7 @@ func (c Client) DeleteAsset(asset Asset) error {
 	}
 
 	var r assetResponse
-	if err := unmarshalResponse(resp.Body, &r); err != nil {
+	if err := unmarshalResponse(resp, &r); err != nil {
 		return err
 	}
 
@@ -361,27 +352,6 @@ func (c Client) assetPath(query map[string]string) string {
 	return formatted
 }
 
-func unmarshalResponse(body io.ReadCloser, data interface{}) error {
-	reqBody, err := ioutil.ReadAll(body)
-	if err != nil {
-		return ErrMalformedResponse
-	}
-	err = body.Close()
-	if err != nil {
-		return err
-	}
-	var re reqErr
-	mainErr := json.Unmarshal(reqBody, data)
-	basicErr := json.Unmarshal(reqBody, &re)
-	if mainErr != nil && basicErr != nil {
-		return ErrMalformedResponse
-	}
-	if len(re.Errors) > 0 {
-		return errors.New(re.Errors)
-	}
-	return nil
-}
-
 func toMessages(a map[string][]string) []string {
 	out := []string{}
 	for attr, errs := range a {
diff --git a/src/shopify/unmarshal.go b/src/shopify/unmarshal.go
new file mode 100644
index 0000000..ea59361
--- /dev/null
+++ b/src/shopify/unmarshal.go
@@ -0,0 +1,92 @@
+package shopify
+
+import (
+	"bytes"
+	"encoding/json"
+	"errors"
+	"io/ioutil"
+	"net/http"
+	"os"
+	"text/template"
+)
+
+// reqErr is the expected response structure of errors from asset operations.
+type reqErr struct {
+	Errors string `json:"errors"`
+}
+
+// Err will return an error if the struct contains any errors, otherwise it will return nil
+func (err reqErr) Err() error {
+	if len(err.Errors) > 0 {
+		return errors.New(err.Errors)
+	}
+	return nil
+}
+
+// RespUnmarshalError is an error struct to allow us to present response issues
+// in a more intelligent way, and in a way that will make analyzing the errors
+// easier.
+type RespUnmarshalError struct {
+	Resp       *http.Response
+	Problem    string
+	Suggestion string
+	ReadErr    error
+	TmpFile    *os.File
+}
+
+var respUnmarshalErrorTemplate = template.Must(template.New("respUnmarshalError").Parse(
+	`{{ .Problem }}
+{{ .Suggestion }}
+Http Response Status: {{ .Resp.StatusCode }}
+Request ID: {{ .RequestID }}{{ if .ReadErr }}
+Error: {{ .ReadErr }}{{ end }}{{ with .TmpFile }}
+ResponseBody: {{ .Name }}{{ end }}`))
+
+// RequestID is a helper for the template
+func (err RespUnmarshalError) RequestID() string {
+	return err.Resp.Header.Get("X-Request-Id")
+}
+
+// Error satisfies the Error interface
+func (err RespUnmarshalError) Error() string {
+	var tpl bytes.Buffer
+	respUnmarshalErrorTemplate.Execute(&tpl, err)
+	return tpl.String()
+}
+
+// unmarshalResponse is the bottle neck of receiving an http response and unmarshalling
+// into assets or theme data. If this data cannot be successfully unmarshalled, then
+// it will fallback to try to unmarshal expected error responses in the JSON format.
+// If this finally does not work, then this will return an error of what went wrong
+// and dump the response body into a temporary file for later examination.
+func unmarshalResponse(resp *http.Response, data interface{}) error {
+	reqBody, err := ioutil.ReadAll(resp.Body)
+	if err != nil {
+		return RespUnmarshalError{
+			Resp:       resp,
+			Problem:    "could not read response body",
+			Suggestion: "This may mean that the request was not able to finish successfully",
+			ReadErr:    err,
+		}
+	}
+	defer resp.Body.Close()
+
+	var re reqErr
+	mainErr := json.Unmarshal(reqBody, data) // check if we can unmarshal into the expected returned data
+	basicErr := json.Unmarshal(reqBody, &re) // if no returned data, check if we can get errors from the body
+	if mainErr != nil && basicErr != nil {
+		tmpFile, err := ioutil.TempFile(os.TempDir(), "themekit-response-*.txt")
+		if err == nil {
+			defer tmpFile.Close()
+			tmpFile.Write([]byte(reqBody))
+		}
+		return RespUnmarshalError{
+			Resp:       resp,
+			Problem:    "could not unmarshal JSON from response body",
+			Suggestion: "This usually means Theme Kit received an HTML error page.",
+			TmpFile:    tmpFile,
+		}
+	}
+
+	return re.Err()
+}


Test patch:
diff --git a/src/shopify/theme_client_test.go b/src/shopify/theme_client_test.go
index 04a9c4e..817b67e 100644
--- a/src/shopify/theme_client_test.go
+++ b/src/shopify/theme_client_test.go
@@ -468,33 +468,6 @@ func TestThemeClient_assetPath(t *testing.T) {
 	}
 }
 
-func TestUnmarshalResponse(t *testing.T) {
-	testcases := []struct {
-		input, err    string
-		out, expected themeResponse
-	}{
-		{input: `{"errors":{"name":["can't be blank"]}}`, expected: themeResponse{Errors: map[string][]string{"name": {"can't be blank"}}}},
-		{input: `{"errors": "Not Found"}`, err: "Not Found"},
-		{input: `{"theme":{"id": 123456}}`, expected: themeResponse{Theme: Theme{ID: int64(123456)}}},
-		{input: `{"theme":{"id": 123456}}`, expected: themeResponse{Theme: Theme{ID: int64(123456)}}},
-	}
-
-	for _, testcase := range testcases {
-		err := unmarshalResponse(&StringReadCloser{strings.NewReader(testcase.input)}, &testcase.out)
-		assert.Equal(t, testcase.expected, testcase.out)
-		if testcase.err == "" {
-			assert.Nil(t, err)
-		} else if assert.NotNil(t, err) {
-			assert.Contains(t, err.Error(), testcase.err)
-		}
-	}
-
-	out := assetsResponse{}
-	err := unmarshalResponse(&StringReadCloser{strings.NewReader(`{"errors":"oh no"}`)}, &out)
-	assert.NotNil(t, err)
-	assert.Equal(t, err.Error(), "oh no")
-}
-
 func TestToMessages(t *testing.T) {
 	testcases := []struct {
 		input    map[string][]string
diff --git a/src/shopify/unmarshal_test.go b/src/shopify/unmarshal_test.go
new file mode 100644
index 0000000..dcafa3e
--- /dev/null
+++ b/src/shopify/unmarshal_test.go
@@ -0,0 +1,87 @@
+package shopify
+
+import (
+	"fmt"
+	"io/ioutil"
+	"net/http"
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestRespUnmarshalError(t *testing.T) {
+	resp := &http.Response{
+		StatusCode: 442,
+		Header:     http.Header{"X-Request-Id": []string{"abc-123-456"}},
+	}
+
+	err := RespUnmarshalError{
+		Resp:       resp,
+		Problem:    "this is your problem",
+		Suggestion: "test your things",
+	}
+
+	assert.Equal(t, err.Error(), `this is your problem
+test your things
+Http Response Status: 442
+Request ID: abc-123-456`)
+
+	err = RespUnmarshalError{
+		Resp:       resp,
+		Problem:    "this is your problem",
+		Suggestion: "test your things",
+		ReadErr:    fmt.Errorf("Bad READ"),
+	}
+
+	assert.Equal(t, err.Error(), `this is your problem
+test your things
+Http Response Status: 442
+Request ID: abc-123-456
+Error: Bad READ`)
+
+	err = RespUnmarshalError{
+		Resp:       resp,
+		Problem:    "this is your problem",
+		Suggestion: "test your things",
+	}
+	err.TmpFile, _ = ioutil.TempFile(os.TempDir(), "themekit-response-*.txt")
+	err.TmpFile.Write([]byte("body"))
+	err.TmpFile.Close()
+	defer os.Remove(err.TmpFile.Name())
+
+	assert.Equal(t, err.Error(), fmt.Sprintf(`this is your problem
+test your things
+Http Response Status: 442
+Request ID: abc-123-456
+ResponseBody: %v`, err.TmpFile.Name()))
+}
+
+func TestUnmarshalResponse(t *testing.T) {
+	testcases := []struct {
+		input, err    string
+		code          int
+		out, expected themeResponse
+	}{
+		{input: `{"errors":{"name":["can't be blank"]}}`, code: 200, expected: themeResponse{Errors: map[string][]string{"name": {"can't be blank"}}}},
+		{input: `{"errors": "Not Found"}`, code: 404, err: "Not Found"},
+		{input: `{"theme":{"id": 123456}}`, code: 200, expected: themeResponse{Theme: Theme{ID: int64(123456)}}},
+		{input: `{"theme":{"id": 123456}}`, code: 200, expected: themeResponse{Theme: Theme{ID: int64(123456)}}},
+		{input: `<html><body>BAD ERROR</body></html>`, code: 500, err: "could not unmarshal JSON from response body"},
+	}
+
+	for _, testcase := range testcases {
+		err := unmarshalResponse(jsonResponse(testcase.input, testcase.code), &testcase.out)
+		assert.Equal(t, testcase.expected, testcase.out)
+		if testcase.err == "" {
+			assert.Nil(t, err)
+		} else if assert.NotNil(t, err) {
+			assert.Contains(t, err.Error(), testcase.err)
+		}
+	}
+
+	out := assetsResponse{}
+	err := unmarshalResponse(jsonResponse(`{"errors":"oh no"}`, 200), &out)
+	assert.NotNil(t, err)
+	assert.Equal(t, err.Error(), "oh no")
+}