From 17799f5e69ba0da6c55e6e5b8210b83d8f48f993 Mon Sep 17 00:00:00 2001 From: Andrey Markelov Date: Thu, 25 Jun 2026 09:09:35 -0700 Subject: [PATCH] Add coded errors, JSON warnings, and contract tests Replace fragile string-matching in jsonErrorCode with typed coded errors set at the point of creation. Add warnings infrastructure for deprecated commands and skipped symlinks during recursive upload. Add contract tests that audit the structured output surface and verify the JSON operation output envelope shape. --- README.md | 1 + cmd/account.go | 3 +- cmd/add-member.go | 3 +- cmd/cp.go | 3 +- cmd/get.go | 14 ++-- cmd/get_test.go | 26 +++++++ cmd/json_contract_test.go | 130 ++++++++++++++++++++++++++++++++ cmd/json_output.go | 11 ++- cmd/mkdir.go | 7 +- cmd/mv.go | 2 +- cmd/output.go | 83 ++++++++++++++++---- cmd/output_test.go | 54 +++++++++---- cmd/put.go | 56 ++++++++------ cmd/put_test.go | 70 ++++++++++++++++- cmd/remove-member.go | 3 +- cmd/restore.go | 3 +- cmd/revs.go | 3 +- cmd/rm.go | 5 +- cmd/rm_test.go | 3 + cmd/search.go | 5 +- cmd/share-list-links.go | 13 +++- cmd/share_link_create.go | 16 ++-- cmd/share_link_download.go | 20 ++--- cmd/share_link_download_test.go | 6 ++ cmd/share_link_info.go | 6 +- cmd/share_link_json_test.go | 78 ++++++++++++++++++- cmd/share_link_password.go | 5 +- cmd/share_link_revoke.go | 10 +-- cmd/share_link_update.go | 15 ++-- cmd/team.go | 8 +- 30 files changed, 529 insertions(+), 133 deletions(-) create mode 100644 cmd/json_contract_test.go diff --git a/README.md b/README.md index a18f088..84f878f 100644 --- a/README.md +++ b/README.md @@ -168,6 +168,7 @@ $ dbxcli restore --output=json /Reports/old.pdf 015f... Structured success output is rolling out command by command. Currently migrated commands are `version`, `account`, `du`, `ls`, `search`, `revs`, `cp`, `mv`, `put`, `get`, `share-link create`, `share-link list`, `share-link info`, `share-link update`, `share-link revoke`, `share-link download`, `share list folder`, `team info`, `team list-members`, `team list-groups`, `team add-member`, `team remove-member`, `mkdir`, `rm`, and `restore`. Commands that have not been migrated return a JSON error whose `error.message` is `structured output is not supported for this command yet` when used with `--output=json`. Command results and JSON errors are written to stdout. Status, progress, human-facing warnings, diagnostics, and verbose logs are written to stderr. JSON errors include a `warnings` array for machine-actionable warnings; it is `[]` when no warnings are present. Successful JSON payloads use the same `warnings` field. +Current warning codes include `deprecated_command` for deprecated command paths and `skipped_symlink` for symlinks skipped by recursive upload. Successful JSON responses for migrated commands return an `input` object, a `results` array, and a `warnings` array. Result payloads are command-specific. For commands such as `mkdir`, each result reports what happened to the requested path: diff --git a/cmd/account.go b/cmd/account.go index 71c919f..b81984b 100644 --- a/cmd/account.go +++ b/cmd/account.go @@ -15,7 +15,6 @@ package cmd import ( - "errors" "fmt" "io" "text/tabwriter" @@ -102,7 +101,7 @@ func renderBasicAccount(out io.Writer, ba *users.BasicAccount) error { func account(cmd *cobra.Command, args []string) error { if len(args) > 1 { - return errors.New("`account` accepts an optional `id` argument") + return invalidArgumentsError("`account` accepts an optional `id` argument") } dbx := usersNewFunc(config) diff --git a/cmd/add-member.go b/cmd/add-member.go index 72beb01..ab161ac 100644 --- a/cmd/add-member.go +++ b/cmd/add-member.go @@ -15,7 +15,6 @@ package cmd import ( - "errors" "fmt" "io" @@ -25,7 +24,7 @@ import ( func addMember(cmd *cobra.Command, args []string) (err error) { if len(args) != 3 { - return errors.New("`add-member` requires `email`, `first`, and `last` arguments") + return invalidArgumentsError("`add-member` requires `email`, `first`, and `last` arguments") } dbx := teamNewFunc(config) diff --git a/cmd/cp.go b/cmd/cp.go index 6beb4ce..19c9a3f 100644 --- a/cmd/cp.go +++ b/cmd/cp.go @@ -15,7 +15,6 @@ package cmd import ( - "errors" "fmt" "strings" @@ -34,7 +33,7 @@ func cp(cmd *cobra.Command, args []string) error { destination = args[1] argsToCopy = append(argsToCopy, args[0]) } else { - return errors.New("cp requires a source and a destination") + return invalidArgumentsError("cp requires a source and a destination") } var cpErrors []error diff --git a/cmd/get.go b/cmd/get.go index b9f061d..11cd69e 100644 --- a/cmd/get.go +++ b/cmd/get.go @@ -65,7 +65,7 @@ type getResult struct { func get(cmd *cobra.Command, args []string) (err error) { if len(args) == 0 || len(args) > 2 { - return errors.New("`get` requires `src` and/or `dst` arguments") + return invalidArgumentsError("`get` requires `src` and/or `dst` arguments") } src, err := validatePath(args[0]) @@ -83,7 +83,7 @@ func get(cmd *cobra.Command, args []string) (err error) { if dst == "-" { if commandOutputFormat(cmd) == output.FormatJSON { - return errors.New("`get --output=json` cannot be used with stdout target `-`") + return invalidArgumentsError("`get --output=json` cannot be used with stdout target `-`") } return getStdout(cmd, src, recursive) } @@ -113,7 +113,7 @@ func get(cmd *cobra.Command, args []string) (err error) { if _, ok := meta.(*files.FolderMetadata); ok { if !recursive { - return fmt.Errorf("%s is a folder (use --recursive to download folders)", src) + return invalidArgumentsErrorf("%s is a folder (use --recursive to download folders)", src) } if f, statErr := os.Stat(dst); statErr == nil && f.IsDir() { dst = filepath.Join(dst, path.Base(src)) @@ -196,7 +196,7 @@ func getOperationResults(results []getResult) []jsonOperationResult { func getStdout(cmd *cobra.Command, src string, recursive bool) error { if recursive { - return errors.New("`get -` cannot be used with --recursive") + return invalidArgumentsError("`get -` cannot be used with --recursive") } dbx := filesNewFunc(config) @@ -204,7 +204,7 @@ 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 fmt.Errorf("%s is a folder; cannot download folder to stdout", src) + return invalidArgumentsErrorf("%s is a folder; cannot download folder to stdout", src) } } @@ -213,7 +213,7 @@ func getStdout(cmd *cobra.Command, src string, recursive bool) error { func getWithClient(dbx files.Client, args []string) (err error) { if len(args) == 0 || len(args) > 2 { - return errors.New("`get` requires `src` and/or `dst` arguments") + return invalidArgumentsError("`get` requires `src` and/or `dst` arguments") } src, err := validatePath(args[0]) @@ -342,7 +342,7 @@ func ensureLocalDirectoryResult(source, target string, metadata files.IsMetadata status := getStatusCreated if info, err := os.Stat(target); err == nil { if !info.IsDir() { - return getResult{}, fmt.Errorf("path exists and is not a folder: %s", target) + return getResult{}, pathConflictErrorf("path exists and is not a folder: %s", target) } status = getStatusExisting } else if !os.IsNotExist(err) { diff --git a/cmd/get_test.go b/cmd/get_test.go index 6c10962..84d3a3d 100644 --- a/cmd/get_test.go +++ b/cmd/get_test.go @@ -509,6 +509,9 @@ func TestGetFolderWithoutRecursiveFlag(t *testing.T) { if !strings.Contains(err.Error(), "--recursive") { t.Errorf("error = %q, want mention of --recursive", err.Error()) } + if got, want := jsonErrorCode(err), jsonErrorCodeInvalidArguments; got != want { + t.Fatalf("jsonErrorCode = %q, want %q", got, want) + } } func TestGetRecursiveCommandGetsMetadataThenListsFolder(t *testing.T) { @@ -718,11 +721,34 @@ func TestGetJSONRejectsStdoutTarget(t *testing.T) { if !strings.Contains(err.Error(), "stdout target") { t.Fatalf("error = %q, want stdout target", err.Error()) } + if got, want := jsonErrorCode(err), jsonErrorCodeInvalidArguments; got != want { + t.Fatalf("jsonErrorCode = %q, want %q", got, want) + } if stdout.Len() != 0 { t.Fatalf("stdout = %q, want empty", stdout.String()) } } +func TestGetStdoutFolderErrorHasInvalidArgumentsCode(t *testing.T) { + mock := &mockFilesClient{ + getMetadataFn: func(arg *files.GetMetadataArg) (files.IsMetadata, error) { + return getTestFolderMetadata(arg.Path), nil + }, + } + stubFilesClient(t, mock) + + err := getStdout(&cobra.Command{}, "/remote-folder", false) + if err == nil { + t.Fatal("expected folder stdout error") + } + if !strings.Contains(err.Error(), "cannot download folder to stdout") { + t.Fatalf("error = %q, want stdout folder error", err.Error()) + } + if got, want := jsonErrorCode(err), jsonErrorCodeInvalidArguments; got != want { + t.Fatalf("jsonErrorCode = %q, want %q", got, want) + } +} + func TestGetTextStdoutTargetWritesOnlyFileBytes(t *testing.T) { mock := &mockFilesClient{ getMetadataFn: func(arg *files.GetMetadataArg) (files.IsMetadata, error) { diff --git a/cmd/json_contract_test.go b/cmd/json_contract_test.go new file mode 100644 index 0000000..af5add7 --- /dev/null +++ b/cmd/json_contract_test.go @@ -0,0 +1,130 @@ +package cmd + +import ( + "bytes" + "encoding/json" + "errors" + "sort" + "strings" + "testing" + + "github.com/dropbox/dbxcli/internal/output" + "github.com/spf13/cobra" +) + +func TestStructuredOutputCommandAudit(t *testing.T) { + got := structuredOutputCommandPaths(RootCmd) + got = append(got, NewVersionCommand("test").Name()) + sort.Strings(got) + + want := []string{ + "account", + "cp", + "du", + "get", + "ls", + "mkdir", + "mv", + "put", + "restore", + "revs", + "rm", + "search", + "share list folder", + "share list link", + "share-link create", + "share-link download", + "share-link info", + "share-link list", + "share-link revoke", + "share-link update", + "team add-member", + "team info", + "team list-groups", + "team list-members", + "team remove-member", + "version", + } + sort.Strings(want) + + if len(got) != len(want) { + t.Fatalf("structured commands = %v, want %v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("structured commands = %v, want %v", got, want) + } + } +} + +func structuredOutputCommandPaths(root *cobra.Command) []string { + var paths []string + var walk func(*cobra.Command, []string) + walk = func(cmd *cobra.Command, parents []string) { + parts := parents + if cmd != root { + parts = append(append([]string{}, parents...), cmd.Name()) + if commandSupportsStructuredOutput(cmd) { + paths = append(paths, strings.Join(parts, " ")) + } + } + for _, child := range cmd.Commands() { + walk(child, parts) + } + } + walk(root, nil) + return paths +} + +func TestJSONOperationOutputContractShape(t *testing.T) { + encoded, err := json.Marshal(newJSONOperationOutput(nil, nil, nil)) + if err != nil { + t.Fatalf("marshal operation output: %v", err) + } + + var raw map[string]json.RawMessage + if err := json.Unmarshal(encoded, &raw); err != nil { + t.Fatalf("decode operation output: %v", err) + } + for _, key := range []string{"input", "results", "warnings"} { + if _, ok := raw[key]; !ok { + t.Fatalf("operation output = %s, missing %q", encoded, key) + } + } + if len(raw) != 3 { + t.Fatalf("operation output = %s, want only input/results/warnings", encoded) + } +} + +func TestUnsupportedCommandsReturnJSONErrorEnvelope(t *testing.T) { + for _, name := range []string{"login", "logout", "completion"} { + t.Run(name, func(t *testing.T) { + var stdout, stderr bytes.Buffer + cmd := &cobra.Command{Use: name} + cmd.SetOut(&stdout) + cmd.SetErr(&stderr) + cmd.Flags().String(outputFlag, "json", "") + + err := validateOutputFormat(cmd) + if !errors.Is(err, output.ErrStructuredOutputUnsupported) { + t.Fatalf("validateOutputFormat error = %v, want structured output unsupported", err) + } + + renderCommandError(cmd, err) + if stderr.Len() != 0 { + t.Fatalf("stderr = %q, want empty", stderr.String()) + } + + got := decodeJSONErrorResponse(t, stdout.String()) + if got.OK { + t.Fatalf("ok = true, want false") + } + if got.Error.Code != jsonErrorCodeStructuredOutputUnsupported { + t.Fatalf("code = %q, want %q", got.Error.Code, jsonErrorCodeStructuredOutputUnsupported) + } + if got.Warnings == nil || len(got.Warnings) != 0 { + t.Fatalf("warnings = %+v, want empty array", got.Warnings) + } + }) + } +} diff --git a/cmd/json_output.go b/cmd/json_output.go index 0d0f344..85486ec 100644 --- a/cmd/json_output.go +++ b/cmd/json_output.go @@ -19,6 +19,11 @@ type jsonWarning struct { Path string `json:"path,omitempty"` } +const ( + jsonWarningCodeDeprecatedCommand = "deprecated_command" + jsonWarningCodeSkippedSymlink = "skipped_symlink" +) + type jsonOperationOutput struct { Input any `json:"input"` Results []jsonOperationResult `json:"results"` @@ -52,7 +57,11 @@ func newJSONOperationOutput(input any, results []jsonOperationResult, warnings [ } func renderJSONOperationOutput(cmd *cobra.Command, input any, results []jsonOperationResult) error { - return commandOutput(cmd).Render(nil, newJSONOperationOutput(input, results, nil)) + return renderJSONOperationOutputWithWarnings(cmd, input, results, nil) +} + +func renderJSONOperationOutputWithWarnings(cmd *cobra.Command, input any, results []jsonOperationResult, warnings []jsonWarning) error { + return commandOutput(cmd).Render(nil, newJSONOperationOutput(input, results, warnings)) } func newJSONOperationResult(status, kind string, input any, result any) jsonOperationResult { diff --git a/cmd/mkdir.go b/cmd/mkdir.go index f6c8413..ca0d0bd 100644 --- a/cmd/mkdir.go +++ b/cmd/mkdir.go @@ -16,7 +16,6 @@ package cmd import ( "errors" - "fmt" "strings" "github.com/dropbox/dropbox-sdk-go-unofficial/v6/dropbox/files" @@ -44,7 +43,7 @@ type mkdirResult struct { func mkdir(cmd *cobra.Command, args []string) (err error) { if len(args) != 1 { - return errors.New("`mkdir` requires a `directory` argument") + return invalidArgumentsError("`mkdir` requires a `directory` argument") } dst, err := validatePath(args[0]) @@ -77,7 +76,7 @@ func mkdir(cmd *cobra.Command, args []string) (err error) { } status = mkdirStatusExisting case ok && (conflictTag == files.WriteConflictErrorFile || conflictTag == files.WriteConflictErrorFileAncestor): - return fmt.Errorf("path exists and is not a folder: %s", dst) + return pathConflictErrorf("path exists and is not a folder: %s", dst) case ok: return err case isConflictError(err): @@ -110,7 +109,7 @@ func existingFolderMetadata(dbx files.Client, dst string) (*files.FolderMetadata } folder, ok := metadata.(*files.FolderMetadata) if !ok || folder == nil { - return nil, fmt.Errorf("path exists and is not a folder: %s", dst) + return nil, pathConflictErrorf("path exists and is not a folder: %s", dst) } return folder, nil } diff --git a/cmd/mv.go b/cmd/mv.go index 7cbbfa2..74bbb9a 100644 --- a/cmd/mv.go +++ b/cmd/mv.go @@ -33,7 +33,7 @@ func mv(cmd *cobra.Command, args []string) error { destination = args[1] argsToMove = append(argsToMove, args[0]) } else { - return fmt.Errorf("mv command requires a source and a destination") + return invalidArgumentsError("mv command requires a source and a destination") } var mvErrors []error diff --git a/cmd/output.go b/cmd/output.go index 3e0cdf5..224f609 100644 --- a/cmd/output.go +++ b/cmd/output.go @@ -12,8 +12,64 @@ import ( const ( outputFlag = "output" structuredOutputSupportedAnnotation = "dbxcli.supportsStructuredOutput" + + jsonErrorCodeCommandFailed = "command_failed" + jsonErrorCodeInvalidArguments = "invalid_arguments" + jsonErrorCodePathConflict = "path_conflict" + jsonErrorCodeStructuredOutputUnsupported = "structured_output_unsupported" + jsonErrorCodeUnknownCommand = "unknown_command" + jsonErrorCodeUnknownFlag = "unknown_flag" + jsonErrorCodeUnsupportedOutputFormat = "unsupported_output_format" ) +type jsonCodedError interface { + error + JSONErrorCode() string +} + +type codedError struct { + code string + err error +} + +func (e codedError) Error() string { + return e.err.Error() +} + +func (e codedError) Unwrap() error { + return e.err +} + +func (e codedError) JSONErrorCode() string { + return e.code +} + +func newCodedError(code string, err error) error { + if err == nil { + return nil + } + return codedError{ + code: code, + err: err, + } +} + +func invalidArgumentsError(message string) error { + return newCodedError(jsonErrorCodeInvalidArguments, errors.New(message)) +} + +func invalidArgumentsErrorf(format string, args ...any) error { + return newCodedError(jsonErrorCodeInvalidArguments, fmt.Errorf(format, args...)) +} + +func pathConflictErrorf(format string, args ...any) error { + return newCodedError(jsonErrorCodePathConflict, fmt.Errorf(format, args...)) +} + +func unsupportedOutputFormatErrorf(format string, args ...any) error { + return newCodedError(jsonErrorCodeUnsupportedOutputFormat, fmt.Errorf(format, args...)) +} + func commandOutput(cmd *cobra.Command) *output.Renderer { if cmd == nil { return output.New(nil, nil, output.FormatText) @@ -61,7 +117,7 @@ func parseOutputFormat(value string) (output.Format, error) { case output.FormatJSON: return output.FormatJSON, nil default: - return "", fmt.Errorf("unsupported output format %q: use text or json", value) + return "", unsupportedOutputFormatErrorf("unsupported output format %q: use text or json", value) } } @@ -130,7 +186,7 @@ func renderCommandErrorWithJSON(cmd *cobra.Command, err error, forceJSON bool) { } _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Error: %v\n", err) - if jsonErrorCode(err) == "unknown_command" { + if jsonErrorCode(err) == jsonErrorCodeUnknownCommand { _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "Run '%s --help' for usage.\n", cmd.CommandPath()) } } @@ -164,27 +220,24 @@ func outputJSONRequested(args []string) bool { } // jsonErrorCode derives stable script-facing codes from existing CLI errors. -// If a matched error message changes, update this mapping with it. +// Prefer coded errors for dbxcli-owned validation failures. String matching is +// kept only for Cobra-generated unknown command/flag errors and legacy fallback. func jsonErrorCode(err error) string { + var coded jsonCodedError + if errors.As(err, &coded) { + return coded.JSONErrorCode() + } if errors.Is(err, output.ErrStructuredOutputUnsupported) { - return "structured_output_unsupported" + return jsonErrorCodeStructuredOutputUnsupported } message := err.Error() switch { - case strings.Contains(message, "unsupported output format"): - return "unsupported_output_format" case strings.Contains(message, "unknown command"): - return "unknown_command" + return jsonErrorCodeUnknownCommand case strings.Contains(message, "unknown flag"): - return "unknown_flag" - case strings.Contains(message, "path exists and is not a folder"): - return "path_conflict" - case strings.Contains(message, "requires "): - return "invalid_arguments" - case strings.Contains(message, "accepts an optional"): - return "invalid_arguments" + return jsonErrorCodeUnknownFlag default: - return "command_failed" + return jsonErrorCodeCommandFailed } } diff --git a/cmd/output_test.go b/cmd/output_test.go index 41dc6af..86f30cd 100644 --- a/cmd/output_test.go +++ b/cmd/output_test.go @@ -594,24 +594,52 @@ func TestOutputJSONRequested(t *testing.T) { } } -func TestJSONErrorCodePathConflict(t *testing.T) { - err := errors.New("path exists and is not a folder: /file") - if got, want := jsonErrorCode(err), "path_conflict"; got != want { - t.Fatalf("jsonErrorCode = %q, want %q", got, want) +func TestJSONErrorCodeUsesCodedErrors(t *testing.T) { + tests := []struct { + name string + err error + want string + }{ + { + name: "path conflict", + err: pathConflictErrorf("path exists and is not a folder: /file"), + want: jsonErrorCodePathConflict, + }, + { + name: "optional argument validation", + err: invalidArgumentsError("`account` accepts an optional `id` argument"), + want: jsonErrorCodeInvalidArguments, + }, + { + name: "required argument validation", + err: invalidArgumentsError("`add-member` requires `email`, `first`, and `last` arguments"), + want: jsonErrorCodeInvalidArguments, + }, + { + name: "unsupported output format", + err: unsupportedOutputFormatErrorf("unsupported output format %q: use text or json", "yaml"), + want: jsonErrorCodeUnsupportedOutputFormat, + }, } -} -func TestJSONErrorCodeOptionalArgumentValidation(t *testing.T) { - err := errors.New("`account` accepts an optional `id` argument") - if got, want := jsonErrorCode(err), "invalid_arguments"; got != want { - t.Fatalf("jsonErrorCode = %q, want %q", got, want) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := jsonErrorCode(tt.err); got != tt.want { + t.Fatalf("jsonErrorCode = %q, want %q", got, tt.want) + } + }) } } -func TestJSONErrorCodeRequiredArgumentValidation(t *testing.T) { - err := errors.New("`add-member` requires `email`, `first`, and `last` arguments") - if got, want := jsonErrorCode(err), "invalid_arguments"; got != want { - t.Fatalf("jsonErrorCode = %q, want %q", got, want) +func TestJSONErrorCodeDoesNotClassifyPlainValidationStrings(t *testing.T) { + for _, err := range []error{ + errors.New("path exists and is not a folder: /file"), + errors.New("Dropbox API requires team admin permissions"), + errors.New("`account` accepts an optional `id` argument"), + } { + if got := jsonErrorCode(err); got != jsonErrorCodeCommandFailed { + t.Fatalf("jsonErrorCode(%q) = %q, want %q", err.Error(), got, jsonErrorCodeCommandFailed) + } } } diff --git a/cmd/put.go b/cmd/put.go index 257831f..3a9551f 100644 --- a/cmd/put.go +++ b/cmd/put.go @@ -260,7 +260,11 @@ func newPutResult(status, kind, source, target string, metadata files.IsMetadata } func renderPutResults(cmd *cobra.Command, input putCommandInput, results []putResult) error { - return renderJSONOperationOutput(cmd, input, putOperationResults(results)) + return renderPutResultsWithWarnings(cmd, input, results, nil) +} + +func renderPutResultsWithWarnings(cmd *cobra.Command, input putCommandInput, results []putResult, warnings []jsonWarning) error { + return renderJSONOperationOutputWithWarnings(cmd, input, putOperationResults(results), warnings) } func putOperationResults(results []putResult) []jsonOperationResult { @@ -277,7 +281,7 @@ func putOperationResults(results []putResult) []jsonOperationResult { func put(cmd *cobra.Command, args []string) (err error) { if len(args) == 0 || len(args) > 2 { - return errors.New("`put` requires `src` and/or `dst` arguments") + return invalidArgumentsError("`put` requires `src` and/or `dst` arguments") } opts, err := parsePutOptions(cmd) @@ -299,7 +303,7 @@ func put(cmd *cobra.Command, args []string) (err error) { } if srcInfo.IsDir() && !recursive { - return fmt.Errorf("%s is a directory (use --recursive to upload directories)", src) + return invalidArgumentsErrorf("%s is a directory (use --recursive to upload directories)", src) } // Default `dst` to the base segment of the source path; use the second argument if provided. @@ -321,17 +325,17 @@ func put(cmd *cobra.Command, args []string) (err error) { if commandOutputFormat(cmd) == output.FormatText { return putRecursive(src, dst, opts) } - results, err := putRecursiveWithResults(src, dst, opts) + results, warnings, err := putRecursiveWithResults(src, dst, opts) if err != nil { return err } - return renderPutResults(cmd, putCommandInput{ + return renderPutResultsWithWarnings(cmd, putCommandInput{ Source: src, Target: dst, Recursive: true, IfExists: opts.ifExists, Stdin: false, - }, results) + }, results, warnings) } result, err := putFileWithResult(src, dst, opts) @@ -349,15 +353,15 @@ func put(cmd *cobra.Command, args []string) (err error) { func putStdin(cmd *cobra.Command, args []string, opts putOptions, recursive bool) error { if len(args) < 2 { - return errors.New("`put -` requires an explicit target path") + return invalidArgumentsError("`put -` requires an explicit target path") } if recursive { - return errors.New("`put -` cannot be used with --recursive") + return invalidArgumentsError("`put -` cannot be used with --recursive") } dst := args[1] if strings.HasSuffix(dst, "/") { - return fmt.Errorf("cannot upload stdin to directory target %q; provide a full Dropbox file path", dst) + return invalidArgumentsErrorf("cannot upload stdin to directory target %q; provide a full Dropbox file path", dst) } dstPath, err := validatePath(dst) @@ -436,7 +440,7 @@ func parsePutOptions(cmd *cobra.Command) (putOptions, error) { return putOptions{}, err } if chunkSize%(1<<22) != 0 { - return putOptions{}, errors.New("`put` requires chunk size to be multiple of 4MiB") + return putOptions{}, invalidArgumentsError("`put` requires chunk size to be multiple of 4MiB") } workers, err := cmd.Flags().GetInt("workers") if err != nil { @@ -476,7 +480,7 @@ func normalizePutIfExists(ifExists string) (string, error) { case putIfExistsOverwrite, putIfExistsSkip, putIfExistsFail: return ifExists, nil default: - return "", fmt.Errorf("invalid --if-exists %q (use overwrite, skip, or fail)", ifExists) + return "", invalidArgumentsErrorf("invalid --if-exists %q (use overwrite, skip, or fail)", ifExists) } } @@ -568,7 +572,7 @@ func checkPutStdinDestination(dbx files.Client, dst string, ifExists string) (pu return putDestinationUpload, nil, nil } if _, ok := meta.(*files.FolderMetadata); ok { - return putDestinationUpload, nil, fmt.Errorf("cannot upload stdin to folder %q; provide a full Dropbox file path", dst) + return putDestinationUpload, nil, invalidArgumentsErrorf("cannot upload stdin to folder %q; provide a full Dropbox file path", dst) } return actionForExistingDestination(dst, ifExists, meta) } @@ -590,7 +594,7 @@ func checkPutDestination(dbx files.Client, dst string, ifExists string) (putDest return putDestinationUpload, nil, nil } if _, ok := meta.(*files.FolderMetadata); ok { - return putDestinationUpload, nil, fmt.Errorf("destination %q is a folder", dst) + return putDestinationUpload, nil, pathConflictErrorf("destination %q is a folder", dst) } return actionForExistingDestination(dst, ifExists, meta) } @@ -600,7 +604,7 @@ func actionForExistingDestination(dst string, ifExists string, metadata files.Is case putIfExistsSkip: return putDestinationSkip, metadata, nil case putIfExistsFail: - return putDestinationUpload, nil, fmt.Errorf("destination %q already exists", dst) + return putDestinationUpload, nil, pathConflictErrorf("destination %q already exists", dst) default: return putDestinationUpload, nil, nil } @@ -702,17 +706,18 @@ func putErrorOutput(opts putOptions) io.Writer { } func putRecursive(src, dst string, opts putOptions) error { - _, err := putRecursiveInternal(src, dst, opts, false) + _, _, err := putRecursiveInternal(src, dst, opts, false) return err } -func putRecursiveWithResults(src, dst string, opts putOptions) ([]putResult, error) { +func putRecursiveWithResults(src, dst string, opts putOptions) ([]putResult, []jsonWarning, error) { return putRecursiveInternal(src, dst, opts, true) } -func putRecursiveInternal(src, dst string, opts putOptions, collectResults bool) ([]putResult, error) { +func putRecursiveInternal(src, dst string, opts putOptions, collectResults bool) ([]putResult, []jsonWarning, error) { src = filepath.Clean(src) var results []putResult + var warnings []jsonWarning var uploadErrors []error dirsWithFiles := make(map[string]bool) @@ -724,6 +729,13 @@ func putRecursiveInternal(src, dst string, opts putOptions, collectResults bool) return nil } if !d.Type().IsRegular() { + if collectResults && d.Type()&os.ModeSymlink != 0 { + warnings = append(warnings, jsonWarning{ + Code: jsonWarningCodeSkippedSymlink, + Message: "skipped symlink during recursive upload", + Path: filePath, + }) + } return nil } @@ -753,7 +765,7 @@ func putRecursiveInternal(src, dst string, opts putOptions, collectResults bool) return nil }) if err != nil { - return nil, err + return nil, nil, err } dbx := filesNewFunc(config) @@ -809,13 +821,13 @@ func putRecursiveInternal(src, dst string, opts putOptions, collectResults bool) return nil }) if err != nil { - return nil, err + return nil, nil, err } if len(uploadErrors) > 0 { - return nil, fmt.Errorf("failed to upload %d file(s): %v", len(uploadErrors), uploadErrors[0]) + return nil, nil, fmt.Errorf("failed to upload %d file(s): %v", len(uploadErrors), uploadErrors[0]) } - return results, nil + return results, warnings, nil } func putDirectory(dbx files.Client, dst string) error { @@ -851,7 +863,7 @@ func putDirectoryConflictError(dst string, err error) error { case ok && conflictTag == files.WriteConflictErrorFolder: return nil case ok && (conflictTag == files.WriteConflictErrorFile || conflictTag == files.WriteConflictErrorFileAncestor): - return fmt.Errorf("path exists and is not a folder: %s", dst) + return pathConflictErrorf("path exists and is not a folder: %s", dst) case ok: return err case isConflictError(err): diff --git a/cmd/put_test.go b/cmd/put_test.go index f24ac79..fc506a7 100644 --- a/cmd/put_test.go +++ b/cmd/put_test.go @@ -273,6 +273,16 @@ type putOutputData struct { func decodePutOutput(t *testing.T, stdout *bytes.Buffer) putOutputData { t.Helper() + got := decodePutOutputWithWarnings(t, stdout) + if len(got.Warnings) != 0 { + t.Fatalf("warnings = %+v, want empty", got.Warnings) + } + return got +} + +func decodePutOutputWithWarnings(t *testing.T, stdout *bytes.Buffer) putOutputData { + t.Helper() + var got putOutputData if err := json.Unmarshal(stdout.Bytes(), &got); err != nil { t.Fatalf("decode put JSON output: %v\noutput: %s", err, stdout.String()) @@ -280,9 +290,6 @@ func decodePutOutput(t *testing.T, stdout *bytes.Buffer) putOutputData { if got.Warnings == nil { t.Fatalf("warnings = nil, want empty array") } - if len(got.Warnings) != 0 { - t.Fatalf("warnings = %+v, want empty", got.Warnings) - } return got } @@ -947,6 +954,63 @@ func TestPutJSONRecursiveOutputsDirectoryAndFileResults(t *testing.T) { } } +func TestPutJSONRecursiveWarnsForSkippedSymlink(t *testing.T) { + dir := t.TempDir() + realPath := filepath.Join(dir, "real.txt") + linkPath := filepath.Join(dir, "link.txt") + if err := os.WriteFile(realPath, []byte("real"), 0644); err != nil { + t.Fatal(err) + } + if err := os.Symlink(realPath, linkPath); err != nil { + t.Fatal(err) + } + + mock := &mockFilesClient{ + uploadFn: func(arg *files.UploadArg, content io.Reader) (*files.FileMetadata, error) { + if arg.Path != "/remote/real.txt" { + t.Fatalf("upload path = %q, want /remote/real.txt", arg.Path) + } + if _, err := io.ReadAll(content); err != nil { + t.Fatal(err) + } + return putFileMetadata(arg.Path, 4), nil + }, + createFolderV2Fn: func(arg *files.CreateFolderArg) (*files.CreateFolderResult, error) { + return files.NewCreateFolderResult(putFolderMetadata(arg.Path)), nil + }, + } + stubFilesClient(t, mock) + + var stdout bytes.Buffer + cmd := testPutJSONCmd(&stdout, nil) + if err := cmd.Flags().Set("recursive", "true"); err != nil { + t.Fatal(err) + } + if err := put(cmd, []string{dir, "/remote"}); err != nil { + t.Fatalf("put error: %v", err) + } + + got := decodePutOutputWithWarnings(t, &stdout) + if len(got.Warnings) != 1 { + t.Fatalf("warnings = %+v, want one skipped symlink warning", got.Warnings) + } + warning := got.Warnings[0] + if warning.Code != jsonWarningCodeSkippedSymlink || warning.Path != linkPath { + t.Fatalf("warning = %+v, want skipped symlink at %s", warning, linkPath) + } + if !strings.Contains(warning.Message, "skipped symlink") { + t.Fatalf("warning message = %q, want skipped symlink", warning.Message) + } + for _, result := range got.Results { + if result.Input.Source == linkPath || result.Input.Target == "/remote/link.txt" { + t.Fatalf("symlink should not be uploaded: %+v", result) + } + } + if !json.Valid(stdout.Bytes()) { + t.Fatalf("stdout is not valid JSON: %s", stdout.String()) + } +} + func TestPutJSONRecursiveFailsWhenDirectoryTargetIsExistingFile(t *testing.T) { dir := t.TempDir() diff --git a/cmd/remove-member.go b/cmd/remove-member.go index 3c6c27d..d1c384d 100644 --- a/cmd/remove-member.go +++ b/cmd/remove-member.go @@ -15,7 +15,6 @@ package cmd import ( - "errors" "fmt" "io" @@ -26,7 +25,7 @@ import ( func removeMember(cmd *cobra.Command, args []string) (err error) { if len(args) != 1 { - return errors.New("`remove-member` requires an `email` argument") + return invalidArgumentsError("`remove-member` requires an `email` argument") } dbx := teamNewFunc(config) diff --git a/cmd/restore.go b/cmd/restore.go index 4a3bfcf..1100f2d 100644 --- a/cmd/restore.go +++ b/cmd/restore.go @@ -15,7 +15,6 @@ package cmd import ( - "errors" "fmt" "io" "time" @@ -43,7 +42,7 @@ type restoreResult struct { func restore(cmd *cobra.Command, args []string) (err error) { if len(args) != 2 { - return errors.New("`restore` requires `target-path` and `revision` arguments") + return invalidArgumentsError("`restore` requires `target-path` and `revision` arguments") } path, err := validatePath(args[0]) diff --git a/cmd/revs.go b/cmd/revs.go index ce5aebe..6035781 100644 --- a/cmd/revs.go +++ b/cmd/revs.go @@ -15,7 +15,6 @@ package cmd import ( - "errors" "fmt" "io" "text/tabwriter" @@ -36,7 +35,7 @@ const revsJSONStatusRevision = "revision" func revs(cmd *cobra.Command, args []string) (err error) { if len(args) != 1 { - return errors.New("`revs` requires a `file` argument") + return invalidArgumentsError("`revs` requires a `file` argument") } path, err := validatePath(args[0]) diff --git a/cmd/rm.go b/cmd/rm.go index b187453..741f49f 100644 --- a/cmd/rm.go +++ b/cmd/rm.go @@ -15,7 +15,6 @@ package cmd import ( - "errors" "fmt" "io" @@ -50,7 +49,7 @@ type removeResult struct { func rm(cmd *cobra.Command, args []string) error { if len(args) < 1 { - return errors.New("rm: missing operand") + return invalidArgumentsError("rm: missing operand") } opts, err := parseRemoveOptions(cmd) @@ -134,7 +133,7 @@ func validateRemoveTargets(dbx files.Client, args []string, opts removeOptions) return nil, err } if len(res.Entries) != 0 { - return nil, fmt.Errorf("rm: cannot remove ‘%s’: Directory not empty, use `--force`/`-f` or `--recursive`/`-r` to proceed", path) + return nil, invalidArgumentsErrorf("rm: cannot remove ‘%s’: Directory not empty, use `--force`/`-f` or `--recursive`/`-r` to proceed", path) } } targets = append(targets, removeTarget{path: path, metadata: pathMetaData}) diff --git a/cmd/rm_test.go b/cmd/rm_test.go index 1155600..31f4499 100644 --- a/cmd/rm_test.go +++ b/cmd/rm_test.go @@ -166,6 +166,9 @@ func TestRmNonEmptyFolderRequiresRecursiveOrForce(t *testing.T) { if !strings.Contains(err.Error(), "--recursive") || !strings.Contains(err.Error(), "--force") { t.Fatalf("error = %q, want recursive and force guidance", err.Error()) } + if got, want := jsonErrorCode(err), jsonErrorCodeInvalidArguments; got != want { + t.Fatalf("jsonErrorCode = %q, want %q", got, want) + } if deleteCalled { t.Fatal("delete called after validation failure") } diff --git a/cmd/search.go b/cmd/search.go index 8212df0..c1750c3 100644 --- a/cmd/search.go +++ b/cmd/search.go @@ -15,7 +15,6 @@ package cmd import ( - "errors" "fmt" "io" "strings" @@ -40,14 +39,14 @@ const searchJSONStatusFound = "found" func search(cmd *cobra.Command, args []string) (err error) { if len(args) == 0 { - return errors.New("`search` requires a `query` argument") + return invalidArgumentsError("`search` requires a `query` argument") } var scope string if len(args) == 2 { scope = args[1] if !strings.HasPrefix(scope, "/") { - return errors.New("`search` `path-scope` must begin with \"/\"") + return invalidArgumentsError("`search` `path-scope` must begin with \"/\"") } } diff --git a/cmd/share-list-links.go b/cmd/share-list-links.go index a552ede..95623bc 100644 --- a/cmd/share-list-links.go +++ b/cmd/share-list-links.go @@ -29,12 +29,19 @@ type shareLinkListInput struct { } func shareListLinks(cmd *cobra.Command, args []string) (err error) { - return shareLinkList(cmd, args) + return shareLinkListWithWarnings(cmd, args, []jsonWarning{{ + Code: jsonWarningCodeDeprecatedCommand, + Message: "use `dbxcli share-link list` instead", + }}) } func shareLinkList(cmd *cobra.Command, args []string) error { + return shareLinkListWithWarnings(cmd, args, nil) +} + +func shareLinkListWithWarnings(cmd *cobra.Command, args []string, warnings []jsonWarning) error { if len(args) > 1 { - return errors.New("`share-link list` accepts at most one `path` argument") + return invalidArgumentsError("`share-link list` accepts at most one `path` argument") } arg := sharing.NewListSharedLinksArg() @@ -72,7 +79,7 @@ func shareLinkList(cmd *cobra.Command, args []string) error { DirectOnly: arg.DirectOnly, }, shareLinkJSONOperationResults(shareLinkJSONStatusListed, entries), - nil, + warnings, )) } diff --git a/cmd/share_link_create.go b/cmd/share_link_create.go index e4ec53f..6322ac5 100644 --- a/cmd/share_link_create.go +++ b/cmd/share_link_create.go @@ -49,7 +49,7 @@ type shareLinkCreateInput struct { func shareLinkCreate(cmd *cobra.Command, args []string) error { if len(args) != 1 { - return errors.New("`share-link create` requires a `path` argument") + return invalidArgumentsError("`share-link create` requires a `path` argument") } path, err := validatePath(args[0]) @@ -57,7 +57,7 @@ func shareLinkCreate(cmd *cobra.Command, args []string) error { return err } if path == "" { - return errors.New("cannot create a shared link for Dropbox root") + return invalidArgumentsError("cannot create a shared link for Dropbox root") } opts, err := parseShareLinkCreateOptions(cmd) @@ -203,10 +203,10 @@ func parseShareLinkCreateOptions(cmd *cobra.Command) (shareLinkCreateOptions, er opts.password = password if opts.expires != nil && opts.removeExpiration { - return opts, errors.New("`--expires` and `--remove-expiration` cannot be used together") + return opts, invalidArgumentsError("`--expires` and `--remove-expiration` cannot be used together") } if opts.allowDownload && opts.disallowDownload { - return opts, errors.New("`--allow-download` and `--disallow-download` cannot be used together") + return opts, invalidArgumentsError("`--allow-download` and `--disallow-download` cannot be used together") } return opts, nil @@ -214,7 +214,7 @@ func parseShareLinkCreateOptions(cmd *cobra.Command) (shareLinkCreateOptions, er func applyExistingSharedLinkCreateOptions(dbx sharedLinkClient, link sharing.IsSharedLinkMetadata, opts shareLinkCreateOptions) (sharing.IsSharedLinkMetadata, error) { if opts.access != nil { - return nil, errors.New("cannot apply `--access` because the shared link already exists") + return nil, invalidArgumentsError("cannot apply `--access` because the shared link already exists") } if opts.expires == nil && !opts.removeExpiration && !opts.allowDownload && !opts.disallowDownload && opts.audience == nil && !opts.password.set { return link, nil @@ -409,7 +409,7 @@ func shareLinkExpiresFlag(cmd *cobra.Command) (*time.Time, error) { } parsed, err := time.Parse(time.RFC3339, value) if err != nil { - return nil, fmt.Errorf("invalid --expires %q: use RFC3339 timestamp", value) + return nil, invalidArgumentsErrorf("invalid --expires %q: use RFC3339 timestamp", value) } return &parsed, nil } @@ -427,7 +427,7 @@ func shareLinkAccessFlag(cmd *cobra.Command) (*sharing.RequestedLinkAccessLevel, case sharing.RequestedLinkAccessLevelMax: return requestedLinkAccessLevel(sharing.RequestedLinkAccessLevelMax), nil default: - return nil, fmt.Errorf("invalid --access %q: use viewer, editor, or max", value) + return nil, invalidArgumentsErrorf("invalid --access %q: use viewer, editor, or max", value) } } @@ -450,7 +450,7 @@ func shareLinkAudienceFlag(cmd *cobra.Command) (*sharing.LinkAudience, error) { case "no-one": return linkAudience(sharing.LinkAudienceNoOne), nil default: - return nil, fmt.Errorf("invalid --audience %q: use public, team, members, or no-one", value) + return nil, invalidArgumentsErrorf("invalid --audience %q: use public, team, members, or no-one", value) } } diff --git a/cmd/share_link_download.go b/cmd/share_link_download.go index a62cf34..2d0a007 100644 --- a/cmd/share_link_download.go +++ b/cmd/share_link_download.go @@ -52,19 +52,19 @@ type shareLinkDownloadResult struct { func shareLinkDownload(cmd *cobra.Command, args []string) error { if len(args) == 0 || len(args) > 2 { - return errors.New("`share-link download` requires a `url` and optional `target` argument") + return invalidArgumentsError("`share-link download` requires a `url` and optional `target` argument") } url := args[0] if url == "" { - return errors.New("`share-link download` requires a non-empty URL") + return invalidArgumentsError("`share-link download` requires a non-empty URL") } target := "" if len(args) == 2 { target = args[1] if target == "" { - return errors.New("`share-link download` requires a non-empty target") + return invalidArgumentsError("`share-link download` requires a non-empty target") } } @@ -77,7 +77,7 @@ func shareLinkDownload(cmd *cobra.Command, args []string) error { arg.LinkPassword = opts.password.password } if target == "-" && commandOutputFormat(cmd) == output.FormatJSON { - return errors.New("`share-link download -` cannot be used with --output=json") + return invalidArgumentsError("`share-link download -` cannot be used with --output=json") } dbx := newSharedLinkClient(config) @@ -93,10 +93,10 @@ func shareLinkDownload(cmd *cobra.Command, args []string) error { if folder, ok := link.(*sharing.FolderLinkMetadata); ok { if !opts.recursive { - return errors.New("shared link is a folder (use --recursive to download folders)") + return invalidArgumentsError("shared link is a folder (use --recursive to download folders)") } if target == "-" { - return errors.New("cannot download shared-link folder to stdout") + return invalidArgumentsError("cannot download shared-link folder to stdout") } dst, err := sharedLinkFolderDownloadTarget(target, folder) @@ -112,7 +112,7 @@ func shareLinkDownload(cmd *cobra.Command, args []string) error { if target == "-" { if opts.recursive { - return errors.New("`share-link download -` cannot be used with --recursive") + return invalidArgumentsError("`share-link download -` cannot be used with --recursive") } if err := downloadSharedLinkToStdout(dbx, arg, cmd.OutOrStdout()); err != nil { return err @@ -150,20 +150,20 @@ func parseShareLinkDownloadOptions(cmd *cobra.Command) (shareLinkDownloadOptions return opts, err } if pathArg == "" { - return opts, errors.New("`--path` requires a non-empty path") + return opts, invalidArgumentsError("`--path` requires a non-empty path") } path, err := validatePath(pathArg) if err != nil { return opts, err } if path == "" { - return opts, errors.New("cannot download shared-link root with `--path`") + return opts, invalidArgumentsError("cannot download shared-link root with `--path`") } opts.path = path } if opts.path != "" && opts.recursive { - return opts, errors.New("`--path` cannot be used with --recursive") + return opts, invalidArgumentsError("`--path` cannot be used with --recursive") } return opts, nil diff --git a/cmd/share_link_download_test.go b/cmd/share_link_download_test.go index 0d28112..b1dd4a6 100644 --- a/cmd/share_link_download_test.go +++ b/cmd/share_link_download_test.go @@ -419,6 +419,9 @@ func TestShareLinkDownloadFolderRequiresRecursive(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "--recursive") { t.Fatalf("error = %v, want recursive error", err) } + if got, want := jsonErrorCode(err), jsonErrorCodeInvalidArguments; got != want { + t.Fatalf("jsonErrorCode = %q, want %q", got, want) + } if called { t.Fatal("GetSharedLinkFile should not be called") } @@ -440,6 +443,9 @@ func TestShareLinkDownloadFolderRejectsStdoutTarget(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "stdout") { t.Fatalf("error = %v, want stdout folder error", err) } + if got, want := jsonErrorCode(err), jsonErrorCodeInvalidArguments; got != want { + t.Fatalf("jsonErrorCode = %q, want %q", got, want) + } } func TestShareLinkDownloadFolderRecursiveDownloadsNestedFiles(t *testing.T) { diff --git a/cmd/share_link_info.go b/cmd/share_link_info.go index c760d12..e452469 100644 --- a/cmd/share_link_info.go +++ b/cmd/share_link_info.go @@ -39,12 +39,12 @@ type shareLinkInfoInput struct { func shareLinkInfo(cmd *cobra.Command, args []string) error { if len(args) != 1 { - return errors.New("`share-link info` requires a `url` argument") + return invalidArgumentsError("`share-link info` requires a `url` argument") } url := args[0] if url == "" { - return errors.New("`share-link info` requires a non-empty URL") + return invalidArgumentsError("`share-link info` requires a non-empty URL") } opts, err := parseShareLinkInfoOptions(cmd) @@ -93,7 +93,7 @@ func parseShareLinkInfoOptions(cmd *cobra.Command) (shareLinkInfoOptions, error) return opts, err } if path == "" { - return opts, errors.New("`--path` requires a non-empty path") + return opts, invalidArgumentsError("`--path` requires a non-empty path") } opts.path = path } diff --git a/cmd/share_link_json_test.go b/cmd/share_link_json_test.go index b8acb62..b9ae40f 100644 --- a/cmd/share_link_json_test.go +++ b/cmd/share_link_json_test.go @@ -133,6 +133,72 @@ func TestShareLinkListJSONOutputsResultsAndInput(t *testing.T) { } } +func TestDeprecatedShareListLinkJSONIncludesWarning(t *testing.T) { + stubSharedLinkClient(t, &mockSharedLinkClient{ + listSharedLinksFn: func(arg *sharing.ListSharedLinksArg) (*sharing.ListSharedLinksResult, error) { + return sharing.NewListSharedLinksResult([]sharing.IsSharedLinkMetadata{ + sharedLinkFile("/docs/report.txt", "https://example.com/report"), + }, false), nil + }, + }) + + var stdout bytes.Buffer + cmd := &cobra.Command{} + cmd.SetOut(&stdout) + setShareLinkOutputJSON(t, cmd) + + if err := shareListLinks(cmd, []string{"/docs/report.txt"}); err != nil { + t.Fatalf("shareListLinks error: %v", err) + } + + got := decodeShareLinkOperationOutputWithWarnings[shareLinkListInput, shareLinkJSONMetadata](t, stdout.Bytes()) + if len(got.Warnings) != 1 { + t.Fatalf("warnings = %+v, want one deprecation warning", got.Warnings) + } + warning := got.Warnings[0] + if warning.Code != jsonWarningCodeDeprecatedCommand { + t.Fatalf("warning code = %q, want %q", warning.Code, jsonWarningCodeDeprecatedCommand) + } + if !strings.Contains(warning.Message, "share-link list") { + t.Fatalf("warning message = %q, want share-link list", warning.Message) + } + if len(got.Results) != 1 || got.Results[0].Result.URL != "https://example.com/report" { + t.Fatalf("results = %#v, want listed shared link", got.Results) + } +} + +func TestDeprecatedShareListLinkJSONKeepsDeprecationTextOffStdout(t *testing.T) { + stubSharedLinkClient(t, &mockSharedLinkClient{ + listSharedLinksFn: func(arg *sharing.ListSharedLinksArg) (*sharing.ListSharedLinksResult, error) { + return sharing.NewListSharedLinksResult([]sharing.IsSharedLinkMetadata{ + sharedLinkFile("/docs/report.txt", "https://example.com/report"), + }, false), nil + }, + }) + + var stdout bytes.Buffer + var stderr bytes.Buffer + cmd := &cobra.Command{} + cmd.SetOut(&stdout) + cmd.SetErr(&stderr) + setShareLinkOutputJSON(t, cmd) + + if err := shareListLinksCmd.RunE(cmd, []string{"/docs/report.txt"}); err != nil { + t.Fatalf("share list link error: %v", err) + } + + if strings.Contains(stdout.String(), `Command "link" is deprecated`) { + t.Fatalf("stdout = %q, want JSON only", stdout.String()) + } + got := decodeShareLinkOperationOutputWithWarnings[shareLinkListInput, shareLinkJSONMetadata](t, stdout.Bytes()) + if len(got.Warnings) != 1 || got.Warnings[0].Code != jsonWarningCodeDeprecatedCommand { + t.Fatalf("warnings = %+v, want deprecation warning", got.Warnings) + } + if stderr.String() != "" { + t.Fatalf("stderr = %q, RunE should not print Cobra deprecation text directly", stderr.String()) + } +} + func TestShareLinkInfoJSONOutputsPermissions(t *testing.T) { permissions := sharing.NewLinkPermissions(true, nil, true, true, true, true, true, false, false) permissions.ResolvedVisibility = &sharing.ResolvedVisibility{Tagged: dropbox.Tagged{Tag: sharing.ResolvedVisibilityPublic}} @@ -323,15 +389,21 @@ func decodeJSONOutput(t *testing.T, data []byte, out any) { } func decodeShareLinkOperationOutput[I, R any](t *testing.T, data []byte) shareLinkOperationOutputForTest[I, R] { + t.Helper() + got := decodeShareLinkOperationOutputWithWarnings[I, R](t, data) + if len(got.Warnings) != 0 { + t.Fatalf("warnings = %+v, want empty", got.Warnings) + } + return got +} + +func decodeShareLinkOperationOutputWithWarnings[I, R any](t *testing.T, data []byte) shareLinkOperationOutputForTest[I, R] { t.Helper() var got shareLinkOperationOutputForTest[I, R] decodeJSONOutput(t, data, &got) if got.Warnings == nil { t.Fatalf("warnings = nil, want empty array") } - if len(got.Warnings) != 0 { - t.Fatalf("warnings = %+v, want empty", got.Warnings) - } return got } diff --git a/cmd/share_link_password.go b/cmd/share_link_password.go index 5a6dd5e..32a86b0 100644 --- a/cmd/share_link_password.go +++ b/cmd/share_link_password.go @@ -16,7 +16,6 @@ package cmd import ( "bufio" - "errors" "fmt" "io" "os" @@ -66,7 +65,7 @@ func sharedLinkPasswordFromFlags(cmd *cobra.Command) (sharedLinkPasswordOptions, return sharedLinkPasswordOptions{}, nil } if sourceCount > 1 { - return sharedLinkPasswordOptions{}, errors.New("use only one of `--password`, `--password-prompt`, or `--password-file`") + return sharedLinkPasswordOptions{}, invalidArgumentsError("use only one of `--password`, `--password-prompt`, or `--password-file`") } var password string @@ -82,7 +81,7 @@ func sharedLinkPasswordFromFlags(cmd *cobra.Command) (sharedLinkPasswordOptions, return sharedLinkPasswordOptions{}, err } if password == "" { - return sharedLinkPasswordOptions{}, errors.New("shared link password cannot be empty") + return sharedLinkPasswordOptions{}, invalidArgumentsError("shared link password cannot be empty") } return sharedLinkPasswordOptions{ diff --git a/cmd/share_link_revoke.go b/cmd/share_link_revoke.go index 867c2ef..65b0994 100644 --- a/cmd/share_link_revoke.go +++ b/cmd/share_link_revoke.go @@ -51,12 +51,12 @@ func shareLinkRevoke(cmd *cobra.Command, args []string) error { } if len(args) != 1 { - return errors.New("`share-link revoke` requires a `url` argument") + return invalidArgumentsError("`share-link revoke` requires a `url` argument") } url := args[0] if url == "" { - return errors.New("`share-link revoke` requires a non-empty URL") + return invalidArgumentsError("`share-link revoke` requires a non-empty URL") } dbx := newSharedLinkClient(config) @@ -92,7 +92,7 @@ func parseShareLinkRevokeOptions(cmd *cobra.Command, args []string) (shareLinkRe return opts, nil } if len(args) != 0 { - return opts, errors.New("`--path` cannot be used with a shared link URL") + return opts, invalidArgumentsError("`--path` cannot be used with a shared link URL") } pathArg, err := localStringFlag(cmd, "path") @@ -100,7 +100,7 @@ func parseShareLinkRevokeOptions(cmd *cobra.Command, args []string) (shareLinkRe return opts, err } if pathArg == "" { - return opts, errors.New("`--path` requires a non-empty path") + return opts, invalidArgumentsError("`--path` requires a non-empty path") } path, err := validatePath(pathArg) @@ -108,7 +108,7 @@ func parseShareLinkRevokeOptions(cmd *cobra.Command, args []string) (shareLinkRe return opts, err } if path == "" { - return opts, errors.New("cannot revoke shared links for Dropbox root") + return opts, invalidArgumentsError("cannot revoke shared links for Dropbox root") } opts.path = path diff --git a/cmd/share_link_update.go b/cmd/share_link_update.go index bad9399..b71e3af 100644 --- a/cmd/share_link_update.go +++ b/cmd/share_link_update.go @@ -16,7 +16,6 @@ package cmd import ( "errors" - "fmt" "time" "github.com/dropbox/dbxcli/internal/output" @@ -47,12 +46,12 @@ type shareLinkUpdateInput struct { func shareLinkUpdate(cmd *cobra.Command, args []string) error { if len(args) != 1 { - return errors.New("`share-link update` requires a `url` argument") + return invalidArgumentsError("`share-link update` requires a `url` argument") } url := args[0] if url == "" { - return errors.New("`share-link update` requires a non-empty URL") + return invalidArgumentsError("`share-link update` requires a non-empty URL") } opts, err := parseShareLinkUpdateOptions(cmd) @@ -171,16 +170,16 @@ func parseShareLinkUpdateOptions(cmd *cobra.Command) (shareLinkUpdateOptions, er } if expiresChanged && removeExpiration { - return shareLinkUpdateOptions{}, errors.New("`--expires` and `--remove-expiration` cannot be used together") + return shareLinkUpdateOptions{}, invalidArgumentsError("`--expires` and `--remove-expiration` cannot be used together") } if allowDownload && disallowDownload { - return shareLinkUpdateOptions{}, errors.New("`--allow-download` and `--disallow-download` cannot be used together") + return shareLinkUpdateOptions{}, invalidArgumentsError("`--allow-download` and `--disallow-download` cannot be used together") } if password.set && removePassword { - return shareLinkUpdateOptions{}, errors.New("password-setting flags and `--remove-password` cannot be used together") + return shareLinkUpdateOptions{}, invalidArgumentsError("password-setting flags and `--remove-password` cannot be used together") } if !expiresChanged && !removeExpiration && !allowDownload && !disallowDownload && !audienceChanged && !password.set && !removePassword { - return shareLinkUpdateOptions{}, errors.New("at least one shared link setting flag is required") + return shareLinkUpdateOptions{}, invalidArgumentsError("at least one shared link setting flag is required") } var expires *time.Time @@ -191,7 +190,7 @@ func parseShareLinkUpdateOptions(cmd *cobra.Command) (shareLinkUpdateOptions, er } parsed, err := time.Parse(time.RFC3339, value) if err != nil { - return shareLinkUpdateOptions{}, fmt.Errorf("invalid --expires %q: use RFC3339 timestamp", value) + return shareLinkUpdateOptions{}, invalidArgumentsErrorf("invalid --expires %q: use RFC3339 timestamp", value) } expires = &parsed } diff --git a/cmd/team.go b/cmd/team.go index 3eb4901..52eda6d 100644 --- a/cmd/team.go +++ b/cmd/team.go @@ -14,11 +14,7 @@ package cmd -import ( - "fmt" - - "github.com/spf13/cobra" -) +import "github.com/spf13/cobra" // teamCmd represents the team command var teamCmd = &cobra.Command{ @@ -29,7 +25,7 @@ var teamCmd = &cobra.Command{ return err } if member, _ := cmd.Flags().GetString("as-member"); member != "" { - return fmt.Errorf("Flag `as-member` is invalid for team sub-commands") + return invalidArgumentsError("Flag `as-member` is invalid for team sub-commands") } return initDbx(cmd, args) },