From 514c1f16174f9fbb461295ba64bf2105dcdc59d0 Mon Sep 17 00:00:00 2001 From: Shobhit Gupta Date: Tue, 27 Nov 2018 12:50:31 -0800 Subject: [PATCH] [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 | 74 +++++++++++++++++++--------------- app/plugin_test.go | 42 +++++++++++++++---- model/plugin_key_value_test.go | 3 ++ 3 files changed, 78 insertions(+), 41 deletions(-) diff --git a/app/plugin_key_value_store.go b/app/plugin_key_value_store.go index f0aed31f04..aecf0b4aef 100644 --- a/app/plugin_key_value_store.go +++ b/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.Err != nil { - mlog.Error(result.Err.Error()) + 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)) + return result.Err } - 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.Err != nil { - if result.Err.StatusCode == http.StatusNotFound { - return nil, nil - } - mlog.Error(result.Err.Error()) + if result := <-a.Srv.Store.Plugin().Get(pluginId, 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", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(result.Err)) 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.Err != nil { - mlog.Error(result.Err.Error()) + 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)) + return result.Err } - return result.Err + // 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.Err != nil { - mlog.Error(result.Err.Error()) + 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)) + return result.Err } - return result.Err + return nil } func (a *App) DeleteAllExpiredPluginKeys() *model.AppError { - if a.Srv == nil { return nil } - result := <-a.Srv.Store.Plugin().DeleteAllExpired() - - if result.Err != nil { - mlog.Error(result.Err.Error()) + if result := <-a.Srv.Store.Plugin().DeleteAllExpired(); result.Err != nil { + mlog.Error("Failed to delete all expired plugin key values", mlog.Err(result.Err)) + return result.Err } - 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 } diff --git a/app/plugin_test.go b/app/plugin_test.go index 0221a763e4..aab684fc45 100644 --- a/app/plugin_test.go +++ b/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,32 +43,52 @@ 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")) - assert.Nil(t, th.App.DeletePluginKey(pluginId, "intkey")) - assert.Nil(t, th.App.DeletePluginKey(pluginId, "postkey")) + // Test deleting non-existent keys. assert.Nil(t, th.App.DeletePluginKey(pluginId, "notrealkey")) - // Test ListKeys - assert.Nil(t, th.App.SetPluginKey(pluginId, "key2", []byte("test"))) - hashedKey := getHashedKey("key") + // Verify behaviour for the old approach that involved storing the hashed keys. 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) assert.Equal(t, 1, len(list)) assert.Equal(t, hashedKey2, list[0]) - //List Keys bad input + // List Keys bad input list, err = th.App.ListPluginKeys(pluginId, 0, 0) assert.Nil(t, err) assert.Equal(t, 2, len(list)) diff --git a/model/plugin_key_value_test.go b/model/plugin_key_value_test.go index 82dd7d561c..cbbab54cd3 100644 --- a/model/plugin_key_value_test.go +++ b/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()) }