Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions internal/app/app_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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").
Expand Down Expand Up @@ -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).
Expand Down
47 changes: 47 additions & 0 deletions internal/app/app_client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
10 changes: 10 additions & 0 deletions internal/auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
24 changes: 24 additions & 0 deletions internal/auth/auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
12 changes: 12 additions & 0 deletions internal/config/project.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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).
Expand Down
32 changes: 32 additions & 0 deletions internal/config/project_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
12 changes: 12 additions & 0 deletions internal/config/system.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: I thought there might be an initialize function, but this same pattern is used elsewhere.

return &config, nil
}

err = json.Unmarshal(configFileBytes, &config)
if err != nil {
return &config, slackerror.New(slackerror.ErrUnableToParseJSON).
Expand Down
31 changes: 31 additions & 0 deletions internal/config/system_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
11 changes: 11 additions & 0 deletions internal/goutils/json.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
20 changes: 20 additions & 0 deletions internal/goutils/json_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down
Loading