From bc49f348bf76b685461f7c036f7a7e415443870c Mon Sep 17 00:00:00 2001 From: Miguel de la Cruz Date: Mon, 8 Jul 2024 11:54:40 +0200 Subject: [PATCH] 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 --- server/cmd/mmctl/commands/config.go | 11 +++ server/cmd/mmctl/commands/config_test.go | 56 ++++++++++++- server/cmd/mmctl/commands/utils.go | 61 ++++++++++++++ server/cmd/mmctl/commands/utils_test.go | 101 +++++++++++++++++++++++ 4 files changed, 225 insertions(+), 4 deletions(-) create mode 100644 server/cmd/mmctl/commands/utils_test.go diff --git a/server/cmd/mmctl/commands/config.go b/server/cmd/mmctl/commands/config.go index d672530a9a..60ffdf4d57 100644 --- 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 index 62603df385..913abb66f8 100644 --- 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 index 5d034358b0..7c7506b27b 100644 --- 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 new file mode 100644 index 0000000000..f88e518000 --- /dev/null +++ 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) +}