Correctly merge plugin configuration on mmctl config patch (#26647)

* Adds a step to the config patch command to merge plugin configurations * Fix linter --------- Co-authored-by: Mattermost Build <[email protected]>
mattermost · Jul 8, 2024 · bc49f34 · bc49f34
1 parent 4e32da6
commit bc49f34
Show file tree

Hide file tree

Showing 4 changed files with 225 additions and 4 deletions.
diff --git a/server/cmd/mmctl/commands/config.go b/server/cmd/mmctl/commands/config.go
@@ -377,10 +377,21 @@ func configPatchCmdF(c client.Client, _ *cobra.Command, args []string) error {
 		return err
 	}
 
+	// get original plugin map
+	var pluginConfig map[string]map[string]any
+	if config.PluginSettings.Plugins != nil {
+		pluginConfig = (config.Clone()).PluginSettings.Plugins
+	}
+
+	// apply path onto the existing config
 	if jErr := json.Unmarshal(configBytes, config); jErr != nil {
 		return jErr
 	}
 
+	// merge config plugin map on top of the original, and assign the
+	// result to the config key
+	config.PluginSettings.Plugins = MergePluginConfigs(pluginConfig, config.PluginSettings.Plugins)
+
 	newConfig, _, err := c.PatchConfig(context.TODO(), config)
 	if err != nil {
 		return err

diff --git a/server/cmd/mmctl/commands/config_test.go b/server/cmd/mmctl/commands/config_test.go
@@ -20,7 +20,8 @@ import (
 )
 
 const (
-	configFilePayload = "{\"TeamSettings\": {\"SiteName\": \"ADifferentName\"}}"
+	configFilePayload       = "{\"TeamSettings\": {\"SiteName\": \"ADifferentName\"}}"
+	configFilePluginPayload = "{\"PluginSettings\": {\"Plugins\": {\"plugin.1\": {\"new\": \"key\", \"existing\": \"replacement\"}, \"plugin.2\": {\"this is\": \"new\"}}}}"
 )
 
 func (s *MmctlUnitTestSuite) TestConfigGetCmd() {
@@ -589,17 +590,24 @@ func (s *MmctlUnitTestSuite) TestConfigSetCmd() {
 
 func (s *MmctlUnitTestSuite) TestConfigPatchCmd() {
 	tmpFile, err := os.CreateTemp(os.TempDir(), "config_*.json")
-	s.Require().Nil(err)
+	s.Require().NoError(err)
 
 	invalidFile, err := os.CreateTemp(os.TempDir(), "invalid_config_*.json")
-	s.Require().Nil(err)
+	s.Require().NoError(err)
+
+	pluginFile, err := os.CreateTemp(os.TempDir(), "plugin_config_*.json")
+	s.Require().NoError(err)
 
 	_, err = tmpFile.Write([]byte(configFilePayload))
-	s.Require().Nil(err)
+	s.Require().NoError(err)
+
+	_, err = pluginFile.Write([]byte(configFilePluginPayload))
+	s.Require().NoError(err)
 
 	defer func() {
 		os.Remove(tmpFile.Name())
 		os.Remove(invalidFile.Name())
+		os.Remove(pluginFile.Name())
 	}()
 
 	s.Run("Patch config with a valid file", func() {
@@ -633,6 +641,46 @@ func (s *MmctlUnitTestSuite) TestConfigPatchCmd() {
 		s.Require().Len(printer.GetErrorLines(), 0)
 	})
 
+	s.Run("Correctly patch with a valid file that affects plugins", func() {
+		printer.Clean()
+		defaultConfig := &model.Config{}
+		defaultConfig.SetDefaults()
+		defaultConfig.PluginSettings.Plugins = map[string]map[string]any{
+			"plugin.1": {
+				"existing": "value",
+			},
+		}
+		expectedPluginConfig := map[string]map[string]any{
+			"plugin.1": {
+				"new":      "key",
+				"existing": "replacement",
+			},
+			"plugin.2": {
+				"this is": "new",
+			},
+		}
+		expectedConfig := &model.Config{}
+		expectedConfig.SetDefaults()
+		expectedConfig.PluginSettings.Plugins = expectedPluginConfig
+
+		s.client.
+			EXPECT().
+			GetConfig(context.TODO()).
+			Return(defaultConfig, &model.Response{}, nil).
+			Times(1)
+		s.client.
+			EXPECT().
+			PatchConfig(context.TODO(), expectedConfig).
+			Return(expectedConfig, &model.Response{}, nil).
+			Times(1)
+
+		err = configPatchCmdF(s.client, &cobra.Command{}, []string{pluginFile.Name()})
+		s.Require().Nil(err)
+		s.Require().Len(printer.GetLines(), 1)
+		s.Require().Equal(printer.GetLines()[0], expectedConfig)
+		s.Require().Len(printer.GetErrorLines(), 0)
+	})
+
 	s.Run("Fail to patch config if file is invalid", func() {
 		printer.Clean()
 		defaultConfig := &model.Config{}

diff --git a/server/cmd/mmctl/commands/utils.go b/server/cmd/mmctl/commands/utils.go
@@ -119,3 +119,64 @@ func getPages[T any](fn func(page, numPerPage int, etag string) ([]T, *model.Res
 	}
 	return results, nil
 }
+
+func MergePluginConfigs(base, patch map[string]map[string]any) map[string]map[string]any {
+	mergedConfigs := map[string]map[string]any{}
+	for k, baseVal := range base {
+		if patchVal, ok := patch[k]; ok {
+			// both values are present, so we do a deep merge
+			mergedConfigs[k] = DeepMergeMaps(baseVal, patchVal)
+			continue
+		}
+
+		// value was not present in patch, so we use base
+		mergedConfigs[k] = baseVal
+	}
+
+	for k, patchVal := range patch {
+		if _, ok := mergedConfigs[k]; ok {
+			// value has already been merged
+			continue
+		}
+
+		// value was not present in base, so we use patch
+		mergedConfigs[k] = patchVal
+	}
+
+	return mergedConfigs
+}
+
+func DeepMergeMaps(base, patch map[string]any) map[string]any {
+	mergedMaps := map[string]any{}
+	for k, baseVal := range base {
+		if patchVal, ok := patch[k]; ok {
+			if baseMap, ok := baseVal.(map[string]any); ok {
+				if patchMap, ok := patchVal.(map[string]any); ok {
+					// both values are map, so we merge recursively
+					mergedMaps[k] = DeepMergeMaps(baseMap, patchMap)
+					continue
+				}
+			}
+
+			// either base is a map but patch is not, or base is not a
+			// map; in any case, patch has precedence
+			mergedMaps[k] = patchVal
+			continue
+		}
+
+		// patch doesn't contain the key, so we use base
+		mergedMaps[k] = baseVal
+	}
+
+	for k, patchVal := range patch {
+		if _, ok := mergedMaps[k]; ok {
+			// value has already been calculated
+			continue
+		}
+
+		// value was not present in base, so we directly use patch
+		mergedMaps[k] = patchVal
+	}
+
+	return mergedMaps
+}
diff --git a/server/cmd/mmctl/commands/utils_test.go b/server/cmd/mmctl/commands/utils_test.go
@@ -0,0 +1,101 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+package commands
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestDeepMergeMaps(t *testing.T) {
+	testCases := []struct {
+		Name     string
+		Base     map[string]any
+		Patch    map[string]any
+		Expected map[string]any
+	}{
+		{
+			Name:     "Values of base doesn't exist in patch",
+			Base:     map[string]any{"Name": "John", "Surname": "Doe"},
+			Patch:    map[string]any{"Name": "Jane"},
+			Expected: map[string]any{"Name": "Jane", "Surname": "Doe"},
+		},
+		{
+			Name:     "Values of patch doesn't exist in base",
+			Base:     map[string]any{"Name": "John"},
+			Patch:    map[string]any{"Name": "Jane", "Surname": "Doe"},
+			Expected: map[string]any{"Name": "Jane", "Surname": "Doe"},
+		},
+		{
+			Name:     "Maps contain nested maps",
+			Base:     map[string]any{"Person": map[string]any{"Name": "John", "Surname": "Doe"}},
+			Patch:    map[string]any{"Person": map[string]any{"Name": "Jane"}, "Age": 27},
+			Expected: map[string]any{"Person": map[string]any{"Name": "Jane", "Surname": "Doe"}, "Age": 27},
+		},
+		{
+			Name:     "Values have different types",
+			Base:     map[string]any{"Person": "John Doe"},
+			Patch:    map[string]any{"Person": map[string]any{"Name": "John", "Surname": "Doe"}},
+			Expected: map[string]any{"Person": map[string]any{"Name": "John", "Surname": "Doe"}},
+		},
+		{
+			Name:     "Values have different types, reversed",
+			Base:     map[string]any{"Person": map[string]any{"Name": "John", "Surname": "Doe"}},
+			Patch:    map[string]any{"Person": "John Doe"},
+			Expected: map[string]any{"Person": "John Doe"},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.Name, func(t *testing.T) {
+			require.Equal(t, DeepMergeMaps(tc.Base, tc.Patch), tc.Expected)
+		})
+	}
+}
+
+func TestMergePluginConfigs(t *testing.T) {
+	basePluginConfig := map[string]map[string]any{
+		"plugin.1": {
+			"First": "key",
+			"Second": map[string]any{
+				"nested": "key",
+			},
+		},
+		"plugin.2": {
+			"Name": "John",
+		},
+	}
+
+	patchPluginConfig := map[string]map[string]any{
+		"plugin.1": {
+			"Second": map[string]any{
+				"nested": false,
+				"new":    1,
+			},
+		},
+		"plugin.3": {
+			"New": "plugin",
+		},
+	}
+
+	expectedPluginConfig := map[string]map[string]any{
+		"plugin.1": {
+			"First": "key",
+			"Second": map[string]any{
+				"nested": false,
+				"new":    1,
+			},
+		},
+		"plugin.2": {
+			"Name": "John",
+		},
+		"plugin.3": {
+			"New": "plugin",
+		},
+	}
+
+	mergedPluginConfigs := MergePluginConfigs(basePluginConfig, patchPluginConfig)
+	require.Equal(t, expectedPluginConfig, mergedPluginConfigs)
+}