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

@@ -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
 }

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)
 	})
 }