Merge pull request #287 from dropbox/json-coded-errors-warnings

Add coded errors, JSON warnings, and contract tests

Author: AndreyVMarkelov

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:
 

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)

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)
 

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

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,15 +196,15 @@ 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)
 
 	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) {

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) {

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)
+			}
+		})
+	}
+}

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 {

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
 }

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