From 8bd182c38fb8de54d8a85755754237da0d184105 Mon Sep 17 00:00:00 2001 From: Christopher Poile Date: Thu, 28 Feb 2019 10:51:42 -0500 Subject: [PATCH] MM-14145: The config store Set will now Save automatically (#10377) * MM-14145: The config store Set will now Save automatically When UpdateConfig (and configStore.Set) is called in admin.go and config.go, commonStore.Set now takes a store-specific persist function. It uses that persist function to save the configuration automatically. Removed: Now callers do not have to call configStore.Save or app.PersistConfig, and those functions have been removed. Possible downside: this means a "failed to persist config" error can now be thrown during a app.UpdateConfig or commonStore.Set call. But considering application code never really sets a config without saving it (except in the test cases, which were testing that -- see below), it seems fine to group these responsibilities. Also removed: tests for 'set without save'. Since that can not happen anymore, the tests are not needed. * Removed Save completely, cleaned up formatting, joined save test with set tests. * fixed shadowed variable error --- app/admin.go | 1 - app/config.go | 6 --- app/saml.go | 6 --- cmd/mattermost/commands/config.go | 7 ++-- config/common.go | 14 +++---- config/database.go | 13 ++----- config/database_test.go | 62 ++++++++++++------------------- config/file.go | 12 +----- config/file_test.go | 15 +------- config/memory.go | 5 --- config/store.go | 5 +-- 11 files changed, 39 insertions(+), 107 deletions(-) diff --git a/app/admin.go b/app/admin.go index 695f004819..0723573552 100644 --- a/app/admin.go +++ b/app/admin.go @@ -168,7 +168,6 @@ func (a *App) SaveConfig(newCfg *model.Config, sendConfigChangeClusterMessage bo return model.NewAppError("saveConfig", "app.save_config.app_error", nil, err.Error(), http.StatusInternalServerError) } - a.PersistConfig() if a.Metrics != nil { if *a.Config().MetricsSettings.Enable { a.Metrics.StartServer() diff --git a/app/config.go b/app/config.go index f671b99a07..5cc7cdb886 100644 --- a/app/config.go +++ b/app/config.go @@ -56,12 +56,6 @@ func (a *App) UpdateConfig(f func(*model.Config)) { a.Srv.UpdateConfig(f) } -func (a *App) PersistConfig() { - if err := a.Srv.configStore.Save(); err != nil { - mlog.Error("Failed to persist config", mlog.Err(err)) - } -} - func (s *Server) ReloadConfig() error { debug.FreeOSMemory() if err := s.configStore.Load(); err != nil { diff --git a/app/saml.go b/app/saml.go index 0c4a1a3b0f..f985b328a5 100644 --- a/app/saml.go +++ b/app/saml.go @@ -65,7 +65,6 @@ func (a *App) AddSamlPublicCertificate(fileData *multipart.FileHeader) *model.Ap } a.UpdateConfig(func(dest *model.Config) { *dest = *cfg }) - a.PersistConfig() return nil } @@ -83,7 +82,6 @@ func (a *App) AddSamlPrivateCertificate(fileData *multipart.FileHeader) *model.A } a.UpdateConfig(func(dest *model.Config) { *dest = *cfg }) - a.PersistConfig() return nil } @@ -101,7 +99,6 @@ func (a *App) AddSamlIdpCertificate(fileData *multipart.FileHeader) *model.AppEr } a.UpdateConfig(func(dest *model.Config) { *dest = *cfg }) - a.PersistConfig() return nil } @@ -128,7 +125,6 @@ func (a *App) RemoveSamlPublicCertificate() *model.AppError { } a.UpdateConfig(func(dest *model.Config) { *dest = *cfg }) - a.PersistConfig() return nil } @@ -147,7 +143,6 @@ func (a *App) RemoveSamlPrivateCertificate() *model.AppError { } a.UpdateConfig(func(dest *model.Config) { *dest = *cfg }) - a.PersistConfig() return nil } @@ -166,7 +161,6 @@ func (a *App) RemoveSamlIdpCertificate() *model.AppError { } a.UpdateConfig(func(dest *model.Config) { *dest = *cfg }) - a.PersistConfig() return nil } diff --git a/cmd/mattermost/commands/config.go b/cmd/mattermost/commands/config.go index 1829f2e0de..17085134b1 100644 --- a/cmd/mattermost/commands/config.go +++ b/cmd/mattermost/commands/config.go @@ -226,9 +226,6 @@ func configSetCmdF(command *cobra.Command, args []string) error { f := updateConfigValue(configSetting, newVal, oldConfig, newConfig) f(newConfig) - if _, err := configStore.Set(newConfig); err != nil { - return errors.Wrap(err, "failed to set config") - } // UpdateConfig above would have already fixed these invalid locales, but we check again // in the context of an explicit change to these parameters to avoid saving the fixed @@ -237,7 +234,9 @@ func configSetCmdF(command *cobra.Command, args []string) error { return errors.New("Invalid locale configuration") } - configStore.Save() + if _, err := configStore.Set(newConfig); err != nil { + return errors.Wrap(err, "failed to set config") + } return nil } diff --git a/config/common.go b/config/common.go index 039cf8cb14..416060074b 100644 --- a/config/common.go +++ b/config/common.go @@ -36,10 +36,11 @@ func (cs *commonStore) GetEnvironmentOverrides() map[string]interface{} { return cs.environmentOverrides } -// set replaces the current configuration in its entirety, without updating the backing store. +// set replaces the current configuration in its entirety, and updates the backing store +// using the persist function argument. // // This function assumes no lock has been acquired, as it acquires a write lock itself. -func (cs *commonStore) set(newCfg *model.Config, isValid func(*model.Config) error) (*model.Config, error) { +func (cs *commonStore) set(newCfg *model.Config, isValid func(*model.Config) error, persist func(*model.Config) error) (*model.Config, error) { cs.configLock.Lock() var unlockOnce sync.Once defer unlockOnce.Do(cs.configLock.Unlock) @@ -71,12 +72,9 @@ func (cs *commonStore) set(newCfg *model.Config, isValid func(*model.Config) err } } - // Ideally, Set would persist automatically and abstract this completely away from the - // client. Doing so requires a few upstream changes first, so for now an explicit Save() - // remains required. - // if err := cs.persist(newCfg); err != nil { - // return nil, errors.Wrap(err, "failed to persist") - // } + if err := persist(newCfg); err != nil { + return nil, errors.Wrap(err, "failed to persist") + } cs.config = newCfg diff --git a/config/database.go b/config/database.go index e8ff20cd3e..cfc95ae029 100644 --- a/config/database.go +++ b/config/database.go @@ -111,9 +111,10 @@ func parseDSN(dsn string) (string, string, error) { return scheme, dsn, nil } -// Set replaces the current configuration in its entirety, without updating the backing store. +// Set replaces the current configuration in its entirety and updates the backing store. func (ds *DatabaseStore) Set(newCfg *model.Config) (*model.Config, error) { - return ds.commonStore.set(newCfg, nil) + return ds.commonStore.set(newCfg, nil, ds.persist) + } // persist writes the configuration to the configured database. @@ -192,14 +193,6 @@ func (ds *DatabaseStore) Load() (err error) { return ds.commonStore.load(ioutil.NopCloser(bytes.NewReader(configurationData)), needsSave, ds.persist) } -// Save writes the current configuration to the backing store. -func (ds *DatabaseStore) Save() error { - ds.configLock.RLock() - defer ds.configLock.RUnlock() - - return ds.persist(ds.config) -} - // String returns the path to the database backing the config, masking the password. func (ds *DatabaseStore) String() string { u, _ := url.Parse(ds.originalDsn) diff --git a/config/database_test.go b/config/database_test.go index b596090c35..a3c03bdaa4 100644 --- a/config/database_test.go +++ b/config/database_test.go @@ -268,6 +268,29 @@ func TestDatabaseStoreSet(t *testing.T) { assert.Equal(t, "http://new", *ds.Get().ServiceSettings.SiteURL) }) + t.Run("set with automatic save", func(t *testing.T) { + _, tearDown := setupConfigDatabase(t, minimalConfig) + defer tearDown() + + ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *sqlSettings.DriverName, *sqlSettings.DataSource)) + require.NoError(t, err) + defer ds.Close() + + newCfg := &model.Config{ + ServiceSettings: model.ServiceSettings{ + SiteURL: sToP("http://new"), + }, + } + + _, err = ds.Set(newCfg) + require.NoError(t, err) + + err = ds.Load() + require.NoError(t, err) + + assert.Equal(t, "http://new", *ds.Get().ServiceSettings.SiteURL) + }) + t.Run("persist failed", func(t *testing.T) { t.Skip("skipping persistence test inside Set") _, tearDown := setupConfigDatabase(t, emptyConfig) @@ -421,45 +444,6 @@ func TestDatabaseStoreLoad(t *testing.T) { }) } -func TestDatabaseStoreSave(t *testing.T) { - _, tearDown := setupConfigDatabase(t, minimalConfig) - defer tearDown() - - sqlSettings := mainHelper.GetSqlSettings() - ds, err := config.NewDatabaseStore(fmt.Sprintf("%s://%s", *sqlSettings.DriverName, *sqlSettings.DataSource)) - require.NoError(t, err) - defer ds.Close() - - newCfg := &model.Config{ - ServiceSettings: model.ServiceSettings{ - SiteURL: sToP("http://new"), - }, - } - - t.Run("set without save", func(t *testing.T) { - _, err = ds.Set(newCfg) - require.NoError(t, err) - - err = ds.Load() - require.NoError(t, err) - - assert.Equal(t, "http://minimal", *ds.Get().ServiceSettings.SiteURL) - }) - - t.Run("set with save", func(t *testing.T) { - _, err = ds.Set(newCfg) - require.NoError(t, err) - - err = ds.Save() - require.NoError(t, err) - - err = ds.Load() - require.NoError(t, err) - - assert.Equal(t, "http://new", *ds.Get().ServiceSettings.SiteURL) - }) -} - func TestDatabaseStoreString(t *testing.T) { _, tearDown := setupConfigDatabase(t, emptyConfig) defer tearDown() diff --git a/config/file.go b/config/file.go index 24740d8f0a..6475713e8d 100644 --- a/config/file.go +++ b/config/file.go @@ -86,7 +86,7 @@ func resolveConfigFilePath(path string) (string, error) { return "", fmt.Errorf("failed to find config file %s", path) } -// Set replaces the current configuration in its entirety, without updating the backing store. +// Set replaces the current configuration in its entirety and updates the backing store. func (fs *FileStore) Set(newCfg *model.Config) (*model.Config, error) { return fs.commonStore.set(newCfg, func(cfg *model.Config) error { if *fs.config.ClusterSettings.Enable && *fs.config.ClusterSettings.ReadOnlyConfig { @@ -94,7 +94,7 @@ func (fs *FileStore) Set(newCfg *model.Config) (*model.Config, error) { } return nil - }) + }, fs.persist) } // persist writes the configuration to the configured file. @@ -152,14 +152,6 @@ func (fs *FileStore) Load() (err error) { return fs.commonStore.load(f, needsSave, fs.persist) } -// Save writes the current configuration to the backing store. -func (fs *FileStore) Save() error { - fs.configLock.Lock() - defer fs.configLock.Unlock() - - return fs.persist(fs.config) -} - // startWatcher starts a watcher to monitor for external config file changes. func (fs *FileStore) startWatcher() error { if fs.watcher != nil { diff --git a/config/file_test.go b/config/file_test.go index bbac1e88a5..0c75b29218 100644 --- a/config/file_test.go +++ b/config/file_test.go @@ -615,26 +615,13 @@ func TestFileStoreSave(t *testing.T) { }, } - t.Run("set without save", func(t *testing.T) { + t.Run("set with automatic save", func(t *testing.T) { _, err = fs.Set(newCfg) require.NoError(t, err) err = fs.Load() require.NoError(t, err) - assert.Equal(t, "http://minimal", *fs.Get().ServiceSettings.SiteURL) - }) - - t.Run("set with save", func(t *testing.T) { - _, err = fs.Set(newCfg) - require.NoError(t, err) - - err = fs.Save() - require.NoError(t, err) - - err = fs.Load() - require.NoError(t, err) - assert.Equal(t, "http://new", *fs.Get().ServiceSettings.SiteURL) }) } diff --git a/config/memory.go b/config/memory.go index e93f68e453..edf8ed2d4f 100644 --- a/config/memory.go +++ b/config/memory.go @@ -87,11 +87,6 @@ func (ms *memoryStore) Load() (err error) { return nil } -// Save does nothing, as there is no backing store. -func (ms *memoryStore) Save() error { - return nil -} - // String returns a hard-coded description, as there is no backing store. func (ms *memoryStore) String() string { return "mock://" diff --git a/config/store.go b/config/store.go index a1787cda35..4dbc9e6978 100644 --- a/config/store.go +++ b/config/store.go @@ -20,15 +20,12 @@ type Store interface { // GetEnvironmentOverrides fetches the configuration fields overridden by environment variables. GetEnvironmentOverrides() map[string]interface{} - // Set replaces the current configuration in its entirety, without updating the backing store. + // Set replaces the current configuration in its entirety and updates the backing store. Set(*model.Config) (*model.Config, error) // Load updates the current configuration from the backing store, possibly initializing. Load() (err error) - // Save writes the current configuration to the backing store. - Save() error - // AddListener adds a callback function to invoke when the configuration is modified. AddListener(listener Listener) string