[MM-12452] Stop inserting hashed plugin keys (#9817)

* Stop inserting hashed plugin keys

* Address comments

* Remove else statement

* Address comments

* Add comment

* Update as per comments

* Refactor as per comments

* Update plugin_key_value_store.go

minor comment tweaks

* fail on non StatusNotFound for original key query

* idiomatic error handling, and do not clean up if insert fails

app/plugin_key_value_store.go

@@ -23,83 +23,91 @@ func (a *App) SetPluginKey(pluginId string, key string, value []byte) *model.App
 }
 
 func (a *App) SetPluginKeyWithExpiry(pluginId string, key string, value []byte, expireInSeconds int64) *model.AppError {
-
 	if expireInSeconds > 0 {
 		expireInSeconds = model.GetMillis() + (expireInSeconds * 1000)
 	}
 
 	kv := &model.PluginKeyValue{
 		PluginId: pluginId,
-		Key:      getKeyHash(key),
+		Key:      key,
 		Value:    value,
 		ExpireAt: expireInSeconds,
 	}
 
-	result := <-a.Srv.Store.Plugin().SaveOrUpdate(kv)
+	if result := <-a.Srv.Store.Plugin().SaveOrUpdate(kv); result.Err != nil {
-
+		mlog.Error("Failed to set plugin key value", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(result.Err))
-	if result.Err != nil {
+		return result.Err
-		mlog.Error(result.Err.Error())
 	}
 
-	return result.Err
+	// Clean up a previous entry using the hashed key, if it exists.
+	if result := <-a.Srv.Store.Plugin().Delete(pluginId, getKeyHash(key)); result.Err != nil {
+		mlog.Error("Failed to clean up previously hashed plugin key value", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(result.Err))
+	}
+
+	return nil
 }
 
 func (a *App) GetPluginKey(pluginId string, key string) ([]byte, *model.AppError) {
-	result := <-a.Srv.Store.Plugin().Get(pluginId, getKeyHash(key))
+	if result := <-a.Srv.Store.Plugin().Get(pluginId, key); result.Err == nil {
-
+		return result.Data.(*model.PluginKeyValue).Value, nil
-	if result.Err != nil {
+	} else if result.Err.StatusCode != http.StatusNotFound {
-		if result.Err.StatusCode == http.StatusNotFound {
+		mlog.Error("Failed to query plugin key value", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(result.Err))
-			return nil, nil
-		}
-		mlog.Error(result.Err.Error())
 		return nil, result.Err
 	}
 
-	kv := result.Data.(*model.PluginKeyValue)
+	// Lookup using the hashed version of the key for keys written prior to v5.6.
+	if result := <-a.Srv.Store.Plugin().Get(pluginId, getKeyHash(key)); result.Err == nil {
+		return result.Data.(*model.PluginKeyValue).Value, nil
+	} else if result.Err.StatusCode != http.StatusNotFound {
+		mlog.Error("Failed to query plugin key value using hashed key", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(result.Err))
+		return nil, result.Err
+	}
 
-	return kv.Value, nil
+	return nil, nil
 }
 
 func (a *App) DeletePluginKey(pluginId string, key string) *model.AppError {
-	result := <-a.Srv.Store.Plugin().Delete(pluginId, getKeyHash(key))
+	if result := <-a.Srv.Store.Plugin().Delete(pluginId, getKeyHash(key)); result.Err != nil {
-
+		mlog.Error("Failed to delete plugin key value", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(result.Err))
-	if result.Err != nil {
+		return result.Err
-		mlog.Error(result.Err.Error())
 	}
 
+	// Also delete the key without hashing
+	if result := <-a.Srv.Store.Plugin().Delete(pluginId, key); result.Err != nil {
+		mlog.Error("Failed to delete plugin key value using hashed key", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(result.Err))
 		return result.Err
 	}
 
+	return nil
+}
+
 func (a *App) DeleteAllKeysForPlugin(pluginId string) *model.AppError {
-	result := <-a.Srv.Store.Plugin().DeleteAllForPlugin(pluginId)
+	if result := <-a.Srv.Store.Plugin().DeleteAllForPlugin(pluginId); result.Err != nil {
-
+		mlog.Error("Failed to delete all plugin key values", mlog.String("plugin_id", pluginId), mlog.Err(result.Err))
-	if result.Err != nil {
-		mlog.Error(result.Err.Error())
-	}
-
 		return result.Err
 	}
 
-func (a *App) DeleteAllExpiredPluginKeys() *model.AppError {
+	return nil
+}
 
+func (a *App) DeleteAllExpiredPluginKeys() *model.AppError {
 	if a.Srv == nil {
 		return nil
 	}
 
-	result := <-a.Srv.Store.Plugin().DeleteAllExpired()
+	if result := <-a.Srv.Store.Plugin().DeleteAllExpired(); result.Err != nil {
-
+		mlog.Error("Failed to delete all expired plugin key values", mlog.Err(result.Err))
-	if result.Err != nil {
+		return result.Err
-		mlog.Error(result.Err.Error())
 	}
 
-	return result.Err
+	return nil
 }
 
 func (a *App) ListPluginKeys(pluginId string, page, perPage int) ([]string, *model.AppError) {
 	result := <-a.Srv.Store.Plugin().List(pluginId, page, perPage)
 
 	if result.Err != nil {
-		mlog.Error(result.Err.Error())
+		mlog.Error("Failed to list plugin key values", mlog.Int("page", page), mlog.Int("perPage", perPage), mlog.Err(result.Err))
 		return nil, result.Err
 	}
 

app/plugin_test.go

@@ -24,12 +24,18 @@ func getHashedKey(key string) string {
 	hash.Write([]byte(key))
 	return base64.StdEncoding.EncodeToString(hash.Sum(nil))
 }
+
 func TestPluginKeyValueStore(t *testing.T) {
 	th := Setup().InitBasic()
 	defer th.TearDown()
 
 	pluginId := "testpluginid"
 
+	defer func() {
+		assert.Nil(t, th.App.DeletePluginKey(pluginId, "key"))
+		assert.Nil(t, th.App.DeletePluginKey(pluginId, "key2"))
+	}()
+
 	assert.Nil(t, th.App.SetPluginKey(pluginId, "key", []byte("test")))
 	ret, err := th.App.GetPluginKey(pluginId, "key")
 	assert.Nil(t, err)
@@ -37,25 +43,45 @@ func TestPluginKeyValueStore(t *testing.T) {
 
 	// Test inserting over existing entries
 	assert.Nil(t, th.App.SetPluginKey(pluginId, "key", []byte("test2")))
+	ret, err = th.App.GetPluginKey(pluginId, "key")
+	assert.Nil(t, err)
+	assert.Equal(t, []byte("test2"), ret)
 
 	// Test getting non-existent key
 	ret, err = th.App.GetPluginKey(pluginId, "notakey")
 	assert.Nil(t, err)
 	assert.Nil(t, ret)
 
-	assert.Nil(t, th.App.DeletePluginKey(pluginId, "stringkey"))
+	// Test deleting non-existent keys.
-	assert.Nil(t, th.App.DeletePluginKey(pluginId, "intkey"))
-	assert.Nil(t, th.App.DeletePluginKey(pluginId, "postkey"))
 	assert.Nil(t, th.App.DeletePluginKey(pluginId, "notrealkey"))
 
-	// Test ListKeys
+	// Verify behaviour for the old approach that involved storing the hashed keys.
-	assert.Nil(t, th.App.SetPluginKey(pluginId, "key2", []byte("test")))
-	hashedKey := getHashedKey("key")
 	hashedKey2 := getHashedKey("key2")
+	kv := &model.PluginKeyValue{
+		PluginId: pluginId,
+		Key:      hashedKey2,
+		Value:    []byte("test"),
+		ExpireAt: 0,
+	}
+
+	result := <-th.App.Srv.Store.Plugin().SaveOrUpdate(kv)
+	assert.Nil(t, result.Err)
+
+	// Test fetch by keyname (this key does not exist but hashed key will be used for lookup)
+	ret, err = th.App.GetPluginKey(pluginId, "key2")
+	assert.Nil(t, err)
+	assert.Equal(t, kv.Value, ret)
+
+	// Test fetch by hashed keyname
+	ret, err = th.App.GetPluginKey(pluginId, hashedKey2)
+	assert.Nil(t, err)
+	assert.Equal(t, kv.Value, ret)
+
+	// Test ListKeys
 	list, err := th.App.ListPluginKeys(pluginId, 0, 1)
 	assert.Nil(t, err)
 	assert.Equal(t, 1, len(list))
-	assert.Equal(t, hashedKey, list[0])
+	assert.Equal(t, "key", list[0])
 
 	list, err = th.App.ListPluginKeys(pluginId, 1, 1)
 	assert.Nil(t, err)

model/plugin_key_value_test.go

@@ -19,4 +19,7 @@ func TestPluginKeyIsValid(t *testing.T) {
 	kv.PluginId = "someid"
 	kv.Key = ""
 	assert.NotNil(t, kv.IsValid())
+
+	kv.Key = "this is an extremely long key and should be invalid and this is being verified in this test"
+	assert.NotNil(t, kv.IsValid())
 }