From 65633a11be55f93ed602b3a58a3fb54ac8dbdf95 Mon Sep 17 00:00:00 2001 From: Andrey Markelov Date: Tue, 30 Jun 2026 09:00:26 -0700 Subject: [PATCH] Harden JSON error details and contract tests Add structured JSON error details for operations, paths, URLs, revisions, emails, member IDs, retry-after values, and Dropbox API summaries across file, share-link, restore, and team commands. Collect details through wrapped and joined errors, preserve explicit command-owned details over command context, and avoid false-positive api_summary detection for local path errors. Add golden JSON error output fixtures, schema/docs updates, and subprocess tests that validate JSON error envelopes. --- README.md | 9 + cmd/add-member.go | 2 +- cmd/cp.go | 7 +- cmd/cp_test.go | 4 + cmd/get.go | 28 +-- cmd/get_test.go | 4 + cmd/json_contract_test.go | 168 ++++++++++++++ cmd/json_output.go | 2 +- cmd/mv.go | 7 +- cmd/mv_test.go | 4 + cmd/output.go | 148 +++++++++++- cmd/output_test.go | 213 ++++++++++++++++++ cmd/put.go | 34 +-- cmd/put_test.go | 4 + cmd/relocation_if_exists.go | 12 + cmd/remove-member.go | 2 +- cmd/restore.go | 8 +- cmd/restore_test.go | 7 +- cmd/rm.go | 19 +- cmd/rm_test.go | 7 +- cmd/root_test.go | 44 ++++ cmd/search.go | 4 +- cmd/search_test.go | 8 + cmd/share-list-links.go | 14 +- cmd/share_create_link_test.go | 15 ++ cmd/share_link_create.go | 16 +- cmd/share_link_download.go | 33 +-- cmd/share_link_download_test.go | 15 +- cmd/share_link_info.go | 6 +- cmd/share_link_info_test.go | 4 + cmd/share_link_revoke.go | 27 ++- cmd/share_link_revoke_test.go | 20 ++ cmd/share_link_update.go | 10 +- cmd/team_json_test.go | 42 ++++ cmd/testdata/json_contract/error_outputs.json | 179 +++++++++++++++ docs/json-schema/v1/README.md | 36 ++- docs/json-schema/v1/error.schema.json | 33 +++ 37 files changed, 1089 insertions(+), 106 deletions(-) create mode 100644 cmd/testdata/json_contract/error_outputs.json diff --git a/README.md b/README.md index dcbf2db..1976a8b 100644 --- a/README.md +++ b/README.md @@ -37,6 +37,15 @@ dbxcli put --help --output=json Stable JSON error codes and process exit codes are documented in [Automation and JSON output](https://github.com/dropbox/dbxcli/blob/master/docs/automation.md). +## JSON contract stability + +`--output=json` uses schema v1 success and error envelopes. Schema v1 keeps +top-level fields, stable error codes, and result status meanings stable within +the v1 contract; minor releases may add fields, commands, warnings, and error +details. Use JSON help for machine-readable command manifests, and use the +[JSON schema v1 docs](https://github.com/dropbox/dbxcli/blob/master/docs/json-schema/v1/README.md) +for schemas, command contracts, and examples. + ## Common workflows Upload a file: diff --git a/cmd/add-member.go b/cmd/add-member.go index 6bab5e7..6269bd1 100644 --- a/cmd/add-member.go +++ b/cmd/add-member.go @@ -37,7 +37,7 @@ func addMember(cmd *cobra.Command, args []string) (err error) { arg := team.NewMembersAddArg([]*team.MemberAddArg{member}) res, err := dbx.MembersAdd(arg) if err != nil { - return err + return withJSONErrorDetails(err, operationErrorDetails("team_add_member"), emailErrorDetails(email)) } input := teamMemberAddInput{ Email: email, diff --git a/cmd/cp.go b/cmd/cp.go index b29cbe9..2c62555 100644 --- a/cmd/cp.go +++ b/cmd/cp.go @@ -43,6 +43,7 @@ func cp(cmd *cobra.Command, args []string) error { } var cpErrors []error + var cpErrorDetails []map[string]any var relocationArgs []*files.RelocationArg var results []jsonOperationResult collectResults := commandOutputFormat(cmd) == output.FormatJSON @@ -56,10 +57,12 @@ func cp(cmd *cobra.Command, args []string) error { if err != nil { relocationError := fmt.Errorf("Error validating copy for %s to %s: %v", argument, dst, err) cpErrors = append(cpErrors, relocationError) + cpErrorDetails = append(cpErrorDetails, relocationFailureDetails(argument, dst)) } else { result, skipped, err := relocationSkipIfDestinationExists(dbx, arg, opts) if err != nil { cpErrors = append(cpErrors, fmt.Errorf("copy %q to %q: %v", arg.FromPath, arg.ToPath, err)) + cpErrorDetails = append(cpErrorDetails, relocationFailureDetails(arg.FromPath, arg.ToPath)) continue } if skipped { @@ -83,6 +86,7 @@ func cp(cmd *cobra.Command, args []string) error { } copyError := fmt.Errorf("copy %q to %q: %v", arg.FromPath, arg.ToPath, err) cpErrors = append(cpErrors, copyError) + cpErrorDetails = append(cpErrorDetails, relocationFailureDetails(arg.FromPath, arg.ToPath)) continue } if collectResults { @@ -90,6 +94,7 @@ func cp(cmd *cobra.Command, args []string) error { if err != nil { copyError := fmt.Errorf("copy %q to %q: %v", arg.FromPath, arg.ToPath, err) cpErrors = append(cpErrors, copyError) + cpErrorDetails = append(cpErrorDetails, relocationFailureDetails(arg.FromPath, arg.ToPath)) continue } results = append(results, relocationOperationResult(relocationJSONStatusCopied, result)) @@ -100,7 +105,7 @@ func cp(cmd *cobra.Command, args []string) error { for _, cpError := range cpErrors { _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "%v\n", cpError) } - return fmt.Errorf("cp: %d error(s)", len(cpErrors)) + return relocationAggregateError("cp", "copy", len(cpErrors), cpErrorDetails) } if !collectResults { diff --git a/cmd/cp_test.go b/cmd/cp_test.go index 2c43b4f..9056aff 100644 --- a/cmd/cp_test.go +++ b/cmd/cp_test.go @@ -288,6 +288,10 @@ func TestCpJSONErrorUsesCommandStderr(t *testing.T) { if !strings.Contains(stderr.String(), `copy "/src/file.txt" to "/dest/file.txt": path/malformed_path/`) { t.Fatalf("stderr = %q, want copy API error", stderr.String()) } + details := jsonErrorDetails(err) + if details["operation"] != "copy" || details["from_path"] != "/src/file.txt" || details["to_path"] != "/dest/file.txt" { + t.Fatalf("details = %#v, want copy from/to paths", details) + } } func TestCpCommandDefinesIfExistsFlag(t *testing.T) { diff --git a/cmd/get.go b/cmd/get.go index 72dc3e8..f0dd5b9 100644 --- a/cmd/get.go +++ b/cmd/get.go @@ -83,7 +83,7 @@ func get(cmd *cobra.Command, args []string) (err error) { if dst == "-" { if commandOutputFormat(cmd) == output.FormatJSON { - return invalidArgumentsErrorWithDetails("`get --output=json` cannot be used with stdout target `-`", mergeJSONErrorDetails(argumentErrorDetails("dst"), flagErrorDetails("output"))) + return invalidArgumentsErrorWithDetails("`get --output=json` cannot be used with stdout target `-`", mergeJSONErrorDetails(operationErrorDetails("download"), argumentErrorDetails("dst"), flagErrorDetails("output"))) } return getStdout(cmd, src, recursive) } @@ -93,7 +93,7 @@ func get(cmd *cobra.Command, args []string) (err error) { meta, err := dbx.GetMetadata(files.NewGetMetadataArg(src)) if err != nil { if recursive { - return fmt.Errorf("get metadata for %s: %v", src, err) + return withJSONErrorDetails(fmt.Errorf("get metadata for %s: %v", src, err), operationErrorDetails("download"), pathErrorDetails(src)) } // For non-recursive, fall through to download (will fail with proper error) if f, statErr := os.Stat(dst); statErr == nil && f.IsDir() { @@ -101,7 +101,7 @@ func get(cmd *cobra.Command, args []string) (err error) { } result, err := downloadFileWithResult(dbx, src, dst, opts) if err != nil { - return err + return withJSONErrorDetails(err, operationErrorDetails("download"), pathErrorDetails(src), relocationErrorDetails(src, dst)) } return renderGetResults(cmd, getCommandInput{ Source: src, @@ -113,17 +113,17 @@ func get(cmd *cobra.Command, args []string) (err error) { if _, ok := meta.(*files.FolderMetadata); ok { if !recursive { - return invalidArgumentsErrorfWithDetails("%s is a folder (use --recursive to download folders)", pathErrorDetails(src), src) + return invalidArgumentsErrorfWithDetails("%s is a folder (use --recursive to download folders)", mergeJSONErrorDetails(operationErrorDetails("download"), pathErrorDetails(src)), src) } if f, statErr := os.Stat(dst); statErr == nil && f.IsDir() { dst = filepath.Join(dst, path.Base(src)) } if commandOutputFormat(cmd) == output.FormatText { - return getRecursive(dbx, src, dst) + return withJSONErrorDetails(getRecursive(dbx, src, dst), operationErrorDetails("download"), pathErrorDetails(src), relocationErrorDetails(src, dst)) } results, err := getRecursiveWithResults(dbx, src, dst, meta, opts) if err != nil { - return err + return withJSONErrorDetails(err, operationErrorDetails("download"), pathErrorDetails(src), relocationErrorDetails(src, dst)) } return renderGetResults(cmd, getCommandInput{ Source: src, @@ -139,7 +139,7 @@ func get(cmd *cobra.Command, args []string) (err error) { result, err := downloadFileWithResult(dbx, src, dst, opts) if err != nil { - return err + return withJSONErrorDetails(err, operationErrorDetails("download"), pathErrorDetails(src), relocationErrorDetails(src, dst)) } return renderGetResults(cmd, getCommandInput{ Source: src, @@ -199,7 +199,7 @@ func getOperationResults(results []getResult) []jsonOperationResult { func getStdout(cmd *cobra.Command, src string, recursive bool) error { if recursive { - return invalidArgumentsErrorWithDetails("`get -` cannot be used with --recursive", flagErrorDetails("recursive")) + return invalidArgumentsErrorWithDetails("`get -` cannot be used with --recursive", mergeJSONErrorDetails(operationErrorDetails("download"), flagErrorDetails("recursive"))) } dbx := filesNewFunc(config) @@ -207,11 +207,11 @@ func getStdout(cmd *cobra.Command, src string, recursive bool) error { meta, err := dbx.GetMetadata(files.NewGetMetadataArg(src)) if err == nil { if _, ok := meta.(*files.FolderMetadata); ok { - return invalidArgumentsErrorfWithDetails("%s is a folder; cannot download folder to stdout", pathErrorDetails(src), src) + return invalidArgumentsErrorfWithDetails("%s is a folder; cannot download folder to stdout", mergeJSONErrorDetails(operationErrorDetails("download"), pathErrorDetails(src)), src) } } - return downloadToStdout(dbx, src, cmd.OutOrStdout()) + return withJSONErrorDetails(downloadToStdout(dbx, src, cmd.OutOrStdout()), operationErrorDetails("download"), pathErrorDetails(src)) } func getWithClient(dbx files.Client, args []string) (err error) { @@ -232,7 +232,7 @@ func getWithClient(dbx files.Client, args []string) (err error) { dst = filepath.Join(dst, path.Base(src)) } - return downloadFile(dbx, src, dst) + return withJSONErrorDetails(downloadFile(dbx, src, dst), operationErrorDetails("download"), pathErrorDetails(src), relocationErrorDetails(src, dst)) } func getRecursive(dbx files.Client, src, dst string) error { @@ -250,7 +250,7 @@ func getRecursiveInternal(dbx files.Client, src, dst string, rootMeta files.IsMe res, err := dbx.ListFolder(arg) if err != nil { - return nil, fmt.Errorf("list folder %s: %v", src, err) + return nil, withJSONErrorDetails(fmt.Errorf("list folder %s: %v", src, err), operationErrorDetails("download"), pathErrorDetails(src)) } var entries []files.IsMetadata @@ -259,7 +259,7 @@ func getRecursiveInternal(dbx files.Client, src, dst string, rootMeta files.IsMe cont := files.NewListFolderContinueArg(res.Cursor) res, err = dbx.ListFolderContinue(cont) if err != nil { - return nil, fmt.Errorf("list folder continue: %v", err) + return nil, withJSONErrorDetails(fmt.Errorf("list folder continue: %v", err), operationErrorDetails("download"), pathErrorDetails(src)) } entries = append(entries, res.Entries...) } @@ -335,7 +335,7 @@ func getRecursiveInternal(dbx files.Client, src, dst string, rootMeta files.IsMe for _, e := range downloadErrors { fmt.Fprintf(getErrorOutput(opts), "Error: %v\n", e) } - return nil, fmt.Errorf("get: %d error(s)", len(downloadErrors)) + return nil, commandFailedErrorfWithDetails("get: %d error(s)", mergeJSONErrorDetails(operationErrorDetails("download"), pathErrorDetails(src), relocationErrorDetails(src, dst)), len(downloadErrors)) } return results, nil diff --git a/cmd/get_test.go b/cmd/get_test.go index 84d3a3d..422726c 100644 --- a/cmd/get_test.go +++ b/cmd/get_test.go @@ -868,6 +868,10 @@ func TestGetJSONRecursiveErrorEmitsNoSuccessJSON(t *testing.T) { if err == nil { t.Fatal("expected recursive error") } + details := jsonErrorDetails(err) + if details["operation"] != "download" || details["path"] != "/remote" { + t.Fatalf("details = %#v, want download operation and source path", details) + } if stdout.Len() != 0 { t.Fatalf("stdout = %q, want empty on recursive error", stdout.String()) } diff --git a/cmd/json_contract_test.go b/cmd/json_contract_test.go index b4cf5c7..182c2a5 100644 --- a/cmd/json_contract_test.go +++ b/cmd/json_contract_test.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "errors" + "fmt" "os" "reflect" "sort" @@ -11,6 +12,10 @@ import ( "testing" "github.com/dropbox/dbxcli/v3/internal/output" + "github.com/dropbox/dropbox-sdk-go-unofficial/v6/dropbox" + dropboxauth "github.com/dropbox/dropbox-sdk-go-unofficial/v6/dropbox/auth" + "github.com/dropbox/dropbox-sdk-go-unofficial/v6/dropbox/files" + "github.com/dropbox/dropbox-sdk-go-unofficial/v6/dropbox/sharing" "github.com/spf13/cobra" ) @@ -145,6 +150,30 @@ func TestStructuredOutputGoldenSuccessOutputAudit(t *testing.T) { } } +func TestStructuredOutputGoldenErrorOutputAudit(t *testing.T) { + fixtures := loadJSONGoldenErrorOutputs(t) + examples := jsonGoldenErrorOutputExamples() + schema := compileJSONSchemaFile(t, "../docs/json-schema/v1/error.schema.json") + + for name, fixture := range fixtures { + if _, ok := examples[name]; !ok { + t.Errorf("golden error output includes unknown example %q", name) + } + if err := schema.Validate(decodeJSONValueForSchema(t, fixture)); err != nil { + t.Errorf("golden error output for %q does not validate: %v", name, err) + } + assertGoldenErrorOutputShape(t, name, fixture) + } + for name, example := range examples { + fixture, ok := fixtures[name] + if !ok { + t.Errorf("code-derived error output example %q has no golden output", name) + continue + } + assertGoldenJSONEqual(t, name, fixture, example) + } +} + func TestAccountAuthContractOmitsSensitiveFields(t *testing.T) { example := jsonGoldenSuccessOutputExamples()["account"] encoded, err := json.Marshal(example) @@ -536,6 +565,24 @@ func loadJSONGoldenSuccessOutputs(t *testing.T) map[string]json.RawMessage { return fixtures } +func loadJSONGoldenErrorOutputs(t *testing.T) map[string]json.RawMessage { + t.Helper() + + data, err := os.ReadFile("testdata/json_contract/error_outputs.json") + if err != nil { + t.Fatalf("read golden error output fixture: %v", err) + } + + var fixtures map[string]json.RawMessage + if err := json.Unmarshal(data, &fixtures); err != nil { + t.Fatalf("decode golden error output fixture: %v", err) + } + if len(fixtures) == 0 { + t.Fatalf("golden error output fixture has no examples") + } + return fixtures +} + func loadPublicJSONSchema(t *testing.T, file string) publicJSONSchema { t.Helper() @@ -620,6 +667,33 @@ func assertGoldenSuccessOutputStatuses(t *testing.T, command string, fixture jso } } +func assertGoldenErrorOutputShape(t *testing.T, name string, fixture json.RawMessage) { + t.Helper() + + var got jsonErrorResponse + if err := json.Unmarshal(fixture, &got); err != nil { + t.Fatalf("decode golden error output for %q: %v", name, err) + } + if got.OK { + t.Errorf("golden error output for %q has ok=true", name) + } + if got.SchemaVersion != jsonSchemaVersion { + t.Errorf("golden error output for %q schema_version = %q, want %q", name, got.SchemaVersion, jsonSchemaVersion) + } + if got.Command == "" { + t.Errorf("golden error output for %q has empty command", name) + } + if got.Error.Message == "" { + t.Errorf("golden error output for %q has empty error.message", name) + } + if got.Error.Code == "" { + t.Errorf("golden error output for %q has empty error.code", name) + } + if got.Warnings == nil { + t.Errorf("golden error output for %q has nil warnings; want [] when empty", name) + } +} + func jsonGoldenSuccessOutputExamples() map[string]jsonOperationOutput { file := sampleJSONFileMetadata("/Reports/old.pdf") copyFile := sampleJSONFileMetadata("/Reports/copy.pdf") @@ -740,6 +814,100 @@ func jsonGoldenSuccessOutputExamples() map[string]jsonOperationOutput { return examples } +func jsonGoldenErrorOutputExamples() map[string]jsonErrorResponse { + rateLimit := dropboxauth.NewRateLimitError(nil) + rateLimit.RetryAfter = 12 + + examples := map[string]jsonErrorResponse{ + "auth_required": newJSONErrorResponse(jsonErrorExampleCommand("account"), missingAccessTokenError(tokenPersonal)), + "as_member_api_error": newJSONErrorResponse( + jsonErrorExampleCommandWithAsMember("ls", "dbmid:member"), + fmt.Errorf("list folder: %w", files.ListFolderAPIError{APIError: dropbox.APIError{ErrorSummary: "path/not_found/."}}), + ), + "deprecated_warning": newJSONErrorResponse( + jsonDeprecatedErrorExampleCommand("share list link", shareListLinksDeprecatedMessage), + invalidArgumentsErrorWithDetails( + "`share-link list` accepts at most one `path` argument", + mergeJSONErrorDetails(operationErrorDetails("share_link_list"), argumentErrorDetails("path"), pathErrorDetails("/extra")), + ), + ), + "dropbox_api_error": newJSONErrorResponse( + jsonErrorExampleCommand("share-link create"), + withJSONErrorDetails( + fmt.Errorf("share: %w", sharing.CreateSharedLinkWithSettingsAPIError{APIError: dropbox.APIError{ErrorSummary: "shared_link_already_exists/."}}), + operationErrorDetails("share_link_create"), + pathErrorDetails("/Reports/old.pdf"), + ), + ), + "invalid_arguments": newJSONErrorResponse( + jsonErrorExampleCommand("put"), + invalidArgumentsErrorfWithDetails("invalid --if-exists %q (use overwrite, skip, or fail)", flagValueErrorDetails("if-exists", "replace"), "replace"), + ), + "not_found": newJSONErrorResponse( + jsonErrorExampleCommand("get"), + withJSONErrorDetails( + fmt.Errorf("download: %w", files.DownloadAPIError{APIError: dropbox.APIError{ErrorSummary: "path/not_found/."}}), + operationErrorDetails("download"), + pathErrorDetails("/missing.txt"), + ), + ), + "partial_transfer": newJSONErrorResponse( + jsonErrorExampleCommand("get"), + withJSONErrorDetails(partialStdoutError(12), operationErrorDetails("download"), pathErrorDetails("/big.bin")), + ), + "path_conflict": newJSONErrorResponse(jsonErrorExampleCommand("mkdir"), pathConflictErrorWithPath("/file", "path exists and is not a folder: %s", "/file")), + "rate_limited": newJSONErrorResponse( + jsonErrorExampleCommand("put"), + withJSONErrorDetails( + fmt.Errorf("upload: %w", dropboxauth.RateLimitAPIError{APIError: dropbox.APIError{ErrorSummary: "too_many_requests/."}, RateLimitError: rateLimit}), + operationErrorDetails("upload"), + pathErrorDetails("/upload.bin"), + ), + ), + "restore_revision": newJSONErrorResponse( + jsonErrorExampleCommand("restore"), + withJSONErrorDetails( + fmt.Errorf("restore: %w", files.RestoreAPIError{APIError: dropbox.APIError{ErrorSummary: "path/not_found/."}}), + restoreErrorDetails("/Reports/old.pdf", "015f"), + ), + ), + "structured_output_unsupported": newJSONErrorResponse(jsonErrorExampleCommand("login"), output.ErrStructuredOutputUnsupported), + "team_email": newJSONErrorResponse( + jsonErrorExampleCommand("team add-member"), + withJSONErrorDetails(errors.New("add failed"), operationErrorDetails("team_add_member"), emailErrorDetails("new@example.com")), + ), + } + return examples +} + +func jsonErrorExampleCommand(path string) *cobra.Command { + root := &cobra.Command{Use: "dbxcli"} + if path == "" || path == "dbxcli" { + return root + } + + parent := root + for _, part := range strings.Split(path, " ") { + child := &cobra.Command{Use: part} + parent.AddCommand(child) + parent = child + } + return parent +} + +func jsonErrorExampleCommandWithAsMember(path string, memberID string) *cobra.Command { + cmd := jsonErrorExampleCommand(path) + cmd.Flags().String("as-member", "", "") + _ = cmd.Flags().Set("as-member", memberID) + return cmd +} + +func jsonDeprecatedErrorExampleCommand(path string, deprecated string) *cobra.Command { + cmd := jsonErrorExampleCommand(path) + cmd.Deprecated = deprecated + return cmd +} + func sampleJSONAccount() jsonAccount { return jsonAccount{ Type: "full", diff --git a/cmd/json_output.go b/cmd/json_output.go index 211aff3..9fa78c3 100644 --- a/cmd/json_output.go +++ b/cmd/json_output.go @@ -58,7 +58,7 @@ func newJSONErrorResponse(cmd *cobra.Command, err error) jsonErrorResponse { Error: jsonError{ Message: err.Error(), Code: jsonErrorCode(err), - Details: jsonErrorDetails(err), + Details: jsonErrorDetailsForCommand(cmd, err), }, Warnings: jsonCommandWarnings(cmd), } diff --git a/cmd/mv.go b/cmd/mv.go index ca1aba6..07cc7dd 100644 --- a/cmd/mv.go +++ b/cmd/mv.go @@ -43,6 +43,7 @@ func mv(cmd *cobra.Command, args []string) error { } var mvErrors []error + var mvErrorDetails []map[string]any var relocationArgs []*files.RelocationArg var results []jsonOperationResult collectResults := commandOutputFormat(cmd) == output.FormatJSON @@ -55,10 +56,12 @@ func mv(cmd *cobra.Command, args []string) error { arg, err := makeRelocationArg(argument, dst) if err != nil { mvErrors = append(mvErrors, fmt.Errorf("Error validating move for %s to %s: %v", argument, dst, err)) + mvErrorDetails = append(mvErrorDetails, relocationFailureDetails(argument, dst)) } else { result, skipped, err := relocationSkipIfDestinationExists(dbx, arg, opts) if err != nil { mvErrors = append(mvErrors, fmt.Errorf("move %q to %q: %v", arg.FromPath, arg.ToPath, err)) + mvErrorDetails = append(mvErrorDetails, relocationFailureDetails(arg.FromPath, arg.ToPath)) continue } if skipped { @@ -82,6 +85,7 @@ func mv(cmd *cobra.Command, args []string) error { } moveError := fmt.Errorf("move %q to %q: %v", arg.FromPath, arg.ToPath, err) mvErrors = append(mvErrors, moveError) + mvErrorDetails = append(mvErrorDetails, relocationFailureDetails(arg.FromPath, arg.ToPath)) continue } if collectResults { @@ -89,6 +93,7 @@ func mv(cmd *cobra.Command, args []string) error { if err != nil { moveError := fmt.Errorf("move %q to %q: %v", arg.FromPath, arg.ToPath, err) mvErrors = append(mvErrors, moveError) + mvErrorDetails = append(mvErrorDetails, relocationFailureDetails(arg.FromPath, arg.ToPath)) continue } results = append(results, relocationOperationResult(relocationJSONStatusMoved, result)) @@ -99,7 +104,7 @@ func mv(cmd *cobra.Command, args []string) error { for _, mvError := range mvErrors { _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "%v\n", mvError) } - return fmt.Errorf("mv: %d error(s)", len(mvErrors)) + return relocationAggregateError("mv", "move", len(mvErrors), mvErrorDetails) } if !collectResults { diff --git a/cmd/mv_test.go b/cmd/mv_test.go index d215ccb..f43eb2a 100644 --- a/cmd/mv_test.go +++ b/cmd/mv_test.go @@ -167,6 +167,10 @@ func TestMvJSONErrorUsesCommandStderr(t *testing.T) { if !strings.Contains(stderr.String(), `move "/src/file.txt" to "/dest/file.txt": path/malformed_path/`) { t.Fatalf("stderr = %q, want move API error", stderr.String()) } + details := jsonErrorDetails(err) + if details["operation"] != "move" || details["from_path"] != "/src/file.txt" || details["to_path"] != "/dest/file.txt" { + t.Fatalf("details = %#v, want move from/to paths", details) + } } func TestMvCommandDefinesIfExistsFlag(t *testing.T) { diff --git a/cmd/output.go b/cmd/output.go index e408237..ce361ed 100644 --- a/cmd/output.go +++ b/cmd/output.go @@ -62,6 +62,11 @@ type codedError struct { details map[string]any } +type detailedError struct { + err error + details map[string]any +} + func (e codedError) Error() string { return e.err.Error() } @@ -78,6 +83,18 @@ func (e codedError) JSONErrorDetails() map[string]any { return cloneJSONErrorDetails(e.details) } +func (e detailedError) Error() string { + return e.err.Error() +} + +func (e detailedError) Unwrap() error { + return e.err +} + +func (e detailedError) JSONErrorDetails() map[string]any { + return cloneJSONErrorDetails(e.details) +} + func newCodedError(code string, err error, details ...map[string]any) error { if err == nil { return nil @@ -89,6 +106,20 @@ func newCodedError(code string, err error, details ...map[string]any) error { } } +func withJSONErrorDetails(err error, details ...map[string]any) error { + if err == nil { + return nil + } + return detailedError{ + err: err, + details: mergeJSONErrorDetails(details...), + } +} + +func commandFailedErrorfWithDetails(format string, details map[string]any, args ...any) error { + return newCodedError(jsonErrorCodeCommandFailed, fmt.Errorf(format, args...), details) +} + func invalidArgumentsErrorWithDetails(message string, details map[string]any) error { return newCodedError(jsonErrorCodeInvalidArguments, errors.New(message), details) } @@ -164,10 +195,37 @@ func flagValueErrorDetails(flag, value string) map[string]any { } } +func operationErrorDetails(operation string) map[string]any { + return map[string]any{"operation": operation} +} + func pathErrorDetails(path string) map[string]any { return map[string]any{"path": path} } +func revisionErrorDetails(revision string) map[string]any { + return map[string]any{"revision": revision} +} + +func emailErrorDetails(email string) map[string]any { + return map[string]any{"email": email} +} + +func memberIDErrorDetails(memberID string) map[string]any { + return map[string]any{"member_id": memberID} +} + +func relocationErrorDetails(fromPath, toPath string) map[string]any { + return map[string]any{ + "from_path": fromPath, + "to_path": toPath, + } +} + +func urlErrorDetails(url string) map[string]any { + return map[string]any{"url": url} +} + func commandOutput(cmd *cobra.Command) *output.Renderer { if cmd == nil { return output.New(nil, nil, output.FormatText) @@ -382,13 +440,11 @@ func jsonErrorCode(err error) string { func jsonErrorDetails(err error) map[string]any { details := make(map[string]any) - var detailed jsonDetailedError - if errors.As(err, &detailed) { - for key, value := range detailed.JSONErrorDetails() { - details[key] = value - } - } + collectJSONErrorDetails(err, details) + if retryAfterSeconds, ok := rateLimitRetryAfterSeconds(err); ok { + details["retry_after_seconds"] = retryAfterSeconds + } if summary, ok := dropboxAPIErrorSummary(err); ok { details["api_summary"] = summary } else if summary, ok := dropboxAPISummaryFromMessage(err.Error()); ok { @@ -404,6 +460,79 @@ func jsonErrorDetails(err error) map[string]any { return details } +func jsonErrorDetailsForCommand(cmd *cobra.Command, err error) map[string]any { + details := jsonErrorDetails(err) + if memberID := commandAsMemberID(cmd); memberID != "" && shouldAttachMemberIDErrorDetails(err) { + details = mergeJSONErrorDetails(memberIDErrorDetails(memberID), details) + } + return details +} + +func collectJSONErrorDetails(err error, details map[string]any) { + if err == nil { + return + } + if detailed, ok := err.(jsonDetailedError); ok { + for key, value := range detailed.JSONErrorDetails() { + details[key] = value + } + } + + if joined, ok := err.(interface{ Unwrap() []error }); ok { + for _, inner := range joined.Unwrap() { + collectJSONErrorDetails(inner, details) + } + } + if wrapped, ok := err.(interface{ Unwrap() error }); ok { + collectJSONErrorDetails(wrapped.Unwrap(), details) + } +} + +func rateLimitRetryAfterSeconds(err error) (uint64, bool) { + var rateLimitErr dropboxauth.RateLimitAPIError + if errors.As(err, &rateLimitErr) && rateLimitErr.RateLimitError != nil { + return rateLimitErr.RateLimitError.RetryAfter, true + } + + var rateLimitErrPtr *dropboxauth.RateLimitAPIError + if errors.As(err, &rateLimitErrPtr) && rateLimitErrPtr != nil && rateLimitErrPtr.RateLimitError != nil { + return rateLimitErrPtr.RateLimitError.RetryAfter, true + } + return 0, false +} + +func commandAsMemberID(cmd *cobra.Command) string { + if cmd == nil { + return "" + } + value, err := cmd.Flags().GetString("as-member") + if err == nil { + return value + } + value, err = cmd.InheritedFlags().GetString("as-member") + if err == nil { + return value + } + value, err = cmd.PersistentFlags().GetString("as-member") + if err == nil { + return value + } + return "" +} + +func shouldAttachMemberIDErrorDetails(err error) bool { + if err == nil { + return false + } + if _, ok := dropboxAPIErrorSummary(err); ok { + return true + } + if _, ok := dropboxAPISummaryFromMessage(err.Error()); ok { + return true + } + return false +} + func cloneJSONErrorDetails(details map[string]any) map[string]any { if len(details) == 0 { return nil @@ -456,7 +585,10 @@ func dropboxAPIJSONErrorCode(err error) string { } if summary, ok := dropboxAPIErrorSummary(err); ok { - return dropboxAPIMessageErrorCode(summary) + if code := dropboxAPIMessageErrorCode(summary); code != "" { + return code + } + return jsonErrorCodeDropboxAPIError } if summary, ok := dropboxAPISummaryFromMessage(err.Error()); ok { return dropboxAPIMessageErrorCode(summary) @@ -551,7 +683,7 @@ func dropboxAPISummaryFromMessage(message string) (string, bool) { } if idx := strings.LastIndex(trimmed, ": "); idx >= 0 { tail := strings.TrimSpace(trimmed[idx+2:]) - if isDropboxAPISummary(tail) { + if isDropboxAPISummary(tail) && dropboxAPIMessageErrorCode(tail) != "" { return tail, true } } diff --git a/cmd/output_test.go b/cmd/output_test.go index c6559b1..f5efeb8 100644 --- a/cmd/output_test.go +++ b/cmd/output_test.go @@ -13,6 +13,7 @@ import ( "github.com/dropbox/dropbox-sdk-go-unofficial/v6/dropbox" dropboxauth "github.com/dropbox/dropbox-sdk-go-unofficial/v6/dropbox/auth" "github.com/dropbox/dropbox-sdk-go-unofficial/v6/dropbox/files" + "github.com/dropbox/dropbox-sdk-go-unofficial/v6/dropbox/sharing" "github.com/spf13/cobra" ) @@ -414,6 +415,169 @@ func TestRenderCommandErrorIncludesDropboxAPISummaryDetails(t *testing.T) { } } +func TestRenderCommandErrorIncludesAsMemberForDropboxAPIErrors(t *testing.T) { + var stdout bytes.Buffer + var stderr bytes.Buffer + cmd := &cobra.Command{Use: "ls"} + cmd.SetOut(&stdout) + cmd.SetErr(&stderr) + cmd.Flags().String(outputFlag, "json", "") + cmd.Flags().String("as-member", "dbmid:member", "") + + err := fmt.Errorf("get metadata: %w", files.GetMetadataAPIError{APIError: dropbox.APIError{ErrorSummary: "path/not_found/"}}) + renderCommandError(cmd, err) + + if got := stderr.String(); got != "" { + t.Fatalf("stderr = %q, want empty", got) + } + got := decodeJSONErrorResponse(t, stdout.String()) + if got.Error.Details["member_id"] != "dbmid:member" || got.Error.Details["api_summary"] != "path/not_found/" { + t.Fatalf("details = %+v, want member_id and api_summary", got.Error.Details) + } +} + +func TestRenderCommandErrorPreservesExplicitMemberIDDetails(t *testing.T) { + var stdout bytes.Buffer + var stderr bytes.Buffer + cmd := &cobra.Command{Use: "ls"} + cmd.SetOut(&stdout) + cmd.SetErr(&stderr) + cmd.Flags().String(outputFlag, "json", "") + cmd.Flags().String("as-member", "dbmid:context", "") + + err := withJSONErrorDetails( + fmt.Errorf("get metadata: %w", files.GetMetadataAPIError{APIError: dropbox.APIError{ErrorSummary: "path/not_found/"}}), + memberIDErrorDetails("dbmid:explicit"), + ) + renderCommandError(cmd, err) + + if got := stderr.String(); got != "" { + t.Fatalf("stderr = %q, want empty", got) + } + got := decodeJSONErrorResponse(t, stdout.String()) + if got.Error.Details["member_id"] != "dbmid:explicit" { + t.Fatalf("details = %+v, want explicit member_id", got.Error.Details) + } +} + +func TestRenderCommandErrorDoesNotIncludeAsMemberForLocalValidation(t *testing.T) { + var stdout bytes.Buffer + var stderr bytes.Buffer + cmd := &cobra.Command{Use: "put"} + cmd.SetOut(&stdout) + cmd.SetErr(&stderr) + cmd.Flags().String(outputFlag, "json", "") + cmd.Flags().String("as-member", "dbmid:member", "") + + renderCommandError(cmd, invalidArgumentsErrorWithDetails("missing path", argumentErrorDetails("path"))) + + if got := stderr.String(); got != "" { + t.Fatalf("stderr = %q, want empty", got) + } + got := decodeJSONErrorResponse(t, stdout.String()) + if _, ok := got.Error.Details["member_id"]; ok { + t.Fatalf("details = %+v, did not expect member_id for local validation error", got.Error.Details) + } +} + +func TestJSONErrorDetailsIncludesSDKAPISummaries(t *testing.T) { + tests := []struct { + name string + err error + code string + summary string + }{ + { + name: "download not found", + err: fmt.Errorf("download: %w", files.DownloadAPIError{APIError: dropbox.APIError{ErrorSummary: "path/not_found/"}}), + code: jsonErrorCodeNotFound, + summary: "path/not_found/", + }, + { + name: "upload conflict pointer", + err: fmt.Errorf("upload: %w", &files.UploadAPIError{APIError: dropbox.APIError{ErrorSummary: "path/conflict/file/"}}), + code: jsonErrorCodePathConflict, + summary: "path/conflict/file/", + }, + { + name: "sharing create conflict", + err: fmt.Errorf("share: %w", sharing.CreateSharedLinkWithSettingsAPIError{APIError: dropbox.APIError{ErrorSummary: "shared_link_already_exists/."}}), + code: jsonErrorCodeDropboxAPIError, + summary: "shared_link_already_exists/.", + }, + { + name: "sharing modify permission denied", + err: fmt.Errorf("share update: %w", &sharing.ModifySharedLinkSettingsAPIError{APIError: dropbox.APIError{ErrorSummary: "settings_error/access_denied/"}}), + code: jsonErrorCodePermissionDenied, + summary: "settings_error/access_denied/", + }, + { + name: "auth token revoke", + err: fmt.Errorf("logout: %w", dropboxauth.TokenRevokeAPIError{APIError: dropbox.APIError{ErrorSummary: "invalid_access_token/."}}), + code: jsonErrorCodeAuthRequired, + summary: "invalid_access_token/.", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := jsonErrorCode(tt.err); got != tt.code { + t.Fatalf("jsonErrorCode = %q, want %q", got, tt.code) + } + details := jsonErrorDetails(tt.err) + if details["api_summary"] != tt.summary { + t.Fatalf("details = %+v, want api_summary %q", details, tt.summary) + } + if _, ok := details["api_endpoint"]; ok { + t.Fatalf("details = %+v, did not expect api_endpoint from SDK wrapper alone", details) + } + }) + } +} + +func TestRenderCommandErrorIncludesAdditionalContextDetails(t *testing.T) { + var stdout bytes.Buffer + var stderr bytes.Buffer + cmd := &cobra.Command{Use: "put"} + cmd.SetOut(&stdout) + cmd.SetErr(&stderr) + cmd.Flags().String(outputFlag, "json", "") + + rateLimit := dropboxauth.NewRateLimitError(nil) + rateLimit.RetryAfter = 12 + err := withJSONErrorDetails( + dropboxauth.RateLimitAPIError{RateLimitError: rateLimit}, + operationErrorDetails("upload"), + urlErrorDetails("https://example.com/link"), + ) + renderCommandError(cmd, err) + + if got := stderr.String(); got != "" { + t.Fatalf("stderr = %q, want empty", got) + } + got := decodeJSONErrorResponse(t, stdout.String()) + if got.Error.Code != jsonErrorCodeRateLimited { + t.Fatalf("code = %q, want %q", got.Error.Code, jsonErrorCodeRateLimited) + } + if got.Error.Details["operation"] != "upload" || + got.Error.Details["url"] != "https://example.com/link" || + got.Error.Details["retry_after_seconds"] != float64(12) { + t.Fatalf("details = %+v, want operation/url/retry_after_seconds", got.Error.Details) + } +} + +func TestJSONErrorDetailsIncludesJoinedErrorDetails(t *testing.T) { + err := errors.Join( + partialStdoutError(7), + invalidArgumentsErrorWithDetails("missing path", flagErrorDetails("path")), + ) + + details := jsonErrorDetails(err) + if details["bytes_written"] != int64(7) || details["flag"] != "path" { + t.Fatalf("details = %+v, want joined error details", details) + } +} + func TestRenderCommandErrorIncludesDropboxAPIEndpointDetails(t *testing.T) { var stdout bytes.Buffer var stderr bytes.Buffer @@ -440,6 +604,55 @@ func TestRenderCommandErrorIncludesDropboxAPIEndpointDetails(t *testing.T) { } } +func TestJSONErrorDetailsIncludesDropboxAPIEndpointForCommonMessages(t *testing.T) { + tests := []struct { + name string + message string + endpoint string + code string + }{ + { + name: "files upload", + message: `Error in call to API function "files/upload": path/conflict/file/.`, + endpoint: "files/upload", + code: jsonErrorCodePathConflict, + }, + { + name: "sharing create shared link", + message: `Error in call to API function "sharing/create_shared_link_with_settings": shared_link_already_exists/.`, + endpoint: "sharing/create_shared_link_with_settings", + code: jsonErrorCodeDropboxAPIError, + }, + { + name: "auth token revoke", + message: `Error in call to API function "auth/token/revoke": invalid_access_token/.`, + endpoint: "auth/token/revoke", + code: jsonErrorCodeAuthRequired, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := errors.New(tt.message) + if got := jsonErrorCode(err); got != tt.code { + t.Fatalf("jsonErrorCode = %q, want %q", got, tt.code) + } + details := jsonErrorDetails(err) + if details["api_endpoint"] != tt.endpoint || details["api_summary"] != tt.message { + t.Fatalf("details = %+v, want endpoint %q and summary %q", details, tt.endpoint, tt.message) + } + }) + } +} + +func TestJSONErrorDetailsDoesNotTreatLocalPathAsAPISummary(t *testing.T) { + err := pathConflictErrorWithPath("/file", "path exists and is not a folder: %s", "/file") + details := jsonErrorDetails(err) + if details["api_summary"] != nil { + t.Fatalf("details = %+v, did not expect api_summary for local path error", details) + } +} + func TestJSONErrorDetailsIncludesAuthRemediation(t *testing.T) { got := newJSONErrorResponse(&cobra.Command{Use: "account"}, missingAccessTokenError(tokenPersonal)) diff --git a/cmd/put.go b/cmd/put.go index 5bc42dc..ce41357 100644 --- a/cmd/put.go +++ b/cmd/put.go @@ -307,11 +307,11 @@ func put(cmd *cobra.Command, args []string) (err error) { srcInfo, err := os.Stat(src) if err != nil { - return + return withJSONErrorDetails(err, operationErrorDetails("upload"), pathErrorDetails(src)) } if srcInfo.IsDir() && !recursive { - return invalidArgumentsErrorfWithDetails("%s is a directory (use --recursive to upload directories)", pathErrorDetails(src), src) + return invalidArgumentsErrorfWithDetails("%s is a directory (use --recursive to upload directories)", mergeJSONErrorDetails(operationErrorDetails("upload"), pathErrorDetails(src)), src) } // Default `dst` to the base segment of the source path; use the second argument if provided. @@ -331,11 +331,11 @@ func put(cmd *cobra.Command, args []string) (err error) { if srcInfo.IsDir() { if commandOutputFormat(cmd) == output.FormatText { - return putRecursive(src, dst, opts) + return withJSONErrorDetails(putRecursive(src, dst, opts), operationErrorDetails("upload"), relocationErrorDetails(src, dst)) } results, warnings, err := putRecursiveWithResults(src, dst, opts) if err != nil { - return err + return withJSONErrorDetails(err, operationErrorDetails("upload"), relocationErrorDetails(src, dst)) } return renderPutResultsWithWarnings(cmd, putCommandInput{ Source: src, @@ -348,7 +348,7 @@ func put(cmd *cobra.Command, args []string) (err error) { result, err := putFileWithResult(src, dst, opts) if err != nil { - return err + return withJSONErrorDetails(err, operationErrorDetails("upload"), relocationErrorDetails(src, dst)) } return renderPutResults(cmd, putCommandInput{ Source: src, @@ -364,12 +364,12 @@ func putStdin(cmd *cobra.Command, args []string, opts putOptions, recursive bool return invalidArgumentsErrorWithDetails("`put -` requires an explicit target path", argumentErrorDetails("dst")) } if recursive { - return invalidArgumentsErrorWithDetails("`put -` cannot be used with --recursive", flagErrorDetails("recursive")) + return invalidArgumentsErrorWithDetails("`put -` cannot be used with --recursive", mergeJSONErrorDetails(operationErrorDetails("upload"), flagErrorDetails("recursive"))) } dst := args[1] if strings.HasSuffix(dst, "/") { - return invalidArgumentsErrorfWithDetails("cannot upload stdin to directory target %q; provide a full Dropbox file path", pathErrorDetails(dst), dst) + return invalidArgumentsErrorfWithDetails("cannot upload stdin to directory target %q; provide a full Dropbox file path", mergeJSONErrorDetails(operationErrorDetails("upload"), pathErrorDetails(dst)), dst) } dstPath, err := validatePath(dst) @@ -380,7 +380,7 @@ func putStdin(cmd *cobra.Command, args []string, opts putOptions, recursive bool dbx := filesNewFunc(config) action, existingMetadata, err := checkPutStdinDestination(dbx, dstPath, opts.ifExists) if err != nil { - return err + return withJSONErrorDetails(err, operationErrorDetails("upload"), relocationErrorDetails("-", dstPath)) } if action == putDestinationSkip { reportPutSkipped(opts, dstPath) @@ -399,7 +399,7 @@ func putStdin(cmd *cobra.Command, args []string, opts putOptions, recursive bool tmpPath, _, cleanup, err := spoolStdinToTemp(cmd.InOrStdin()) if err != nil { - return err + return withJSONErrorDetails(err, operationErrorDetails("upload"), relocationErrorDetails("-", dstPath)) } result, uploadErr := putFileWithResult(tmpPath, dstPath, opts) @@ -409,12 +409,12 @@ func putStdin(cmd *cobra.Command, args []string, opts putOptions, recursive bool if cleanupErr != nil { reportStdinCleanupFailure(opts, tmpPath, cleanupErr) } - return uploadErr + return withJSONErrorDetails(uploadErr, operationErrorDetails("upload"), relocationErrorDetails("-", dstPath)) } if cleanupErr != nil { reportStdinCleanupFailure(opts, tmpPath, cleanupErr) - return fmt.Errorf("failed to remove temp file %s after upload; sensitive stdin data may remain on disk: %w", tmpPath, cleanupErr) + return withJSONErrorDetails(fmt.Errorf("failed to remove temp file %s after upload; sensitive stdin data may remain on disk: %w", tmpPath, cleanupErr), operationErrorDetails("upload"), relocationErrorDetails("-", dstPath)) } result.Input.Source = "-" @@ -589,7 +589,7 @@ func checkPutStdinDestination(dbx files.Client, dst string, ifExists string) (pu return putDestinationUpload, nil, nil } if _, ok := meta.(*files.FolderMetadata); ok { - return putDestinationUpload, nil, invalidArgumentsErrorfWithDetails("cannot upload stdin to folder %q; provide a full Dropbox file path", pathErrorDetails(dst), dst) + return putDestinationUpload, nil, invalidArgumentsErrorfWithDetails("cannot upload stdin to folder %q; provide a full Dropbox file path", mergeJSONErrorDetails(operationErrorDetails("upload"), pathErrorDetails(dst)), dst) } return actionForExistingDestination(dst, ifExists, meta) } @@ -782,7 +782,7 @@ func putRecursiveInternal(src, dst string, opts putOptions, collectResults bool) return nil }) if err != nil { - return nil, nil, err + return nil, nil, withJSONErrorDetails(err, operationErrorDetails("upload"), pathErrorDetails(src)) } dbx := filesNewFunc(config) @@ -791,13 +791,13 @@ func putRecursiveInternal(src, dst string, opts putOptions, collectResults bool) if collectResults { result, mkdirErr := putDirectoryWithResult(dbx, src, dst) if mkdirErr != nil { - uploadErrors = append(uploadErrors, fmt.Errorf("mkdir %s: %w", dst, mkdirErr)) + uploadErrors = append(uploadErrors, withJSONErrorDetails(fmt.Errorf("mkdir %s: %w", dst, mkdirErr), operationErrorDetails("create_folder"), pathErrorDetails(dst))) } else { results = append(results, result) } } else { if mkdirErr := putDirectory(dbx, dst); mkdirErr != nil { - uploadErrors = append(uploadErrors, fmt.Errorf("mkdir %s: %w", dst, mkdirErr)) + uploadErrors = append(uploadErrors, withJSONErrorDetails(fmt.Errorf("mkdir %s: %w", dst, mkdirErr), operationErrorDetails("create_folder"), pathErrorDetails(dst))) } } @@ -838,11 +838,11 @@ func putRecursiveInternal(src, dst string, opts putOptions, collectResults bool) return nil }) if err != nil { - return nil, nil, err + return nil, nil, withJSONErrorDetails(err, operationErrorDetails("upload"), pathErrorDetails(src)) } if len(uploadErrors) > 0 { - return nil, nil, fmt.Errorf("failed to upload %d file(s): %v", len(uploadErrors), uploadErrors[0]) + return nil, nil, commandFailedErrorfWithDetails("failed to upload %d file(s): %v", mergeJSONErrorDetails(operationErrorDetails("upload"), relocationErrorDetails(src, dst)), len(uploadErrors), uploadErrors[0]) } return results, warnings, nil } diff --git a/cmd/put_test.go b/cmd/put_test.go index 6ee68ec..db53b58 100644 --- a/cmd/put_test.go +++ b/cmd/put_test.go @@ -869,6 +869,10 @@ func TestPutJSONIfExistsFailReturnsErrorWithoutStdout(t *testing.T) { if err == nil { t.Fatal("expected error") } + details := jsonErrorDetails(err) + if details["operation"] != "upload" || details["from_path"] != tmpFile || details["to_path"] != "/existing.txt" { + t.Fatalf("details = %#v, want upload operation and source/destination", details) + } if stdout.Len() != 0 { t.Fatalf("stdout = %q, want empty on error", stdout.String()) } diff --git a/cmd/relocation_if_exists.go b/cmd/relocation_if_exists.go index e0e8a57..6b2a85a 100644 --- a/cmd/relocation_if_exists.go +++ b/cmd/relocation_if_exists.go @@ -114,3 +114,15 @@ func isRelocationWriteConflict(err *files.WriteError) bool { (err.Conflict.Tag == files.WriteConflictErrorFile || err.Conflict.Tag == files.WriteConflictErrorFolder) } + +func relocationFailureDetails(fromPath, toPath string) map[string]any { + return relocationErrorDetails(fromPath, toPath) +} + +func relocationAggregateError(commandName, operation string, count int, failures []map[string]any) error { + details := operationErrorDetails(operation) + if len(failures) == 1 { + details = mergeJSONErrorDetails(details, failures[0]) + } + return commandFailedErrorfWithDetails("%s: %d error(s)", details, commandName, count) +} diff --git a/cmd/remove-member.go b/cmd/remove-member.go index a182044..f5f0fb1 100644 --- a/cmd/remove-member.go +++ b/cmd/remove-member.go @@ -35,7 +35,7 @@ func removeMember(cmd *cobra.Command, args []string) (err error) { arg := team.NewMembersRemoveArg(selector) res, err := dbx.MembersRemove(arg) if err != nil { - return err + return withJSONErrorDetails(err, operationErrorDetails("team_remove_member"), emailErrorDetails(email)) } input := teamMemberRemoveInput{Email: email} return commandOutput(cmd).Render(func(w io.Writer) error { diff --git a/cmd/restore.go b/cmd/restore.go index 6bdfd6c..54d4a73 100644 --- a/cmd/restore.go +++ b/cmd/restore.go @@ -57,13 +57,13 @@ func restore(cmd *cobra.Command, args []string) (err error) { dbx := filesNewFunc(config) metadata, err := dbx.Restore(arg) if err != nil { - return + return withJSONErrorDetails(err, restoreErrorDetails(path, rev)) } verbose, _ := cmd.Flags().GetBool("verbose") result, err := newRestoreResult(path, rev, metadata) if err != nil { - return err + return withJSONErrorDetails(err, restoreErrorDetails(path, rev)) } return commandOutput(cmd).Render(func(w io.Writer) error { @@ -74,6 +74,10 @@ func restore(cmd *cobra.Command, args []string) (err error) { }, newJSONCommandOperationOutput(cmd, result.Input, []jsonOperationResult{restoreOperationResult(result)}, nil)) } +func restoreErrorDetails(path, revision string) map[string]any { + return mergeJSONErrorDetails(operationErrorDetails("restore"), pathErrorDetails(path), revisionErrorDetails(revision)) +} + func newRestoreResult(path, revision string, metadata *files.FileMetadata) (restoreResult, error) { result, err := restoreMetadataFromDropbox(path, metadata) if err != nil { diff --git a/cmd/restore_test.go b/cmd/restore_test.go index 0b0222d..f29dea8 100644 --- a/cmd/restore_test.go +++ b/cmd/restore_test.go @@ -255,9 +255,14 @@ func TestRestoreJSONErrorWritesNoOutput(t *testing.T) { } stubFilesClient(t, mock) - if err := restore(cmd, []string{"/Reports/old.pdf", "target-rev"}); err == nil { + err := restore(cmd, []string{"/Reports/old.pdf", "target-rev"}) + if err == nil { t.Fatal("expected restore error") } + details := jsonErrorDetails(err) + if details["operation"] != "restore" || details["path"] != "/Reports/old.pdf" || details["revision"] != "target-rev" { + t.Fatalf("details = %#v, want restore operation, path, and revision", details) + } if got := stdout.String(); got != "" { t.Fatalf("stdout = %q, want empty output on error", got) } diff --git a/cmd/rm.go b/cmd/rm.go index 8a1eb66..68c1d9a 100644 --- a/cmd/rm.go +++ b/cmd/rm.go @@ -135,17 +135,17 @@ func validateRemoveTargets(dbx files.Client, args []string, opts removeOptions) pathMetaData, err := getFileMetadata(dbx, path) if err != nil { - return nil, err + return nil, withJSONErrorDetails(err, operationErrorDetails(removeOperation(opts)), pathErrorDetails(path)) } if _, ok := pathMetaData.(*files.FileMetadata); !ok && !opts.allowNonEmptyFolder() { folderArg := files.NewListFolderArg(path) res, err := dbx.ListFolder(folderArg) if err != nil { - return nil, err + return nil, withJSONErrorDetails(err, operationErrorDetails(removeOperation(opts)), pathErrorDetails(path)) } if len(res.Entries) != 0 { - return nil, invalidArgumentsErrorfWithDetails("rm: cannot remove ‘%s’: Directory not empty, use `--force`/`-f` or `--recursive`/`-r` to proceed", pathErrorDetails(path), path) + return nil, invalidArgumentsErrorfWithDetails("rm: cannot remove ‘%s’: Directory not empty, use `--force`/`-f` or `--recursive`/`-r` to proceed", mergeJSONErrorDetails(operationErrorDetails(removeOperation(opts)), pathErrorDetails(path)), path) } } targets = append(targets, removeTarget{path: path, metadata: pathMetaData}) @@ -163,12 +163,12 @@ func removeTargets(dbx files.Client, targets []removeTarget, opts removeOptions) if opts.permanent { if err := dbx.PermanentlyDelete(arg); err != nil { - return nil, err + return nil, withJSONErrorDetails(err, operationErrorDetails(removeOperation(opts)), pathErrorDetails(target.path)) } } else { res, err := dbx.DeleteV2(arg) if err != nil { - return nil, err + return nil, withJSONErrorDetails(err, operationErrorDetails(removeOperation(opts)), pathErrorDetails(target.path)) } if res != nil && res.Metadata != nil { metadata = res.Metadata @@ -177,7 +177,7 @@ func removeTargets(dbx files.Client, targets []removeTarget, opts removeOptions) result, err := newRemoveResult(target.path, metadata, opts) if err != nil { - return nil, err + return nil, withJSONErrorDetails(err, operationErrorDetails(removeOperation(opts)), pathErrorDetails(target.path)) } results = append(results, result) } @@ -236,6 +236,13 @@ func (o removeOptions) allowNonEmptyFolder() bool { return o.force || o.recursive } +func removeOperation(opts removeOptions) string { + if opts.permanent { + return "permanent_delete" + } + return "delete" +} + // rmCmd represents the rm command var rmCmd = &cobra.Command{ Use: "rm [flags] ", diff --git a/cmd/rm_test.go b/cmd/rm_test.go index 0b04f1c..754ba82 100644 --- a/cmd/rm_test.go +++ b/cmd/rm_test.go @@ -635,9 +635,14 @@ func TestRmJSONErrorWritesNoOutput(t *testing.T) { } stubFilesClient(t, mock) - if err := rm(cmd, []string{"/File.txt"}); err == nil { + err := rm(cmd, []string{"/File.txt"}) + if err == nil { t.Fatal("expected rm error") } + details := jsonErrorDetails(err) + if details["operation"] != "delete" || details["path"] != "/File.txt" { + t.Fatalf("details = %#v, want delete operation and path", details) + } if got := stdout.String(); got != "" { t.Fatalf("stdout = %q, want empty output on error", got) } diff --git a/cmd/root_test.go b/cmd/root_test.go index 2ae27c0..81dc186 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -131,6 +131,50 @@ func TestExecuteExitsWithMappedCodes(t *testing.T) { } } +func TestExecuteJSONErrorOutputsValidateAgainstSchema(t *testing.T) { + missingAuthFile := filepath.Join(t.TempDir(), "missing-auth.json") + + tests := []struct { + name string + args []string + env map[string]string + code string + }{ + { + name: "unknown command", + args: []string{"--output=json", "does-not-exist"}, + code: jsonErrorCodeUnknownCommand, + }, + { + name: "auth required", + args: []string{"ls", "--output=json", "/"}, + env: map[string]string{envAccessToken: "", envAuthFile: missingAuthFile}, + code: jsonErrorCodeAuthRequired, + }, + { + name: "structured output unsupported", + args: []string{"completion", "--output=json"}, + code: jsonErrorCodeStructuredOutputUnsupported, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + exitCode, stdout, stderr := executeExitTestSubprocess(t, tt.args, tt.env) + if exitCode == exitCodeSuccess { + t.Fatalf("exit code = 0, want non-zero\nstdout: %s\nstderr: %s", stdout, stderr) + } + if stderr != "" { + t.Fatalf("stderr = %q, want JSON error on stdout only", stderr) + } + got := decodeJSONErrorResponse(t, stdout) + if got.Error.Code != tt.code { + t.Fatalf("code = %q, want %q\nstdout: %s", got.Error.Code, tt.code, stdout) + } + }) + } +} + func executeExitTestSubprocess(t *testing.T, args []string, env map[string]string) (int, string, string) { t.Helper() diff --git a/cmd/search.go b/cmd/search.go index 392b65b..38dede9 100644 --- a/cmd/search.go +++ b/cmd/search.go @@ -53,14 +53,14 @@ func search(cmd *cobra.Command, args []string) (err error) { return invalidArgumentsErrorWithDetails("`search` requires a `query` argument", argumentErrorDetails("query")) } if len(args) > 2 { - return invalidArgumentsErrorWithDetails("`search` accepts at most one optional `path-scope` argument", argumentErrorDetails("path-scope")) + return invalidArgumentsErrorWithDetails("`search` accepts at most one optional `path-scope` argument", mergeJSONErrorDetails(argumentErrorDetails("path-scope"), pathErrorDetails(args[2]))) } var scope string if len(args) == 2 { scope = args[1] if !strings.HasPrefix(scope, "/") { - return invalidArgumentsErrorWithDetails("`search` `path-scope` must begin with \"/\"", argumentErrorDetails("path-scope")) + return invalidArgumentsErrorWithDetails("`search` `path-scope` must begin with \"/\"", mergeJSONErrorDetails(argumentErrorDetails("path-scope"), pathErrorDetails(scope))) } } diff --git a/cmd/search_test.go b/cmd/search_test.go index b070881..64b4450 100644 --- a/cmd/search_test.go +++ b/cmd/search_test.go @@ -24,6 +24,10 @@ func TestSearchPathScopeValidation(t *testing.T) { if err == nil { t.Error("expected error for path-scope without leading slash") } + details := jsonErrorDetails(err) + if details["argument"] != "path-scope" || details["path"] != "no-slash" { + t.Fatalf("details = %#v, want path-scope argument and path", details) + } } func TestSearchRejectsExtraArgs(t *testing.T) { @@ -31,6 +35,10 @@ func TestSearchRejectsExtraArgs(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "path-scope") { t.Fatalf("error = %v, want extra path-scope error", err) } + details := jsonErrorDetails(err) + if details["argument"] != "path-scope" || details["path"] != "extra" { + t.Fatalf("details = %#v, want extra path-scope argument path", details) + } } func TestSearchOrderByValidation(t *testing.T) { diff --git a/cmd/share-list-links.go b/cmd/share-list-links.go index dd65512..e023965 100644 --- a/cmd/share-list-links.go +++ b/cmd/share-list-links.go @@ -40,7 +40,7 @@ func shareLinkList(cmd *cobra.Command, args []string) error { func shareLinkListWithWarnings(cmd *cobra.Command, args []string, warnings []jsonWarning) error { if len(args) > 1 { - return invalidArgumentsErrorWithDetails("`share-link list` accepts at most one `path` argument", argumentErrorDetails("path")) + return invalidArgumentsErrorWithDetails("`share-link list` accepts at most one `path` argument", mergeJSONErrorDetails(operationErrorDetails("share_link_list"), argumentErrorDetails("path"), pathErrorDetails(args[1]))) } arg := sharing.NewListSharedLinksArg() @@ -56,7 +56,7 @@ func shareLinkListWithWarnings(cmd *cobra.Command, args []string, warnings []jso dbx := newSharedLinkClient(config) links, err := listSharedLinks(dbx, arg) if err != nil { - return err + return withJSONErrorDetails(err, shareLinkListErrorDetails(arg.Path)) } if arg.Path != "" { @@ -67,7 +67,7 @@ func shareLinkListWithWarnings(cmd *cobra.Command, args []string, warnings []jso entries, ok := shareLinkJSONMetadataListFromDropbox(links) if !ok { - return errors.New("found unknown shared link type") + return withJSONErrorDetails(errors.New("found unknown shared link type"), shareLinkListErrorDetails(arg.Path)) } return commandOutput(cmd).Render(func(w io.Writer) error { @@ -83,6 +83,14 @@ func shareLinkListWithWarnings(cmd *cobra.Command, args []string, warnings []jso )) } +func shareLinkListErrorDetails(path string) map[string]any { + details := operationErrorDetails("share_link_list") + if path == "" { + return details + } + return mergeJSONErrorDetails(details, pathErrorDetails(path)) +} + func listSharedLinks(dbx sharedLinkClient, arg *sharing.ListSharedLinksArg) ([]sharing.IsSharedLinkMetadata, error) { var links []sharing.IsSharedLinkMetadata for { diff --git a/cmd/share_create_link_test.go b/cmd/share_create_link_test.go index 4b79536..c9d540b 100644 --- a/cmd/share_create_link_test.go +++ b/cmd/share_create_link_test.go @@ -757,6 +757,10 @@ func TestSharedLinkCreateReturnsNonAlreadyExistsError(t *testing.T) { if !errors.Is(err, wantErr) { t.Fatalf("error = %v, want original error", err) } + details := jsonErrorDetails(err) + if details["operation"] != "share_link_create" || details["path"] != "/docs" { + t.Fatalf("details = %#v, want share_link_create operation and path", details) + } } func TestSharedLinkCreateExistingMetadataPrintsURLWithoutList(t *testing.T) { @@ -1298,6 +1302,17 @@ func TestShareLinkListListsAllLinks(t *testing.T) { } } +func TestShareLinkListRejectsExtraPathWithDetails(t *testing.T) { + err := shareLinkList(&cobra.Command{}, []string{"/docs/one.txt", "/docs/two.txt"}) + if err == nil || !strings.Contains(err.Error(), "at most one `path` argument") { + t.Fatalf("error = %v, want extra path error", err) + } + details := jsonErrorDetails(err) + if details["operation"] != "share_link_list" || details["argument"] != "path" || details["path"] != "/docs/two.txt" { + t.Fatalf("details = %#v, want share_link_list operation, path argument, and extra path", details) + } +} + func TestShareLinkListVerboseWritesStatusToStderr(t *testing.T) { stubSharedLinkClient(t, &mockSharedLinkClient{ listSharedLinksFn: func(arg *sharing.ListSharedLinksArg) (*sharing.ListSharedLinksResult, error) { diff --git a/cmd/share_link_create.go b/cmd/share_link_create.go index 8a8007c..7f3928f 100644 --- a/cmd/share_link_create.go +++ b/cmd/share_link_create.go @@ -57,7 +57,7 @@ func shareLinkCreate(cmd *cobra.Command, args []string) error { return err } if path == "" { - return invalidArgumentsErrorWithDetails("cannot create a shared link for Dropbox root", pathErrorDetails("/")) + return invalidArgumentsErrorWithDetails("cannot create a shared link for Dropbox root", mergeJSONErrorDetails(operationErrorDetails("share_link_create"), pathErrorDetails("/"))) } opts, err := parseShareLinkCreateOptions(cmd) @@ -71,18 +71,18 @@ func shareLinkCreate(cmd *cobra.Command, args []string) error { if err != nil { link, err = existingSharedLink(dbx, path, err) if err != nil { - return err + return withJSONErrorDetails(err, operationErrorDetails("share_link_create"), pathErrorDetails(path)) } link, err = applyExistingSharedLinkCreateOptions(dbx, link, opts) if err != nil { - return err + return withJSONErrorDetails(err, operationErrorDetails("share_link_create"), pathErrorDetails(path)) } usedExisting = true } url, ok := sharedLinkURL(link) if !ok { - return errors.New("shared link response did not include a URL") + return withJSONErrorDetails(errors.New("shared link response did not include a URL"), operationErrorDetails("share_link_create"), pathErrorDetails(path)) } out := commandOutput(cmd) @@ -94,7 +94,7 @@ func shareLinkCreate(cmd *cobra.Command, args []string) error { result, ok := shareLinkJSONMetadataFromDropbox(link) if !ok { - return errors.New("found unknown shared link type") + return withJSONErrorDetails(errors.New("found unknown shared link type"), operationErrorDetails("share_link_create"), pathErrorDetails(path)) } status := shareLinkJSONStatusCreated @@ -228,7 +228,7 @@ func applyExistingSharedLinkCreateOptions(dbx sharedLinkClient, link sharing.IsS if opts.disallowDownload { if err := dbx.ModifySharedLinkSettingsRaw(url, rawSharedLinkSettingsFromCreateOptions(opts), opts.removeExpiration); err != nil { - return nil, err + return nil, withJSONErrorDetails(err, operationErrorDetails("share_link_create"), urlErrorDetails(url)) } return link, nil } @@ -242,7 +242,7 @@ func applyExistingSharedLinkCreateOptions(dbx sharedLinkClient, link sharing.IsS updated, err := dbx.ModifySharedLinkSettings(arg) if err != nil { - return nil, err + return nil, withJSONErrorDetails(err, operationErrorDetails("share_link_create"), urlErrorDetails(url)) } link = updated } @@ -336,7 +336,7 @@ func findExistingSharedLink(dbx sharedLinkClient, requestedPath string) (sharing for { res, err := dbx.ListSharedLinks(arg) if err != nil { - return nil, err + return nil, withJSONErrorDetails(err, operationErrorDetails("share_link_create"), pathErrorDetails(requestedPath)) } for _, link := range res.Links { diff --git a/cmd/share_link_download.go b/cmd/share_link_download.go index b465c30..83bf34a 100644 --- a/cmd/share_link_download.go +++ b/cmd/share_link_download.go @@ -57,7 +57,7 @@ func shareLinkDownload(cmd *cobra.Command, args []string) error { url := args[0] if url == "" { - return invalidArgumentsErrorWithDetails("`share-link download` requires a non-empty URL", argumentErrorDetails("url")) + return invalidArgumentsErrorWithDetails("`share-link download` requires a non-empty URL", mergeJSONErrorDetails(argumentErrorDetails("url"), urlErrorDetails(url))) } target := "" @@ -77,45 +77,48 @@ func shareLinkDownload(cmd *cobra.Command, args []string) error { arg.LinkPassword = opts.password.password } if target == "-" && commandOutputFormat(cmd) == output.FormatJSON { - return invalidArgumentsErrorWithDetails("`share-link download -` cannot be used with --output=json", mergeJSONErrorDetails(argumentErrorDetails("target"), flagErrorDetails("output"))) + return invalidArgumentsErrorWithDetails("`share-link download -` cannot be used with --output=json", mergeJSONErrorDetails(operationErrorDetails("share_link_download"), argumentErrorDetails("target"), flagErrorDetails("output"), urlErrorDetails(url))) } dbx := newSharedLinkClient(config) if opts.path != "" { arg.Path = opts.path - return downloadSharedLinkPath(cmd, dbx, arg, target, opts) + if err := downloadSharedLinkPath(cmd, dbx, arg, target, opts); err != nil { + return withJSONErrorDetails(err, urlErrorDetails(url), operationErrorDetails("share_link_download")) + } + return nil } link, err := dbx.GetSharedLinkMetadata(arg) if err != nil { - return err + return withJSONErrorDetails(err, urlErrorDetails(url), operationErrorDetails("share_link_download")) } if folder, ok := link.(*sharing.FolderLinkMetadata); ok { if !opts.recursive { - return invalidArgumentsErrorWithDetails("shared link is a folder (use --recursive to download folders)", flagErrorDetails("recursive")) + return invalidArgumentsErrorWithDetails("shared link is a folder (use --recursive to download folders)", mergeJSONErrorDetails(operationErrorDetails("share_link_download"), flagErrorDetails("recursive"), urlErrorDetails(url))) } if target == "-" { - return invalidArgumentsErrorWithDetails("cannot download shared-link folder to stdout", argumentErrorDetails("target")) + return invalidArgumentsErrorWithDetails("cannot download shared-link folder to stdout", mergeJSONErrorDetails(operationErrorDetails("share_link_download"), argumentErrorDetails("target"), urlErrorDetails(url))) } dst, err := sharedLinkFolderDownloadTarget(target, folder) if err != nil { - return err + return withJSONErrorDetails(err, urlErrorDetails(url), operationErrorDetails("share_link_download")) } if err := downloadSharedLinkFolder(filesNewFunc(config), dbx, arg, folder.Name, dst, cmd.ErrOrStderr()); err != nil { - return err + return withJSONErrorDetails(err, urlErrorDetails(url), operationErrorDetails("share_link_download")) } commandVerboseStatus(cmd, "Downloaded shared link folder to %s", dst) - return renderShareLinkDownloadOutput(cmd, newShareLinkDownloadInput(url, target, opts), dst, folder) + return withJSONErrorDetails(renderShareLinkDownloadOutput(cmd, newShareLinkDownloadInput(url, target, opts), dst, folder), urlErrorDetails(url), operationErrorDetails("share_link_download")) } if target == "-" { if opts.recursive { - return invalidArgumentsErrorWithDetails("`share-link download -` cannot be used with --recursive", flagErrorDetails("recursive")) + return invalidArgumentsErrorWithDetails("`share-link download -` cannot be used with --recursive", mergeJSONErrorDetails(operationErrorDetails("share_link_download"), flagErrorDetails("recursive"), urlErrorDetails(url))) } if err := downloadSharedLinkToStdout(dbx, arg, cmd.OutOrStdout()); err != nil { - return err + return withJSONErrorDetails(err, urlErrorDetails(url), operationErrorDetails("share_link_download")) } commandVerboseStatus(cmd, "Downloaded shared link to stdout") return nil @@ -123,10 +126,10 @@ func shareLinkDownload(cmd *cobra.Command, args []string) error { dst, downloaded, err := downloadSharedLinkToFile(dbx, arg, target, cmd.ErrOrStderr()) if err != nil { - return err + return withJSONErrorDetails(err, urlErrorDetails(url), operationErrorDetails("share_link_download")) } commandVerboseStatus(cmd, "Downloaded shared link to %s", dst) - return renderShareLinkDownloadOutput(cmd, newShareLinkDownloadInput(url, target, opts), dst, downloaded) + return withJSONErrorDetails(renderShareLinkDownloadOutput(cmd, newShareLinkDownloadInput(url, target, opts), dst, downloaded), urlErrorDetails(url), operationErrorDetails("share_link_download")) } func parseShareLinkDownloadOptions(cmd *cobra.Command) (shareLinkDownloadOptions, error) { @@ -157,13 +160,13 @@ func parseShareLinkDownloadOptions(cmd *cobra.Command) (shareLinkDownloadOptions return opts, err } if dropboxPath == "" { - return opts, invalidArgumentsErrorWithDetails("cannot download shared-link root with `--path`", pathErrorDetails("/")) + return opts, invalidArgumentsErrorWithDetails("cannot download shared-link root with `--path`", mergeJSONErrorDetails(operationErrorDetails("share_link_download"), pathErrorDetails("/"))) } opts.path = dropboxPath } if opts.path != "" && opts.recursive { - return opts, invalidArgumentsErrorWithDetails("`--path` cannot be used with --recursive", flagsErrorDetails("path", "recursive")) + return opts, invalidArgumentsErrorWithDetails("`--path` cannot be used with --recursive", mergeJSONErrorDetails(operationErrorDetails("share_link_download"), flagsErrorDetails("path", "recursive"), pathErrorDetails(opts.path))) } return opts, nil diff --git a/cmd/share_link_download_test.go b/cmd/share_link_download_test.go index 47f7804..e120692 100644 --- a/cmd/share_link_download_test.go +++ b/cmd/share_link_download_test.go @@ -363,10 +363,11 @@ func TestShareLinkDownloadPathRejectsInvalidCombinations(t *testing.T) { path string recursive bool want string + wantPath string }{ {name: "empty path", path: "", want: "`--path` requires a non-empty path"}, - {name: "root path", path: "/", want: "cannot download shared-link root with `--path`"}, - {name: "recursive path", path: "/sub/nested.txt", recursive: true, want: "`--path` cannot be used with --recursive"}, + {name: "root path", path: "/", want: "cannot download shared-link root with `--path`", wantPath: "/"}, + {name: "recursive path", path: "/sub/nested.txt", recursive: true, want: "`--path` cannot be used with --recursive", wantPath: "/sub/nested.txt"}, } for _, tt := range tests { @@ -397,6 +398,12 @@ func TestShareLinkDownloadPathRejectsInvalidCombinations(t *testing.T) { if err == nil || !strings.Contains(err.Error(), tt.want) { t.Fatalf("error = %v, want %q", err, tt.want) } + if tt.wantPath != "" { + details := jsonErrorDetails(err) + if details["path"] != tt.wantPath { + t.Fatalf("details = %#v, want path %q", details, tt.wantPath) + } + } if called { t.Fatal("shared link API should not be called") } @@ -447,6 +454,10 @@ func TestShareLinkDownloadFolderRejectsStdoutTarget(t *testing.T) { if got, want := jsonErrorCode(err), jsonErrorCodeInvalidArguments; got != want { t.Fatalf("jsonErrorCode = %q, want %q", got, want) } + details := jsonErrorDetails(err) + if details["operation"] != "share_link_download" || details["url"] != "https://example.com/folder" { + t.Fatalf("details = %#v, want share_link_download operation and URL", details) + } } func TestShareLinkDownloadFolderRecursiveDownloadsNestedFiles(t *testing.T) { diff --git a/cmd/share_link_info.go b/cmd/share_link_info.go index 06d2615..0c09a00 100644 --- a/cmd/share_link_info.go +++ b/cmd/share_link_info.go @@ -44,7 +44,7 @@ func shareLinkInfo(cmd *cobra.Command, args []string) error { url := args[0] if url == "" { - return invalidArgumentsErrorWithDetails("`share-link info` requires a non-empty URL", argumentErrorDetails("url")) + return invalidArgumentsErrorWithDetails("`share-link info` requires a non-empty URL", mergeJSONErrorDetails(argumentErrorDetails("url"), urlErrorDetails(url))) } opts, err := parseShareLinkInfoOptions(cmd) @@ -63,12 +63,12 @@ func shareLinkInfo(cmd *cobra.Command, args []string) error { link, err := dbx.GetSharedLinkMetadata(arg) if err != nil { - return err + return withJSONErrorDetails(err, urlErrorDetails(url), operationErrorDetails("share_link_info")) } result, ok := shareLinkJSONMetadataFromDropbox(link) if !ok { - return errors.New("found unknown shared link type") + return withJSONErrorDetails(errors.New("found unknown shared link type"), operationErrorDetails("share_link_info"), urlErrorDetails(url)) } return commandOutput(cmd).Render(func(w io.Writer) error { diff --git a/cmd/share_link_info_test.go b/cmd/share_link_info_test.go index 8362dc4..1eaf8d0 100644 --- a/cmd/share_link_info_test.go +++ b/cmd/share_link_info_test.go @@ -217,6 +217,10 @@ func TestShareLinkInfoReturnsAPIError(t *testing.T) { if !errors.Is(err, wantErr) { t.Fatalf("error = %v, want API error", err) } + details := jsonErrorDetails(err) + if details["operation"] != "share_link_info" || details["url"] != "https://www.dropbox.com/s/abc123" { + t.Fatalf("details = %#v, want share_link_info URL context", details) + } } func TestShareLinkInfoDoesNotBreakOtherCommands(t *testing.T) { diff --git a/cmd/share_link_revoke.go b/cmd/share_link_revoke.go index 3f4de48..1cecd26 100644 --- a/cmd/share_link_revoke.go +++ b/cmd/share_link_revoke.go @@ -56,13 +56,13 @@ func shareLinkRevoke(cmd *cobra.Command, args []string) error { url := args[0] if url == "" { - return invalidArgumentsErrorWithDetails("`share-link revoke` requires a non-empty URL", argumentErrorDetails("url")) + return invalidArgumentsErrorWithDetails("`share-link revoke` requires a non-empty URL", mergeJSONErrorDetails(argumentErrorDetails("url"), urlErrorDetails(url))) } dbx := newSharedLinkClient(config) arg := sharing.NewRevokeSharedLinkArg(url) if err := dbx.RevokeSharedLink(arg); err != nil { - return err + return withJSONErrorDetails(err, urlErrorDetails(url), operationErrorDetails("share_link_revoke")) } commandVerboseStatus(cmd, "Revoked shared link %s", url) @@ -91,14 +91,17 @@ func parseShareLinkRevokeOptions(cmd *cobra.Command, args []string) (shareLinkRe if !localFlagChanged(cmd, "path") { return opts, nil } - if len(args) != 0 { - return opts, invalidArgumentsErrorWithDetails("`--path` cannot be used with a shared link URL", mergeJSONErrorDetails(flagErrorDetails("path"), argumentErrorDetails("url"))) - } - pathArg, err := localStringFlag(cmd, "path") if err != nil { return opts, err } + if len(args) != 0 { + details := mergeJSONErrorDetails(operationErrorDetails("share_link_revoke"), flagErrorDetails("path"), argumentErrorDetails("url")) + if pathArg != "" { + details = mergeJSONErrorDetails(details, pathErrorDetails(pathArg)) + } + return opts, invalidArgumentsErrorWithDetails("`--path` cannot be used with a shared link URL", details) + } if pathArg == "" { return opts, invalidArgumentsErrorWithDetails("`--path` requires a non-empty path", flagErrorDetails("path")) } @@ -108,7 +111,7 @@ func parseShareLinkRevokeOptions(cmd *cobra.Command, args []string) (shareLinkRe return opts, err } if dropboxPath == "" { - return opts, invalidArgumentsErrorWithDetails("cannot revoke shared links for Dropbox root", pathErrorDetails("/")) + return opts, invalidArgumentsErrorWithDetails("cannot revoke shared links for Dropbox root", mergeJSONErrorDetails(operationErrorDetails("share_link_revoke"), pathErrorDetails("/"))) } opts.path = dropboxPath @@ -123,24 +126,24 @@ func revokeSharedLinksForPath(cmd *cobra.Command, path string) ([]shareLinkRevok dbx := newSharedLinkClient(config) links, err := listSharedLinks(dbx, arg) if err != nil { - return nil, err + return nil, withJSONErrorDetails(err, operationErrorDetails("share_link_revoke"), pathErrorDetails(path)) } if len(links) == 0 { - return nil, fmt.Errorf("no direct shared links found for %q", path) + return nil, withJSONErrorDetails(fmt.Errorf("no direct shared links found for %q", path), operationErrorDetails("share_link_revoke"), pathErrorDetails(path)) } revoked := make([]shareLinkRevokeResult, 0, len(links)) for _, link := range links { url, ok := sharedLinkURL(link) if !ok { - return nil, errors.New("shared link response did not include a URL") + return nil, withJSONErrorDetails(errors.New("shared link response did not include a URL"), operationErrorDetails("share_link_revoke"), pathErrorDetails(path)) } metadata, ok := shareLinkJSONMetadataFromDropbox(link) if !ok { - return nil, errors.New("found unknown shared link type") + return nil, withJSONErrorDetails(errors.New("found unknown shared link type"), operationErrorDetails("share_link_revoke"), pathErrorDetails(path)) } if err := dbx.RevokeSharedLink(sharing.NewRevokeSharedLinkArg(url)); err != nil { - return nil, fmt.Errorf("revoke shared link %s: %w", url, err) + return nil, withJSONErrorDetails(fmt.Errorf("revoke shared link %s: %w", url, err), urlErrorDetails(url), operationErrorDetails("share_link_revoke")) } revoked = append(revoked, shareLinkRevokeResult{ URL: url, diff --git a/cmd/share_link_revoke_test.go b/cmd/share_link_revoke_test.go index a7a4e75..fec2080 100644 --- a/cmd/share_link_revoke_test.go +++ b/cmd/share_link_revoke_test.go @@ -146,6 +146,10 @@ func TestShareLinkRevokePathRequiresNoURL(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "`--path` cannot be used with a shared link URL") { t.Fatalf("error = %v, want path and URL conflict error", err) } + details := jsonErrorDetails(err) + if details["flag"] != "path" || details["argument"] != "url" || details["path"] != "/docs/file.txt" { + t.Fatalf("details = %#v, want flag, url argument, and path", details) + } if called { t.Fatal("shared link API should not be called") } @@ -192,6 +196,10 @@ func TestShareLinkRevokePathRejectsRoot(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "cannot revoke shared links for Dropbox root") { t.Fatalf("error = %v, want root path error", err) } + details := jsonErrorDetails(err) + if details["operation"] != "share_link_revoke" || details["path"] != "/" { + t.Fatalf("details = %#v, want share_link_revoke operation and root path", details) + } if called { t.Fatal("ListSharedLinks should not be called") } @@ -300,6 +308,10 @@ func TestShareLinkRevokePathReturnsErrorWhenNoLinksFound(t *testing.T) { if err == nil || !strings.Contains(err.Error(), `no direct shared links found for "/docs/file.txt"`) { t.Fatalf("error = %v, want no links found error", err) } + details := jsonErrorDetails(err) + if details["operation"] != "share_link_revoke" || details["path"] != "/docs/file.txt" { + t.Fatalf("details = %#v, want share_link_revoke operation and path", details) + } if calledRevoke { t.Fatal("RevokeSharedLink should not be called") } @@ -322,6 +334,10 @@ func TestShareLinkRevokePathReturnsListError(t *testing.T) { if !errors.Is(err, wantErr) { t.Fatalf("error = %v, want list error", err) } + details := jsonErrorDetails(err) + if details["operation"] != "share_link_revoke" || details["path"] != "/docs/file.txt" { + t.Fatalf("details = %#v, want share_link_revoke operation and path", details) + } } func TestShareLinkRevokePathReturnsRevokeError(t *testing.T) { @@ -352,6 +368,10 @@ func TestShareLinkRevokePathReturnsRevokeError(t *testing.T) { if !strings.Contains(err.Error(), "revoke shared link https://example.com/one") { t.Fatalf("error = %v, want failing URL context", err) } + details := jsonErrorDetails(err) + if details["operation"] != "share_link_revoke" || details["url"] != "https://example.com/one" { + t.Fatalf("details = %#v, want share_link_revoke URL context", details) + } } func TestShareLinkRevokePathVerboseWritesStatusToStderr(t *testing.T) { diff --git a/cmd/share_link_update.go b/cmd/share_link_update.go index 5b29305..87fa7cf 100644 --- a/cmd/share_link_update.go +++ b/cmd/share_link_update.go @@ -51,7 +51,7 @@ func shareLinkUpdate(cmd *cobra.Command, args []string) error { url := args[0] if url == "" { - return invalidArgumentsErrorWithDetails("`share-link update` requires a non-empty URL", argumentErrorDetails("url")) + return invalidArgumentsErrorWithDetails("`share-link update` requires a non-empty URL", mergeJSONErrorDetails(argumentErrorDetails("url"), urlErrorDetails(url))) } opts, err := parseShareLinkUpdateOptions(cmd) @@ -62,7 +62,7 @@ func shareLinkUpdate(cmd *cobra.Command, args []string) error { dbx := newSharedLinkClient(config) if opts.usesRawSettings() { if err := dbx.ModifySharedLinkSettingsRaw(url, rawSharedLinkSettingsFromUpdateOptions(opts), opts.removeExpiration); err != nil { - return err + return withJSONErrorDetails(err, urlErrorDetails(url), operationErrorDetails("share_link_update")) } return renderShareLinkUpdateOutput(cmd, dbx, url, opts, nil) } @@ -88,7 +88,7 @@ func shareLinkUpdate(cmd *cobra.Command, args []string) error { if opts.hasSDKSettings() { link, err := dbx.ModifySharedLinkSettings(arg) if err != nil { - return err + return withJSONErrorDetails(err, urlErrorDetails(url), operationErrorDetails("share_link_update")) } return renderShareLinkUpdateOutput(cmd, dbx, url, opts, link) } @@ -111,13 +111,13 @@ func renderShareLinkUpdateOutput(cmd *cobra.Command, dbx sharedLinkClient, url s var err error link, err = dbx.GetSharedLinkMetadata(arg) if err != nil { - return err + return withJSONErrorDetails(err, urlErrorDetails(url), operationErrorDetails("share_link_update")) } } result, ok := shareLinkJSONMetadataFromDropbox(link) if !ok { - return errors.New("found unknown shared link type") + return withJSONErrorDetails(errors.New("found unknown shared link type"), operationErrorDetails("share_link_update"), urlErrorDetails(url)) } return renderJSONOperationOutput( diff --git a/cmd/team_json_test.go b/cmd/team_json_test.go index cf1658f..965ea7c 100644 --- a/cmd/team_json_test.go +++ b/cmd/team_json_test.go @@ -286,6 +286,48 @@ func TestTeamRemoveMemberJSONOutputsMutationResult(t *testing.T) { } } +func TestTeamAddMemberErrorIncludesEmailDetails(t *testing.T) { + stubTeamClient(t, &mockTeamClient{ + membersAddFn: func(arg *team.MembersAddArg) (*team.MembersAddLaunch, error) { + return nil, errors.New("add failed") + }, + }) + + cmd, stdout := teamTestCmd(t, true) + err := addMember(cmd, []string{"new@example.com", "New", "User"}) + if err == nil { + t.Fatal("expected addMember error") + } + details := jsonErrorDetails(err) + if details["operation"] != "team_add_member" || details["email"] != "new@example.com" { + t.Fatalf("details = %#v, want team_add_member operation and email", details) + } + if got := stdout.String(); got != "" { + t.Fatalf("stdout = %q, want no success output", got) + } +} + +func TestTeamRemoveMemberErrorIncludesEmailDetails(t *testing.T) { + stubTeamClient(t, &mockTeamClient{ + membersRemoveFn: func(arg *team.MembersRemoveArg) (*async.LaunchEmptyResult, error) { + return nil, errors.New("remove failed") + }, + }) + + cmd, stdout := teamTestCmd(t, true) + err := removeMember(cmd, []string{"old@example.com"}) + if err == nil { + t.Fatal("expected removeMember error") + } + details := jsonErrorDetails(err) + if details["operation"] != "team_remove_member" || details["email"] != "old@example.com" { + t.Fatalf("details = %#v, want team_remove_member operation and email", details) + } + if got := stdout.String(); got != "" { + t.Fatalf("stdout = %q, want no success output", got) + } +} + func TestTeamJSONErrorWritesNoSuccessOutput(t *testing.T) { stubTeamClient(t, &mockTeamClient{ getInfoFn: func() (*team.TeamGetInfoResult, error) { diff --git a/cmd/testdata/json_contract/error_outputs.json b/cmd/testdata/json_contract/error_outputs.json new file mode 100644 index 0000000..a8c087b --- /dev/null +++ b/cmd/testdata/json_contract/error_outputs.json @@ -0,0 +1,179 @@ +{ + "auth_required": { + "ok": false, + "schema_version": "1", + "command": "account", + "error": { + "message": "no saved Dropbox credentials; run \"dbxcli login\" first or set DBXCLI_ACCESS_TOKEN", + "code": "auth_required", + "details": { + "token_type": "personal", + "login_command": "dbxcli login", + "env_var": "DBXCLI_ACCESS_TOKEN" + } + }, + "warnings": [] + }, + "as_member_api_error": { + "ok": false, + "schema_version": "1", + "command": "ls", + "error": { + "message": "list folder: path/not_found/.", + "code": "not_found", + "details": { + "api_summary": "path/not_found/.", + "member_id": "dbmid:member" + } + }, + "warnings": [] + }, + "deprecated_warning": { + "ok": false, + "schema_version": "1", + "command": "share list link", + "error": { + "message": "`share-link list` accepts at most one `path` argument", + "code": "invalid_arguments", + "details": { + "operation": "share_link_list", + "argument": "path", + "path": "/extra" + } + }, + "warnings": [ + { + "code": "deprecated_command", + "message": "use `dbxcli share-link list` instead" + } + ] + }, + "dropbox_api_error": { + "ok": false, + "schema_version": "1", + "command": "share-link create", + "error": { + "message": "share: shared_link_already_exists/.", + "code": "dropbox_api_error", + "details": { + "operation": "share_link_create", + "path": "/Reports/old.pdf", + "api_summary": "shared_link_already_exists/." + } + }, + "warnings": [] + }, + "invalid_arguments": { + "ok": false, + "schema_version": "1", + "command": "put", + "error": { + "message": "invalid --if-exists \"replace\" (use overwrite, skip, or fail)", + "code": "invalid_arguments", + "details": { + "flag": "if-exists", + "value": "replace" + } + }, + "warnings": [] + }, + "not_found": { + "ok": false, + "schema_version": "1", + "command": "get", + "error": { + "message": "download: path/not_found/.", + "code": "not_found", + "details": { + "operation": "download", + "path": "/missing.txt", + "api_summary": "path/not_found/." + } + }, + "warnings": [] + }, + "partial_transfer": { + "ok": false, + "schema_version": "1", + "command": "get", + "error": { + "message": "download failed after writing 12 bytes to stdout; cannot retry partial output", + "code": "partial_transfer", + "details": { + "operation": "download", + "path": "/big.bin", + "bytes_written": 12 + } + }, + "warnings": [] + }, + "path_conflict": { + "ok": false, + "schema_version": "1", + "command": "mkdir", + "error": { + "message": "path exists and is not a folder: /file", + "code": "path_conflict", + "details": { + "path": "/file" + } + }, + "warnings": [] + }, + "rate_limited": { + "ok": false, + "schema_version": "1", + "command": "put", + "error": { + "message": "upload: too_many_requests/.", + "code": "rate_limited", + "details": { + "operation": "upload", + "path": "/upload.bin", + "api_summary": "too_many_requests/.", + "retry_after_seconds": 12 + } + }, + "warnings": [] + }, + "restore_revision": { + "ok": false, + "schema_version": "1", + "command": "restore", + "error": { + "message": "restore: path/not_found/.", + "code": "not_found", + "details": { + "operation": "restore", + "path": "/Reports/old.pdf", + "revision": "015f", + "api_summary": "path/not_found/." + } + }, + "warnings": [] + }, + "structured_output_unsupported": { + "ok": false, + "schema_version": "1", + "command": "login", + "error": { + "message": "structured output is not supported for this command yet", + "code": "structured_output_unsupported" + }, + "warnings": [] + }, + "team_email": { + "ok": false, + "schema_version": "1", + "command": "team add-member", + "error": { + "message": "add failed", + "code": "command_failed", + "details": { + "operation": "team_add_member", + "email": "new@example.com" + } + }, + "warnings": [] + } +} diff --git a/docs/json-schema/v1/README.md b/docs/json-schema/v1/README.md index c97d3f6..f05834a 100644 --- a/docs/json-schema/v1/README.md +++ b/docs/json-schema/v1/README.md @@ -42,10 +42,42 @@ Error responses always include: - `error.code`: stable machine-readable error code - `error.details`: optional machine-readable context, included only when dbxcli has reliable structured details such as `argument`, `arguments`, - `flag`, `flags`, `value`, `path`, `token_type`, `login_command`, `env_var`, - Dropbox `api_summary`, Dropbox `api_endpoint`, or `bytes_written` + `flag`, `flags`, `value`, `path`, `revision`, `email`, `member_id`, + `from_path`, `to_path`, `url`, `operation`, `token_type`, `login_command`, + `env_var`, Dropbox `api_summary`, Dropbox `api_endpoint`, `bytes_written`, + or `retry_after_seconds` - `warnings`: machine-actionable warnings, or `[]` +Reusable `error.details` keys: + +| Key | Meaning | +| --- | --- | +| `argument` | Single positional argument related to validation or remediation. | +| `arguments` | Multiple positional arguments related to validation or remediation. | +| `flag` | Single CLI flag related to validation or remediation, without leading dashes. | +| `flags` | Multiple CLI flags related to validation or remediation, without leading dashes. | +| `value` | Invalid or relevant user-provided flag value. | +| `path` | Dropbox or local path directly related to the error. | +| `revision` | Dropbox file revision directly related to the error. | +| `email` | Email address directly related to the error. | +| `member_id` | Dropbox team member ID supplied by the caller, such as `--as-member`. | +| `from_path` | Source path for copy, move, upload, download, or another relocation-style operation. | +| `to_path` | Destination path for copy, move, upload, download, or another relocation-style operation. | +| `url` | Shared-link or API URL related to the error. | +| `operation` | High-level operation, such as `upload`, `download`, `delete`, `restore`, or `share_link_create`. | +| `token_type` | Credential type related to an auth error. | +| `login_command` | Suggested login command for auth remediation. | +| `env_var` | Environment variable related to auth or configuration remediation. | +| `api_summary` | Dropbox API error summary when available. | +| `api_endpoint` | Dropbox API endpoint parsed from an SDK error message when available. | +| `bytes_written` | Number of bytes written before a partial stdout transfer failed. | +| `retry_after_seconds` | Number of seconds to wait before retrying a rate-limited request. | + +Prefer these existing path keys before adding new synonyms: use `path` for one +directly relevant path and `from_path`/`to_path` for relocation-style source +and destination context. Add a new key such as `target` or `local_path` only +when the existing keys would be ambiguous. + Command results and JSON errors are written to stdout. Status, progress, human-facing warnings, diagnostics, and verbose logs are written to stderr. In JSON mode, error responses are written to stdout and the process exits with diff --git a/docs/json-schema/v1/error.schema.json b/docs/json-schema/v1/error.schema.json index 312a225..d634bf9 100644 --- a/docs/json-schema/v1/error.schema.json +++ b/docs/json-schema/v1/error.schema.json @@ -93,6 +93,34 @@ "type": "string", "description": "Local or Dropbox path related to the error." }, + "revision": { + "type": "string", + "description": "Dropbox file revision related to the error." + }, + "email": { + "type": "string", + "description": "Email address related to the error." + }, + "member_id": { + "type": "string", + "description": "Dropbox team member ID supplied by the caller." + }, + "from_path": { + "type": "string", + "description": "Source Dropbox path related to a relocation error." + }, + "to_path": { + "type": "string", + "description": "Destination Dropbox path related to a relocation error." + }, + "url": { + "type": "string", + "description": "Shared link URL related to the error." + }, + "operation": { + "type": "string", + "description": "High-level operation related to the error." + }, "token_type": { "type": "string", "description": "Credential type related to an auth error." @@ -117,6 +145,11 @@ "type": "integer", "minimum": 0, "description": "Number of bytes written before a partial stdout transfer failed." + }, + "retry_after_seconds": { + "type": "integer", + "minimum": 0, + "description": "Number of seconds to wait before retrying a rate-limited request." } } }