From 46cd034a7f50c35f44999baf1fc5f3030c1406a7 Mon Sep 17 00:00:00 2001 From: Patrik Cyvoct Date: Mon, 20 Apr 2020 16:04:54 +0200 Subject: [PATCH 1/2] fix(core): allow required nil values Signed-off-by: Patrik Cyvoct --- internal/args/args.go | 7 +++++ internal/core/cobra_utils.go | 2 +- internal/core/validate.go | 19 +++++++----- internal/core/validate_test.go | 55 ++++++++++++++++++++++++++++++++-- 4 files changed, 72 insertions(+), 11 deletions(-) diff --git a/internal/args/args.go b/internal/args/args.go index 0e8517c8ab..ae4799e7d2 100644 --- a/internal/args/args.go +++ b/internal/args/args.go @@ -15,6 +15,13 @@ const emptySliceValue = "none" // RawArgs allows to retrieve a simple []string using UnmarshalStruct() type RawArgs []string +// ExistsArgByName checks if the given argument exists in the raw args +func (a RawArgs) ExistsArgByName(name string) bool { + argsMap := SplitRawMap(a) + _, ok := argsMap[name] + return ok +} + var ( scalarKinds = map[reflect.Kind]bool{ reflect.Int: true, diff --git a/internal/core/cobra_utils.go b/internal/core/cobra_utils.go index 1a74d754c8..8627fe4c48 100644 --- a/internal/core/cobra_utils.go +++ b/internal/core/cobra_utils.go @@ -73,7 +73,7 @@ func cobraRun(ctx context.Context, cmd *Command) func(*cobra.Command, []string) if cmd.ValidateFunc != nil { validateFunc = cmd.ValidateFunc } - err = validateFunc(cmd, cmdArgs) + err = validateFunc(cmd, cmdArgs, rawArgs) if err != nil { return err } diff --git a/internal/core/validate.go b/internal/core/validate.go index 613c459b11..e4e7ec0550 100644 --- a/internal/core/validate.go +++ b/internal/core/validate.go @@ -3,6 +3,7 @@ package core import ( "fmt" "reflect" + "strconv" "strings" "github.com/scaleway/scaleway-cli/internal/args" @@ -13,19 +14,19 @@ import ( // CommandValidateFunc validates en entire command. // Used in core.cobraRun(). -type CommandValidateFunc func(cmd *Command, cmdArgs interface{}) error +type CommandValidateFunc func(cmd *Command, cmdArgs interface{}, rawArgs args.RawArgs) error // ArgSpecValidateFunc validates one argument of a command. type ArgSpecValidateFunc func(argSpec *ArgSpec, value interface{}) error // DefaultCommandValidateFunc is the default validation function for commands. func DefaultCommandValidateFunc() CommandValidateFunc { - return func(cmd *Command, cmdArgs interface{}) error { + return func(cmd *Command, cmdArgs interface{}, rawArgs args.RawArgs) error { err := validateArgValues(cmd, cmdArgs) if err != nil { return err } - err = validateRequiredArgs(cmd, cmdArgs) + err = validateRequiredArgs(cmd, cmdArgs, rawArgs) if err != nil { return err } @@ -59,8 +60,12 @@ func validateArgValues(cmd *Command, cmdArgs interface{}) error { // validateRequiredArgs checks for missing required args with no default value. // Returns an error for the first missing required arg. // Returns nil otherwise. -func validateRequiredArgs(cmd *Command, cmdArgs interface{}) error { +func validateRequiredArgs(cmd *Command, cmdArgs interface{}, rawArgs args.RawArgs) error { for _, arg := range cmd.ArgSpecs { + if !arg.Required { + continue + } + fieldName := strcase.ToPublicGoName(arg.Name) fieldValues, err := getValuesForFieldByName(reflect.ValueOf(cmdArgs), strings.Split(fieldName, ".")) if err != nil { @@ -72,9 +77,9 @@ func validateRequiredArgs(cmd *Command, cmdArgs interface{}) error { panic(validationErr) } - for _, fieldValue := range fieldValues { - if arg.Required && (fieldValue.IsZero() || !fieldValue.IsValid()) { - return MissingRequiredArgumentError(arg.Name) + for i := range fieldValues { + if !rawArgs.ExistsArgByName(strings.Replace(arg.Name, "{index}", strconv.Itoa(i), 1)) { + return MissingRequiredArgumentError(strings.Replace(arg.Name, "{index}", strconv.Itoa(i), 1)) } } } diff --git a/internal/core/validate_test.go b/internal/core/validate_test.go index 73afc1dc99..b10ea3a23d 100644 --- a/internal/core/validate_test.go +++ b/internal/core/validate_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/alecthomas/assert" + "github.com/scaleway/scaleway-cli/internal/args" ) type Element struct { @@ -31,11 +32,12 @@ func Test_DefaultCommandValidateFunc(t *testing.T) { type TestCase struct { command *Command parsedArguments interface{} + rawArgs args.RawArgs } run := func(testCase TestCase) func(t *testing.T) { return func(t *testing.T) { - err := DefaultCommandValidateFunc()(testCase.command, testCase.parsedArguments) + err := DefaultCommandValidateFunc()(testCase.command, testCase.parsedArguments, testCase.rawArgs) assert.Equal(t, fmt.Errorf("arg validation called"), err) } } @@ -187,18 +189,19 @@ func Test_DefaultCommandRequiredFunc(t *testing.T) { type TestCase struct { command *Command parsedArguments interface{} + rawArgs args.RawArgs } runOK := func(testCase TestCase) func(t *testing.T) { return func(t *testing.T) { - err := DefaultCommandValidateFunc()(testCase.command, testCase.parsedArguments) + err := DefaultCommandValidateFunc()(testCase.command, testCase.parsedArguments, testCase.rawArgs) assert.Equal(t, nil, err) } } runErr := func(testCase TestCase, argName string) func(t *testing.T) { return func(t *testing.T) { - err := DefaultCommandValidateFunc()(testCase.command, testCase.parsedArguments) + err := DefaultCommandValidateFunc()(testCase.command, testCase.parsedArguments, testCase.rawArgs) assert.Equal(t, MissingRequiredArgumentError(argName), err) } } @@ -212,6 +215,7 @@ func Test_DefaultCommandRequiredFunc(t *testing.T) { }, }, }, + rawArgs: []string{"first-nested-element.second-nested-element=test"}, parsedArguments: &elementCustom{ Element: &Element{ Name: "nested", @@ -238,4 +242,49 @@ func Test_DefaultCommandRequiredFunc(t *testing.T) { }, }, }, "first-nested-element.second-nested-element")) + + t.Run("required-index", runOK(TestCase{ + command: &Command{ + ArgSpecs: ArgSpecs{ + { + Name: "elements-slice.{index}.id", + Required: true, + }, + }, + }, + rawArgs: []string{"elements-slice.0.id=1"}, + parsedArguments: &Element{ + ElementsSlice: []Element{ + { + ID: 0, + Name: "1", + }, + }, + }, + })) + + t.Run("fail-required-index", runErr(TestCase{ + command: &Command{ + ArgSpecs: ArgSpecs{ + { + Name: "elements-slice.{index}.id", + Required: true, + }, + }, + }, + rawArgs: []string{"elements-slice.0.id=1"}, + parsedArguments: &Element{ + ElementsSlice: []Element{ + { + ID: 0, + Name: "1", + }, + { + ID: 1, + Name: "0", + }, + }, + }, + }, "elements-slice.1.id")) + } From 64eec22c542ca34243eeaa3dba026b4fc1efeca6 Mon Sep 17 00:00:00 2001 From: Patrik Cyvoct Date: Mon, 20 Apr 2020 19:05:52 +0200 Subject: [PATCH 2/2] fix review Signed-off-by: Patrik Cyvoct --- internal/core/validate.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/core/validate.go b/internal/core/validate.go index e4e7ec0550..8500b1215d 100644 --- a/internal/core/validate.go +++ b/internal/core/validate.go @@ -60,6 +60,7 @@ func validateArgValues(cmd *Command, cmdArgs interface{}) error { // validateRequiredArgs checks for missing required args with no default value. // Returns an error for the first missing required arg. // Returns nil otherwise. +// TODO refactor this method which uses a mix of reflect and string arrays func validateRequiredArgs(cmd *Command, cmdArgs interface{}, rawArgs args.RawArgs) error { for _, arg := range cmd.ArgSpecs { if !arg.Required { @@ -77,6 +78,10 @@ func validateRequiredArgs(cmd *Command, cmdArgs interface{}, rawArgs args.RawArg panic(validationErr) } + // Either fieldsValues have a length for 1 and we check for existence in the rawArgs + // or it has multiple values and we loop through each one to get the right element in + // the corresponding rawArgs array and replace {index} by the element's index. + // TODO handle required maps for i := range fieldValues { if !rawArgs.ExistsArgByName(strings.Replace(arg.Name, "{index}", strconv.Itoa(i), 1)) { return MissingRequiredArgumentError(strings.Replace(arg.Name, "{index}", strconv.Itoa(i), 1))