Repository: Shopify/themekit
Pull request #745 (2020-07-30 01:25:55)

Send previous checksum with updates
This adds new behaviour for the `watch` command. We send a new header `X-Shopify-Replace-If-Checksum-Match` when updating an asset. This allows us to determine if an asset has been changed by someone else working on the site at the same time, to avoid accidental overwrites.

The server-side aspect of this has not yet been implemented, but this is a step towards that.

### Warn Checklist
- [ ] This changes the interface and requires a Major/Minor version change.
- [x] I have :tophat:'d these changes by using the commands I changed by hand.
- [ ] I have added a dependancy to the project.


andyw8: @tanema I'm still doing some testing and cleanup, but let me know if you have any initial thoughts here.

Patch:
diff --git a/cmd/deploy.go b/cmd/deploy.go
index c8ac3c0..9836bf0 100644
--- a/cmd/deploy.go
+++ b/cmd/deploy.go
@@ -74,13 +74,13 @@ func deploy(ctx *cmdutil.Ctx) error {
 	ctx.StartProgress(len(assetsActions))
 	for path, op := range assetsActions {
 		if path == settingsDataKey {
-			defer perform(ctx, path, op)
+			defer perform(ctx, path, op, "")
 			continue
 		}
 		deployGroup.Add(1)
 		go func(path string, op file.Op) {
 			defer deployGroup.Done()
-			perform(ctx, path, op)
+			perform(ctx, path, op, "")
 		}(path, op)
 
 		switch op {
diff --git a/cmd/remove.go b/cmd/remove.go
index 5e3a23d..2552c03 100644
--- a/cmd/remove.go
+++ b/cmd/remove.go
@@ -40,7 +40,7 @@ func remove(ctx *cmdutil.Ctx, removeFile func(string) error) error {
 		removeGroup.Add(1)
 		go func(filename string) {
 			defer removeGroup.Done()
-			perform(ctx, filename, file.Remove)
+			perform(ctx, filename, file.Remove, "")
 			removeFile(filepath.Join(ctx.Env.Directory, filename))
 		}(filename)
 	}
diff --git a/cmd/watch.go b/cmd/watch.go
index 20a0cc6..1e87efa 100644
--- a/cmd/watch.go
+++ b/cmd/watch.go
@@ -79,14 +79,14 @@ func watch(ctx *cmdutil.Ctx, events chan file.Event, sig chan os.Signal) error {
 				return cmdutil.ErrReload
 			}
 			ctx.Log.Printf("[%s] processing %s", colors.Green(ctx.Env.Name), colors.Blue(event.Path))
-			perform(ctx, event.Path, event.Op)
+			perform(ctx, event.Path, event.Op, event.LastKnownChecksum)
 		case <-sig:
 			return nil
 		}
 	}
 }
 
-func perform(ctx *cmdutil.Ctx, path string, op file.Op) {
+func perform(ctx *cmdutil.Ctx, path string, op file.Op, checksum string) {
 	defer ctx.DoneTask()
 
 	switch op {
@@ -113,7 +113,7 @@ func perform(ctx *cmdutil.Ctx, path string, op file.Op) {
 			return
 		}
 
-		if err := ctx.Client.UpdateAsset(asset); err != nil {
+		if err := ctx.Client.UpdateAsset(asset, checksum); err != nil {
 			ctx.Err("[%s] (%s) %s", colors.Green(ctx.Env.Name), colors.Blue(asset.Key), err)
 		} else if ctx.Flags.Verbose {
 			ctx.Log.Printf("[%s] Updated %s", colors.Green(ctx.Env.Name), colors.Blue(asset.Key))
diff --git a/src/cmdutil/_mocks/ShopifyClient.go b/src/cmdutil/_mocks/ShopifyClient.go
index 798d3e8..d181a14 100644
--- a/src/cmdutil/_mocks/ShopifyClient.go
+++ b/src/cmdutil/_mocks/ShopifyClient.go
@@ -1,4 +1,4 @@
-// Code generated by mockery v0.0.0-dev. DO NOT EDIT.
+// Code generated by mockery v1.0.0. DO NOT EDIT.
 
 package mocks
 
@@ -7,7 +7,7 @@ import (
 	mock "github.com/stretchr/testify/mock"
 )
 
-// ShopifyClient is an autogenerated mock type for the shopifyClient type
+// ShopifyClient is an autogenerated mock type for the ShopifyClient type
 type ShopifyClient struct {
 	mock.Mock
 }
@@ -170,13 +170,13 @@ func (_m *ShopifyClient) Themes() ([]shopify.Theme, error) {
 	return r0, r1
 }
 
-// UpdateAsset provides a mock function with given fields: _a0
-func (_m *ShopifyClient) UpdateAsset(_a0 shopify.Asset) error {
-	ret := _m.Called(_a0)
+// UpdateAsset provides a mock function with given fields: _a0, _a1
+func (_m *ShopifyClient) UpdateAsset(_a0 shopify.Asset, _a1 string) error {
+	ret := _m.Called(_a0, _a1)
 
 	var r0 error
-	if rf, ok := ret.Get(0).(func(shopify.Asset) error); ok {
-		r0 = rf(_a0)
+	if rf, ok := ret.Get(0).(func(shopify.Asset, string) error); ok {
+		r0 = rf(_a0, _a1)
 	} else {
 		r0 = ret.Error(0)
 	}
diff --git a/src/cmdutil/interfaces.go b/src/cmdutil/interfaces.go
index a371c27..cacb211 100644
--- a/src/cmdutil/interfaces.go
+++ b/src/cmdutil/interfaces.go
@@ -13,7 +13,7 @@ type shopifyClient interface {
 	Themes() ([]shopify.Theme, error)
 	GetAllAssets() ([]shopify.Asset, error)
 	GetAsset(string) (shopify.Asset, error)
-	UpdateAsset(shopify.Asset) error
+	UpdateAsset(shopify.Asset, string) error
 	DeleteAsset(shopify.Asset) error
 }
 
diff --git a/src/file/watcher.go b/src/file/watcher.go
index 7abe923..d0d6377 100644
--- a/src/file/watcher.go
+++ b/src/file/watcher.go
@@ -38,9 +38,10 @@ var (
 
 // Event decsribes a file change event
 type Event struct {
-	Op       Op
-	Path     string
-	checksum string
+	Op                Op
+	Path              string
+	LastKnownChecksum string
+	checksum          string
 }
 
 // Watcher is the object used to watch files for change and notify on any events,
@@ -156,7 +157,7 @@ func (w *Watcher) updateChecksum(e Event) {
 func (w *Watcher) translateEvent(event watcher.Event) []Event {
 	oldPath, currentPath := w.parsePath(event.Path)
 	if isEventType(event.Op, watcher.Rename, watcher.Move) {
-		return []Event{{Op: Remove, Path: oldPath}, {Op: Update, Path: currentPath}}
+		return []Event{{Op: Remove, Path: oldPath}, {Op: Update, Path: currentPath, LastKnownChecksum: w.checksums[currentPath]}}
 	} else if isEventType(event.Op, watcher.Remove) {
 		return []Event{{Op: Remove, Path: currentPath}}
 	} else if isEventType(event.Op, watcher.Create, watcher.Write) {
@@ -165,7 +166,7 @@ func (w *Watcher) translateEvent(event watcher.Event) []Event {
 		if err == nil && checksum == w.checksums[currentPath] {
 			eventOp = Skip
 		}
-		return []Event{{Op: eventOp, Path: currentPath, checksum: checksum}}
+		return []Event{{Op: eventOp, Path: currentPath, checksum: checksum, LastKnownChecksum: w.checksums[currentPath]}}
 	}
 	return []Event{}
 }
diff --git a/src/shopify/theme_client.go b/src/shopify/theme_client.go
index c3eb685..a74387d 100644
--- a/src/shopify/theme_client.go
+++ b/src/shopify/theme_client.go
@@ -283,14 +283,18 @@ func (c Client) GetAsset(filename string) (Asset, error) {
 // If there was an error, in the request then error will be defined otherwise the
 // response will have the appropriate data for usage.
 func (c Client) CreateAsset(asset Asset) error {
-	return c.UpdateAsset(asset)
+	return c.UpdateAsset(asset, "")
 }
 
-// UpdateAsset will take an asset and will return  when the asset has been updated.
+// UpdateAsset will take an asset and will return when the asset has been updated.
 // If there was an error, in the request then error will be defined otherwise the
 // response will have the appropriate data for usage.
-func (c Client) UpdateAsset(asset Asset) error {
-	resp, err := c.http.Put(c.assetPath(map[string]string{}), map[string]Asset{"asset": asset}, nil)
+func (c Client) UpdateAsset(asset Asset, lastKnownChecksum string) error {
+	var header = make(map[string]string)
+	if lastKnownChecksum != "" {
+		header["X-Shopify-Replace-If-Checksum-Match"] = lastKnownChecksum
+	}
+	resp, err := c.http.Put(c.assetPath(map[string]string{}), map[string]Asset{"asset": asset}, header)
 	if err != nil {
 		return err
 	} else if resp.StatusCode == 404 {
@@ -307,7 +311,7 @@ func (c Client) UpdateAsset(asset Asset) error {
 			if resp.StatusCode == 422 && strings.Contains(r.Errors["asset"][0], "Cannot overwrite generated asset") {
 				// No need to check the error because if it fails then remove will be tried again.
 				c.DeleteAsset(Asset{Key: asset.Key + ".liquid"})
-				return c.UpdateAsset(asset)
+				return c.UpdateAsset(asset, lastKnownChecksum)
 			}
 			return errors.New(toSentence(r.Errors["asset"]))
 		}


Test patch:
diff --git a/cmd/deploy_test.go b/cmd/deploy_test.go
index bcb2108..9feed1f 100644
--- a/cmd/deploy_test.go
+++ b/cmd/deploy_test.go
@@ -19,7 +19,7 @@ func TestUploadSingleFile(t *testing.T) {
 	ctx.Args = []string{"templates/layout.liquid"}
 	ctx.Flags.NoDelete = true
 	client.On("GetAllAssets").Return([]shopify.Asset{{Key: "templates/layout.liquid"}}, nil)
-	client.On("UpdateAsset", shopify.Asset{Key: "templates/layout.liquid"}).Return(nil)
+	client.On("UpdateAsset", shopify.Asset{Key: "templates/layout.liquid"}, "").Return(nil)
 	err := deploy(ctx)
 	assert.NotNil(t, err)
 }
@@ -29,7 +29,7 @@ func TestUploadWithReadOnlyOption(t *testing.T) {
 	ctx.Args = []string{"templates/layout.liquid"}
 	ctx.Flags.NoDelete = true
 	ctx.Env.ReadOnly = true
-	client.On("UpdateAsset", shopify.Asset{Key: "templates/layout.liquid"}).Return(nil)
+	client.On("UpdateAsset", shopify.Asset{Key: "templates/layout.liquid"}, "").Return(nil)
 	err := deploy(ctx)
 	if assert.NotNil(t, err) {
 		assert.Contains(t, err.Error(), "environment is readonly")
@@ -44,7 +44,7 @@ func TestUploadWithVerboseOptions(t *testing.T) {
 	ctx.Flags.Verbose = true
 	client.On("GetAllAssets").Return([]shopify.Asset{{Key: "assets/app.js"}}, nil)
 	// This checksum corresponds to a zero-byte file
-	client.On("UpdateAsset", shopify.Asset{Key: "assets/app.js", Checksum: "d41d8cd98f00b204e9800998ecf8427e"}).Return(nil)
+	client.On("UpdateAsset", shopify.Asset{Key: "assets/app.js", Checksum: "d41d8cd98f00b204e9800998ecf8427e"}, "").Return(nil)
 	err := deploy(ctx)
 	assert.Nil(t, err)
 	assert.Contains(t, stdOut.String(), "Updated assets/app.js")
@@ -56,7 +56,7 @@ func TestUploadAllFiles(t *testing.T) {
 	ctx.Flags.Verbose = true
 	ctx.Flags.NoDelete = true
 	client.On("GetAllAssets").Return([]shopify.Asset{{Key: "config/settings_data.json"}}, nil)
-	client.On("UpdateAsset", mock.MatchedBy(func(a shopify.Asset) bool { return true })).Return(nil)
+	client.On("UpdateAsset", mock.MatchedBy(func(a shopify.Asset) bool { return true }), "").Return(nil)
 	err := deploy(ctx)
 	assert.Nil(t, err)
 	assert.Contains(t, stdOut.String(), "Updated config/settings_data.json")
@@ -69,7 +69,7 @@ func TestUploadForSkipFileWhenChecksumsMatch(t *testing.T) {
 	ctx.Flags.NoDelete = true
 	client.On("GetAllAssets").Return([]shopify.Asset{{Key: "config/settings_data.json", Checksum: "d41d8cd98f00b204e9800998ecf8427e"}}, nil)
 	// the _testdirectory contains two assets. We expect one to be uploaded, one to be skipped.
-	client.On("UpdateAsset", shopify.Asset{Key: "assets/app.js", Checksum: "d41d8cd98f00b204e9800998ecf8427e"}).Return(nil)
+	client.On("UpdateAsset", shopify.Asset{Key: "assets/app.js", Checksum: "d41d8cd98f00b204e9800998ecf8427e"}, "").Return(nil)
 	err := deploy(ctx)
 	assert.Nil(t, err)
 	assert.Contains(t, stdOut.String(), "Skipped config/settings_data.json")
@@ -82,7 +82,7 @@ func TestUploadForDoNotSkipWhenChecksumsDiffer(t *testing.T) {
 	ctx.Flags.Verbose = true
 	ctx.Flags.NoDelete = true
 	client.On("GetAllAssets").Return([]shopify.Asset{{Key: "config/settings_data.json", Checksum: "abc123"}}, nil)
-	client.On("UpdateAsset", mock.MatchedBy(func(a shopify.Asset) bool { return true })).Return(nil)
+	client.On("UpdateAsset", mock.MatchedBy(func(a shopify.Asset) bool { return true }), "").Return(nil)
 	err := deploy(ctx)
 	assert.Nil(t, err)
 	assert.Contains(t, stdOut.String(), "Updated config/settings_data.json")
@@ -93,7 +93,7 @@ func TestReplace(t *testing.T) {
 	ctx.Flags.Verbose = true
 	ctx.Env.Directory = filepath.Join("_testdata", "projectdir")
 	client.On("GetAllAssets").Return([]shopify.Asset{{Key: "assets/logo.png"}}, nil)
-	client.On("UpdateAsset", mock.MatchedBy(func(shopify.Asset) bool { return true })).Return(nil).Times(2)
+	client.On("UpdateAsset", mock.MatchedBy(func(shopify.Asset) bool { return true }), "").Return(nil).Times(2)
 	client.On("DeleteAsset", mock.MatchedBy(func(shopify.Asset) bool { return true })).Return(nil).Once()
 	err := deploy(ctx)
 	assert.Nil(t, err)
diff --git a/cmd/watch_test.go b/cmd/watch_test.go
index be38872..b20505e 100644
--- a/cmd/watch_test.go
+++ b/cmd/watch_test.go
@@ -48,7 +48,7 @@ func TestWatch(t *testing.T) {
 	signalChan = make(chan os.Signal)
 	eventChan = make(chan file.Event)
 	ctx, client, _, stdOut, stdErr := createTestCtx()
-	client.On("UpdateAsset", shopify.Asset{Key: "assets/app.js", Checksum: "d41d8cd98f00b204e9800998ecf8427e"}).Return(nil)
+	client.On("UpdateAsset", shopify.Asset{Key: "assets/app.js", Checksum: "d41d8cd98f00b204e9800998ecf8427e"}, "").Return(nil)
 	ctx.Flags.ConfigPath = "config.yml"
 	ctx.Env.Directory = "_testdata/projectdir"
 	go func() {
@@ -82,29 +82,29 @@ func TestPerform(t *testing.T) {
 	key := "assets/app.js"
 
 	ctx, m, _, _, se := createTestCtx()
-	perform(ctx, "bad", file.Update)
+	perform(ctx, "bad", file.Update, "")
 	assert.Contains(t, se.String(), "readAsset: ")
 	m.AssertExpectations(t)
 
 	ctx, m, _, _, se = createTestCtx()
 	ctx.Env.Directory = "_testdata/projectdir"
-	m.On("UpdateAsset", shopify.Asset{Key: key, Checksum: "d41d8cd98f00b204e9800998ecf8427e"}).Return(fmt.Errorf("shopify says no update"))
-	perform(ctx, key, file.Update)
+	m.On("UpdateAsset", shopify.Asset{Key: key, Checksum: "d41d8cd98f00b204e9800998ecf8427e"}, "").Return(fmt.Errorf("shopify says no update"), "")
+	perform(ctx, key, file.Update, "")
 	assert.Contains(t, se.String(), "shopify says no update")
 	m.AssertExpectations(t)
 
 	ctx, m, _, so, _ := createTestCtx()
 	ctx.Env.Directory = "_testdata/projectdir"
-	m.On("UpdateAsset", shopify.Asset{Key: key, Checksum: "d41d8cd98f00b204e9800998ecf8427e"}).Return(nil)
-	perform(ctx, key, file.Update)
+	m.On("UpdateAsset", shopify.Asset{Key: key, Checksum: "d41d8cd98f00b204e9800998ecf8427e"}, "").Return(nil)
+	perform(ctx, key, file.Update, "")
 	assert.NotContains(t, so.String(), "Updated")
 	m.AssertExpectations(t)
 
 	ctx, m, _, so, _ = createTestCtx()
 	ctx.Env.Directory = "_testdata/projectdir"
 	ctx.Flags.Verbose = true
-	m.On("UpdateAsset", shopify.Asset{Key: key, Checksum: "d41d8cd98f00b204e9800998ecf8427e"}).Return(nil)
-	perform(ctx, key, file.Update)
+	m.On("UpdateAsset", shopify.Asset{Key: key, Checksum: "d41d8cd98f00b204e9800998ecf8427e"}, "").Return(nil)
+	perform(ctx, key, file.Update, "")
 	assert.Contains(t, so.String(), "Updated")
 	m.AssertExpectations(t)
 
@@ -112,14 +112,14 @@ func TestPerform(t *testing.T) {
 	m.On("DeleteAsset", mock.MatchedBy(func(a shopify.Asset) bool { return a.Key == "good" })).Return(nil)
 	m.On("DeleteAsset", mock.MatchedBy(func(a shopify.Asset) bool { return a.Key == "bad" })).Return(fmt.Errorf("shopify says no update"))
 
-	perform(ctx, "bad", file.Remove)
+	perform(ctx, "bad", file.Remove, "")
 	assert.Contains(t, se.String(), "shopify says no update")
 
-	perform(ctx, "good", file.Remove)
+	perform(ctx, "good", file.Remove, "")
 	assert.NotContains(t, so.String(), "Deleted")
 
 	ctx.Flags.Verbose = true
-	perform(ctx, "good", file.Remove)
+	perform(ctx, "good", file.Remove, "")
 	assert.Contains(t, so.String(), "Deleted")
 
 	m.AssertExpectations(t)
diff --git a/src/shopify/theme_client_test.go b/src/shopify/theme_client_test.go
index 5fd07de..865e25c 100644
--- a/src/shopify/theme_client_test.go
+++ b/src/shopify/theme_client_test.go
@@ -358,14 +358,14 @@ 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"}}, NoHeaders)
+		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 {
 			expectation.Return(jsonResponse(testcase.resp, testcase.code), nil)
 		}
 
-		err := client.UpdateAsset(Asset{Key: "filename.txt"})
+		err := client.UpdateAsset(Asset{Key: "filename.txt"}, "")
 
 		if testcase.err == "" {
 			assert.Nil(t, err)
@@ -392,7 +392,7 @@ func TestThemeClient_UpdateAsset(t *testing.T) {
 			return false
 		}),
 		map[string]Asset{"asset": asset},
-		NoHeaders,
+		map[string]string{},
 	).Return(&http.Response{
 		Body:       &StringReadCloser{strings.NewReader(`{"errors":{"asset":["Cannot overwrite generated asset filename.txt"]}}`)},
 		StatusCode: 422,
@@ -408,10 +408,10 @@ func TestThemeClient_UpdateAsset(t *testing.T) {
 		"Put",
 		APIPath + "themes/123/assets.json",
 		map[string]Asset{"asset": asset},
-		NoHeaders,
+		map[string]string{},
 	).Return(jsonResponse(`{"asset":{"key":"assets/hello.txt"}}`, 200), nil)
 
-	assert.Nil(t, client.UpdateAsset(asset))
+	assert.Nil(t, client.UpdateAsset(asset, ""))
 	m.AssertExpectations(t)
 }