From 8afbe9c6a4e15ef39898aa801679ea6176759fd0 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Tue, 3 Sep 2019 17:17:20 -0300 Subject: [PATCH] MM-18167: fix KV* helpers error handling (#12023) * MM-18167: fix KV* helpers error handling Returning a `nil` `*model.AppError` does not make a `nil` `error` interface. As a consequence, all of the KV* helper methods would always appear to fail, even if the underlying API call was successful. Fix this mismatch by explicitly assigning `appErr` in the helpers and only ever returning a `nil` `error` interface. Extend unit tests to achieve 100% coverage of the associated files. In the long run, we must change all function signatures to return the `error` interface instead of the abomination that is returning `*model.AppError` today. --- plugin/helpers_kv.go | 28 ++++++++++-- plugin/helpers_kv_test.go | 89 ++++++++++++++++++++++++++++++++------- 2 files changed, 97 insertions(+), 20 deletions(-) diff --git a/plugin/helpers_kv.go b/plugin/helpers_kv.go index b489ba7591..78efa31467 100644 --- a/plugin/helpers_kv.go +++ b/plugin/helpers_kv.go @@ -34,7 +34,12 @@ func (p *HelpersImpl) KVSetJSON(key string, value interface{}) error { return err } - return p.API.KVSet(key, data) + appErr := p.API.KVSet(key, data) + if appErr != nil { + return appErr + } + + return nil } // KVCompareAndSetJSON is a wrapper around KVCompareAndSet to simplify atomically writing a JSON object to the key value store. @@ -56,7 +61,12 @@ func (p *HelpersImpl) KVCompareAndSetJSON(key string, oldValue interface{}, newV } } - return p.API.KVCompareAndSet(key, oldData, newData) + set, appErr := p.API.KVCompareAndSet(key, oldData, newData) + if appErr != nil { + return set, appErr + } + + return set, nil } // KVCompareAndDeleteJSON is a wrapper around KVCompareAndDelete to simplify atomically deleting a JSON object from the key value store. @@ -71,7 +81,12 @@ func (p *HelpersImpl) KVCompareAndDeleteJSON(key string, oldValue interface{}) ( } } - return p.API.KVCompareAndDelete(key, oldData) + deleted, appErr := p.API.KVCompareAndDelete(key, oldData) + if appErr != nil { + return deleted, appErr + } + + return deleted, nil } // KVSetWithExpiryJSON is a wrapper around KVSetWithExpiry to simplify atomically writing a JSON object with expiry to the key value store. @@ -81,5 +96,10 @@ func (p *HelpersImpl) KVSetWithExpiryJSON(key string, value interface{}, expireI return err } - return p.API.KVSetWithExpiry(key, data, expireInSeconds) + appErr := p.API.KVSetWithExpiry(key, data, expireInSeconds) + if appErr != nil { + return appErr + } + + return nil } diff --git a/plugin/helpers_kv_test.go b/plugin/helpers_kv_test.go index 23a6d37fdf..f61033dacb 100644 --- a/plugin/helpers_kv_test.go +++ b/plugin/helpers_kv_test.go @@ -22,7 +22,7 @@ func TestKVGetJSON(t *testing.T) { ok, err := p.KVGetJSON("test-key", dat) api.AssertExpectations(t) assert.False(t, ok) - assert.NotNil(t, err) + assert.Error(t, err) assert.Nil(t, dat) }) @@ -38,7 +38,7 @@ func TestKVGetJSON(t *testing.T) { ok, err := p.KVGetJSON("test-key", dat) api.AssertExpectations(t) assert.False(t, ok) - assert.Nil(t, err) + assert.NoError(t, err) assert.Nil(t, dat) }) @@ -54,7 +54,7 @@ func TestKVGetJSON(t *testing.T) { ok, err := p.KVGetJSON("test-key", &dat) api.AssertExpectations(t) assert.False(t, ok) - assert.NotNil(t, err) + assert.Error(t, err) assert.Nil(t, dat) }) @@ -70,7 +70,7 @@ func TestKVGetJSON(t *testing.T) { ok, err := p.KVGetJSON("test-key", &dat) assert.True(t, ok) api.AssertExpectations(t) - assert.Nil(t, err) + assert.NoError(t, err) assert.Equal(t, map[string]interface{}{ "val-a": float64(10), }, dat) @@ -86,7 +86,21 @@ func TestKVSetJSON(t *testing.T) { err := p.KVSetJSON("test-key", func() { return }) api.AssertExpectations(t) - assert.NotNil(t, err) + assert.Error(t, err) + }) + + t.Run("KVSet error", func(t *testing.T) { + api := &plugintest.API{} + api.On("KVSet", "test-key", []byte(`{"val-a":10}`)).Return(&model.AppError{}) + + p := &plugin.HelpersImpl{API: api} + + err := p.KVSetJSON("test-key", map[string]interface{}{ + "val-a": float64(10), + }) + + api.AssertExpectations(t) + assert.Error(t, err) }) t.Run("marshallable struct", func(t *testing.T) { @@ -100,7 +114,7 @@ func TestKVSetJSON(t *testing.T) { }) api.AssertExpectations(t) - assert.Nil(t, err) + assert.NoError(t, err) }) } @@ -114,7 +128,7 @@ func TestKVCompareAndSetJSON(t *testing.T) { api.AssertExpectations(t) assert.Equal(t, false, ok) - assert.NotNil(t, err) + assert.Error(t, err) }) t.Run("new value JSON marshal error", func(t *testing.T) { @@ -127,7 +141,23 @@ func TestKVCompareAndSetJSON(t *testing.T) { api.AssertExpectations(t) assert.False(t, ok) - assert.NotNil(t, err) + assert.Error(t, err) + }) + + t.Run("KVCompareAndSet error", func(t *testing.T) { + api := &plugintest.API{} + api.On("KVCompareAndSet", "test-key", []byte(`{"val-a":10}`), []byte(`{"val-b":20}`)).Return(false, &model.AppError{}) + p := &plugin.HelpersImpl{API: api} + + ok, err := p.KVCompareAndSetJSON("test-key", map[string]interface{}{ + "val-a": 10, + }, map[string]interface{}{ + "val-b": 20, + }) + + api.AssertExpectations(t) + assert.False(t, ok) + assert.Error(t, err) }) t.Run("old value nil", func(t *testing.T) { @@ -141,7 +171,7 @@ func TestKVCompareAndSetJSON(t *testing.T) { api.AssertExpectations(t) assert.True(t, ok) - assert.Nil(t, err) + assert.NoError(t, err) }) t.Run("old value non-nil", func(t *testing.T) { @@ -157,7 +187,7 @@ func TestKVCompareAndSetJSON(t *testing.T) { api.AssertExpectations(t) assert.True(t, ok) - assert.Nil(t, err) + assert.NoError(t, err) }) t.Run("new value nil", func(t *testing.T) { @@ -171,7 +201,7 @@ func TestKVCompareAndSetJSON(t *testing.T) { api.AssertExpectations(t) assert.True(t, ok) - assert.Nil(t, err) + assert.NoError(t, err) }) } @@ -185,7 +215,21 @@ func TestKVCompareAndDeleteJSON(t *testing.T) { api.AssertExpectations(t) assert.Equal(t, false, ok) - assert.NotNil(t, err) + assert.Error(t, err) + }) + + t.Run("KVCompareAndDelete error", func(t *testing.T) { + api := &plugintest.API{} + api.On("KVCompareAndDelete", "test-key", []byte(`{"val-a":10}`)).Return(false, &model.AppError{}) + p := &plugin.HelpersImpl{API: api} + + ok, err := p.KVCompareAndDeleteJSON("test-key", map[string]interface{}{ + "val-a": 10, + }) + + api.AssertExpectations(t) + assert.False(t, ok) + assert.Error(t, err) }) t.Run("old value nil", func(t *testing.T) { @@ -197,7 +241,7 @@ func TestKVCompareAndDeleteJSON(t *testing.T) { api.AssertExpectations(t) assert.True(t, ok) - assert.Nil(t, err) + assert.NoError(t, err) }) t.Run("old value non-nil", func(t *testing.T) { @@ -211,7 +255,7 @@ func TestKVCompareAndDeleteJSON(t *testing.T) { api.AssertExpectations(t) assert.True(t, ok) - assert.Nil(t, err) + assert.NoError(t, err) }) } @@ -225,7 +269,20 @@ func TestKVSetWithExpiryJSON(t *testing.T) { err := p.KVSetWithExpiryJSON("test-key", func() { return }, 100) api.AssertExpectations(t) - assert.NotNil(t, err) + assert.Error(t, err) + }) + + t.Run("KVSetWithExpiry error", func(t *testing.T) { + api := &plugintest.API{} + api.On("KVSetWithExpiry", "test-key", []byte(`{"val-a":10}`), int64(100)).Return(&model.AppError{}) + p := &plugin.HelpersImpl{API: api} + + err := p.KVSetWithExpiryJSON("test-key", map[string]interface{}{ + "val-a": float64(10), + }, 100) + + api.AssertExpectations(t) + assert.Error(t, err) }) t.Run("wellformed JSON", func(t *testing.T) { @@ -239,6 +296,6 @@ func TestKVSetWithExpiryJSON(t *testing.T) { }, 100) api.AssertExpectations(t) - assert.Nil(t, err) + assert.NoError(t, err) }) }