Distinguish between usage errors and other errors

Cobra automatically displays usage messages, and also a gratuitous
"Error: [...]" line in some cases, when any error at all occurs
running a command. To suppress the usage message, you have to set
cmd.SilenceUsage to true. But the code that would do this doesn't
have access to it. To address this, we introduce a category of
"usage error", implemented with stdlib error wrapping (%w) and
use errors.Is to check for it. There's also utility functions
to do this checking automatically, or indeed, to handle wrapping
of the ctl.SomethingCommand and handle running it with a suitable
context and everything.

In fact, several of the places we're checking for usage errors,
we can never actually report one, but we're checking consistently
so that if we want to report usage errors, we can.

For instance, server.Start and (dax)server.Start don't ever
return usage errors, right now, but we're checking their responses
anyway.

cmd/auth_token.go

@@ -2,7 +2,6 @@
 package cmd
 
 import (
-	"context"
 	"io"
 
 	"github.com/molecula/featurebase/v3/ctl"
@@ -17,9 +16,7 @@ func newAuthTokenCommand(stdin io.Reader, stdout io.Writer, stderr io.Writer) *c
 		Long: `
 Retrieves an auth-token for use in authenticating with FeatureBase from the configured identity provider.
 `,
-		RunE: func(c *cobra.Command, args []string) error {
-			return cmd.Run(context.Background())
-		},
+		RunE: usageErrorWrapper(cmd),
 	}
 
 	flags := ccmd.Flags()

cmd/backup.go

@@ -2,7 +2,6 @@
 package cmd
 
 import (
-	"context"
 	"io"
 
 	"github.com/molecula/featurebase/v3/ctl"
@@ -17,9 +16,7 @@ func newBackupCommand(stdin io.Reader, stdout io.Writer, stderr io.Writer) *cobr
 		Long: `
 Backs up a FeatureBase server to a local, tar-formatted snapshot file.
 `,
-		RunE: func(c *cobra.Command, args []string) error {
-			return cmd.Run(context.Background())
-		},
+		RunE: usageErrorWrapper(cmd),
 	}
 
 	flags := ccmd.Flags()

cmd/backup_tar.go

@@ -2,7 +2,6 @@
 package cmd
 
 import (
-	"context"
 	"io"
 
 	"github.com/molecula/featurebase/v3/ctl"
@@ -17,9 +16,7 @@ func newBackupTarCommand(stdin io.Reader, stdout io.Writer, stderr io.Writer) *c
 		Long: `
 Backs up a FeatureBase server to a local, tar-formatted snapshot file.
 `,
-		RunE: func(c *cobra.Command, args []string) error {
-			return cmd.Run(context.Background())
-		},
+		RunE: usageErrorWrapper(cmd),
 	}
 
 	flags := ccmd.Flags()

cmd/chksum.go

@@ -2,7 +2,6 @@
 package cmd
 
 import (
-	"context"
 	"io"
 
 	"github.com/molecula/featurebase/v3/ctl"
@@ -18,9 +17,7 @@ func newChkSumCommand(stdin io.Reader, stdout io.Writer, stderr io.Writer) *cobr
 			Generates a digital signature of all the data associated with a provided FeatureBase server
 			WARNING: could be slow if high cardinality fields exist
 `,
-		RunE: func(c *cobra.Command, args []string) error {
-			return cmd.Run(context.Background())
-		},
+		RunE: usageErrorWrapper(cmd),
 	}
 
 	flags := ccmd.Flags()

cmd/cli.go

@@ -2,7 +2,6 @@
 package cmd
 
 import (
-	"context"
 	"io"
 
 	"github.com/molecula/featurebase/v3/ctl"
@@ -18,9 +17,7 @@ func newCLICommand(stdin io.Reader, stdout, stderr io.Writer) *cobra.Command {
 		Use:   "cli",
 		Short: "Query FB with SQL3 from the command line",
 		Long:  ``,
-		RunE: func(cmd *cobra.Command, args []string) error {
-			return cli.Run(context.Background())
-		},
+		RunE:  usageErrorWrapper(cli),
 	}
 
 	flags := cliCmd.Flags()

cmd/config.go

@@ -23,7 +23,7 @@ func newConfigCommand(stdin io.Reader, stdout, stderr io.Writer) *cobra.Command
 
 		RunE: func(cmd *cobra.Command, args []string) error {
 			conf.Config = Server.Config
-			return conf.Run(context.Background())
+			return considerUsageError(cmd, conf.Run(context.Background()))
 		},
 	}
 

cmd/dax.go

@@ -19,7 +19,7 @@ func newDAXCommand(stdin io.Reader, stdout, stderr io.Writer) *cobra.Command {
 		Long:  ``,
 		RunE: func(cmd *cobra.Command, args []string) error {
 			if err := server.Start(); err != nil {
-				return errors.Wrap(err, "running server")
+				return considerUsageError(cmd, errors.Wrap(err, "running server"))
 			}
 			return errors.Wrap(server.Wait(), "waiting on Server")
 		},

cmd/export.go

@@ -2,7 +2,6 @@
 package cmd
 
 import (
-	"context"
 	"io"
 
 	"github.com/spf13/cobra"
@@ -27,9 +26,7 @@ The format of the CSV file is:
 
 The file does not contain any headers.
 `,
-		RunE: func(cmd *cobra.Command, args []string) error {
-			return Exporter.Run(context.Background())
-		},
+		RunE: usageErrorWrapper(Exporter),
 	}
 	flags := exportCmd.Flags()
 

cmd/generate_config.go

@@ -2,7 +2,6 @@
 package cmd
 
 import (
-	"context"
 	"io"
 
 	"github.com/spf13/cobra"
@@ -19,9 +18,7 @@ func newGenerateConfigCommand(stdin io.Reader, stdout io.Writer, stderr io.Write
 		Short: "Print the default configuration.",
 		Long: `generate-config prints the default configuration to stdout
 `,
-		RunE: func(cmd *cobra.Command, args []string) error {
-			return generateConf.Run(context.Background())
-		},
+		RunE: usageErrorWrapper(generateConf),
 	}
 
 	return confCmd

cmd/import.go

@@ -60,7 +60,7 @@ omitted. If it is present then its format should be YYYY-MM-DDTHH:MM.
 `,
 		RunE: func(cmd *cobra.Command, args []string) error {
 			Importer.Paths = args
-			return Importer.Run(context.Background())
+			return considerUsageError(cmd, Importer.Run(context.Background()))
 		},
 	}
 

cmd/keygen.go

@@ -2,7 +2,6 @@
 package cmd
 
 import (
-	"context"
 	"io"
 
 	"github.com/molecula/featurebase/v3/ctl"
@@ -17,9 +16,7 @@ func newKeygenCommand(stdin io.Reader, stdout io.Writer, stderr io.Writer) *cobr
 		Long: `
 Generate secret key to configure FeatureBase for Authentication.
 `,
-		RunE: func(c *cobra.Command, args []string) error {
-			return cmd.Run(context.Background())
-		},
+		RunE: usageErrorWrapper(cmd),
 	}
 
 	flags := ccmd.Flags()

cmd/rbf.go

@@ -2,7 +2,6 @@
 package cmd
 
 import (
-	"context"
 	"errors"
 	"fmt"
 	"io"
@@ -44,9 +43,7 @@ Executes a consistency check on an RBF data directory.
 			c.Path = args[0]
 			return nil
 		},
-		RunE: func(cmd *cobra.Command, args []string) error {
-			return c.Run(context.Background())
-		},
+		RunE: usageErrorWrapper(c),
 	}
 	return cmd
 }
@@ -78,9 +75,7 @@ Dumps the raw hex data for one or more RBF pages.
 
 			return nil
 		},
-		RunE: func(cmd *cobra.Command, args []string) error {
-			return c.Run(context.Background())
-		},
+		RunE: usageErrorWrapper(c),
 	}
 	return cmd
 }
@@ -102,9 +97,7 @@ Prints a line for every page in the database with its type/status.
 			c.Path = args[0]
 			return nil
 		},
-		RunE: func(cmd *cobra.Command, args []string) error {
-			return c.Run(context.Background())
-		},
+		RunE: usageErrorWrapper(c),
 	}
 
 	flags := cmd.Flags()
@@ -139,9 +132,7 @@ Prints the header & cell data for one or more pages.
 
 			return nil
 		},
-		RunE: func(cmd *cobra.Command, args []string) error {
-			return c.Run(context.Background())
-		},
+		RunE: usageErrorWrapper(c),
 	}
 	return cmd
 }

cmd/restore.go

@@ -2,7 +2,6 @@
 package cmd
 
 import (
-	"context"
 	"io"
 
 	"github.com/molecula/featurebase/v3/ctl"
@@ -17,9 +16,7 @@ func newRestoreCommand(stdin io.Reader, stdout, stderr io.Writer) *cobra.Command
 		Long: `
 The Restore command will take a backup archive and restore it to a new, clean cluster.
 `,
-		RunE: func(c *cobra.Command, args []string) error {
-			return cmd.Run(context.Background())
-		},
+		RunE: usageErrorWrapper(cmd),
 	}
 	flags := restoreCmd.Flags()
 	flags.StringVarP(&cmd.Path, "source", "s", "", "backup file; specify '-' to restore from stdin tar stream")

cmd/restore_tar.go

@@ -2,7 +2,6 @@
 package cmd
 
 import (
-	"context"
 	"io"
 
 	"github.com/molecula/featurebase/v3/ctl"
@@ -17,9 +16,7 @@ func newRestoreTarCommand(stdin io.Reader, stdout, stderr io.Writer) *cobra.Comm
 		Long: `
 The Restore command will take a tar-formatted backup archive and restore it to a new, clean cluster.
 `,
-		RunE: func(c *cobra.Command, args []string) error {
-			return cmd.Run(context.Background())
-		},
+		RunE: usageErrorWrapper(cmd),
 	}
 	flags := restoreCmd.Flags()
 	flags.StringVarP(&cmd.Path, "source", "s", "", "backup file; specify '-' to restore from stdin tar stream")

cmd/root.go

@@ -2,16 +2,49 @@
 package cmd
 
 import (
+	"context"
+	"errors"
 	"fmt"
 	"io"
 	"strings"
 
 	pilosa "github.com/molecula/featurebase/v3"
+	"github.com/molecula/featurebase/v3/ctl"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 	"github.com/spf13/viper"
 )
 
+// runner represents a thing, like any of the NewFooCommand we produce
+// in ctl/*.go, which has a Run(context) error method, which is used
+// to implement a command. We use this so we can just specify the
+// object, rather than its run method, in calling usageErrorWrapper.
+type runner interface {
+	Run(context.Context) error
+}
+
+// usageErrorWrapper takes a thing with a Run(context) error, and produces
+// a func(*cobra.Command, []string) error from it which will run that
+// command, and then set Cobra's SilenceUsage flag unless the returned
+// error errors.Is() a ctl.UsageError.
+func usageErrorWrapper(inner runner) func(*cobra.Command, []string) error {
+	return func(c *cobra.Command, args []string) error {
+		return considerUsageError(c, inner.Run(context.Background()))
+	}
+}
+
+// considerUsageError sets a command to silence usage errors if
+// the given error is not a ctl.UsageError, then returns the
+// unmodified error. It's here to let us write one-liner Run
+// wrappers.
+func considerUsageError(cmd *cobra.Command, err error) error {
+	cmd.SilenceErrors = true
+	if !errors.Is(err, ctl.UsageError) {
+		cmd.SilenceUsage = true
+	}
+	return err
+}
+
 func NewRootCommand(stdin io.Reader, stdout, stderr io.Writer) *cobra.Command {
 	rc := &cobra.Command{
 		Use: "featurebase",
@@ -48,6 +81,7 @@ at https://docs.molecula.cloud/.
 
 			return nil
 		},
+		SilenceErrors: true,
 	}
 	rc.PersistentFlags().Bool("dry-run", false, "stop before executing")
 	_ = rc.PersistentFlags().MarkHidden("dry-run")

cmd/server.go

@@ -58,8 +58,11 @@ on the configured port.`,
 		RunE: func(cmd *cobra.Command, args []string) error {
 			// Start & run the server.
 			if err := Server.Start(); err != nil {
-				return errors.Wrap(err, "running server")
+				return considerUsageError(cmd, errors.Wrap(err, "running server"))
 			}
+			// anything past here is definitely not a usage error
+			cmd.SilenceErrors = true
+			cmd.SilenceUsage = true
 			if Server.Config.DataDog.Enable {
 				opts := make([]profiler.ProfileType, 0)
 				if Server.Config.DataDog.CPUProfile {

ctl/backup.go

@@ -5,8 +5,10 @@ import (
 	"context"
 	"crypto/tls"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"time"
@@ -16,7 +18,6 @@ import (
 	"github.com/molecula/featurebase/v3/disco"
 	"github.com/molecula/featurebase/v3/encoding/proto"
 	"github.com/molecula/featurebase/v3/server"
-	"github.com/pkg/errors"
 	"github.com/ricochet2200/go-disk-usage/du"
 	"golang.org/x/sync/errgroup"
 )
@@ -80,19 +81,19 @@ func (cmd *BackupCommand) Run(ctx context.Context) (err error) {
 	logger := cmd.Logger()
 	close, err := startProfilingServer(cmd.Pprof, logger)
 	if err != nil {
-		return errors.Wrap(err, "starting profiling server")
+		return fmt.Errorf("starting profiling server: %w", err)
 	}
 	defer close()
 
 	// Validate arguments.
 	if cmd.OutputDir == "" {
-		return fmt.Errorf("-o flag required")
+		return fmt.Errorf("%w: -o flag required", UsageError)
 	} else if cmd.Concurrency <= 0 {
-		return fmt.Errorf("concurrency must be at least one")
+		return fmt.Errorf("%w: concurrency must be at least one", UsageError)
 	}
 	if cmd.HeaderTimeoutStr != "" {
 		if dur, err := time.ParseDuration(cmd.HeaderTimeoutStr); err != nil {
-			return fmt.Errorf("could not parse '%s' as a duration: %v", cmd.HeaderTimeoutStr, err)
+			return fmt.Errorf("%w: could not parse '%s' as a duration: %v", UsageError, cmd.HeaderTimeoutStr, err)
 		} else {
 			cmd.HeaderTimeout = dur
 		}
@@ -136,7 +137,7 @@ func (cmd *BackupCommand) Run(ctx context.Context) (err error) {
 	schema := &pilosa.Schema{Indexes: indexes}
 
 	// Ensure output directory doesn't exist; then create output directory.
-	if _, err := os.Stat(cmd.OutputDir); !os.IsNotExist(err) {
+	if _, err := os.Stat(cmd.OutputDir); !errors.Is(err, fs.ErrNotExist) {
 		return fmt.Errorf("output directory already exists")
 	} else if err := os.MkdirAll(cmd.OutputDir, 0o750); err != nil {
 		return err

ctl/backup_tar.go

@@ -76,13 +76,13 @@ func (cmd *BackupTarCommand) Run(ctx context.Context) (err error) {
 
 	// Validate arguments.
 	if cmd.OutputPath == "" {
-		return fmt.Errorf("-o flag required")
+		return fmt.Errorf("%w: -o flag required", UsageError)
 	}
 	useStdout := cmd.OutputPath == "-"
 
 	if cmd.HeaderTimeoutStr != "" {
 		if dur, err := time.ParseDuration(cmd.HeaderTimeoutStr); err != nil {
-			return fmt.Errorf("could not parse '%s' as a duration: %v", cmd.HeaderTimeoutStr, err)
+			return fmt.Errorf("%w: could not parse '%s' as a duration: %v", UsageError, cmd.HeaderTimeoutStr, err)
 		} else {
 			cmd.HeaderTimeout = dur
 		}

ctl/export.go

@@ -3,10 +3,11 @@ package ctl
 
 import (
 	"context"
+	"fmt"
 	"io"
 	"os"
 
-	"github.com/molecula/featurebase/v3"
+	pilosa "github.com/molecula/featurebase/v3"
 	"github.com/molecula/featurebase/v3/server"
 	"github.com/pkg/errors"
 )
@@ -42,9 +43,9 @@ func (cmd *ExportCommand) Run(ctx context.Context) error {
 
 	// Validate arguments.
 	if cmd.Index == "" {
-		return pilosa.ErrIndexRequired
+		return fmt.Errorf("%w: %v", UsageError, pilosa.ErrIndexRequired)
 	} else if cmd.Field == "" {
-		return pilosa.ErrFieldRequired
+		return fmt.Errorf("%w: %v", UsageError, pilosa.ErrFieldRequired)
 	}
 
 	// Use output file, if specified.

ctl/export_test.go

@@ -8,7 +8,7 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/molecula/featurebase/v3"
+	pilosa "github.com/molecula/featurebase/v3"
 	"github.com/molecula/featurebase/v3/test"
 )
 
@@ -19,14 +19,14 @@ func TestExportCommand_Validation(t *testing.T) {
 	cm := NewExportCommand(stdin, stdout, stderr)
 
 	err := cm.Run(context.Background())
-	if err != pilosa.ErrIndexRequired {
-		t.Fatalf("Command not working, expect: %s, actual: '%s'", pilosa.ErrIndexRequired, err)
+	if !errContains(err, pilosa.ErrIndexRequired) {
+		t.Fatalf("wrong error, expected %q, got: '%s'", pilosa.ErrIndexRequired, err)
 	}
 
 	cm.Index = "i"
 	err = cm.Run(context.Background())
-	if err != pilosa.ErrFieldRequired {
-		t.Fatalf("Command not working, expect: %s, actual: '%s'", pilosa.ErrFieldRequired, err)
+	if !errContains(err, pilosa.ErrFieldRequired) {
+		t.Fatalf("wrong error, expected %q, got: '%s'", pilosa.ErrFieldRequired, err)
 	}
 }
 

ctl/import.go

@@ -79,11 +79,11 @@ func (cmd *ImportCommand) Run(ctx context.Context) error {
 	// Validate arguments.
 	// Index and field are validated early before the files are parsed.
 	if cmd.Index == "" {
-		return pilosa.ErrIndexRequired
+		return fmt.Errorf("%w: %v", UsageError, pilosa.ErrIndexRequired)
 	} else if cmd.Field == "" {
-		return pilosa.ErrFieldRequired
+		return fmt.Errorf("%w: %v", UsageError, pilosa.ErrFieldRequired)
 	} else if len(cmd.Paths) == 0 {
-		return errors.New("path required")
+		return fmt.Errorf("%w: path required", UsageError)
 	}
 	// Create a client to the server.
 	client, err := commandClient(cmd)

ctl/import_test.go

@@ -6,6 +6,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -25,25 +26,47 @@ import (
 	"github.com/molecula/featurebase/v3/testhook"
 )
 
+// errContains reports whether the first error is or
+// contains the second, either via errors.Is, or by
+// containing its Error() string. a nil error contains
+// a nil error, but does not contain any non-nil error,
+// and a non-nil error does not contain a nil error.
+func errContains(err error, expected error) bool {
+	if err == nil {
+		if expected == nil {
+			return true
+		}
+		return false
+	}
+	if expected == nil {
+		return false
+	}
+	if errors.Is(err, expected) {
+		return true
+	}
+	e1, e2 := err.Error(), expected.Error()
+	return strings.Contains(e1, e2)
+}
 func TestImportCommand_Validation(t *testing.T) {
 	buf := bytes.Buffer{}
 	stdin, stdout, stderr := GetIO(buf)
 	cm := NewImportCommand(stdin, stdout, stderr)
 	err := cm.Run(context.Background())
-	if err != pilosa.ErrIndexRequired {
-		t.Fatalf("Command not working, expect: %s, actual: '%s'", pilosa.ErrIndexRequired, err)
+	if !errContains(err, pilosa.ErrIndexRequired) {
+		t.Fatalf("wrong error: expected %q, got: '%v'", pilosa.ErrIndexRequired, err)
 	}
 
 	cm.Index = "i"
 	err = cm.Run(context.Background())
-	if err != pilosa.ErrFieldRequired {
-		t.Fatalf("Command not working, expect: %s, actual: '%s'", pilosa.ErrFieldRequired, err)
+	if !errContains(err, pilosa.ErrFieldRequired) {
+		t.Fatalf("wrong error: expected %q, got: '%v'", pilosa.ErrFieldRequired, err)
 	}
 
 	cm.Field = "f"
 	err = cm.Run(context.Background())
-	if err.Error() != "path required" {
-		t.Fatalf("Command not working, expect: %s, actual: '%s'", "path required", err)
+	pathRequired := errors.New("path required")
+	if !errContains(err, pathRequired) {
+		t.Fatalf("wrong error: expected %q, got: '%v'", pathRequired, err)
 	}
 }
 

ctl/restore.go

@@ -77,9 +77,9 @@ func (cmd *RestoreCommand) Run(ctx context.Context) (err error) {
 
 	// Validate arguments.
 	if cmd.Path == "" {
-		return fmt.Errorf("-s flag required")
+		return fmt.Errorf("%w: -s flag required", UsageError)
 	} else if cmd.Concurrency <= 0 {
-		return fmt.Errorf("concurrency must be at least one")
+		return fmt.Errorf("%w: concurrency must be at least one", UsageError)
 	}
 
 	// Parse TLS configuration for node-specific clients.

ctl/restore_tar.go

@@ -69,7 +69,7 @@ func (cmd *RestoreTarCommand) Run(ctx context.Context) (err error) {
 
 	// Validate arguments.
 	if cmd.Path == "" {
-		return fmt.Errorf("-s flag required")
+		return fmt.Errorf("%w: -s flag required", UsageError)
 	}
 	useStdin := cmd.Path == "-"
 

ctl/util.go

@@ -13,6 +13,14 @@ import (
 	"github.com/pkg/errors"
 )
 
+type ctlUsageError struct{}
+
+func (c ctlUsageError) Error() string {
+	return "usage error"
+}
+
+var UsageError ctlUsageError
+
 // startProfilingServer starts a server which handles /debug/pprof and
 // /debug/fgprof for use in utilities we might want to profile but
 // wouldn't otherwise be running an http server. Caller should call