diff --git a/internal/config/config.go b/internal/config/config.go index 9855825f..8d3e4237 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -15,7 +15,6 @@ package config import ( - "github.com/slackapi/slack-cli/internal/experiment" "github.com/slackapi/slack-cli/internal/shared/types" "github.com/spf13/afero" "github.com/spf13/pflag" @@ -63,7 +62,7 @@ type Config struct { // Feature experiments ExperimentsFlag []string - experiments []experiment.Experiment + experiments map[string]bool // Eventually this will also load the global and project slack config files DomainAuthTokens string diff --git a/internal/config/experiments.go b/internal/config/experiments.go index 2cb5705a..3da1b534 100644 --- a/internal/config/experiments.go +++ b/internal/config/experiments.go @@ -17,7 +17,9 @@ package config import ( "context" "fmt" - "slices" + "maps" + "sort" + "strings" "github.com/slackapi/slack-cli/internal/experiment" "github.com/slackapi/slack-cli/internal/slackerror" @@ -29,47 +31,73 @@ func (c *Config) LoadExperiments( ctx context.Context, printDebug func(ctx context.Context, format string, a ...interface{}), ) { - experiments := []experiment.Experiment{} + experiments := map[string]bool{} // Load from flags for _, flagValue := range c.ExperimentsFlag { - experiments = append(experiments, experiment.Experiment(flagValue)) + experiments[flagValue] = true } - printDebug(ctx, fmt.Sprintf("active flag experiments: %s", experiments)) + printDebug(ctx, fmt.Sprintf("active flag experiments: %s", formatExperimentMap(experiments))) // Load from project config file projectConfig, err := ReadProjectConfigFile(ctx, c.fs, c.os) if err != nil && slackerror.ToSlackError(err).Code != slackerror.ErrInvalidAppDirectory { printDebug(ctx, fmt.Sprintf("failed to parse project-level config file: %s", err)) } else if err == nil { - printDebug(ctx, fmt.Sprintf("active project experiments: %s", projectConfig.Experiments)) - experiments = append(experiments, projectConfig.Experiments...) + printDebug(ctx, fmt.Sprintf("active project experiments: %s", formatExperimentMap(projectConfig.Experiments))) + maps.Copy(experiments, projectConfig.Experiments) } // Load from system config file userConfig, err := c.SystemConfig.UserConfig(ctx) if err != nil { printDebug(ctx, fmt.Sprintf("failed to parse system-level config file: %s", err)) } else { - printDebug(ctx, fmt.Sprintf("active system experiments: %s", userConfig.Experiments)) - experiments = append(experiments, userConfig.Experiments...) + printDebug(ctx, fmt.Sprintf("active system experiments: %s", formatExperimentMap(userConfig.Experiments))) + maps.Copy(experiments, userConfig.Experiments) } // Load from permanently enabled list - experiments = append(experiments, experiment.EnabledExperiments...) + for _, exp := range experiment.EnabledExperiments { + experiments[string(exp)] = true + } printDebug(ctx, fmt.Sprintf("active permanently enabled experiments: %s", experiment.EnabledExperiments)) - // Sort, trim, and audit the experiments list - slices.Sort(experiments) - c.experiments = slices.Compact(experiments) - for _, exp := range c.experiments { - if !experiment.Includes(exp) { - printDebug(ctx, fmt.Sprintf("invalid experiment found: %s", exp)) + // Audit the experiments + c.experiments = experiments + for name := range c.experiments { + if !experiment.Includes(experiment.Experiment(name)) { + printDebug(ctx, fmt.Sprintf("invalid experiment found: %s", name)) } } } // GetExperiments returns the set of active experiments func (c *Config) GetExperiments() []experiment.Experiment { - return c.experiments + result := make([]experiment.Experiment, 0, len(c.experiments)) + for name, enabled := range c.experiments { + if enabled { + result = append(result, experiment.Experiment(name)) + } + } + sort.Slice(result, func(i, j int) bool { + return result[i] < result[j] + }) + return result } // WithExperimentOn checks whether an experiment is currently toggled on func (c *Config) WithExperimentOn(experimentToCheck experiment.Experiment) bool { - return slices.Contains(c.experiments, experimentToCheck) + return c.experiments[string(experimentToCheck)] +} + +// formatExperimentMap returns a string representation of the experiments map +// for debug logging, formatted similar to the old slice format. +func formatExperimentMap(m map[string]bool) string { + if len(m) == 0 { + return "[]" + } + names := make([]string, 0, len(m)) + for name, enabled := range m { + if enabled { + names = append(names, name) + } + } + sort.Strings(names) + return "[" + strings.Join(names, " ") + "]" } diff --git a/internal/config/experiments_test.go b/internal/config/experiments_test.go index 04690076..657f5d33 100644 --- a/internal/config/experiments_test.go +++ b/internal/config/experiments_test.go @@ -19,7 +19,6 @@ import ( "context" "fmt" "path/filepath" - "slices" "testing" "github.com/slackapi/slack-cli/internal/experiment" @@ -40,7 +39,7 @@ func Test_Config_WithExperimentOn(t *testing.T) { mockOutput, mockPrintDebug := setupMockPrintDebug() // Write our test script to the memory file system used by the mock - jsonContents := []byte(fmt.Sprintf("{\"last_update_checked_at\":\"2023-05-11T15:41:07.799619-07:00\",\"experiments\":[\"%s\"]}", validExperiment)) + jsonContents := []byte(fmt.Sprintf(`{"last_update_checked_at":"2023-05-11T15:41:07.799619-07:00","experiments":{"%s":true}}`, validExperiment)) _ = afero.WriteFile(fs, pathToConfigJSON, jsonContents, 0600) config.LoadExperiments(ctx, mockPrintDebug) @@ -50,13 +49,28 @@ func Test_Config_WithExperimentOn(t *testing.T) { fmt.Sprintf("active system experiments: [%s]", validExperiment)) }) + t.Run("Correctly finds experiments from old array format in config.json", func(t *testing.T) { + // Setup + ctx, fs, _, config, pathToConfigJSON, teardown := setup(t) + defer teardown(t) + _, mockPrintDebug := setupMockPrintDebug() + + // Write old array format + jsonContents := []byte(fmt.Sprintf(`{"last_update_checked_at":"2023-05-11T15:41:07.799619-07:00","experiments":["%s"]}`, validExperiment)) + _ = afero.WriteFile(fs, pathToConfigJSON, jsonContents, 0600) + + config.LoadExperiments(ctx, mockPrintDebug) + experimentOn := config.WithExperimentOn(validExperiment) + assert.Equal(t, true, experimentOn) + }) + t.Run("Correctly returns false when experiments are not in config.json", func(t *testing.T) { // Setup ctx, fs, _, config, pathToConfigJSON, teardown := setup(t) defer teardown(t) mockOutput, mockPrintDebug := setupMockPrintDebug() - jsonContents := []byte("{\"last_update_checked_at\":\"2023-05-11T15:41:07.799619-07:00\",\"experiments\":[]}") + jsonContents := []byte(`{"last_update_checked_at":"2023-05-11T15:41:07.799619-07:00","experiments":{}}`) _ = afero.WriteFile(fs, pathToConfigJSON, jsonContents, 0600) config.LoadExperiments(ctx, mockPrintDebug) @@ -72,7 +86,7 @@ func Test_Config_WithExperimentOn(t *testing.T) { mockOutput, mockPrintDebug := setupMockPrintDebug() // Write no contents via config.json - jsonContents := []byte("{\"last_update_checked_at\":\"2023-05-11T15:41:07.799619-07:00\",\"experiments\":[]}") + jsonContents := []byte(`{"last_update_checked_at":"2023-05-11T15:41:07.799619-07:00","experiments":{}}`) _ = afero.WriteFile(fs, pathToConfigJSON, jsonContents, 0600) config.ExperimentsFlag = []string{string(validExperiment)} @@ -89,7 +103,7 @@ func Test_Config_WithExperimentOn(t *testing.T) { defer teardown(t) mockOutput, mockPrintDebug := setupMockPrintDebug() - jsonContents := []byte(fmt.Sprintf("{\"last_update_checked_at\":\"2023-05-11T15:41:07.799619-07:00\",\"experiments\":[\"%s\"]}", invalidExperiment)) + jsonContents := []byte(fmt.Sprintf(`{"last_update_checked_at":"2023-05-11T15:41:07.799619-07:00","experiments":{"%s":true}}`, invalidExperiment)) _ = afero.WriteFile(fs, pathToConfigJSON, jsonContents, 0600) config.LoadExperiments(ctx, mockPrintDebug) assert.Contains(t, mockOutput.String(), @@ -120,7 +134,7 @@ func Test_Config_WithExperimentOn(t *testing.T) { mockOutput, mockPrintDebug := setupMockPrintDebug() // Write contents via config.json - jsonContents := []byte(fmt.Sprintf("{\"last_update_checked_at\":\"2023-05-11T15:41:07.799619-07:00\",\"experiments\":[\"%s\"]}", tc.experiment)) + jsonContents := []byte(fmt.Sprintf(`{"last_update_checked_at":"2023-05-11T15:41:07.799619-07:00","experiments":{"%s":true}}`, tc.experiment)) _ = afero.WriteFile(fs, pathToConfigJSON, jsonContents, 0600) config.LoadExperiments(ctx, mockPrintDebug) @@ -172,7 +186,7 @@ func Test_Config_WithExperimentOn(t *testing.T) { require.NoError(t, err) err = afero.WriteFile(fs, GetProjectHooksJSONFilePath(slackdeps.MockWorkingDirectory), []byte("{}\n"), 0600) require.NoError(t, err) - jsonContents := []byte(fmt.Sprintf(`{"experiments":["%s"]}`, experiment.Placeholder)) + jsonContents := []byte(fmt.Sprintf(`{"experiments":{"%s":true}}`, experiment.Placeholder)) err = afero.WriteFile(fs, GetProjectConfigJSONFilePath(slackdeps.MockWorkingDirectory), []byte(jsonContents), 0600) require.NoError(t, err) @@ -182,39 +196,34 @@ func Test_Config_WithExperimentOn(t *testing.T) { fmt.Sprintf("active project experiments: [%s]", experiment.Placeholder)) }) - t.Run("Loads valid experiments from project configs and removes duplicates", func(t *testing.T) { + t.Run("Loads valid experiments from project configs and deduplicates via map merge", func(t *testing.T) { ctx, fs, _, config, _, teardown := setup(t) defer teardown(t) - mockOutput, mockPrintDebug := setupMockPrintDebug() + _, mockPrintDebug := setupMockPrintDebug() err := fs.Mkdir(slackdeps.MockWorkingDirectory, 0755) require.NoError(t, err) err = fs.Mkdir(filepath.Join(slackdeps.MockWorkingDirectory, ProjectConfigDirName), 0755) require.NoError(t, err) err = afero.WriteFile(fs, GetProjectHooksJSONFilePath(slackdeps.MockWorkingDirectory), []byte("{}\n"), 0600) require.NoError(t, err) - jsonContents := []byte(fmt.Sprintf( - `{"experiments":["%s", "%s", "%s"]}`, - experiment.Placeholder, - experiment.Placeholder, - experiment.Placeholder, - )) + jsonContents := []byte(fmt.Sprintf(`{"experiments":{"%s":true}}`, experiment.Placeholder)) err = afero.WriteFile(fs, GetProjectConfigJSONFilePath(slackdeps.MockWorkingDirectory), []byte(jsonContents), 0600) require.NoError(t, err) + // Also set via flag to test dedup + config.ExperimentsFlag = []string{string(experiment.Placeholder)} + config.LoadExperiments(ctx, mockPrintDebug) assert.True(t, config.WithExperimentOn(experiment.Placeholder)) - assert.Contains(t, mockOutput.String(), fmt.Sprintf( - "active project experiments: [%s %s %s]", - experiment.Placeholder, - experiment.Placeholder, - experiment.Placeholder, - )) - // Assert that duplicates are removed and slice length is reduced - // Add in the permanently enabled experiments before comparing - activeExperiments := append(experiment.EnabledExperiments, experiment.Placeholder) - slices.Sort(activeExperiments) - activeExperiments = slices.Compact(activeExperiments) - assert.Equal(t, activeExperiments, config.experiments) + // Assert that experiments are deduplicated via map + exps := config.GetExperiments() + count := 0 + for _, exp := range exps { + if exp == experiment.Placeholder { + count++ + } + } + assert.Equal(t, 1, count, "experiment should appear exactly once") }) t.Run("Loads valid experiments and enabled default experiments", func(t *testing.T) { @@ -227,7 +236,7 @@ func Test_Config_WithExperimentOn(t *testing.T) { require.NoError(t, err) err = afero.WriteFile(fs, GetProjectHooksJSONFilePath(slackdeps.MockWorkingDirectory), []byte("{}\n"), 0600) require.NoError(t, err) - jsonContents := []byte(`{"experiments":[]}`) // No experiments + jsonContents := []byte(`{"experiments":{}}`) // No experiments err = afero.WriteFile(fs, GetProjectConfigJSONFilePath(slackdeps.MockWorkingDirectory), []byte(jsonContents), 0600) require.NoError(t, err) @@ -244,7 +253,7 @@ func Test_Config_WithExperimentOn(t *testing.T) { assert.True(t, config.WithExperimentOn(experiment.Placeholder)) assert.Contains(t, mockOutput.String(), "active project experiments: []") assert.Contains(t, mockOutput.String(), fmt.Sprintf("active permanently enabled experiments: [%s]", experiment.Placeholder)) - assert.Equal(t, []experiment.Experiment{experiment.Placeholder}, config.experiments) + assert.ElementsMatch(t, []experiment.Experiment{experiment.Placeholder}, config.GetExperiments()) }) t.Run("Logs an invalid experiments in project configs", func(t *testing.T) { @@ -257,7 +266,7 @@ func Test_Config_WithExperimentOn(t *testing.T) { require.NoError(t, err) err = afero.WriteFile(fs, GetProjectHooksJSONFilePath(slackdeps.MockWorkingDirectory), []byte("{}\n"), 0600) require.NoError(t, err) - jsonContents := []byte(fmt.Sprintf("{\"experiments\":[\"%s\"]}", "experiment-37")) + jsonContents := []byte(`{"experiments":{"experiment-37":true}}`) err = afero.WriteFile(fs, GetProjectConfigJSONFilePath(slackdeps.MockWorkingDirectory), []byte(jsonContents), 0600) require.NoError(t, err) @@ -283,7 +292,7 @@ func Test_Config_GetExperiments(t *testing.T) { _, mockPrintDebug := setupMockPrintDebug() // Write contents via config.json - var configJSON = []byte(fmt.Sprintf("{\"last_update_checked_at\":\"2023-05-11T15:41:07.799619-07:00\",\"experiments\":[\"%s\",\"%s\"]}", validExperiment, validExperiment)) + var configJSON = []byte(fmt.Sprintf(`{"last_update_checked_at":"2023-05-11T15:41:07.799619-07:00","experiments":{"%s":true}}`, validExperiment)) _ = afero.WriteFile(fs, pathToConfigJSON, configJSON, 0600) // Set contexts of experiment flag diff --git a/internal/config/experiments_unmarshal.go b/internal/config/experiments_unmarshal.go new file mode 100644 index 00000000..360be53e --- /dev/null +++ b/internal/config/experiments_unmarshal.go @@ -0,0 +1,47 @@ +// Copyright 2022-2026 Salesforce, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package config + +import "encoding/json" + +// unmarshalExperimentsField handles backwards-compatible unmarshaling of the +// experiments field. It accepts both the old array format (["charm", "sandboxes"]) +// and the new map format ({"charm": true, "sandboxes": true}). +func unmarshalExperimentsField(raw json.RawMessage) map[string]bool { + if len(raw) == 0 { + return nil + } + + // Try new map format first + var mapFormat map[string]bool + if err := json.Unmarshal(raw, &mapFormat); err == nil { + return mapFormat + } + + // Fall back to old array format + var arrayFormat []string + if err := json.Unmarshal(raw, &arrayFormat); err == nil { + if len(arrayFormat) == 0 { + return nil + } + result := make(map[string]bool, len(arrayFormat)) + for _, exp := range arrayFormat { + result[exp] = true + } + return result + } + + return nil +} diff --git a/internal/config/project.go b/internal/config/project.go index 8baca15c..2cd1b581 100644 --- a/internal/config/project.go +++ b/internal/config/project.go @@ -25,7 +25,6 @@ import ( "github.com/google/uuid" "github.com/opentracing/opentracing-go" "github.com/slackapi/slack-cli/internal/cache" - "github.com/slackapi/slack-cli/internal/experiment" "github.com/slackapi/slack-cli/internal/shared/types" "github.com/slackapi/slack-cli/internal/slackerror" "github.com/slackapi/slack-cli/internal/style" @@ -60,7 +59,7 @@ type ProjectConfigManager interface { // ProjectConfig is the project-level config file type ProjectConfig struct { - Experiments []experiment.Experiment `json:"experiments,omitempty"` + Experiments map[string]bool `json:"experiments,omitempty"` Manifest *ManifestConfig `json:"manifest,omitempty"` ProjectID string `json:"project_id,omitempty"` Surveys map[string]SurveyConfig `json:"surveys,omitempty"` @@ -72,6 +71,23 @@ type ProjectConfig struct { os types.Os } +// UnmarshalJSON implements custom JSON unmarshaling for ProjectConfig to support +// backwards compatibility with the old array format for experiments. +func (c *ProjectConfig) UnmarshalJSON(data []byte) error { + type Alias ProjectConfig + aux := &struct { + RawExperiments json.RawMessage `json:"experiments,omitempty"` + *Alias + }{ + Alias: (*Alias)(c), + } + if err := json.Unmarshal(data, aux); err != nil { + return err + } + c.Experiments = unmarshalExperimentsField(aux.RawExperiments) + return nil +} + // NewProjectConfig read and writes to the project-level configuration file func NewProjectConfig(fs afero.Fs, os types.Os) *ProjectConfig { projectConfig := &ProjectConfig{ diff --git a/internal/config/system.go b/internal/config/system.go index abb0db4b..be662f60 100644 --- a/internal/config/system.go +++ b/internal/config/system.go @@ -26,7 +26,6 @@ import ( "github.com/google/uuid" "github.com/opentracing/opentracing-go" - "github.com/slackapi/slack-cli/internal/experiment" "github.com/slackapi/slack-cli/internal/shared/types" "github.com/slackapi/slack-cli/internal/slackerror" "github.com/slackapi/slack-cli/internal/style" @@ -63,7 +62,7 @@ type SystemConfigManager interface { // SystemConfig contains the system-level config file type SystemConfig struct { - Experiments []experiment.Experiment `json:"experiments,omitempty"` + Experiments map[string]bool `json:"experiments,omitempty"` LastUpdateCheckedAt time.Time `json:"last_update_checked_at,omitempty"` Surveys map[string]SurveyConfig `json:"surveys,omitempty"` SystemID string `json:"system_id,omitempty"` @@ -82,6 +81,25 @@ type SystemConfig struct { configFileLock sync.Mutex } +// UnmarshalJSON implements custom JSON unmarshaling for SystemConfig to support +// backwards compatibility with the old array format for experiments. +// Old format: "experiments": ["charm", "sandboxes"] +// New format: "experiments": {"charm": true, "sandboxes": true} +func (c *SystemConfig) UnmarshalJSON(data []byte) error { + type Alias SystemConfig + aux := &struct { + RawExperiments json.RawMessage `json:"experiments,omitempty"` + *Alias + }{ + Alias: (*Alias)(c), + } + if err := json.Unmarshal(data, aux); err != nil { + return err + } + c.Experiments = unmarshalExperimentsField(aux.RawExperiments) + return nil +} + // NewSystemConfig read and writes to the system-level configuration directory func NewSystemConfig(fs afero.Fs, os types.Os) *SystemConfig { systemConfig := &SystemConfig{