Merge pull request #819 from lynncyrin/required-flags-take-2

Required flags

app.go

@@ -228,6 +228,12 @@ func (a *App) Run(arguments []string) (err error) {
 		return nil
 	}
 
+	cerr := checkRequiredFlags(a.Flags, context)
+	if cerr != nil {
+		ShowAppHelp(context)
+		return cerr
+	}
+
 	if a.After != nil {
 		defer func() {
 			if afterErr := a.After(context); afterErr != nil {
@@ -352,6 +358,12 @@ func (a *App) RunAsSubcommand(ctx *Context) (err error) {
 		}
 	}
 
+	cerr := checkRequiredFlags(a.Flags, context)
+	if cerr != nil {
+		ShowSubcommandHelp(context)
+		return cerr
+	}
+
 	if a.After != nil {
 		defer func() {
 			afterErr := a.After(context)

app_test.go

@@ -877,6 +877,145 @@ func TestAppNoHelpFlag(t *testing.T) {
 	}
 }
 
+func TestRequiredFlagAppRunBehavior(t *testing.T) {
+	tdata := []struct {
+		testCase        string
+		appFlags        []Flag
+		appRunInput     []string
+		appCommands     []Command
+		expectedAnError bool
+	}{
+		// assertion: empty input, when a required flag is present, errors
+		{
+			testCase:        "error_case_empty_input_with_required_flag_on_app",
+			appRunInput:     []string{"myCLI"},
+			appFlags:        []Flag{StringFlag{Name: "requiredFlag", Required: true}},
+			expectedAnError: true,
+		},
+		{
+			testCase:    "error_case_empty_input_with_required_flag_on_command",
+			appRunInput: []string{"myCLI", "myCommand"},
+			appCommands: []Command{Command{
+				Name:  "myCommand",
+				Flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}},
+			}},
+			expectedAnError: true,
+		},
+		{
+			testCase:    "error_case_empty_input_with_required_flag_on_subcommand",
+			appRunInput: []string{"myCLI", "myCommand", "mySubCommand"},
+			appCommands: []Command{Command{
+				Name: "myCommand",
+				Subcommands: []Command{Command{
+					Name:  "mySubCommand",
+					Flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}},
+				}},
+			}},
+			expectedAnError: true,
+		},
+		// assertion: inputing --help, when a required flag is present, does not error
+		{
+			testCase:    "valid_case_help_input_with_required_flag_on_app",
+			appRunInput: []string{"myCLI", "--help"},
+			appFlags:    []Flag{StringFlag{Name: "requiredFlag", Required: true}},
+		},
+		{
+			testCase:    "valid_case_help_input_with_required_flag_on_command",
+			appRunInput: []string{"myCLI", "myCommand", "--help"},
+			appCommands: []Command{Command{
+				Name:  "myCommand",
+				Flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}},
+			}},
+		},
+		{
+			testCase:    "valid_case_help_input_with_required_flag_on_subcommand",
+			appRunInput: []string{"myCLI", "myCommand", "mySubCommand", "--help"},
+			appCommands: []Command{Command{
+				Name: "myCommand",
+				Subcommands: []Command{Command{
+					Name:  "mySubCommand",
+					Flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}},
+				}},
+			}},
+		},
+		// assertion: giving optional input, when a required flag is present, errors
+		{
+			testCase:        "error_case_optional_input_with_required_flag_on_app",
+			appRunInput:     []string{"myCLI", "--optional", "cats"},
+			appFlags:        []Flag{StringFlag{Name: "requiredFlag", Required: true}, StringFlag{Name: "optional"}},
+			expectedAnError: true,
+		},
+		{
+			testCase:    "error_case_optional_input_with_required_flag_on_command",
+			appRunInput: []string{"myCLI", "myCommand", "--optional", "cats"},
+			appCommands: []Command{Command{
+				Name:  "myCommand",
+				Flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}, StringFlag{Name: "optional"}},
+			}},
+			expectedAnError: true,
+		},
+		{
+			testCase:    "error_case_optional_input_with_required_flag_on_subcommand",
+			appRunInput: []string{"myCLI", "myCommand", "mySubCommand", "--optional", "cats"},
+			appCommands: []Command{Command{
+				Name: "myCommand",
+				Subcommands: []Command{Command{
+					Name:  "mySubCommand",
+					Flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}, StringFlag{Name: "optional"}},
+				}},
+			}},
+			expectedAnError: true,
+		},
+		// assertion: when a required flag is present, inputting that required flag does not error
+		{
+			testCase:    "valid_case_required_flag_input_on_app",
+			appRunInput: []string{"myCLI", "--requiredFlag", "cats"},
+			appFlags:    []Flag{StringFlag{Name: "requiredFlag", Required: true}},
+		},
+		{
+			testCase:    "valid_case_required_flag_input_on_command",
+			appRunInput: []string{"myCLI", "myCommand", "--requiredFlag", "cats"},
+			appCommands: []Command{Command{
+				Name:  "myCommand",
+				Flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}},
+			}},
+		},
+		{
+			testCase:    "valid_case_required_flag_input_on_subcommand",
+			appRunInput: []string{"myCLI", "myCommand", "mySubCommand", "--requiredFlag", "cats"},
+			appCommands: []Command{Command{
+				Name: "myCommand",
+				Subcommands: []Command{Command{
+					Name:  "mySubCommand",
+					Flags: []Flag{StringFlag{Name: "requiredFlag", Required: true}},
+				}},
+			}},
+		},
+	}
+	for _, test := range tdata {
+		t.Run(test.testCase, func(t *testing.T) {
+			// setup
+			app := NewApp()
+			app.Flags = test.appFlags
+			app.Commands = test.appCommands
+
+			// logic under test
+			err := app.Run(test.appRunInput)
+
+			// assertions
+			if test.expectedAnError && err == nil {
+				t.Errorf("expected an error, but there was none")
+			}
+			if _, ok := err.(requiredFlagsErr); test.expectedAnError && !ok {
+				t.Errorf("expected a requiredFlagsErr, but got: %s", err)
+			}
+			if !test.expectedAnError && err != nil {
+				t.Errorf("did not expected an error, but there was one: %s", err)
+			}
+		})
+	}
+}
+
 func TestAppHelpPrinter(t *testing.T) {
 	oldPrinter := HelpPrinter
 	defer func() {

command.go

@@ -135,6 +135,12 @@ func (c Command) Run(ctx *Context) (err error) {
 		return nil
 	}
 
+	cerr := checkRequiredFlags(c.Flags, context)
+	if cerr != nil {
+		ShowCommandHelp(context, c.Name)
+		return cerr
+	}
+
 	if c.After != nil {
 		defer func() {
 			afterErr := c.After(context)

context.go

@@ -3,6 +3,7 @@ package cli
 import (
 	"errors"
 	"flag"
+	"fmt"
 	"os"
 	"reflect"
 	"strings"
@@ -285,3 +286,43 @@ func normalizeFlags(flags []Flag, set *flag.FlagSet) error {
 	}
 	return nil
 }
+
+type requiredFlagsErr interface {
+	error
+	getMissingFlags() []string
+}
+
+type errRequiredFlags struct {
+	missingFlags []string
+}
+
+func (e *errRequiredFlags) Error() string {
+	numberOfMissingFlags := len(e.missingFlags)
+	if numberOfMissingFlags == 1 {
+		return fmt.Sprintf("Required flag %q not set", e.missingFlags[0])
+	}
+	joinedMissingFlags := strings.Join(e.missingFlags, ", ")
+	return fmt.Sprintf("Required flags %q not set", joinedMissingFlags)
+}
+
+func (e *errRequiredFlags) getMissingFlags() []string {
+	return e.missingFlags
+}
+
+func checkRequiredFlags(flags []Flag, context *Context) requiredFlagsErr {
+	var missingFlags []string
+	for _, f := range flags {
+		if rf, ok := f.(RequiredFlag); ok && rf.IsRequired() {
+			key := strings.Split(f.GetName(), ",")[0]
+			if !context.IsSet(key) {
+				missingFlags = append(missingFlags, key)
+			}
+		}
+	}
+
+	if len(missingFlags) != 0 {
+		return &errRequiredFlags{missingFlags: missingFlags}
+	}
+
+	return nil
+}

context_test.go

@@ -3,6 +3,7 @@ package cli
 import (
 	"flag"
 	"os"
+	"strings"
 	"testing"
 	"time"
 )
@@ -415,3 +416,139 @@ func TestContext_GlobalSet(t *testing.T) {
 	expect(t, c.GlobalInt("int"), 1)
 	expect(t, c.GlobalIsSet("int"), true)
 }
+
+func TestCheckRequiredFlags(t *testing.T) {
+	tdata := []struct {
+		testCase              string
+		parseInput            []string
+		envVarInput           [2]string
+		flags                 []Flag
+		expectedAnError       bool
+		expectedErrorContents []string
+	}{
+		{
+			testCase: "empty",
+		},
+		{
+			testCase: "optional",
+			flags: []Flag{
+				StringFlag{Name: "optionalFlag"},
+			},
+		},
+		{
+			testCase: "required",
+			flags: []Flag{
+				StringFlag{Name: "requiredFlag", Required: true},
+			},
+			expectedAnError:       true,
+			expectedErrorContents: []string{"requiredFlag"},
+		},
+		{
+			testCase: "required_and_present",
+			flags: []Flag{
+				StringFlag{Name: "requiredFlag", Required: true},
+			},
+			parseInput: []string{"--requiredFlag", "myinput"},
+		},
+		{
+			testCase: "required_and_present_via_env_var",
+			flags: []Flag{
+				StringFlag{Name: "requiredFlag", Required: true, EnvVar: "REQUIRED_FLAG"},
+			},
+			envVarInput: [2]string{"REQUIRED_FLAG", "true"},
+		},
+		{
+			testCase: "required_and_optional",
+			flags: []Flag{
+				StringFlag{Name: "requiredFlag", Required: true},
+				StringFlag{Name: "optionalFlag"},
+			},
+			expectedAnError: true,
+		},
+		{
+			testCase: "required_and_optional_and_optional_present",
+			flags: []Flag{
+				StringFlag{Name: "requiredFlag", Required: true},
+				StringFlag{Name: "optionalFlag"},
+			},
+			parseInput:      []string{"--optionalFlag", "myinput"},
+			expectedAnError: true,
+		},
+		{
+			testCase: "required_and_optional_and_optional_present_via_env_var",
+			flags: []Flag{
+				StringFlag{Name: "requiredFlag", Required: true},
+				StringFlag{Name: "optionalFlag", EnvVar: "OPTIONAL_FLAG"},
+			},
+			envVarInput:     [2]string{"OPTIONAL_FLAG", "true"},
+			expectedAnError: true,
+		},
+		{
+			testCase: "required_and_optional_and_required_present",
+			flags: []Flag{
+				StringFlag{Name: "requiredFlag", Required: true},
+				StringFlag{Name: "optionalFlag"},
+			},
+			parseInput: []string{"--requiredFlag", "myinput"},
+		},
+		{
+			testCase: "two_required",
+			flags: []Flag{
+				StringFlag{Name: "requiredFlagOne", Required: true},
+				StringFlag{Name: "requiredFlagTwo", Required: true},
+			},
+			expectedAnError:       true,
+			expectedErrorContents: []string{"requiredFlagOne", "requiredFlagTwo"},
+		},
+		{
+			testCase: "two_required_and_one_present",
+			flags: []Flag{
+				StringFlag{Name: "requiredFlag", Required: true},
+				StringFlag{Name: "requiredFlagTwo", Required: true},
+			},
+			parseInput:      []string{"--requiredFlag", "myinput"},
+			expectedAnError: true,
+		},
+		{
+			testCase: "two_required_and_both_present",
+			flags: []Flag{
+				StringFlag{Name: "requiredFlag", Required: true},
+				StringFlag{Name: "requiredFlagTwo", Required: true},
+			},
+			parseInput: []string{"--requiredFlag", "myinput", "--requiredFlagTwo", "myinput"},
+		},
+	}
+	for _, test := range tdata {
+		t.Run(test.testCase, func(t *testing.T) {
+			// setup
+			set := flag.NewFlagSet("test", 0)
+			for _, flags := range test.flags {
+				flags.Apply(set)
+			}
+			set.Parse(test.parseInput)
+			if test.envVarInput[0] != "" {
+				os.Clearenv()
+				os.Setenv(test.envVarInput[0], test.envVarInput[1])
+			}
+			ctx := &Context{}
+			context := NewContext(ctx.App, set, ctx)
+			context.Command.Flags = test.flags
+
+			// logic under test
+			err := checkRequiredFlags(test.flags, context)
+
+			// assertions
+			if test.expectedAnError && err == nil {
+				t.Errorf("expected an error, but there was none")
+			}
+			if !test.expectedAnError && err != nil {
+				t.Errorf("did not expected an error, but there was one: %s", err)
+			}
+			for _, errString := range test.expectedErrorContents {
+				if !strings.Contains(err.Error(), errString) {
+					t.Errorf("expected error %q to contain %q, but it didn't!", err.Error(), errString)
+				}
+			}
+		})
+	}
+}

flag.go

@@ -75,6 +75,14 @@ type Flag interface {
 	GetName() string
 }
 
+// RequiredFlag is an interface that allows us to mark flags as required
+// it allows flags required flags to be backwards compatible with the Flag interface
+type RequiredFlag interface {
+	Flag
+
+	IsRequired() bool
+}
+
 // errorableFlag is an interface that allows us to return errors during apply
 // it allows flags defined in this library to return errors in a fashion backwards compatible
 // TODO remove in v2 and modify the existing Flag interface to return errors