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

@@ -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()

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 {

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
 }

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
 }

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
 

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)

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

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 {

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

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://"

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