-
Notifications
You must be signed in to change notification settings - Fork 37
fix: treat empty on-disk JSON state files as empty state #616
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
581ee14
4f63517
77804be
6d374f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
Comment on lines
+219
to
229
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👾 thought: Perhaps covered in the "atomic" reads and writes once more, but I find these functions pair well together and am curious if our own |
||
| if err != nil { | ||
| return auths, slackerror.New(slackerror.ErrUnableToParseJSON). | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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{} | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
| } | ||
|
Comment on lines
+95
to
+97
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🪬 quibble: Calling this "empty JSON" to me is confusing because I'd expect: {}And instead might prefer to call this |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔭 thought: The follow up "atomic" writes is a clever and good workaround to have! I'm worried this patch might cause existing project apps to seem missing instead of a JSON error appearing, but the root cause is unrelated to this PR.