diff --git a/internal/app/app_client.go b/internal/app/app_client.go index aecaf513..cc44830f 100644 --- a/internal/app/app_client.go +++ b/internal/app/app_client.go @@ -22,6 +22,7 @@ import ( "strings" "github.com/slackapi/slack-cli/internal/config" + "github.com/slackapi/slack-cli/internal/goutils" "github.com/slackapi/slack-cli/internal/shared/types" "github.com/slackapi/slack-cli/internal/slackerror" "github.com/slackapi/slack-cli/internal/style" @@ -297,6 +298,22 @@ func (ac *AppClient) readDeployedApps() error { return err } + // Treat an empty (or whitespace-only) file as "no apps saved yet" rather + // than a parse error. This state occurs when a prior write was truncated + // but not completed (e.g. a process was interrupted between afero.WriteFile's + // O_TRUNC and the actual write). Without this guard, every subsequent CLI + // invocation logs ErrUnableToParseJSON on the same file. + if goutils.IsEmptyJSON(f) { + ac.apps = types.Apps{ + DeployedApps: map[string]types.App{}, + LocalApps: map[string]types.App{}, + } + if err = ac.saveDeployedApps(); err != nil { + return err + } + return nil + } + if err = json.Unmarshal(f, &ac.apps); err != nil { return slackerror.New(slackerror.ErrUnableToParseJSON). WithMessage("Failed to parse contents of deployed apps file"). @@ -371,6 +388,17 @@ func (ac *AppClient) readLocalApps() error { return err } + // Treat an empty (or whitespace-only) file as "no local apps saved yet" + // rather than a parse error. See readDeployedApps for the corresponding + // note on why this state occurs. + if goutils.IsEmptyJSON(f) { + ac.apps.LocalApps = map[string]types.App{} + if err = ac.saveLocalApps(); err != nil { + return err + } + return nil + } + err = json.Unmarshal(f, &ac.apps.LocalApps) if err != nil { return slackerror.New(slackerror.ErrUnableToParseJSON). diff --git a/internal/app/app_client_test.go b/internal/app/app_client_test.go index 56778e70..f3151732 100644 --- a/internal/app/app_client_test.go +++ b/internal/app/app_client_test.go @@ -165,6 +165,30 @@ func Test_AppClient_ReadDeployedApps_BrokenAppsJSON(t *testing.T) { assert.Equal(t, err.(*slackerror.Error).Code, slackerror.ErrUnableToParseJSON) } +// Test that a zero-byte or whitespace-only apps.json (e.g. from a truncated +// write after a prior process interrupt) is treated as empty state, not a +// parse error. This prevents a hot loop of unable_to_parse_json events on +// every CLI invocation while the file remains empty on disk. +func Test_AppClient_ReadDeployedApps_EmptyAppsJSON(t *testing.T) { + tests := map[string]string{ + "zero-byte file": "", + "whitespace-only file": " \n\t\n", + } + for name, contents := range tests { + t.Run(name, func(t *testing.T) { + ac, _, _, pathToAppsJSON, _, teardown := setup(t) + defer teardown(t) + err := afero.WriteFile(ac.fs, pathToAppsJSON, []byte(contents), 0600) + require.NoError(t, err) + err = ac.readDeployedApps() + require.NoError(t, err) + // The empty file should be rewritten as a valid empty apps object. + f, _ := afero.ReadFile(ac.fs, pathToAppsJSON) + assert.Equal(t, "{}", string(f)) + }) + } +} + // Test that pre-existing dev app details get read from apps.dev.json func Test_AppClient_ReadDevApps_ExistingAppsJSON(t *testing.T) { ac, _, _, _, pathToDevAppsJSON, teardown := setup(t) @@ -207,6 +231,29 @@ func Test_AppClient_ReadDevApps_BrokenAppsJSON(t *testing.T) { assert.Equal(t, err.(*slackerror.Error).Code, slackerror.ErrUnableToParseJSON) } +// Test that a zero-byte or whitespace-only apps.dev.json (e.g. from a +// truncated write after a prior process interrupt) is treated as empty +// state, not a parse error. See Test_AppClient_ReadDeployedApps_EmptyAppsJSON. +func Test_AppClient_ReadDevApps_EmptyAppsJSON(t *testing.T) { + tests := map[string]string{ + "zero-byte file": "", + "whitespace-only file": " \n\t\n", + } + for name, contents := range tests { + t.Run(name, func(t *testing.T) { + ac, _, _, _, pathToDevAppsJSON, teardown := setup(t) + defer teardown(t) + err := afero.WriteFile(ac.fs, pathToDevAppsJSON, []byte(contents), 0600) + require.NoError(t, err) + err = ac.readLocalApps() + require.NoError(t, err) + // The empty file should be rewritten as a valid empty apps object. + f, _ := afero.ReadFile(ac.fs, pathToDevAppsJSON) + assert.Equal(t, "{}", string(f)) + }) + } +} + // Test that a team flag config defines the default app name in an empty AppClient func Test_AppClient_getDeployedAppTeamDomain_ViaCLIFlag(t *testing.T) { ac, _, _, _, _, teardown := setup(t) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index b3dcd8c6..624b6247 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -216,6 +216,16 @@ func (c *Client) auths(ctx context.Context) (map[string]types.SlackAuth, error) return auths, err } + // Treat an empty (or whitespace-only) credentials file as "no credentials + // stored" rather than a parse error. This state occurs when a prior write + // was truncated but not completed (e.g. a process was interrupted between + // afero.WriteFile's O_TRUNC and the actual write). Without this guard, + // every subsequent CLI invocation reads the same 0-byte file and logs + // ErrUnableToParseJSON in a hot loop. + if goutils.IsEmptyJSON(raw) { + return types.AuthByTeamID{}, nil + } + err = json.Unmarshal(raw, &auths) if err != nil { return auths, slackerror.New(slackerror.ErrUnableToParseJSON). diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 3d7cc5b7..e0a4c70c 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -222,6 +222,30 @@ func Test_AuthGettersAndSetters(t *testing.T) { require.Error(t, err) assert.Equal(t, err.(*slackerror.Error).Code, slackerror.ErrUnableToParseJSON) }) + + // A zero-byte or whitespace-only credentials.json (e.g. from a truncated + // write after a prior process interrupt) should be treated as empty state + // rather than a parse error, otherwise every CLI invocation would log + // ErrUnableToParseJSON in a hot loop. + t.Run("empty credentials file returns empty auth state, not a parse error", func(t *testing.T) { + cases := map[string]string{ + "zero-byte file": "", + "whitespace-only file": " \n\t\n", + } + for name, contents := range cases { + t.Run(name, func(t *testing.T) { + ctx, authClient := setup(t) + dir, err := authClient.config.SystemConfig.SlackConfigDir(ctx) + require.NoError(t, err) + path := filepath.Join(dir, credentialsFileName) + err = afero.WriteFile(authClient.fs, path, []byte(contents), 0o600) + require.NoError(t, err) + got, err := authClient.auths(ctx) + require.NoError(t, err) + require.Empty(t, got) + }) + } + }) } func Test_AuthsRotation(t *testing.T) { diff --git a/internal/config/project.go b/internal/config/project.go index c2c03be1..26381e33 100644 --- a/internal/config/project.go +++ b/internal/config/project.go @@ -25,6 +25,7 @@ import ( "github.com/google/uuid" "github.com/opentracing/opentracing-go" "github.com/slackapi/slack-cli/internal/cache" + "github.com/slackapi/slack-cli/internal/goutils" "github.com/slackapi/slack-cli/internal/shared/types" "github.com/slackapi/slack-cli/internal/slackerror" "github.com/slackapi/slack-cli/internal/style" @@ -254,6 +255,17 @@ func ReadProjectConfigFile(ctx context.Context, fs afero.Fs, os types.Os) (Proje return projectConfig, err } + // Treat an empty (or whitespace-only) project config as "no project config + // saved yet" rather than a parse error. This state occurs when a prior + // write was truncated but not completed (e.g. a process was interrupted + // between afero.WriteFile's O_TRUNC and the actual write). Without this + // guard, every subsequent CLI invocation reads the same 0-byte file and + // logs ErrUnableToParseJSON in a hot loop. + if goutils.IsEmptyJSON(projectConfigFileBytes) { + projectConfig.Surveys = map[string]SurveyConfig{} + return projectConfig, nil + } + err = json.Unmarshal(projectConfigFileBytes, &projectConfig) if err != nil { return projectConfig, slackerror.New(slackerror.ErrUnableToParseJSON). diff --git a/internal/config/project_test.go b/internal/config/project_test.go index 7c3f1573..1ffa36d3 100644 --- a/internal/config/project_test.go +++ b/internal/config/project_test.go @@ -352,6 +352,38 @@ func Test_ProjectConfig_ReadProjectConfigFile(t *testing.T) { assert.Equal(t, slackerror.ToSlackError(err).Code, slackerror.ErrUnableToParseJSON) assert.Equal(t, slackerror.ToSlackError(err).Message, "Failed to parse contents of project-level config file") }) + + // A zero-byte or whitespace-only .slack/config.json (e.g. from a truncated + // write after a prior process interrupt) should be treated as empty state + // rather than a parse error, otherwise every CLI invocation would log + // ErrUnableToParseJSON in a hot loop. + t.Run("empty project config file returns default config, not a parse error", func(t *testing.T) { + cases := map[string]string{ + "zero-byte file": "", + "whitespace-only file": " \n\t\n", + } + for name, contents := range cases { + t.Run(name, func(t *testing.T) { + ctx := slackcontext.MockContext(t.Context()) + fs := slackdeps.NewFsMock() + os := slackdeps.NewOsMock() + + os.AddDefaultMocks() + addProjectMocks(t, fs) + + projectDirPath, err := GetProjectDirPath(fs, os) + require.NoError(t, err) + projectConfigFilePath := GetProjectConfigJSONFilePath(projectDirPath) + + err = afero.WriteFile(fs, projectConfigFilePath, []byte(contents), 0600) + require.NoError(t, err) + + got, err := ReadProjectConfigFile(ctx, fs, os) + require.NoError(t, err) + require.NotNil(t, got.Surveys) + }) + } + }) } func Test_ProjectConfig_WriteProjectConfigFile(t *testing.T) { diff --git a/internal/config/system.go b/internal/config/system.go index b1ae969c..28945b53 100644 --- a/internal/config/system.go +++ b/internal/config/system.go @@ -26,6 +26,7 @@ import ( "github.com/google/uuid" "github.com/opentracing/opentracing-go" + "github.com/slackapi/slack-cli/internal/goutils" "github.com/slackapi/slack-cli/internal/shared/types" "github.com/slackapi/slack-cli/internal/slackerror" "github.com/slackapi/slack-cli/internal/style" @@ -127,6 +128,17 @@ func (c *SystemConfig) UserConfig(ctx context.Context) (*SystemConfig, error) { return &config, err } + // Treat an empty (or whitespace-only) config file as "no user config saved + // yet" rather than a parse error. This state occurs when a prior write was + // truncated but not completed (e.g. a process was interrupted between + // afero.WriteFile's O_TRUNC and the actual write). Without this guard, + // every subsequent CLI invocation reads the same 0-byte file and logs + // ErrUnableToParseJSON in a hot loop. + if goutils.IsEmptyJSON(configFileBytes) { + config.Surveys = map[string]SurveyConfig{} + return &config, nil + } + err = json.Unmarshal(configFileBytes, &config) if err != nil { return &config, slackerror.New(slackerror.ErrUnableToParseJSON). diff --git a/internal/config/system_test.go b/internal/config/system_test.go index 34f85b7a..835953a2 100644 --- a/internal/config/system_test.go +++ b/internal/config/system_test.go @@ -107,6 +107,37 @@ func Test_SystemConfig_UserConfig(t *testing.T) { assert.Equal(t, slackerror.ToSlackError(err).Code, slackerror.ErrUnableToParseJSON) assert.Equal(t, slackerror.ToSlackError(err).Message, "Failed to parse contents of system-level config file") }) + + // A zero-byte or whitespace-only ~/.slack/config.json (e.g. from a truncated + // write after a prior process interrupt) should be treated as empty state + // rather than a parse error, otherwise every CLI invocation would log + // ErrUnableToParseJSON in a hot loop. + t.Run("empty configuration file returns default config, not a parse error", func(t *testing.T) { + cases := map[string]string{ + "zero-byte file": "", + "whitespace-only file": " \n\t\n", + } + for name, contents := range cases { + t.Run(name, func(t *testing.T) { + ctx := slackcontext.MockContext(t.Context()) + fs := slackdeps.NewFsMock() + os := slackdeps.NewOsMock() + + os.AddDefaultMocks() + + configFilePath := filepath.Join(slackdeps.MockHomeDirectory, configFolderName, configFileName) + err := afero.WriteFile(fs, configFilePath, []byte(contents), 0600) + require.NoError(t, err) + + systemConfig := NewSystemConfig(fs, os) + got, err := systemConfig.UserConfig(ctx) + + require.NoError(t, err) + require.NotNil(t, got) + require.NotNil(t, got.Surveys) + }) + } + }) } func Test_Config_SlackConfigDir(t *testing.T) { diff --git a/internal/goutils/json.go b/internal/goutils/json.go index 59b5a95a..94aeb2af 100644 --- a/internal/goutils/json.go +++ b/internal/goutils/json.go @@ -84,3 +84,14 @@ func JSONUnmarshal(data []byte, v interface{}) error { } return nil } + +// IsEmptyJSON returns true when the provided bytes are empty or contain only +// whitespace. This matches the state left behind when a JSON state file was +// truncated but not yet rewritten, e.g. after a process was interrupted between +// afero.WriteFile's O_TRUNC and the actual write. Callers that read on-disk +// config/auth/app state should treat this case as "empty state" rather than a +// parse error, otherwise every subsequent CLI invocation logs +// ErrUnableToParseJSON until the user manually clears the file. +func IsEmptyJSON(data []byte) bool { + return len(bytes.TrimSpace(data)) == 0 +} diff --git a/internal/goutils/json_test.go b/internal/goutils/json_test.go index 62640fd3..b8f91622 100644 --- a/internal/goutils/json_test.go +++ b/internal/goutils/json_test.go @@ -159,6 +159,26 @@ func Test_JSONMarshalUnescapedIndent(t *testing.T) { } } +func Test_IsEmptyJSON(t *testing.T) { + for name, tc := range map[string]struct { + data string + expected bool + }{ + "nil bytes": {data: "", expected: true}, + "empty string": {data: "", expected: true}, + "single space": {data: " ", expected: true}, + "whitespace mix": {data: " \n\t\n", expected: true}, + "empty JSON object": {data: "{}", expected: false}, + "whitespace around JSON": {data: " {} ", expected: false}, + "JSON payload": {data: `{"one":"1"}`, expected: false}, + "invalid JSON is nonzero": {data: "{", expected: false}, + } { + t.Run(name, func(t *testing.T) { + assert.Equal(t, tc.expected, IsEmptyJSON([]byte(tc.data))) + }) + } +} + func Test_UnmarshalJSON(t *testing.T) { type testConfig struct { One string `json:"one,omitempty"`