From f3716caba9d6b55520e8431047db4b87b6f9f88f Mon Sep 17 00:00:00 2001 From: jfrerich Date: Thu, 31 Oct 2019 11:25:19 -0500 Subject: [PATCH 01/15] [MM-18331] When patching a bot send websocket notification (#12373) * When patching a bot, perform these two additional steps: 1. Update the user.UpdateAt value for the user/bot. 2. send the websocket event so all clients know a user update has occured. This will tell clients to update the displayname * Add check for UpdateAt. Check that createdBot.UpdateAt is not equal to patchedBot.UpdateAt * re-add unintentional empty line delete in previous commit * Don't create a fake updateAt time. Let the Update() method handle the change. User the returned updateUser for sending updated user event --- app/bot.go | 7 ++++++- app/bot_test.go | 3 +++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/app/bot.go b/app/bot.go index 0691e127cc..1e9561488f 100644 --- a/app/bot.go +++ b/app/bot.go @@ -77,10 +77,15 @@ func (a *App) PatchBot(botUserId string, botPatch *model.BotPatch) (*model.Bot, user.Username = patchedUser.Username user.Email = patchedUser.Email user.FirstName = patchedUser.FirstName - if _, err := a.Srv.Store.User().Update(user, true); err != nil { + + userUpdate, err := a.Srv.Store.User().Update(user, true) + if err != nil { return nil, err } + ruser := userUpdate.New + a.sendUpdatedUserEvent(*ruser) + return a.Srv.Store.Bot().Update(bot) } diff --git a/app/bot_test.go b/app/bot_test.go index b8a563047a..11c91b0a00 100644 --- a/app/bot_test.go +++ b/app/bot_test.go @@ -172,6 +172,9 @@ func TestPatchBot(t *testing.T) { patchedBot, err := th.App.PatchBot(createdBot.UserId, botPatch) require.Nil(t, err) + // patchedBot should create a new .UpdateAt time + require.NotEqual(t, createdBot.UpdateAt, patchedBot.UpdateAt) + createdBot.Username = "username2" createdBot.DisplayName = "updated bot" createdBot.Description = "an updated bot" From 7c658a98f01aaaabfd3cb9eaad28355f19fa38a3 Mon Sep 17 00:00:00 2001 From: catalintomai <56169943+catalintomai@users.noreply.github.com> Date: Thu, 31 Oct 2019 09:31:18 -0700 Subject: [PATCH 02/15] MM-18060: Include deleted posts in compliance export. (#12957) --- model/message_export.go | 1 + store/sqlstore/compliance_store.go | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/model/message_export.go b/model/message_export.go index 834c84e9eb..6011948dd3 100644 --- a/model/message_export.go +++ b/model/message_export.go @@ -21,6 +21,7 @@ type MessageExport struct { PostId *string PostCreateAt *int64 PostUpdateAt *int64 + PostDeleteAt *int64 PostMessage *string PostType *string PostRootId *string diff --git a/store/sqlstore/compliance_store.go b/store/sqlstore/compliance_store.go index 6dfcafca3f..35ff5f4039 100644 --- a/store/sqlstore/compliance_store.go +++ b/store/sqlstore/compliance_store.go @@ -213,6 +213,7 @@ func (s SqlComplianceStore) MessageExport(after int64, limit int) ([]*model.Mess Posts.Id AS PostId, Posts.CreateAt AS PostCreateAt, Posts.UpdateAt AS PostUpdateAt, + Posts.DeleteAt AS PostDeleteAt, Posts.Message AS PostMessage, Posts.Type AS PostType, Posts.OriginalId AS PostOriginalId, @@ -241,7 +242,7 @@ func (s SqlComplianceStore) MessageExport(after int64, limit int) ([]*model.Mess LEFT OUTER JOIN Users ON Posts.UserId = Users.Id LEFT JOIN Bots ON Bots.UserId = Posts.UserId WHERE - (Posts.CreateAt > :StartTime OR Posts.EditAt > :StartTime) AND + (Posts.CreateAt > :StartTime OR Posts.EditAt > :StartTime OR Posts.DeleteAt > :StartTime) AND Posts.Type = '' ORDER BY PostUpdateAt LIMIT :Limit` From 0697f5206cbad5103a893ec33de39aed25e55cc3 Mon Sep 17 00:00:00 2001 From: Christopher Poile Date: Thu, 31 Oct 2019 13:27:49 -0400 Subject: [PATCH 03/15] [MM-16437] Plugin crashes the server when calling WriteHeader with an invalid http code (#11276) * [MM-16437] add a check so that we don't write an invalid header * better solution * * passing in the logger; logging the error; fixed a spelling mistake * trigger jenkins --- plugin/client_rpc.go | 4 ++-- plugin/http.go | 13 ++++++++++++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/plugin/client_rpc.go b/plugin/client_rpc.go index 0ef0709358..66b7fcf489 100644 --- a/plugin/client_rpc.go +++ b/plugin/client_rpc.go @@ -286,8 +286,8 @@ func (g *hooksRPCClient) ServeHTTP(c *Context, w http.ResponseWriter, r *http.Re defer connection.Close() rpcServer := rpc.NewServer() - if err := rpcServer.RegisterName("Plugin", &httpResponseWriterRPCServer{w: w}); err != nil { - g.log.Error("Plugin failed to ServeHTTP, coulden't register RPC name", mlog.Err(err)) + if err := rpcServer.RegisterName("Plugin", &httpResponseWriterRPCServer{w: w, log: g.log}); err != nil { + g.log.Error("Plugin failed to ServeHTTP, couldn't register RPC name", mlog.Err(err)) return } rpcServer.ServeConn(connection) diff --git a/plugin/http.go b/plugin/http.go index 7d16503695..e39e36001f 100644 --- a/plugin/http.go +++ b/plugin/http.go @@ -4,13 +4,18 @@ package plugin import ( + "errors" + "fmt" "io" "net/http" "net/rpc" + + "github.com/mattermost/mattermost-server/mlog" ) type httpResponseWriterRPCServer struct { - w http.ResponseWriter + w http.ResponseWriter + log *mlog.Logger } func (w *httpResponseWriterRPCServer) Header(args struct{}, reply *http.Header) error { @@ -24,6 +29,12 @@ func (w *httpResponseWriterRPCServer) Write(args []byte, reply *struct{}) error } func (w *httpResponseWriterRPCServer) WriteHeader(args int, reply *struct{}) error { + // Check if args is a valid http status code. This prevents plugins from crashing the server with a panic. + // This is a copy of the checkWriteHeaderCode function in net/http/server.go in the go source. + if args < 100 || args > 999 { + w.log.Error(fmt.Sprintf("Plugin tried to write an invalid http status code: %v. Did not write the invalid header.", args)) + return errors.New("invalid http status code") + } w.w.WriteHeader(args) return nil } From 0b24c336b347b2c38fb06eb18906e35f2a869b34 Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Thu, 31 Oct 2019 21:53:40 +0100 Subject: [PATCH 04/15] Fix golangci-lint target (#12985) --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 08c3896881..4edd8b5b03 100644 --- a/Makefile +++ b/Makefile @@ -175,7 +175,7 @@ gofmt: ## Runs gofmt against all packages. golangci-lint: ## Run golangci-lint on codebasis # https://stackoverflow.com/a/677212/1027058 (check if a command exists or not) - @if ! [ -x "$$(command -v golangci-lintt)" ]; then \ + @if ! [ -x "$$(command -v golangci-lint)" ]; then \ echo "golangci-lint is not installed. Please see https://github.com/golangci/golangci-lint#install for installation instructions."; \ exit 1; \ fi; \ From 3687a0b6c1d4e69b9a58bb0daeab6fe59bc5966e Mon Sep 17 00:00:00 2001 From: Nikhil Ranjan Date: Fri, 1 Nov 2019 12:52:18 +0100 Subject: [PATCH 05/15] =?UTF-8?q?Migrate=20tests=20from=20store/storetest/?= =?UTF-8?q?status=5Fstore.go=20to=20use=20test=E2=80=A6=20(#12873)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- store/storetest/status_store.go | 43 ++++++++++++--------------------- 1 file changed, 15 insertions(+), 28 deletions(-) diff --git a/store/storetest/status_store.go b/store/storetest/status_store.go index e93489f166..a4073a1942 100644 --- a/store/storetest/status_store.go +++ b/store/storetest/status_store.go @@ -23,9 +23,8 @@ func testStatusStore(t *testing.T, ss store.Store) { status.LastActivityAt = 10 - if _, err := ss.Status().Get(status.UserId); err != nil { - t.Fatal(err) - } + _, err := ss.Status().Get(status.UserId) + require.Nil(t, err) status2 := &model.Status{UserId: model.NewId(), Status: model.STATUS_AWAY, Manual: false, LastActivityAt: 0, ActiveChannel: ""} require.Nil(t, ss.Status().SaveOrUpdate(status2)) @@ -33,40 +32,28 @@ func testStatusStore(t *testing.T, ss store.Store) { status3 := &model.Status{UserId: model.NewId(), Status: model.STATUS_OFFLINE, Manual: false, LastActivityAt: 0, ActiveChannel: ""} require.Nil(t, ss.Status().SaveOrUpdate(status3)) - if statuses, err := ss.Status().GetByIds([]string{status.UserId, "junk"}); err != nil { - t.Fatal(err) - } else { - if len(statuses) != 1 { - t.Fatal("should only have 1 status") - } - } + statuses, err := ss.Status().GetByIds([]string{status.UserId, "junk"}) + require.Nil(t, err) + require.Len(t, statuses, 1, "should only have 1 status") - if err := ss.Status().ResetAll(); err != nil { - t.Fatal(err) - } + err = ss.Status().ResetAll() + require.Nil(t, err) - if statusParameter, err := ss.Status().Get(status.UserId); err != nil { - t.Fatal(err) - } else { - if statusParameter.Status != model.STATUS_OFFLINE { - t.Fatal("should be offline") - } - } + statusParameter, err := ss.Status().Get(status.UserId) + require.Nil(t, err) + require.Equal(t, statusParameter.Status, model.STATUS_OFFLINE, "should be offline") - if err := ss.Status().UpdateLastActivityAt(status.UserId, 10); err != nil { - t.Fatal(err) - } + err = ss.Status().UpdateLastActivityAt(status.UserId, 10) + require.Nil(t, err) } func testActiveUserCount(t *testing.T, ss store.Store) { status := &model.Status{UserId: model.NewId(), Status: model.STATUS_ONLINE, Manual: false, LastActivityAt: model.GetMillis(), ActiveChannel: ""} require.Nil(t, ss.Status().SaveOrUpdate(status)) - if count, err := ss.Status().GetTotalActiveUsersCount(); err != nil { - t.Fatal(err) - } else { - require.True(t, count > 0, "expected count > 0, got %d", count) - } + count, err := ss.Status().GetTotalActiveUsersCount() + require.Nil(t, err) + require.True(t, count > 0, "expected count > 0, got %d", count) } type ByUserId []*model.Status From 63a28700f5e6d46bd37b64e2be7fd8556f05d3f2 Mon Sep 17 00:00:00 2001 From: Joshua Bezaleel Abednego Date: Sat, 2 Nov 2019 01:29:02 +0700 Subject: [PATCH 06/15] [MM-12623] Create CLI command "config reset" (#10296) --- cmd/mattermost/commands/config.go | 98 +++++++++++++++++++++++++- cmd/mattermost/commands/config_test.go | 61 +++++++++++++++- 2 files changed, 156 insertions(+), 3 deletions(-) diff --git a/cmd/mattermost/commands/config.go b/cmd/mattermost/commands/config.go index 81ed6d7236..2ca3ac7331 100644 --- a/cmd/mattermost/commands/config.go +++ b/cmd/mattermost/commands/config.go @@ -11,6 +11,7 @@ import ( "strconv" "strings" + "github.com/mattermost/viper" "github.com/pkg/errors" "github.com/spf13/cobra" @@ -18,7 +19,6 @@ import ( "github.com/mattermost/mattermost-server/mlog" "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/utils" - "github.com/mattermost/viper" ) const noSettingsNamed = "unable to find a setting named: %s" @@ -80,8 +80,17 @@ var MigrateConfigCmd = &cobra.Command{ RunE: configMigrateCmdF, } +var ConfigResetCmd = &cobra.Command{ + Use: "reset", + Short: "Reset config setting", + Long: "Resets the value of a config setting by its name in dot notation or a setting section. Accepts multiple values for array settings.", + Example: "config reset SqlSettings.DriverName LogSettings", + RunE: configResetCmdF, +} + func init() { ConfigSubpathCmd.Flags().String("path", "", "Optional subpath; defaults to value in SiteURL") + ConfigResetCmd.Flags().Bool("confirm", false, "Confirm you really want to reset all configuration settings to its default value") ConfigShowCmd.Flags().Bool("json", false, "Output the configuration as JSON.") ConfigCmd.AddCommand( @@ -91,6 +100,7 @@ func init() { ConfigShowCmd, ConfigSetCmd, MigrateConfigCmd, + ConfigResetCmd, ) RootCmd.AddCommand(ConfigCmd) } @@ -367,6 +377,92 @@ func UpdateMap(configMap map[string]interface{}, configSettings []string, newVal } } +func configResetCmdF(command *cobra.Command, args []string) error { + configStore, err := getConfigStore(command) + if err != nil { + return err + } + + defaultConfig := &model.Config{} + defaultConfig.SetDefaults() + + confirmFlag, _ := command.Flags().GetBool("confirm") + if confirmFlag { + if _, err = configStore.Set(defaultConfig); err != nil { + return errors.Wrap(err, "failed to set config") + } + } + + if !confirmFlag && len(args) == 0 { + var confirmResetAll string + CommandPrettyPrintln("Are you sure you want to reset all the configuration settings?(YES/NO): ") + fmt.Scanln(&confirmResetAll) + if confirmResetAll == "YES" { + if _, err = configStore.Set(defaultConfig); err != nil { + return errors.Wrap(err, "failed to set config") + } + } + } + + tempConfig := configStore.Get() + tempConfigMap := configToMap(*tempConfig) + defaultConfigMap := configToMap(*defaultConfig) + for _, arg := range args { + err = changeMap(tempConfigMap, defaultConfigMap, strings.Split(arg, ".")) + if err != nil { + return errors.Wrap(err, "Failed to reset config") + } + } + bs, err := json.Marshal(tempConfigMap) + if err != nil { + fmt.Printf("Error while marshalling map to json %s\n", err) + os.Exit(1) + } + err = json.Unmarshal(bs, tempConfig) + if err != nil { + fmt.Printf("Error while unmarshalling json to struct %s\n", err) + os.Exit(1) + } + if changed := config.FixInvalidLocales(tempConfig); changed { + return errors.New("Invalid locale configuration") + } + + if _, err := configStore.Set(tempConfig); err != nil { + return errors.Wrap(err, "failed to set config") + } + + return nil +} + +func changeMap(oldConfigMap, defaultConfigMap map[string]interface{}, configSettings []string) error { + resOld, ok := oldConfigMap[configSettings[0]] + if !ok { + return fmt.Errorf("Unable to find a setting with that name %s", configSettings[0]) + } + resDef := defaultConfigMap[configSettings[0]] + valueOld := reflect.ValueOf(resOld) + + if valueOld.Kind() == reflect.Map { + if len(configSettings) == 1 { + return changeSection(resOld.(map[string]interface{}), resDef.(map[string]interface{})) + } + return changeMap(resOld.(map[string]interface{}), resDef.(map[string]interface{}), configSettings[1:]) + } + if len(configSettings) == 1 { + oldConfigMap[configSettings[0]] = defaultConfigMap[configSettings[0]] + return nil + } + return fmt.Errorf("Unable to find a setting with that name %s", configSettings[0]) +} + +func changeSection(oldConfigMap, defaultConfigMap map[string]interface{}) error { + valueOld := reflect.ValueOf(oldConfigMap) + for _, key := range valueOld.MapKeys() { + oldConfigMap[key.String()] = defaultConfigMap[key.String()] + } + return nil +} + // configToMap converts our config into a map func configToMap(s interface{}) map[string]interface{} { return structToMap(s) diff --git a/cmd/mattermost/commands/config_test.go b/cmd/mattermost/commands/config_test.go index 3268066084..d830a3b1dc 100644 --- a/cmd/mattermost/commands/config_test.go +++ b/cmd/mattermost/commands/config_test.go @@ -13,11 +13,10 @@ import ( "strings" "testing" - "github.com/mattermost/mattermost-server/config" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/mattermost/mattermost-server/config" "github.com/mattermost/mattermost-server/model" ) @@ -168,6 +167,64 @@ func TestConfigSet(t *testing.T) { }) } +func TestConfigReset(t *testing.T) { + th := Setup() + defer th.TearDown() + + t.Run("No Error when no arguments are given (reset all the configurations)", func(t *testing.T) { + assert.NoError(t, th.RunCommand(t, "config", "reset")) + }) + + t.Run("No Error when a configuration section is given", func(t *testing.T) { + assert.NoError(t, th.RunCommand(t, "config", "reset", "JobSettings")) + }) + + t.Run("No Error when a configuration setting is given", func(t *testing.T) { + assert.NoError(t, th.RunCommand(t, "config", "reset", "JobSettings.RunJobs")) + }) + + t.Run("Error when the wrong configuration section is given", func(t *testing.T) { + assert.Error(t, th.RunCommand(t, "config", "reset", "InvalidSettings")) + }) + + t.Run("Error when the wrong configuration setting is given", func(t *testing.T) { + assert.Error(t, th.RunCommand(t, "config", "reset", "JobSettings.InvalidConfiguration")) + }) + + t.Run("Success when the confirm boolean flag is given", func(t *testing.T) { + assert.NoError(t, th.RunCommand(t, "config", "set", "JobSettings.RunJobs", "false")) + assert.NoError(t, th.RunCommand(t, "config", "set", "PrivacySettings.ShowFullName", "false")) + assert.NoError(t, th.RunCommand(t, "config", "reset", "--confirm")) + output1 := th.CheckCommand(t, "config", "get", "JobSettings.RunJobs") + output2 := th.CheckCommand(t, "config", "get", "PrivacySettings.ShowFullName") + assert.Contains(t, output1, "true") + assert.Contains(t, output2, "true") + }) + + t.Run("Success when a configuration section is given", func(t *testing.T) { + assert.NoError(t, th.RunCommand(t, "config", "set", "JobSettings.RunJobs", "false")) + assert.NoError(t, th.RunCommand(t, "config", "set", "JobSettings.RunScheduler", "false")) + assert.NoError(t, th.RunCommand(t, "config", "set", "PrivacySettings.ShowFullName", "false")) + assert.NoError(t, th.RunCommand(t, "config", "reset", "JobSettings")) + output1 := th.CheckCommand(t, "config", "get", "JobSettings.RunJobs") + output2 := th.CheckCommand(t, "config", "get", "JobSettings.RunScheduler") + output3 := th.CheckCommand(t, "config", "get", "PrivacySettings.ShowFullName") + assert.Contains(t, output1, "true") + assert.Contains(t, output2, "true") + assert.Contains(t, output3, "false") + }) + + t.Run("Success when a configuration setting is given", func(t *testing.T) { + assert.NoError(t, th.RunCommand(t, "config", "set", "JobSettings.RunJobs", "false")) + assert.NoError(t, th.RunCommand(t, "config", "set", "JobSettings.RunScheduler", "false")) + assert.NoError(t, th.RunCommand(t, "config", "reset", "JobSettings.RunJobs")) + output1 := th.CheckCommand(t, "config", "get", "JobSettings.RunJobs") + output2 := th.CheckCommand(t, "config", "get", "JobSettings.RunScheduler") + assert.Contains(t, output1, "true") + assert.Contains(t, output2, "false") + }) +} + func TestConfigToMap(t *testing.T) { // This test is almost the same as TestStructToMap, but I have it here for the sake of completions cases := []struct { From a2adf7b3f5ee106ea81a8996c10e026d87e07e34 Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Fri, 1 Nov 2019 23:08:01 +0100 Subject: [PATCH 07/15] Run unused against codebase (#12968) * Remove unused methods * Fix missed issues --- api4/job_test.go | 2 +- config/export_test.go | 5 ----- plugin/stringifier.go | 2 +- plugin/stringifier_test.go | 4 ++-- store/layered_store_hints.go | 9 --------- store/local_cache_supplier.go | 24 ------------------------ store/localcachelayer/layer_test.go | 13 ------------- web/handlers.go | 19 ------------------- 8 files changed, 4 insertions(+), 74 deletions(-) diff --git a/api4/job_test.go b/api4/job_test.go index e2426c5478..f24d4abde3 100644 --- a/api4/job_test.go +++ b/api4/job_test.go @@ -108,7 +108,7 @@ func TestGetJobs(t *testing.T) { received, resp = th.SystemAdminClient.GetJobs(1, 2) require.Nil(t, resp.Error) - require.Equal(t,jobs[1].Id, received[0].Id, "should've received oldest job last") + require.Equal(t, jobs[1].Id, received[0].Id, "should've received oldest job last") _, resp = th.Client.GetJobs(0, 60) CheckForbiddenStatus(t, resp) diff --git a/config/export_test.go b/config/export_test.go index b8ef523982..e818975c94 100644 --- a/config/export_test.go +++ b/config/export_test.go @@ -21,8 +21,3 @@ func UnmarshalConfig(r io.Reader, allowEnvironmentOverrides bool) (*model.Config func InitializeConfigurationsTable(db *sqlx.DB) error { return initializeConfigurationsTable(db) } - -// ResolveConfigFilePath exposes the internal resolveConfigFilePath to test only. -func ResolveConfigFilePath(path string) (string, error) { - return resolveConfigFilePath(path) -} diff --git a/plugin/stringifier.go b/plugin/stringifier.go index 1455fb4e63..4a438d1ce2 100644 --- a/plugin/stringifier.go +++ b/plugin/stringifier.go @@ -8,7 +8,7 @@ import ( ) func stringify(objects []interface{}) []string { - stringified := make([]string, len(objects), len(objects)) + stringified := make([]string, len(objects)) for i, object := range objects { stringified[i] = fmt.Sprintf("%+v", object) } diff --git a/plugin/stringifier_test.go b/plugin/stringifier_test.go index 58ae7e44d1..c9ac212939 100644 --- a/plugin/stringifier_test.go +++ b/plugin/stringifier_test.go @@ -15,7 +15,7 @@ func TestStringify(t *testing.T) { assert.Empty(t, strings) }) t.Run("EmptyShouldReturnEmpty", func(t *testing.T) { - strings := stringify(make([]interface{}, 0, 0)) + strings := stringify(make([]interface{}, 0)) assert.Empty(t, strings) }) t.Run("PrimitivesAndCompositesShouldReturnCorrectValues", func(t *testing.T) { @@ -83,7 +83,7 @@ func TestToObjects(t *testing.T) { assert.Nil(t, objects) }) t.Run("EmptyShouldReturnEmpty", func(t *testing.T) { - objects := toObjects(make([]string, 0, 0)) + objects := toObjects(make([]string, 0)) assert.Empty(t, objects) }) t.Run("ShouldReturnSliceOfObjects", func(t *testing.T) { diff --git a/store/layered_store_hints.go b/store/layered_store_hints.go index 066f0a2fab..6154af7c9f 100644 --- a/store/layered_store_hints.go +++ b/store/layered_store_hints.go @@ -9,12 +9,3 @@ const ( LSH_NO_CACHE LayeredStoreHint = iota LSH_MASTER_ONLY ) - -func hintsContains(hints []LayeredStoreHint, contains LayeredStoreHint) bool { - for _, hint := range hints { - if hint == contains { - return true - } - } - return false -} diff --git a/store/local_cache_supplier.go b/store/local_cache_supplier.go index 6dc971c226..76f084a115 100644 --- a/store/local_cache_supplier.go +++ b/store/local_cache_supplier.go @@ -49,30 +49,6 @@ func (s *LocalCacheSupplier) Next() LayeredStoreSupplier { return s.next } -func (s *LocalCacheSupplier) doStandardReadCache(ctx context.Context, cache ObjectCache, key string, hints ...LayeredStoreHint) *LayeredStoreSupplierResult { - if hintsContains(hints, LSH_NO_CACHE) { - if s.metrics != nil { - s.metrics.IncrementMemCacheMissCounter(cache.Name()) - } - return nil - } - - if cacheItem, ok := cache.Get(key); ok { - if s.metrics != nil { - s.metrics.IncrementMemCacheHitCounter(cache.Name()) - } - result := NewSupplierResult() - result.Data = cacheItem - return result - } - - if s.metrics != nil { - s.metrics.IncrementMemCacheMissCounter(cache.Name()) - } - - return nil -} - func (s *LocalCacheSupplier) doStandardAddToCache(ctx context.Context, cache ObjectCache, key string, result *LayeredStoreSupplierResult, hints ...LayeredStoreHint) { if result.Err == nil && result.Data != nil { cache.AddWithDefaultExpires(key, result.Data) diff --git a/store/localcachelayer/layer_test.go b/store/localcachelayer/layer_test.go index 71f3fefa82..d180ee9bdd 100644 --- a/store/localcachelayer/layer_test.go +++ b/store/localcachelayer/layer_test.go @@ -35,19 +35,6 @@ func StoreTest(t *testing.T, f func(*testing.T, store.Store)) { } } -func StoreTestWithSqlSupplier(t *testing.T, f func(*testing.T, store.Store, storetest.SqlSupplier)) { - defer func() { - if err := recover(); err != nil { - tearDownStores() - panic(err) - } - }() - for _, st := range storeTypes { - st := st - t.Run(st.Name, func(t *testing.T) { f(t, st.Store, st.SqlSupplier) }) - } -} - func initStores() { storeTypes = append(storeTypes, &storeType{ Name: "LocalCache+MySQL", diff --git a/web/handlers.go b/web/handlers.go index ea2885ba13..3bdaff286e 100644 --- a/web/handlers.go +++ b/web/handlers.go @@ -315,22 +315,3 @@ func (w *Web) ApiSessionRequired(h func(*Context, http.ResponseWriter, *http.Req } return handler } - -// apiHandlerTrustRequester provides a handler for API endpoints which do not require the user to be logged in and are -// allowed to be requested directly rather than via javascript/XMLHttpRequest, such as site branding images or the -// websocket. -func (w *Web) apiHandlerTrustRequester(h func(*Context, http.ResponseWriter, *http.Request)) http.Handler { - handler := &Handler{ - GetGlobalAppOptions: w.GetGlobalAppOptions, - HandleFunc: h, - HandlerName: GetHandlerName(h), - RequireSession: false, - TrustRequester: true, - RequireMfa: false, - IsStatic: false, - } - if *w.ConfigService.Config().ServiceSettings.WebserverMode == "gzip" { - return gziphandler.GzipHandler(handler) - } - return handler -} From cacdda702e2f8750f2fe0652e052f3a35c912b95 Mon Sep 17 00:00:00 2001 From: Adarsh K Kumar Date: Sun, 3 Nov 2019 20:43:23 +0530 Subject: [PATCH 08/15] MM-19663 | Migrate brand_test and cluster_test to testify (#12935) * MM-19663 | Migrate brand_test and cluster_test to testify * Use require.Fail instead of require.FailNow Co-Authored-By: Ben Schumacher --- api4/brand_test.go | 11 ++++------- api4/cluster_test.go | 5 ++--- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/api4/brand_test.go b/api4/brand_test.go index c454789cd6..a3cd5738b2 100644 --- a/api4/brand_test.go +++ b/api4/brand_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/mattermost/mattermost-server/utils/testutils" + "github.com/stretchr/testify/require" ) func TestGetBrandImage(t *testing.T) { @@ -32,9 +33,7 @@ func TestUploadBrandImage(t *testing.T) { Client := th.Client data, err := testutils.ReadTestFile("test.png") - if err != nil { - t.Fatal(err) - } + require.Nil(t, err) _, resp := Client.UploadBrandImage(data) CheckForbiddenStatus(t, resp) @@ -48,7 +47,7 @@ func TestUploadBrandImage(t *testing.T) { } else if resp.StatusCode == http.StatusUnauthorized { CheckUnauthorizedStatus(t, resp) } else { - t.Fatal("Should have failed either forbidden or unauthorized") + require.Fail(t, "Should have failed either forbidden or unauthorized") } _, resp = th.SystemAdminClient.UploadBrandImage(data) @@ -60,9 +59,7 @@ func TestDeleteBrandImage(t *testing.T) { defer th.TearDown() data, err := testutils.ReadTestFile("test.png") - if err != nil { - t.Fatal(err) - } + require.Nil(t, err) _, resp := th.SystemAdminClient.UploadBrandImage(data) CheckCreatedStatus(t, resp) diff --git a/api4/cluster_test.go b/api4/cluster_test.go index abc1fb515b..25287671f2 100644 --- a/api4/cluster_test.go +++ b/api4/cluster_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/mattermost/mattermost-server/model" + "github.com/stretchr/testify/require" ) func TestGetClusterStatus(t *testing.T) { @@ -22,9 +23,7 @@ func TestGetClusterStatus(t *testing.T) { infos, resp := th.SystemAdminClient.GetClusterStatus() CheckNoError(t, resp) - if infos == nil { - t.Fatal("should not be nil") - } + require.NotNil(t, infos, "cluster status should not be nil") }) t.Run("as restricted system admin", func(t *testing.T) { From 3a1deeaac5db6eded6573993865dbc5c400d4eb5 Mon Sep 17 00:00:00 2001 From: Amy Blais Date: Mon, 4 Nov 2019 07:47:23 -0500 Subject: [PATCH 09/15] Update en.json (#12684) --- i18n/en.json | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/i18n/en.json b/i18n/en.json index 784942e5b1..27478cae3b 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -4524,15 +4524,15 @@ }, { "id": "model.config.is_valid.elastic_search.connection_url.app_error", - "translation": "Elasticsearch ConnectionUrl setting must be provided when Elastic Search indexing is enabled." + "translation": "Elasticsearch ConnectionUrl setting must be provided when Elasticsearch indexing is enabled." }, { "id": "model.config.is_valid.elastic_search.enable_autocomplete.app_error", - "translation": "Elasticsearch IndexingEnabled setting must be set to true when Elastic Search AutocompleteEnabled is set to true." + "translation": "Elasticsearch IndexingEnabled setting must be set to true when Elasticsearch AutocompleteEnabled is set to true." }, { "id": "model.config.is_valid.elastic_search.enable_searching.app_error", - "translation": "Elasticsearch IndexingEnabled setting must be set to true when Elastic Search SearchEnabled is set to true." + "translation": "Elasticsearch IndexingEnabled setting must be set to true when Elasticsearch SearchEnabled is set to true." }, { "id": "model.config.is_valid.elastic_search.live_indexing_batch_size.app_error", @@ -7480,11 +7480,11 @@ }, { "id": "web.error.unsupported_browser.no_longer_support", - "translation": "We no longer support this browser" + "translation": "This browser is no longer supported by Mattermost" }, { "id": "web.error.unsupported_browser.no_longer_support_version", - "translation": "We no longer support this version of your browser" + "translation": "This version of your browser is no longer supported by Mattermost" }, { "id": "web.error.unsupported_browser.open_system_browser.edge", From 812c40a30703efd159675a1ff1b26a64f18b14d0 Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Mon, 4 Nov 2019 13:47:59 +0100 Subject: [PATCH 10/15] Adjust govet settings and fix issues found by it (#12947) --- .golangci.yml | 5 +++-- api4/user.go | 5 ----- app/config.go | 12 ++++++++---- app/session.go | 3 +-- store/sqlstore/audit_store.go | 6 +++--- store/sqlstore/compliance_store.go | 2 +- store/sqlstore/emoji_store.go | 2 +- store/sqlstore/file_info_store.go | 11 +++++++---- store/sqlstore/group_store.go | 4 ++-- store/sqlstore/supplier_reactions.go | 4 ++-- store/sqlstore/user_store.go | 21 +++++++++------------ 11 files changed, 37 insertions(+), 38 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 6788712517..7fe08ea5ec 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -3,10 +3,11 @@ run: modules-download-mode: vendor linters-settings: - govet: - check-shadowing: true gofmt: simplify: true + govet: + check-shadowing: true + enable-all: true linters: disable-all: true diff --git a/api4/user.go b/api4/user.go index 06c44975fc..25984727f5 100644 --- a/api4/user.go +++ b/api4/user.go @@ -925,11 +925,6 @@ func patchUser(c *Context, w http.ResponseWriter, r *http.Request) { } if c.App.Session.IsOAuth && patch.Email != nil { - if err != nil { - c.Err = err - return - } - if ouser.Email != *patch.Email { c.SetPermissionError(model.PERMISSION_EDIT_OTHER_USERS) c.Err.DetailedError += ", attempted email update by oauth app" diff --git a/app/config.go b/app/config.go index 3537ff7a44..b86dcf5abc 100644 --- a/app/config.go +++ b/app/config.go @@ -138,8 +138,10 @@ func (a *App) ensurePostActionCookieSecret() error { return err } system.Value = string(v) - if err = a.Srv.Store.System().Save(system); err == nil { - // If we were able to save the key, use it, otherwise ignore the error. + // If we were able to save the key, use it, otherwise log the error. + if appErr := a.Srv.Store.System().Save(system); appErr != nil { + mlog.Error("Failed to save PostActionCookieSecret", mlog.Err(appErr)) + } else { secret = newSecret } } @@ -199,8 +201,10 @@ func (a *App) ensureAsymmetricSigningKey() error { return err } system.Value = string(v) - if err = a.Srv.Store.System().Save(system); err == nil { - // If we were able to save the key, use it, otherwise ignore the error. + // If we were able to save the key, use it, otherwise log the error. + if appErr := a.Srv.Store.System().Save(system); appErr != nil { + mlog.Error("Failed to save AsymmetricSigningKey", mlog.Err(appErr)) + } else { key = newKey } } diff --git a/app/session.go b/app/session.go index dcfde84102..fa51e255d9 100644 --- a/app/session.go +++ b/app/session.go @@ -72,8 +72,7 @@ func (a *App) GetSession(token string) (*model.Session, *model.AppError) { return nil, model.NewAppError("GetSession", "api.context.invalid_token.error", map[string]interface{}{"Token": token}, "", http.StatusUnauthorized) } - if session != nil && - *a.Config().ServiceSettings.SessionIdleTimeoutInMinutes > 0 && + if *a.Config().ServiceSettings.SessionIdleTimeoutInMinutes > 0 && !session.IsOAuth && session.Props[model.SESSION_PROP_TYPE] != model.SESSION_TYPE_USER_ACCESS_TOKEN { diff --git a/store/sqlstore/audit_store.go b/store/sqlstore/audit_store.go index 98758b8907..65d8bee87a 100644 --- a/store/sqlstore/audit_store.go +++ b/store/sqlstore/audit_store.go @@ -85,9 +85,9 @@ func (s SqlAuditStore) PermanentDeleteBatch(endTime int64, limit int64) (int64, return 0, model.NewAppError("SqlAuditStore.PermanentDeleteBatch", "store.sql_audit.permanent_delete_batch.app_error", nil, ""+err.Error(), http.StatusInternalServerError) } - rowsAffected, err1 := sqlResult.RowsAffected() - if err1 != nil { - return 0, model.NewAppError("SqlAuditStore.PermanentDeleteBatch", "store.sql_audit.permanent_delete_batch.app_error", nil, ""+err1.Error(), http.StatusInternalServerError) + rowsAffected, err := sqlResult.RowsAffected() + if err != nil { + return 0, model.NewAppError("SqlAuditStore.PermanentDeleteBatch", "store.sql_audit.permanent_delete_batch.app_error", nil, ""+err.Error(), http.StatusInternalServerError) } return rowsAffected, nil } diff --git a/store/sqlstore/compliance_store.go b/store/sqlstore/compliance_store.go index 35ff5f4039..b41884d3a1 100644 --- a/store/sqlstore/compliance_store.go +++ b/store/sqlstore/compliance_store.go @@ -75,7 +75,7 @@ func (us SqlComplianceStore) Get(id string) (*model.Compliance, *model.AppError) return nil, model.NewAppError("SqlComplianceStore.Get", "store.sql_compliance.get.finding.app_error", nil, err.Error(), http.StatusInternalServerError) } if obj == nil { - return nil, model.NewAppError("SqlComplianceStore.Get", "store.sql_compliance.get.finding.app_error", nil, err.Error(), http.StatusNotFound) + return nil, model.NewAppError("SqlComplianceStore.Get", "store.sql_compliance.get.finding.app_error", nil, "", http.StatusNotFound) } return obj.(*model.Compliance), nil } diff --git a/store/sqlstore/emoji_store.go b/store/sqlstore/emoji_store.go index 533f88e1f5..60868b326c 100644 --- a/store/sqlstore/emoji_store.go +++ b/store/sqlstore/emoji_store.go @@ -136,7 +136,7 @@ func (es SqlEmojiStore) Delete(emoji *model.Emoji, time int64) *model.AppError { AND DeleteAt = 0`, map[string]interface{}{"DeleteAt": time, "UpdateAt": time, "Id": emoji.Id}); err != nil { return model.NewAppError("SqlEmojiStore.Delete", "store.sql_emoji.delete.app_error", nil, "id="+emoji.Id+", err="+err.Error(), http.StatusInternalServerError) } else if rows, _ := sqlResult.RowsAffected(); rows == 0 { - return model.NewAppError("SqlEmojiStore.Delete", "store.sql_emoji.delete.no_results", nil, "id="+emoji.Id+", err="+err.Error(), http.StatusBadRequest) + return model.NewAppError("SqlEmojiStore.Delete", "store.sql_emoji.delete.no_results", nil, "id="+emoji.Id, http.StatusBadRequest) } es.removeFromCache(emoji) diff --git a/store/sqlstore/file_info_store.go b/store/sqlstore/file_info_store.go index 156840968d..99b67438d7 100644 --- a/store/sqlstore/file_info_store.go +++ b/store/sqlstore/file_info_store.go @@ -266,10 +266,12 @@ func (s SqlFileInfoStore) PermanentDeleteBatch(endTime int64, limit int64) (int6 if err != nil { return 0, model.NewAppError("SqlFileInfoStore.PermanentDeleteBatch", "store.sql_file_info.permanent_delete_batch.app_error", nil, ""+err.Error(), http.StatusInternalServerError) } - rowsAffected, err1 := sqlResult.RowsAffected() - if err1 != nil { + + rowsAffected, err := sqlResult.RowsAffected() + if err != nil { return 0, model.NewAppError("SqlFileInfoStore.PermanentDeleteBatch", "store.sql_file_info.permanent_delete_batch.app_error", nil, ""+err.Error(), http.StatusInternalServerError) } + return rowsAffected, nil } @@ -281,9 +283,10 @@ func (s SqlFileInfoStore) PermanentDeleteByUser(userId string) (int64, *model.Ap return 0, model.NewAppError("SqlFileInfoStore.PermanentDeleteByUser", "store.sql_file_info.PermanentDeleteByUser.app_error", nil, ""+err.Error(), http.StatusInternalServerError) } - rowsAffected, err1 := sqlResult.RowsAffected() - if err1 != nil { + rowsAffected, err := sqlResult.RowsAffected() + if err != nil { return 0, model.NewAppError("SqlFileInfoStore.PermanentDeleteByUser", "store.sql_file_info.PermanentDeleteByUser.app_error", nil, ""+err.Error(), http.StatusInternalServerError) } + return rowsAffected, nil } diff --git a/store/sqlstore/group_store.go b/store/sqlstore/group_store.go index f0e4555ff3..796b38e414 100644 --- a/store/sqlstore/group_store.go +++ b/store/sqlstore/group_store.go @@ -586,7 +586,7 @@ func (s *SqlGroupStore) UpdateGroupSyncable(groupSyncable *model.GroupSyncable) case model.GroupSyncableTypeChannel: _, err = s.GetMaster().Update(groupSyncableToGroupChannel(groupSyncable)) default: - return nil, model.NewAppError("SqlGroupStore.GroupUpdateGroupSyncable", "model.group_syncable.type.app_error", nil, "group_id="+groupSyncable.GroupId+", syncable_id="+groupSyncable.SyncableId+", "+err.Error(), http.StatusInternalServerError) + return nil, model.NewAppError("SqlGroupStore.GroupUpdateGroupSyncable", "model.group_syncable.type.app_error", nil, "group_id="+groupSyncable.GroupId+", syncable_id="+groupSyncable.SyncableId, http.StatusInternalServerError) } if err != nil { @@ -619,7 +619,7 @@ func (s *SqlGroupStore) DeleteGroupSyncable(groupID string, syncableID string, s case model.GroupSyncableTypeChannel: _, err = s.GetMaster().Update(groupSyncableToGroupChannel(groupSyncable)) default: - return nil, model.NewAppError("SqlGroupStore.GroupDeleteGroupSyncable", "model.group_syncable.type.app_error", nil, "group_id="+groupSyncable.GroupId+", syncable_id="+groupSyncable.SyncableId+", "+err.Error(), http.StatusInternalServerError) + return nil, model.NewAppError("SqlGroupStore.GroupDeleteGroupSyncable", "model.group_syncable.type.app_error", nil, "group_id="+groupSyncable.GroupId+", syncable_id="+groupSyncable.SyncableId, http.StatusInternalServerError) } if err != nil { diff --git a/store/sqlstore/supplier_reactions.go b/store/sqlstore/supplier_reactions.go index e3822af708..7128c76a7e 100644 --- a/store/sqlstore/supplier_reactions.go +++ b/store/sqlstore/supplier_reactions.go @@ -160,8 +160,8 @@ func (s *SqlReactionStore) PermanentDeleteBatch(endTime int64, limit int64) (int return 0, model.NewAppError("SqlReactionStore.PermanentDeleteBatch", "store.sql_reaction.permanent_delete_batch.app_error", nil, ""+err.Error(), http.StatusInternalServerError) } - rowsAffected, err1 := sqlResult.RowsAffected() - if err1 != nil { + rowsAffected, err := sqlResult.RowsAffected() + if err != nil { return 0, model.NewAppError("SqlReactionStore.PermanentDeleteBatch", "store.sql_reaction.permanent_delete_batch.app_error", nil, ""+err.Error(), http.StatusInternalServerError) } return rowsAffected, nil diff --git a/store/sqlstore/user_store.go b/store/sqlstore/user_store.go index 5f15cf23b1..84f5911a07 100644 --- a/store/sqlstore/user_store.go +++ b/store/sqlstore/user_store.go @@ -1448,10 +1448,9 @@ func (us SqlUserStore) GetUsersBatchForIndexing(startTime, endTime int64, limit OrderBy("u.CreateAt"). Limit(uint64(limit)). ToSql() - _, err1 := us.GetSearchReplica().Select(&users, usersQuery, args...) - - if err1 != nil { - return nil, model.NewAppError("SqlUserStore.GetUsersBatchForIndexing", "store.sql_user.get_users_batch_for_indexing.get_users.app_error", nil, err1.Error(), http.StatusInternalServerError) + _, err := us.GetSearchReplica().Select(&users, usersQuery, args...) + if err != nil { + return nil, model.NewAppError("SqlUserStore.GetUsersBatchForIndexing", "store.sql_user.get_users_batch_for_indexing.get_users.app_error", nil, err.Error(), http.StatusInternalServerError) } userIds := []string{} @@ -1478,10 +1477,9 @@ func (us SqlUserStore) GetUsersBatchForIndexing(startTime, endTime int64, limit Join("Channels c ON cm.ChannelId = c.Id"). Where(sq.Eq{"c.Type": "O", "cm.UserId": userIds}). ToSql() - _, err2 := us.GetSearchReplica().Select(&channelMembers, channelMembersQuery, args...) - - if err2 != nil { - return nil, model.NewAppError("SqlUserStore.GetUsersBatchForIndexing", "store.sql_user.get_users_batch_for_indexing.get_channel_members.app_error", nil, err2.Error(), http.StatusInternalServerError) + _, err = us.GetSearchReplica().Select(&channelMembers, channelMembersQuery, args...) + if err != nil { + return nil, model.NewAppError("SqlUserStore.GetUsersBatchForIndexing", "store.sql_user.get_users_batch_for_indexing.get_channel_members.app_error", nil, err.Error(), http.StatusInternalServerError) } var teamMembers []*model.TeamMember @@ -1490,10 +1488,9 @@ func (us SqlUserStore) GetUsersBatchForIndexing(startTime, endTime int64, limit From("TeamMembers"). Where(sq.Eq{"UserId": userIds, "DeleteAt": 0}). ToSql() - _, err3 := us.GetSearchReplica().Select(&teamMembers, teamMembersQuery, args...) - - if err3 != nil { - return nil, model.NewAppError("SqlUserStore.GetUsersBatchForIndexing", "store.sql_user.get_users_batch_for_indexing.get_team_members.app_error", nil, err3.Error(), http.StatusInternalServerError) + _, err = us.GetSearchReplica().Select(&teamMembers, teamMembersQuery, args...) + if err != nil { + return nil, model.NewAppError("SqlUserStore.GetUsersBatchForIndexing", "store.sql_user.get_users_batch_for_indexing.get_team_members.app_error", nil, err.Error(), http.StatusInternalServerError) } userMap := map[string]*model.UserForIndexing{} From 1db045bce379173c5826f1ce72332dd85fed2836 Mon Sep 17 00:00:00 2001 From: Gervasio Marchand Date: Mon, 4 Nov 2019 09:49:54 -0300 Subject: [PATCH 11/15] MM-16822 - Implement KVSetWithOptions (#11818) * Add SetWithOptions * Avoid passing two structs to the functions * Rename ExpiryInSeconds -> ExpireInSeconds * Use t.Run for the tests * Fix build * Address feedback * Update log message * Update docs and use KVSetWithOptions in KVCompareAndSetJSON * Improve code style * Use struct instead of pointer to struct * Fix minimum server versions * Update documentation * Address feedback * Revert new implemention of kv helpers * Adress feedback --- app/plugin_api.go | 4 + app/plugin_key_value_store.go | 44 ++++---- app/plugin_test.go | 146 ++++++++++++++++++++++++++- i18n/en.json | 8 ++ model/plugin_kvset_options.go | 77 ++++++++++++++ plugin/api.go | 9 ++ plugin/client_rpc_generated.go | 31 ++++++ plugin/helpers.go | 6 ++ plugin/helpers_kv.go | 44 ++++---- plugin/plugintest/api.go | 73 +++++++++----- store/sqlstore/plugin_store.go | 31 +++++- store/store.go | 1 + store/storetest/mocks/PluginStore.go | 23 +++++ 13 files changed, 420 insertions(+), 77 deletions(-) create mode 100644 model/plugin_kvset_options.go diff --git a/app/plugin_api.go b/app/plugin_api.go index d228a88d0d..1aaced6823 100644 --- a/app/plugin_api.go +++ b/app/plugin_api.go @@ -695,6 +695,10 @@ func (api *PluginAPI) InstallPlugin(file io.Reader, replace bool) (*model.Manife // KV Store Section +func (api *PluginAPI) KVSetWithOptions(key string, value interface{}, options model.PluginKVSetOptions) (bool, *model.AppError) { + return api.app.SetPluginKeyWithOptions(api.id, key, value, options) +} + func (api *PluginAPI) KVSet(key string, value []byte) *model.AppError { return api.app.SetPluginKey(api.id, key, value) } diff --git a/app/plugin_key_value_store.go b/app/plugin_key_value_store.go index 6bff32c3dd..8599f7e679 100644 --- a/app/plugin_key_value_store.go +++ b/app/plugin_key_value_store.go @@ -23,40 +23,30 @@ 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) + options := model.PluginKVSetOptions{ + ExpireInSeconds: expireInSeconds, } - - kv := &model.PluginKeyValue{ - PluginId: pluginId, - Key: key, - Value: value, - ExpireAt: expireInSeconds, - } - - if _, err := a.Srv.Store.Plugin().SaveOrUpdate(kv); err != nil { - mlog.Error("Failed to set plugin key value", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err)) - return err - } - - // Clean up a previous entry using the hashed key, if it exists. - if err := a.Srv.Store.Plugin().Delete(pluginId, getKeyHash(key)); err != nil { - mlog.Error("Failed to clean up previously hashed plugin key value", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err)) - } - - return nil + _, err := a.SetPluginKeyWithOptions(pluginId, key, value, options) + return err } func (a *App) CompareAndSetPluginKey(pluginId string, key string, oldValue, newValue []byte) (bool, *model.AppError) { - kv := &model.PluginKeyValue{ - PluginId: pluginId, - Key: key, - Value: newValue, + options := model.PluginKVSetOptions{ + Atomic: true, + OldValue: oldValue, + } + return a.SetPluginKeyWithOptions(pluginId, key, newValue, options) +} + +func (a *App) SetPluginKeyWithOptions(pluginId string, key string, value interface{}, options model.PluginKVSetOptions) (bool, *model.AppError) { + if err := options.IsValid(); err != nil { + mlog.Error("Failed to set plugin key value with options", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err)) + return false, err } - updated, err := a.Srv.Store.Plugin().CompareAndSet(kv, oldValue) + updated, err := a.Srv.Store.Plugin().SetWithOptions(pluginId, key, value, options) if err != nil { - mlog.Error("Failed to compare and set plugin key value", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err)) + mlog.Error("Failed to set plugin key value with options", mlog.String("plugin_id", pluginId), mlog.String("key", key), mlog.Err(err)) return updated, err } diff --git a/app/plugin_test.go b/app/plugin_test.go index c505917bea..aa2f5654ac 100644 --- a/app/plugin_test.go +++ b/app/plugin_test.go @@ -16,11 +16,12 @@ import ( "testing" "github.com/gorilla/mux" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/plugin" "github.com/mattermost/mattermost-server/utils/fileutils" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) func getHashedKey(key string) string { @@ -186,6 +187,147 @@ func TestPluginKeyValueStoreCompareAndSet(t *testing.T) { assert.Equal(t, []byte("test2"), ret) } +func TestPluginKeyValueStoreSetWithOptionsJSON(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + pluginId := "testpluginid" + + defer func() { + assert.Nil(t, th.App.DeletePluginKey(pluginId, "key")) + }() + + t.Run("fails with a non-serializable object as the new value", func(t *testing.T) { + result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", func() {}, model.PluginKVSetOptions{ + EncodeJSON: true, + }) + assert.False(t, result) + assert.NotNil(t, err) + + // verify that after the failure it was not set + ret, err := th.App.GetPluginKey(pluginId, "key") + assert.Nil(t, err) + assert.Equal(t, []byte(nil), ret) + }) + + t.Run("fails with a non-serializable object as the old value", func(t *testing.T) { + result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", map[string]interface{}{ + "val-a": 10, + }, model.PluginKVSetOptions{ + EncodeJSON: true, + Atomic: true, + OldValue: func() {}, + }) + assert.False(t, result) + assert.NotNil(t, err) + + // verify that after the failure it was not set + ret, err := th.App.GetPluginKey(pluginId, "key") + assert.Nil(t, err) + assert.Equal(t, []byte(nil), ret) + }) + + t.Run("storing a value json encoded works", func(t *testing.T) { + result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", map[string]interface{}{ + "val-a": 10, + }, model.PluginKVSetOptions{ + EncodeJSON: true, + }) + assert.True(t, result) + assert.Nil(t, err) + + // and I can get it back! + ret, err := th.App.GetPluginKey(pluginId, "key") + assert.Nil(t, err) + assert.Equal(t, []byte(`{"val-a":10}`), ret) + }) + + t.Run("test that setting it atomic when it doesn't match doesn't change anything", func(t *testing.T) { + result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", map[string]interface{}{ + "val-a": 30, + }, model.PluginKVSetOptions{ + EncodeJSON: true, + Atomic: true, + OldValue: map[string]interface{}{ + "val-a": 20, + }, + }) + assert.False(t, result) + assert.Nil(t, err) + + // test that the value didn't change + ret, err := th.App.GetPluginKey(pluginId, "key") + assert.Nil(t, err) + assert.Equal(t, []byte(`{"val-a":10}`), ret) + }) + + t.Run("test the atomic change with the proper old value", func(t *testing.T) { + result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", map[string]interface{}{ + "val-a": 30, + }, model.PluginKVSetOptions{ + EncodeJSON: true, + Atomic: true, + OldValue: map[string]interface{}{ + "val-a": 10, + }, + }) + assert.True(t, result) + assert.Nil(t, err) + + // test that the value did change + ret, err := th.App.GetPluginKey(pluginId, "key") + assert.Nil(t, err) + assert.Equal(t, []byte(`{"val-a":30}`), ret) + }) +} + +func TestPluginKeyValueStoreSetWithOptionsByteArray(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + pluginId := "testpluginid" + + defer func() { + assert.Nil(t, th.App.DeletePluginKey(pluginId, "key")) + }() + + // storing a value works + result, err := th.App.SetPluginKeyWithOptions(pluginId, "key", []byte(`myvalue`), model.PluginKVSetOptions{}) + assert.True(t, result) + assert.Nil(t, err) + + // and I can get it back! + ret, err := th.App.GetPluginKey(pluginId, "key") + assert.Nil(t, err) + assert.Equal(t, []byte(`myvalue`), ret) + + // test that setting it atomic when it doesn't match doesn't change anything + result, err = th.App.SetPluginKeyWithOptions(pluginId, "key", []byte(`newvalue`), model.PluginKVSetOptions{ + Atomic: true, + OldValue: []byte(`differentvalue`), + }) + assert.False(t, result) + assert.Nil(t, err) + + // test that the value didn't change + ret, err = th.App.GetPluginKey(pluginId, "key") + assert.Nil(t, err) + assert.Equal(t, []byte(`myvalue`), ret) + + // now do the atomic change with the proper old value + result, err = th.App.SetPluginKeyWithOptions(pluginId, "key", []byte(`newvalue`), model.PluginKVSetOptions{ + Atomic: true, + OldValue: []byte(`myvalue`), + }) + assert.True(t, result) + assert.Nil(t, err) + + // test that the value did change + ret, err = th.App.GetPluginKey(pluginId, "key") + assert.Nil(t, err) + assert.Equal(t, []byte(`newvalue`), ret) +} + func TestServePluginRequest(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() diff --git a/i18n/en.json b/i18n/en.json index 27478cae3b..c20c213a7f 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -5154,6 +5154,14 @@ "id": "model.plugin_key_value.is_valid.plugin_id.app_error", "translation": "Invalid plugin ID, must be more than {{.Min}} and a of maximum {{.Max}} characters long." }, + { + "id": "model.plugin_kvset_options.is_valid.old_value.app_error", + "translation": "Invalid old value, it shouldn't be set when the operation is not atomic." + }, + { + "id": "model.plugin_kvset_options.serialize_value.app_error", + "translation": "Could not deserialize a value. EncodeJSON: {{.EncodeJSON}}." + }, { "id": "model.post.is_valid.channel_id.app_error", "translation": "Invalid channel id" diff --git a/model/plugin_kvset_options.go b/model/plugin_kvset_options.go new file mode 100644 index 0000000000..6498fa4b9e --- /dev/null +++ b/model/plugin_kvset_options.go @@ -0,0 +1,77 @@ +// Copyright (c) 2017-present Mattermost, Inc. All Rights Reserved. +// See License.txt for license information. + +package model + +import ( + "encoding/json" + "net/http" +) + +// PluginKVSetOptions contains information on how to store a value in the plugin KV store. +type PluginKVSetOptions struct { + EncodeJSON bool // If true, store the JSON encoding of newValue + Atomic bool // Only store the value if the current value matches the oldValue + OldValue interface{} // The value to compare with the current value. Only used when Atomic is true + ExpireInSeconds int64 // Set an expire counter +} + +// IsValid returns nil if the chosen options are valid. +func (opt *PluginKVSetOptions) IsValid() *AppError { + if !opt.Atomic && opt.OldValue != nil { + return NewAppError( + "PluginKVSetOptions.IsValid", + "model.plugin_kvset_options.is_valid.old_value.app_error", + nil, + "", + http.StatusBadRequest, + ) + } + + return nil +} + +// GetOldValueSerialized returns the serialized old value either as directly +// or encoded as JSON depending on the chosen options. +func (opt *PluginKVSetOptions) GetOldValueSerialized() ([]byte, *AppError) { + return opt.serializeValue(opt.OldValue) +} + +func (opt *PluginKVSetOptions) serializeValue(value interface{}) ([]byte, *AppError) { + if opt.EncodeJSON { + data, err := json.Marshal(value) + if err != nil { + return nil, NewAppError("PluginKVSetOptions.serializeValue", "model.plugin_kvset_options.serialize_value.app_error", map[string]interface{}{"EncodeJSON": opt.EncodeJSON}, "Could not serialize JSON value", http.StatusBadRequest) + } + return data, nil + } + + castResult, ok := value.([]byte) + if !ok { + return nil, NewAppError("PluginKVSetOptions.SerializeValue", "model.plugin_kvset_options.serialize_value.app_error", map[string]interface{}{"EncodeJSON": opt.EncodeJSON}, "Could not cast value to []byte", http.StatusBadRequest) + } + + return castResult, nil +} + +// NewPluginKeyValueFromOptions return a PluginKeyValue given a pluginID, a KV pair and options. +func NewPluginKeyValueFromOptions(pluginId, key string, value interface{}, opt PluginKVSetOptions) (*PluginKeyValue, *AppError) { + serializedValue, err := opt.serializeValue(value) + if err != nil { + return nil, err + } + + expireAt := int64(0) + if opt.ExpireInSeconds > 0 { + expireAt = GetMillis() + (opt.ExpireInSeconds * 1000) + } + + kv := &PluginKeyValue{ + PluginId: pluginId, + Key: key, + Value: serializedValue, + ExpireAt: expireAt, + } + + return kv, nil +} diff --git a/plugin/api.go b/plugin/api.go index 6a2ce229c3..7c7a665efa 100644 --- a/plugin/api.go +++ b/plugin/api.go @@ -612,6 +612,15 @@ type API interface { // Minimum server version: 5.16 KVCompareAndDelete(key string, oldValue []byte) (bool, *model.AppError) + // KVSetWithOptions stores a key-value pair, unique per plugin, according to the given options. + // If options.EncodeJSON is not true, the type of newValue must be of type []byte. + // Returns (false, err) if DB error occurred + // Returns (false, nil) if the value was not set + // Returns (true, nil) if the value was set + // + // Minimum server version: 5.18 + KVSetWithOptions(key string, newValue interface{}, options model.PluginKVSetOptions) (bool, *model.AppError) + // KVSet stores a key-value pair with an expiry time, unique per plugin. // // Minimum server version: 5.6 diff --git a/plugin/client_rpc_generated.go b/plugin/client_rpc_generated.go index 37e6587321..67543f50cc 100644 --- a/plugin/client_rpc_generated.go +++ b/plugin/client_rpc_generated.go @@ -3727,6 +3727,37 @@ func (s *apiRPCServer) KVCompareAndDelete(args *Z_KVCompareAndDeleteArgs, return return nil } +type Z_KVSetWithOptionsArgs struct { + A string + B interface{} + C model.PluginKVSetOptions +} + +type Z_KVSetWithOptionsReturns struct { + A bool + B *model.AppError +} + +func (g *apiRPCClient) KVSetWithOptions(key string, newValue interface{}, options model.PluginKVSetOptions) (bool, *model.AppError) { + _args := &Z_KVSetWithOptionsArgs{key, newValue, options} + _returns := &Z_KVSetWithOptionsReturns{} + if err := g.client.Call("Plugin.KVSetWithOptions", _args, _returns); err != nil { + log.Printf("RPC call to KVSetWithOptions API failed: %s", err.Error()) + } + return _returns.A, _returns.B +} + +func (s *apiRPCServer) KVSetWithOptions(args *Z_KVSetWithOptionsArgs, returns *Z_KVSetWithOptionsReturns) error { + if hook, ok := s.impl.(interface { + KVSetWithOptions(key string, newValue interface{}, options model.PluginKVSetOptions) (bool, *model.AppError) + }); ok { + returns.A, returns.B = hook.KVSetWithOptions(args.A, args.B, args.C) + } else { + return encodableError(fmt.Errorf("API KVSetWithOptions called but not implemented.")) + } + return nil +} + type Z_KVSetWithExpiryArgs struct { A string B []byte diff --git a/plugin/helpers.go b/plugin/helpers.go index 94d66c0683..726741ad68 100644 --- a/plugin/helpers.go +++ b/plugin/helpers.go @@ -14,6 +14,8 @@ type Helpers interface { // KVSetJSON stores a key-value pair, unique per plugin, marshalling the given value as a JSON string. // + // Deprecated: Use p.API.KVSetWithOptions instead. + // // Minimum server version: 5.2 KVSetJSON(key string, value interface{}) error @@ -23,6 +25,8 @@ type Helpers interface { // Returns (false, nil) if current value != oldValue or key already exists when inserting // Returns (true, nil) if current value == oldValue or new key is inserted // + // Deprecated: Use p.API.KVSetWithOptions instead. + // // Minimum server version: 5.12 KVCompareAndSetJSON(key string, oldValue interface{}, newValue interface{}) (bool, error) @@ -41,6 +45,8 @@ type Helpers interface { // KVSetWithExpiryJSON stores a key-value pair with an expiry time, unique per plugin, marshalling the given value as a JSON string. // + // Deprecated: Use p.API.KVSetWithOptions instead. + // // Minimum server version: 5.6 KVSetWithExpiryJSON(key string, value interface{}, expireInSeconds int64) error } diff --git a/plugin/helpers_kv.go b/plugin/helpers_kv.go index 78efa31467..9b054502e2 100644 --- a/plugin/helpers_kv.go +++ b/plugin/helpers_kv.go @@ -9,25 +9,7 @@ import ( "github.com/pkg/errors" ) -// KVGetJSON is a wrapper around KVGet to simplify reading a JSON object from the key value store. -func (p *HelpersImpl) KVGetJSON(key string, value interface{}) (bool, error) { - data, appErr := p.API.KVGet(key) - if appErr != nil { - return false, appErr - } - if data == nil { - return false, nil - } - - err := json.Unmarshal(data, value) - if err != nil { - return false, err - } - - return true, nil -} - -// KVSetJSON is a wrapper around KVSet to simplify writing a JSON object to the key value store. +// KVSetJSON implements Helpers.KVSetJSON. func (p *HelpersImpl) KVSetJSON(key string, value interface{}) error { data, err := json.Marshal(value) if err != nil { @@ -42,7 +24,7 @@ func (p *HelpersImpl) KVSetJSON(key string, value interface{}) error { return nil } -// KVCompareAndSetJSON is a wrapper around KVCompareAndSet to simplify atomically writing a JSON object to the key value store. +// KVCompareAndSetJSON implements Helpers.KVCompareAndSetJSON. func (p *HelpersImpl) KVCompareAndSetJSON(key string, oldValue interface{}, newValue interface{}) (bool, error) { var oldData, newData []byte var err error @@ -69,7 +51,7 @@ func (p *HelpersImpl) KVCompareAndSetJSON(key string, oldValue interface{}, newV return set, nil } -// KVCompareAndDeleteJSON is a wrapper around KVCompareAndDelete to simplify atomically deleting a JSON object from the key value store. +// KVCompareAndDeleteJSON implements Helpers.KVCompareAndDeleteJSON. func (p *HelpersImpl) KVCompareAndDeleteJSON(key string, oldValue interface{}) (bool, error) { var oldData []byte var err error @@ -89,7 +71,25 @@ func (p *HelpersImpl) KVCompareAndDeleteJSON(key string, oldValue interface{}) ( return deleted, nil } -// KVSetWithExpiryJSON is a wrapper around KVSetWithExpiry to simplify atomically writing a JSON object with expiry to the key value store. +// KVGetJSON implements Helpers.KVGetJSON. +func (p *HelpersImpl) KVGetJSON(key string, value interface{}) (bool, error) { + data, appErr := p.API.KVGet(key) + if appErr != nil { + return false, appErr + } + if data == nil { + return false, nil + } + + err := json.Unmarshal(data, value) + if err != nil { + return false, err + } + + return true, nil +} + +// KVSetWithExpiryJSON implements Helpers.KVSetWithExpiryJSON. func (p *HelpersImpl) KVSetWithExpiryJSON(key string, value interface{}, expireInSeconds int64) error { data, err := json.Marshal(value) if err != nil { diff --git a/plugin/plugintest/api.go b/plugin/plugintest/api.go index 4db7d5e5e2..81c8eb44d3 100644 --- a/plugin/plugintest/api.go +++ b/plugin/plugintest/api.go @@ -2023,6 +2023,31 @@ func (_m *API) HasPermissionToTeam(userId string, teamId string, permission *mod return r0 } +// InstallPlugin provides a mock function with given fields: file, replace +func (_m *API) InstallPlugin(file io.Reader, replace bool) (*model.Manifest, *model.AppError) { + ret := _m.Called(file, replace) + + var r0 *model.Manifest + if rf, ok := ret.Get(0).(func(io.Reader, bool) *model.Manifest); ok { + r0 = rf(file, replace) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(*model.Manifest) + } + } + + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(io.Reader, bool) *model.AppError); ok { + r1 = rf(file, replace) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 +} + // KVCompareAndDelete provides a mock function with given fields: key, oldValue func (_m *API) KVCompareAndDelete(key string, oldValue []byte) (bool, *model.AppError) { ret := _m.Called(key, oldValue) @@ -2183,6 +2208,29 @@ func (_m *API) KVSetWithExpiry(key string, value []byte, expireInSeconds int64) return r0 } +// KVSetWithOptions provides a mock function with given fields: key, newValue, options +func (_m *API) KVSetWithOptions(key string, newValue interface{}, options model.PluginKVSetOptions) (bool, *model.AppError) { + ret := _m.Called(key, newValue, options) + + var r0 bool + if rf, ok := ret.Get(0).(func(string, interface{}, model.PluginKVSetOptions) bool); ok { + r0 = rf(key, newValue, options) + } else { + r0 = ret.Get(0).(bool) + } + + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string, interface{}, model.PluginKVSetOptions) *model.AppError); ok { + r1 = rf(key, newValue, options) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 +} + // LoadPluginConfiguration provides a mock function with given fields: dest func (_m *API) LoadPluginConfiguration(dest interface{}) error { ret := _m.Called(dest) @@ -2885,28 +2933,3 @@ func (_m *API) UploadFile(data []byte, channelId string, filename string) (*mode return r0, r1 } - -// InstallPlugin provides a mock function with given fields: file, replace -func (_m *API) InstallPlugin(file io.Reader, replace bool) (*model.Manifest, *model.AppError) { - ret := _m.Called(file, replace) - - var r0 *model.Manifest - if rf, ok := ret.Get(0).(func(io.Reader, bool) *model.Manifest); ok { - r0 = rf(file, replace) - } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(*model.Manifest) - } - } - - var r1 *model.AppError - if rf, ok := ret.Get(1).(func(io.Reader, bool) *model.AppError); ok { - r1 = rf(file, replace) - } else { - if ret.Get(1) != nil { - r1 = ret.Get(1).(*model.AppError) - } - } - - return r0, r1 -} diff --git a/store/sqlstore/plugin_store.go b/store/sqlstore/plugin_store.go index 4d2f00a2d8..3c4d61d1ec 100644 --- a/store/sqlstore/plugin_store.go +++ b/store/sqlstore/plugin_store.go @@ -91,12 +91,13 @@ func (ps SqlPluginStore) CompareAndSet(kv *model.PluginKeyValue, oldValue []byte } else { // Update if oldValue is not nil updateResult, err := ps.GetMaster().Exec( - `UPDATE PluginKeyValueStore SET PValue = :New WHERE PluginId = :PluginId AND PKey = :Key AND PValue = :Old`, + `UPDATE PluginKeyValueStore SET PValue = :New, ExpireAt = :ExpireAt WHERE PluginId = :PluginId AND PKey = :Key AND PValue = :Old`, map[string]interface{}{ "PluginId": kv.PluginId, "Key": kv.Key, "Old": oldValue, "New": kv.Value, + "ExpireAt": kv.ExpireAt, }, ) if err != nil { @@ -147,6 +148,34 @@ func (ps SqlPluginStore) CompareAndDelete(kv *model.PluginKeyValue, oldValue []b return true, nil } +func (ps SqlPluginStore) SetWithOptions(pluginId string, key string, value interface{}, opt model.PluginKVSetOptions) (bool, *model.AppError) { + if err := opt.IsValid(); err != nil { + return false, err + } + + kv, err := model.NewPluginKeyValueFromOptions(pluginId, key, value, opt) + if err != nil { + return false, err + } + + if opt.Atomic { + var serializedOldValue []byte + serializedOldValue, err = opt.GetOldValueSerialized() + if err != nil { + return false, err + } + + return ps.CompareAndSet(kv, serializedOldValue) + } + + savedKv, err := ps.SaveOrUpdate(kv) + if err != nil { + return false, err + } + + return savedKv != nil, nil +} + func (ps SqlPluginStore) Get(pluginId, key string) (*model.PluginKeyValue, *model.AppError) { var kv *model.PluginKeyValue currentTime := model.GetMillis() diff --git a/store/store.go b/store/store.go index 26fb519a7e..59ef9f693e 100644 --- a/store/store.go +++ b/store/store.go @@ -528,6 +528,7 @@ type PluginStore interface { SaveOrUpdate(keyVal *model.PluginKeyValue) (*model.PluginKeyValue, *model.AppError) CompareAndSet(keyVal *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError) CompareAndDelete(keyVal *model.PluginKeyValue, oldValue []byte) (bool, *model.AppError) + SetWithOptions(pluginId string, key string, value interface{}, options model.PluginKVSetOptions) (bool, *model.AppError) Get(pluginId, key string) (*model.PluginKeyValue, *model.AppError) Delete(pluginId, key string) *model.AppError DeleteAllForPlugin(PluginId string) *model.AppError diff --git a/store/storetest/mocks/PluginStore.go b/store/storetest/mocks/PluginStore.go index 54812a66a6..71a19c4fb7 100644 --- a/store/storetest/mocks/PluginStore.go +++ b/store/storetest/mocks/PluginStore.go @@ -182,3 +182,26 @@ func (_m *PluginStore) SaveOrUpdate(keyVal *model.PluginKeyValue) (*model.Plugin return r0, r1 } + +// SetWithOptions provides a mock function with given fields: pluginId, key, value, options +func (_m *PluginStore) SetWithOptions(pluginId string, key string, value interface{}, options model.PluginKVSetOptions) (bool, *model.AppError) { + ret := _m.Called(pluginId, key, value, options) + + var r0 bool + if rf, ok := ret.Get(0).(func(string, string, interface{}, model.PluginKVSetOptions) bool); ok { + r0 = rf(pluginId, key, value, options) + } else { + r0 = ret.Get(0).(bool) + } + + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(string, string, interface{}, model.PluginKVSetOptions) *model.AppError); ok { + r1 = rf(pluginId, key, value, options) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 +} From 159dec03a61d02cce4780c19574988b9558cf758 Mon Sep 17 00:00:00 2001 From: Witold Konior Date: Mon, 4 Nov 2019 14:50:17 +0100 Subject: [PATCH 12/15] [MM-19355] Migrate tests from "store/storetest/user_store.go" (#12732) * [MM-19355] Migrate tests from "store/storetest/user_store.go" to use testify * fix error message * make shadow happy! * fix type casting in equal * use assert instead of require to soft fail for email updates --- store/storetest/user_store.go | 262 ++++++++++++---------------------- 1 file changed, 95 insertions(+), 167 deletions(-) diff --git a/store/storetest/user_store.go b/store/storetest/user_store.go index 31494cf3d2..bd67187bf2 100644 --- a/store/storetest/user_store.go +++ b/store/storetest/user_store.go @@ -92,62 +92,57 @@ func testUserStoreSave(t *testing.T, ss store.Store) { Username: model.NewId(), } - if _, err := ss.User().Save(&u1); err != nil { - t.Fatal("couldn't save user", err) - } + _, err := ss.User().Save(&u1) + require.Nil(t, err, "couldn't save user") + defer func() { require.Nil(t, ss.User().PermanentDelete(u1.Id)) }() - _, err := ss.Team().SaveMember(&model.TeamMember{TeamId: teamId, UserId: u1.Id}, maxUsersPerTeam) + _, err = ss.Team().SaveMember(&model.TeamMember{TeamId: teamId, UserId: u1.Id}, maxUsersPerTeam) require.Nil(t, err) - if _, err := ss.User().Save(&u1); err == nil { - t.Fatal("shouldn't be able to update user from save") - } + _, err = ss.User().Save(&u1) + require.NotNil(t, err, "shouldn't be able to update user from save") u2 := model.User{ Email: u1.Email, Username: model.NewId(), } - if _, err := ss.User().Save(&u2); err == nil { - t.Fatal("should be unique email") - } + _, err = ss.User().Save(&u2) + require.NotNil(t, err, "should be unique email") u2.Email = MakeEmail() u2.Username = u1.Username - if _, err := ss.User().Save(&u1); err == nil { - t.Fatal("should be unique username") - } + _, err = ss.User().Save(&u1) + require.NotNil(t, err, "should be unique username") u2.Username = "" - if _, err := ss.User().Save(&u1); err == nil { - t.Fatal("should be unique username") - } + _, err = ss.User().Save(&u1) + require.NotNil(t, err, "should be unique username") for i := 0; i < 49; i++ { u := model.User{ Email: MakeEmail(), Username: model.NewId(), } - if _, err := ss.User().Save(&u); err != nil { - t.Fatal("couldn't save item", err) - } + _, err = ss.User().Save(&u) + require.Nil(t, err, "couldn't save item") + defer func() { require.Nil(t, ss.User().PermanentDelete(u.Id)) }() - _, err := ss.Team().SaveMember(&model.TeamMember{TeamId: teamId, UserId: u.Id}, maxUsersPerTeam) + _, err = ss.Team().SaveMember(&model.TeamMember{TeamId: teamId, UserId: u.Id}, maxUsersPerTeam) require.Nil(t, err) } u2.Id = "" u2.Email = MakeEmail() u2.Username = model.NewId() - if _, err := ss.User().Save(&u2); err != nil { - t.Fatal("couldn't save item", err) - } + _, err = ss.User().Save(&u2) + require.Nil(t, err, "couldn't save item") + defer func() { require.Nil(t, ss.User().PermanentDelete(u2.Id)) }() - if _, err := ss.Team().SaveMember(&model.TeamMember{TeamId: teamId, UserId: u1.Id}, maxUsersPerTeam); err == nil { - t.Fatal("should be the limit") - } + _, err = ss.Team().SaveMember(&model.TeamMember{TeamId: teamId, UserId: u1.Id}, maxUsersPerTeam) + require.NotNil(t, err, "should be the limit") } func testUserStoreUpdate(t *testing.T, ss store.Store) { @@ -170,26 +165,22 @@ func testUserStoreUpdate(t *testing.T, ss store.Store) { _, err = ss.Team().SaveMember(&model.TeamMember{TeamId: model.NewId(), UserId: u2.Id}, -1) require.Nil(t, err) - if _, err = ss.User().Update(u1, false); err != nil { - t.Fatal(err) - } + _, err = ss.User().Update(u1, false) + require.Nil(t, err) missing := &model.User{} - if _, err = ss.User().Update(missing, false); err == nil { - t.Fatal("Update should have failed because of missing key") - } + _, err = ss.User().Update(missing, false) + require.NotNil(t, err, "Update should have failed because of missing key") newId := &model.User{ Id: model.NewId(), } - if _, err = ss.User().Update(newId, false); err == nil { - t.Fatal("Update should have failed because id change") - } + _, err = ss.User().Update(newId, false) + require.NotNil(t, err, "Update should have failed because id change") u2.Email = MakeEmail() - if _, err = ss.User().Update(u2, false); err == nil { - t.Fatal("Update should have failed because you can't modify AD/LDAP fields") - } + _, err = ss.User().Update(u2, false) + require.NotNil(t, err, "Update should have failed because you can't modify AD/LDAP fields") u3 := &model.User{ Email: MakeEmail(), @@ -203,28 +194,17 @@ func testUserStoreUpdate(t *testing.T, ss store.Store) { require.Nil(t, err) u3.Email = MakeEmail() - if userUpdate, err := ss.User().Update(u3, false); err != nil { - t.Fatal("Update should not have failed") - } else { - newUser := userUpdate.New - if newUser.Email != oldEmail { - t.Fatal("Email should not have been updated as the update is not trusted") - } - } + userUpdate, err := ss.User().Update(u3, false) + require.Nil(t, err, "Update should not have failed") + assert.Equal(t, oldEmail, userUpdate.New.Email, "Email should not have been updated as the update is not trusted") u3.Email = MakeEmail() - if userUpdate, err := ss.User().Update(u3, true); err != nil { - t.Fatal("Update should not have failed") - } else { - newUser := userUpdate.New - if newUser.Email == oldEmail { - t.Fatal("Email should have been updated as the update is trusted") - } - } + userUpdate, err = ss.User().Update(u3, true) + require.Nil(t, err, "Update should not have failed") + assert.NotEqual(t, oldEmail, userUpdate.New.Email, "Email should have been updated as the update is trusted") - if err := ss.User().UpdateLastPictureUpdate(u1.Id); err != nil { - t.Fatal("Update should not have failed") - } + err = ss.User().UpdateLastPictureUpdate(u1.Id) + require.Nil(t, err, "Update should not have failed") } func testUserStoreUpdateUpdateAt(t *testing.T, ss store.Store) { @@ -236,16 +216,12 @@ func testUserStoreUpdateUpdateAt(t *testing.T, ss store.Store) { _, err = ss.Team().SaveMember(&model.TeamMember{TeamId: model.NewId(), UserId: u1.Id}, -1) require.Nil(t, err) - if _, err = ss.User().UpdateUpdateAt(u1.Id); err != nil { - t.Fatal(err) - } + _, err = ss.User().UpdateUpdateAt(u1.Id) + require.Nil(t, err) user, err := ss.User().Get(u1.Id) require.Nil(t, err) - if user.UpdateAt <= u1.UpdateAt { - t.Fatal("UpdateAt not updated correctly") - } - + require.Less(t, u1.UpdateAt, user.UpdateAt, "UpdateAt not updated correctly") } func testUserStoreUpdateFailedPasswordAttempts(t *testing.T, ss store.Store) { @@ -257,15 +233,12 @@ func testUserStoreUpdateFailedPasswordAttempts(t *testing.T, ss store.Store) { _, err = ss.Team().SaveMember(&model.TeamMember{TeamId: model.NewId(), UserId: u1.Id}, -1) require.Nil(t, err) - if err = ss.User().UpdateFailedPasswordAttempts(u1.Id, 3); err != nil { - t.Fatal(err) - } + err = ss.User().UpdateFailedPasswordAttempts(u1.Id, 3) + require.Nil(t, err) user, err := ss.User().Get(u1.Id) require.Nil(t, err) - if user.FailedAttempts != 3 { - t.Fatal("FailedAttempts not updated correctly") - } + require.Equal(t, 3, user.FailedAttempts, "FailedAttempts not updated correctly") } func testUserStoreGet(t *testing.T, ss store.Store) { @@ -1448,7 +1421,7 @@ func testUserStoreGetProfileByGroupChannelIdsForUser(t *testing.T, ss store.Stor users, ok := res[channelId] require.True(t, ok) - userIds := []string{} + var userIds []string for _, user := range users { userIds = append(userIds, user.Id) } @@ -1901,17 +1874,12 @@ func testUserStoreUpdatePassword(t *testing.T, ss store.Store) { hashedPassword := model.HashPassword("newpwd") - if err := ss.User().UpdatePassword(u1.Id, hashedPassword); err != nil { - t.Fatal(err) - } + err = ss.User().UpdatePassword(u1.Id, hashedPassword) + require.Nil(t, err) - if user, err := ss.User().GetByEmail(u1.Email); err != nil { - t.Fatal(err) - } else { - if user.Password != hashedPassword { - t.Fatal("Password was not updated correctly") - } - } + user, err := ss.User().GetByEmail(u1.Email) + require.Nil(t, err) + require.Equal(t, user.Password, hashedPassword, "Password was not updated correctly") } func testUserStoreDelete(t *testing.T, ss store.Store) { @@ -1923,9 +1891,8 @@ func testUserStoreDelete(t *testing.T, ss store.Store) { _, err = ss.Team().SaveMember(&model.TeamMember{TeamId: model.NewId(), UserId: u1.Id}, -1) require.Nil(t, err) - if err := ss.User().PermanentDelete(u1.Id); err != nil { - t.Fatal(err) - } + err = ss.User().PermanentDelete(u1.Id) + require.Nil(t, err) } func testUserStoreUpdateAuthData(t *testing.T, ss store.Store) { @@ -1945,19 +1912,11 @@ func testUserStoreUpdateAuthData(t *testing.T, ss store.Store) { _, err = ss.User().UpdateAuthData(u1.Id, service, &authData, "", true) require.Nil(t, err) - if user, err := ss.User().GetByEmail(u1.Email); err != nil { - t.Fatal(err) - } else { - if user.AuthService != service { - t.Fatal("AuthService was not updated correctly") - } - if *user.AuthData != authData { - t.Fatal("AuthData was not updated correctly") - } - if user.Password != "" { - t.Fatal("Password was not cleared properly") - } - } + user, err := ss.User().GetByEmail(u1.Email) + require.Nil(t, err) + require.Equal(t, service, user.AuthService, "AuthService was not updated correctly") + require.Equal(t, authData, *user.AuthData, "AuthData was not updated correctly") + require.Equal(t, "", user.Password, "Password was not cleared properly") } func testUserUnreadCount(t *testing.T, ss store.Store) { @@ -1993,9 +1952,8 @@ func testUserUnreadCount(t *testing.T, ss store.Store) { _, err = ss.Team().SaveMember(&model.TeamMember{TeamId: teamId, UserId: u2.Id}, -1) require.Nil(t, err) - if _, channelErr := ss.Channel().Save(&c1, -1); err != nil { - t.Fatal("couldn't save item", channelErr) - } + _, err = ss.Channel().Save(&c1, -1) + require.Nil(t, err, "couldn't save item") m1 := model.ChannelMember{} m1.ChannelId = c1.Id @@ -2013,9 +1971,8 @@ func testUserUnreadCount(t *testing.T, ss store.Store) { m1.ChannelId = c2.Id m2.ChannelId = c2.Id - if _, err = ss.Channel().SaveDirectChannel(&c2, &m1, &m2); err != nil { - t.Fatal("couldn't save direct channel", err) - } + _, err = ss.Channel().SaveDirectChannel(&c2, &m1, &m2) + require.Nil(t, err, "couldn't save direct channel") p1 := model.Post{} p1.ChannelId = c1.Id @@ -2051,21 +2008,15 @@ func testUserUnreadCount(t *testing.T, ss store.Store) { badge, unreadCountErr := ss.User().GetUnreadCount(u2.Id) require.Nil(t, unreadCountErr) - if badge != 3 { - t.Fatal("should have 3 unread messages") - } + require.Equal(t, int64(3), badge, "should have 3 unread messages") badge, unreadCountErr = ss.User().GetUnreadCountForChannel(u2.Id, c1.Id) require.Nil(t, unreadCountErr) - if badge != 1 { - t.Fatal("should have 1 unread messages for that channel") - } + require.Equal(t, int64(1), badge, "should have 1 unread messages for that channel") badge, unreadCountErr = ss.User().GetUnreadCountForChannel(u2.Id, c2.Id) require.Nil(t, unreadCountErr) - if badge != 2 { - t.Fatal("should have 2 unread messages for that channel") - } + require.Equal(t, int64(2), badge, "should have 2 unread messages for that channel") } func testUserStoreUpdateMfaSecret(t *testing.T, ss store.Store) { @@ -2075,14 +2026,12 @@ func testUserStoreUpdateMfaSecret(t *testing.T, ss store.Store) { require.Nil(t, err) defer func() { require.Nil(t, ss.User().PermanentDelete(u1.Id)) }() - if err = ss.User().UpdateMfaSecret(u1.Id, "12345"); err != nil { - t.Fatal(err) - } + err = ss.User().UpdateMfaSecret(u1.Id, "12345") + require.Nil(t, err) // should pass, no update will occur though - if err = ss.User().UpdateMfaSecret("junk", "12345"); err != nil { - t.Fatal(err) - } + err = ss.User().UpdateMfaSecret("junk", "12345") + require.Nil(t, err) } func testUserStoreUpdateMfaActive(t *testing.T, ss store.Store) { @@ -2094,18 +2043,15 @@ func testUserStoreUpdateMfaActive(t *testing.T, ss store.Store) { time.Sleep(100 * time.Millisecond) - if err = ss.User().UpdateMfaActive(u1.Id, true); err != nil { - t.Fatal(err) - } + err = ss.User().UpdateMfaActive(u1.Id, true) + require.Nil(t, err) - if err = ss.User().UpdateMfaActive(u1.Id, false); err != nil { - t.Fatal(err) - } + err = ss.User().UpdateMfaActive(u1.Id, false) + require.Nil(t, err) // should pass, no update will occur though - if err = ss.User().UpdateMfaActive("junk", true); err != nil { - t.Fatal(err) - } + err = ss.User().UpdateMfaActive("junk", true) + require.Nil(t, err) } func testUserStoreGetRecentlyActiveUsersForTeam(t *testing.T, ss store.Store, s SqlSupplier) { @@ -3516,9 +3462,7 @@ func testUserStoreAnalyticsGetInactiveUsersCount(t *testing.T, ss store.Store) { defer func() { require.Nil(t, ss.User().PermanentDelete(u1.Id)) }() count, err := ss.User().AnalyticsGetInactiveUsersCount() - if err != nil { - t.Fatal(err) - } + require.Nil(t, err) u2 := &model.User{} u2.Email = MakeEmail() @@ -3528,22 +3472,13 @@ func testUserStoreAnalyticsGetInactiveUsersCount(t *testing.T, ss store.Store) { defer func() { require.Nil(t, ss.User().PermanentDelete(u2.Id)) }() newCount, err := ss.User().AnalyticsGetInactiveUsersCount() - if err != nil { - t.Fatal(err) - } - - if count != newCount-1 { - t.Fatal("Expected 1 more inactive users but found otherwise.", count, newCount) - } + require.Nil(t, err) + require.Equal(t, count, newCount-1, "Expected 1 more inactive users but found otherwise.") } func testUserStoreAnalyticsGetSystemAdminCount(t *testing.T, ss store.Store) { - var countBefore int64 - if result, err := ss.User().AnalyticsGetSystemAdminCount(); err != nil { - t.Fatal(err) - } else { - countBefore = result - } + countBefore, err := ss.User().AnalyticsGetSystemAdminCount() + require.Nil(t, err) u1 := model.User{} u1.Email = MakeEmail() @@ -3554,24 +3489,19 @@ func testUserStoreAnalyticsGetSystemAdminCount(t *testing.T, ss store.Store) { u2.Email = MakeEmail() u2.Username = model.NewId() - if _, err := ss.User().Save(&u1); err != nil { - t.Fatal("couldn't save user", err) - } + _, err = ss.User().Save(&u1) + require.Nil(t, err, "couldn't save user") defer func() { require.Nil(t, ss.User().PermanentDelete(u1.Id)) }() - if _, err := ss.User().Save(&u2); err != nil { - t.Fatal("couldn't save user", err) - } + _, err = ss.User().Save(&u2) + require.Nil(t, err, "couldn't save user") + defer func() { require.Nil(t, ss.User().PermanentDelete(u2.Id)) }() - if result, err := ss.User().AnalyticsGetSystemAdminCount(); err != nil { - t.Fatal(err) - } else { - // We expect to find 1 more system admin than there was at the start of this test function. - if count := result; count != countBefore+1 { - t.Fatal("Did not get the expected number of system admins. Expected, got: ", countBefore+1, count) - } - } + result, err := ss.User().AnalyticsGetSystemAdminCount() + require.Nil(t, err) + require.Equal(t, countBefore+1, result, "Did not get the expected number of system admins.") + } func testUserStoreGetProfilesNotInTeam(t *testing.T, ss store.Store) { @@ -4048,9 +3978,8 @@ func testUserStoreGetTeamGroupUsers(t *testing.T, ss store.Store) { require.NotNil(t, user) testUsers = append(testUsers, user) } - userGroupA := testUsers[0] - userGroupB := testUsers[1] - userNoGroup := testUsers[2] + require.Len(t, testUsers, 3, "testUsers length doesn't meet required length") + userGroupA, userGroupB, userNoGroup := testUsers[0], testUsers[1], testUsers[2] // add non-group-member to the team (to prove that the query isn't just returning all members) _, err = ss.Team().SaveMember(&model.TeamMember{ @@ -4075,8 +4004,8 @@ func testUserStoreGetTeamGroupUsers(t *testing.T, ss store.Store) { require.NotNil(t, group) testGroups = append(testGroups, group) } - groupA := testGroups[0] - groupB := testGroups[1] + require.Len(t, testGroups, 2, "testGroups length doesn't meet required length") + groupA, groupB := testGroups[0], testGroups[1] // add members to groups _, err = ss.Group().UpsertMember(groupA.Id, userGroupA.Id) @@ -4169,9 +4098,8 @@ func testUserStoreGetChannelGroupUsers(t *testing.T, ss store.Store) { require.NotNil(t, user) testUsers = append(testUsers, user) } - userGroupA := testUsers[0] - userGroupB := testUsers[1] - userNoGroup := testUsers[2] + require.Len(t, testUsers, 3, "testUsers length doesn't meet required length") + userGroupA, userGroupB, userNoGroup := testUsers[0], testUsers[1], testUsers[2] // add non-group-member to the channel (to prove that the query isn't just returning all members) _, err = ss.Channel().SaveMember(&model.ChannelMember{ @@ -4196,8 +4124,8 @@ func testUserStoreGetChannelGroupUsers(t *testing.T, ss store.Store) { require.NotNil(t, group) testGroups = append(testGroups, group) } - groupA := testGroups[0] - groupB := testGroups[1] + require.Len(t, testGroups, 2, "testGroups length doesn't meet required length") + groupA, groupB := testGroups[0], testGroups[1] // add members to groups _, err = ss.Group().UpsertMember(groupA.Id, userGroupA.Id) From 2ea0c669f6f4e6acf143d12910342283e42f3e25 Mon Sep 17 00:00:00 2001 From: Nicolas Le Cam Date: Mon, 4 Nov 2019 17:01:29 +0100 Subject: [PATCH 13/15] Update minimum Safari supported version to 12 (#12912) --- web/web.go | 2 +- web/web_test.go | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/web/web.go b/web/web.go index cf750055d1..79346f9053 100644 --- a/web/web.go +++ b/web/web.go @@ -47,7 +47,7 @@ func New(config configservice.ConfigService, globalOptions app.AppOptionCreator, // -1 means that the browser is not supported in any version. var browserMinimumSupported = map[string]int{ "BrowserIE": 12, - "BrowserSafari": 9, + "BrowserSafari": 12, } func CheckClientCompatability(agentString string) bool { diff --git a/web/web_test.go b/web/web_test.go index 6995ab1914..463de1d663 100644 --- a/web/web_test.go +++ b/web/web_test.go @@ -245,9 +245,13 @@ func TestCheckClientCompatability(t *testing.T) { {"Internet Explorer 11 2", "Mozilla/5.0 (Windows NT 10.0; WOW64; Trident/7.0; .NET4.0C; .NET4.0E; .NET CLR 2.0.50727; .NET CLR 3.0.30729; .NET CLR 3.5.30729; Zoom 3.6.0; rv:11.0) like Gecko", false}, {"Internet Explorer 11 (Compatibility Mode) 1", "Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 10.0; WOW64; Trident/7.0; .NET4.0C; .NET4.0E; .NET CLR 2.0.50727; .NET CLR 3.0.30729; .NET CLR 3.5.30729; .NET CLR 1.1.4322; InfoPath.3; Zoom 3.6.0)", false}, {"Internet Explorer 11 (Compatibility Mode) 2", "Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 10.0; WOW64; Trident/7.0; .NET4.0C; .NET4.0E; .NET CLR 2.0.50727; .NET CLR 3.0.30729; .NET CLR 3.5.30729; Zoom 3.6.0)", false}, - {"Safari 9", "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/604.1.38 (KHTML, like Gecko) Version/11.0 Safari/604.1.38", true}, + {"Safari 12", "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.0 Safari/605.1.15", true}, + {"Safari 11", "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/604.1.38 (KHTML, like Gecko) Version/11.0 Safari/604.1.38", false}, + {"Safari 10", "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_3) AppleWebKit/602.4.8 (KHTML, like Gecko) Version/10.0.3 Safari/602.4.8", false}, + {"Safari 9", "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_3) AppleWebKit/601.4.4 (KHTML, like Gecko) Version/9.0.3 Safari/601.4.4", false}, {"Safari 8", "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_4) AppleWebKit/600.7.12 (KHTML, like Gecko) Version/8.0.7 Safari/600.7.12", false}, - {"Safari Mobile", "Mozilla/5.0 (iPhone; CPU iPhone OS 9_1 like Mac OS X) AppleWebKit/601.1.46 (KHTML, like Gecko) Version/9.0 Mobile/13B137 Safari/601.1", true}, + {"Safari Mobile 12", "Mozilla/5.0 (iPhone; CPU iPhone OS 12_0 like macOS) AppleWebKit/602.1.50 (KHTML, like Gecko) Version/12.0 Mobile/14A5335d Safari/602.1.50", true}, + {"Safari Mobile 9", "Mozilla/5.0 (iPhone; CPU iPhone OS 9_1 like Mac OS X) AppleWebKit/601.1.46 (KHTML, like Gecko) Version/9.0 Mobile/13B137 Safari/601.1", false}, } for _, browser := range uaTestParameters { t.Run(browser.Name, func(t *testing.T) { From ac8e3a853c4fe372b61d6ae4922ee8d61edcbb6a Mon Sep 17 00:00:00 2001 From: Rajat Varyani <37879062+RajatVaryani@users.noreply.github.com> Date: Mon, 4 Nov 2019 23:18:25 +0530 Subject: [PATCH 14/15] Add gorilla/handlers dependency as it is used in enterprise version (#12991) * Add gorilla/handlers dependency as it is used in enterprise version * Remove unused dependencies --- cmd/mattermost/main.go | 2 +- go.mod | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/cmd/mattermost/main.go b/cmd/mattermost/main.go index 69907b0f3c..c51c36502d 100644 --- a/cmd/mattermost/main.go +++ b/cmd/mattermost/main.go @@ -15,7 +15,7 @@ import ( _ "github.com/mattermost/mattermost-server/imports" // Enterprise Deps - _ "github.com/dgryski/dgoogauth" + _ "github.com/gorilla/handlers" _ "github.com/hako/durafmt" _ "github.com/hashicorp/memberlist" _ "github.com/mattermost/ldap" diff --git a/go.mod b/go.mod index 2c1e6b3078..9f2699c127 100644 --- a/go.mod +++ b/go.mod @@ -52,8 +52,6 @@ require ( github.com/miekg/dns v1.1.19 // indirect github.com/minio/minio-go/v6 v6.0.38 github.com/mitchellh/go-testing-interface v1.0.0 // indirect - github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect - github.com/modern-go/reflect2 v1.0.1 // indirect github.com/muesli/smartcrop v0.3.0 // indirect github.com/olekukonko/tablewriter v0.0.1 // indirect github.com/onsi/ginkgo v1.8.0 // indirect From 501da809f3ec77725bdbf3f78ba43ef723ef6b98 Mon Sep 17 00:00:00 2001 From: Daniel Schalla Date: Mon, 4 Nov 2019 19:03:59 +0100 Subject: [PATCH 15/15] [CR-458] Enhance Logs Endpoint Message (#12984) * Make it possible to identify log entries created via the post API endpoints * Clarify wording --- api4/system.go | 14 ++++++++------ api4/system_test.go | 7 +++++++ 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/api4/system.go b/api4/system.go index 6e3a8866b5..8e192bb382 100644 --- a/api4/system.go +++ b/api4/system.go @@ -262,14 +262,16 @@ func postLog(c *Context, w http.ResponseWriter, r *http.Request) { msg = msg[0:399] } + msg = "Client Logs API Endpoint Message: " + msg + fields := []mlog.Field{ + mlog.String("type", "client_message"), + mlog.String("user_agent", c.App.UserAgent), + } + if !forceToDebug && lvl == "ERROR" { - err := &model.AppError{} - err.Message = msg - err.Id = msg - err.Where = "client" - c.LogError(err) + mlog.Error(msg, fields...) } else { - mlog.Debug("message", mlog.String("message", msg)) + mlog.Debug(msg, fields...) } m["message"] = msg diff --git a/api4/system_test.go b/api4/system_test.go index 0d5985d69b..6d887b8abe 100644 --- a/api4/system_test.go +++ b/api4/system_test.go @@ -283,6 +283,13 @@ func TestPostLog(t *testing.T) { _, resp := Client.PostLog(message) CheckNoError(t, resp) + *th.App.Config().ServiceSettings.EnableDeveloper = false + + _, resp = Client.PostLog(message) + CheckNoError(t, resp) + + *th.App.Config().ServiceSettings.EnableDeveloper = true + Client.Logout() _, resp = Client.PostLog(message)