From c6816427340548234658dbe04265b79d4f60f399 Mon Sep 17 00:00:00 2001 From: Seebs Date: Thu, 17 Nov 2022 11:39:34 -0600 Subject: [PATCH] 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 | 5 +---- cmd/backup.go | 5 +---- cmd/backup_tar.go | 5 +---- cmd/chksum.go | 5 +---- cmd/cli.go | 5 +---- cmd/config.go | 2 +- cmd/dax.go | 2 +- cmd/export.go | 5 +---- cmd/generate_config.go | 5 +---- cmd/import.go | 2 +- cmd/keygen.go | 5 +---- cmd/rbf.go | 17 ++++------------- cmd/restore.go | 5 +---- cmd/restore_tar.go | 5 +---- cmd/root.go | 34 ++++++++++++++++++++++++++++++++++ cmd/server.go | 5 ++++- ctl/backup.go | 13 +++++++------ ctl/backup_tar.go | 4 ++-- ctl/export.go | 7 ++++--- ctl/export_test.go | 10 +++++----- ctl/import.go | 6 +++--- ctl/import_test.go | 35 +++++++++++++++++++++++++++++------ ctl/restore.go | 4 ++-- ctl/restore_tar.go | 2 +- ctl/util.go | 8 ++++++++ 25 files changed, 116 insertions(+), 85 deletions(-) diff --git a/cmd/auth_token.go b/cmd/auth_token.go index 894c0507e..5b4ac0eb2 100644 --- a/cmd/auth_token.go +++ b/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() diff --git a/cmd/backup.go b/cmd/backup.go index 97243d9a0..275b62d2d 100644 --- a/cmd/backup.go +++ b/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() diff --git a/cmd/backup_tar.go b/cmd/backup_tar.go index e480a487c..1c53b5d14 100644 --- a/cmd/backup_tar.go +++ b/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() diff --git a/cmd/chksum.go b/cmd/chksum.go index 54a8643f9..78896027a 100644 --- a/cmd/chksum.go +++ b/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() diff --git a/cmd/cli.go b/cmd/cli.go index 88b279508..412290770 100644 --- a/cmd/cli.go +++ b/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() diff --git a/cmd/config.go b/cmd/config.go index fc3e11b24..e3167657d 100644 --- a/cmd/config.go +++ b/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())) }, } diff --git a/cmd/dax.go b/cmd/dax.go index 84b8bed2a..957db266a 100644 --- a/cmd/dax.go +++ b/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") }, diff --git a/cmd/export.go b/cmd/export.go index aae8f13d9..ec9fbdc4b 100644 --- a/cmd/export.go +++ b/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() diff --git a/cmd/generate_config.go b/cmd/generate_config.go index ce671f886..cd5400cd9 100644 --- a/cmd/generate_config.go +++ b/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 diff --git a/cmd/import.go b/cmd/import.go index fe69c1cb3..067faa97b 100644 --- a/cmd/import.go +++ b/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())) }, } diff --git a/cmd/keygen.go b/cmd/keygen.go index 8bf9166f0..a93d2e69c 100644 --- a/cmd/keygen.go +++ b/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() diff --git a/cmd/rbf.go b/cmd/rbf.go index b7c1925c4..ec4207511 100644 --- a/cmd/rbf.go +++ b/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 } diff --git a/cmd/restore.go b/cmd/restore.go index f9f8c32db..3f1f7b5ec 100644 --- a/cmd/restore.go +++ b/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") diff --git a/cmd/restore_tar.go b/cmd/restore_tar.go index 563487429..fd65f7f95 100644 --- a/cmd/restore_tar.go +++ b/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") diff --git a/cmd/root.go b/cmd/root.go index 08caf6880..8536e1733 100644 --- a/cmd/root.go +++ b/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") diff --git a/cmd/server.go b/cmd/server.go index 9b0d762c1..a0eafb745 100644 --- a/cmd/server.go +++ b/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 { diff --git a/ctl/backup.go b/ctl/backup.go index b97e98a0c..506d1452d 100644 --- a/ctl/backup.go +++ b/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 diff --git a/ctl/backup_tar.go b/ctl/backup_tar.go index 295b50127..879045dbf 100644 --- a/ctl/backup_tar.go +++ b/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 } diff --git a/ctl/export.go b/ctl/export.go index 43c988693..b665bb23b 100644 --- a/ctl/export.go +++ b/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. diff --git a/ctl/export_test.go b/ctl/export_test.go index 8dbdf08c3..6b4432caf 100644 --- a/ctl/export_test.go +++ b/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) } } diff --git a/ctl/import.go b/ctl/import.go index 3818c8f4b..62ba4605b 100644 --- a/ctl/import.go +++ b/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) diff --git a/ctl/import_test.go b/ctl/import_test.go index e80576707..e30cb7960 100644 --- a/ctl/import_test.go +++ b/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) } } diff --git a/ctl/restore.go b/ctl/restore.go index 904f71c5c..be6ee1ce1 100644 --- a/ctl/restore.go +++ b/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. diff --git a/ctl/restore_tar.go b/ctl/restore_tar.go index 7df8f4f34..6b2651e32 100644 --- a/ctl/restore_tar.go +++ b/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 == "-" diff --git a/ctl/util.go b/ctl/util.go index 4ee0762b0..62368b40e 100644 --- a/ctl/util.go +++ b/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