feat(qa): add argspec checking in cli qa (#2852)

Co-authored-by: Rémy Léone <rleone@scaleway.com>

Author: Codelax

internal/args/args.go

@@ -255,3 +255,66 @@ func GetArgType(argType reflect.Type, name string) (reflect.Type, error) {
 
 	return recursiveFunc(argType, strings.Split(name, "."))
 }
+
+var listArgTypeFieldsSkippedArguments = []string{
+	"page",
+	"page-size",
+	"per-page",
+}
+
+func listArgTypeFields(base string, argType reflect.Type) []string {
+	if argType.Kind() != reflect.Ptr {
+		// Can be a handled type like time.Time
+		// If so, use it like a scalar type
+		_, isHandled := unmarshalFuncs[argType]
+		if isHandled {
+			return []string{base}
+		}
+	}
+
+	switch argType.Kind() {
+	case reflect.Ptr:
+		return listArgTypeFields(base, argType.Elem())
+
+	case reflect.Slice:
+		return listArgTypeFields(base+"."+sliceSchema, argType.Elem())
+
+	case reflect.Map:
+		return listArgTypeFields(base+"."+mapSchema, argType.Elem())
+
+	case reflect.Struct:
+		fields := []string(nil)
+
+		for i := 0; i < argType.NumField(); i++ {
+			field := argType.Field(i)
+			fieldBase := base
+
+			// If this is an embedded struct, skip adding its name to base
+			if field.Anonymous {
+				fields = append(fields, listArgTypeFields(fieldBase, field.Type)...)
+				continue
+			}
+
+			if fieldBase == "" {
+				fieldBase = strcase.ToBashArg(field.Name)
+			} else {
+				fieldBase += "." + strcase.ToBashArg(field.Name)
+			}
+			fields = append(fields, listArgTypeFields(fieldBase, field.Type)...)
+		}
+
+		return fields
+	default:
+		for _, skippedArg := range listArgTypeFieldsSkippedArguments {
+			if base == skippedArg {
+				return []string{}
+			}
+		}
+		return []string{base}
+	}
+}
+
+// ListArgTypeFields take a go struct and return a list of name that comply with ArgSpec name notation (e.g "friends.{index}.name")
+func ListArgTypeFields(argType reflect.Type) []string {
+	return listArgTypeFields("", argType)
+}

internal/core/arg_specs.go

@@ -3,11 +3,9 @@ package core
 import (
 	"context"
 	"fmt"
-	"reflect"
 	"strings"
 
 	"github.com/scaleway/scaleway-sdk-go/scw"
-	"github.com/scaleway/scaleway-sdk-go/strcase"
 	"github.com/scaleway/scaleway-sdk-go/validation"
 )
 
@@ -121,13 +119,6 @@ func (a *ArgSpec) ConflictWith(b *ArgSpec) bool {
 		(a.OneOfGroup == b.OneOfGroup)
 }
 
-// GetArgsTypeField returns the type of the argument in the given ArgsType
-func (a *ArgSpec) GetArgsTypeField(argsType reflect.Type) (reflect.Type, error) {
-	argSpecGoName := strcase.ToPublicGoName(a.Name)
-
-	return getTypeForFieldByName(argsType, strings.Split(argSpecGoName, "."))
-}
-
 type DefaultFunc func(ctx context.Context) (value string, doc string)
 
 func ZoneArgSpec(zones ...scw.Zone) *ArgSpec {

internal/core/arg_specs_test.go

@@ -1,7 +1,6 @@
 package core
 
 import (
-	"reflect"
 	"testing"
 
 	"github.com/alecthomas/assert"
@@ -37,35 +36,3 @@ func TestOneOf(t *testing.T) {
 	assert.False(t, a.ConflictWith(c))
 	assert.False(t, e.ConflictWith(e))
 }
-
-func TestArgSpecGetArgsTypeField(t *testing.T) {
-	data := struct {
-		Field       string
-		FieldStruct struct {
-			NestedField int
-		}
-		FieldSlice []float32
-		FieldMap   map[string]bool
-	}{}
-	dataType := reflect.TypeOf(data)
-
-	fieldSpec := ArgSpec{Name: "field"}
-	typ, err := fieldSpec.GetArgsTypeField(dataType)
-	assert.Nil(t, err)
-	assert.Equal(t, reflect.TypeOf("string"), typ, "%s is not string", typ.Name())
-
-	fieldSpec = ArgSpec{Name: "field-struct.nested-field"}
-	typ, err = fieldSpec.GetArgsTypeField(dataType)
-	assert.Nil(t, err)
-	assert.Equal(t, reflect.TypeOf(int(1)), typ, "%s is not int", typ.Name())
-
-	fieldSpec = ArgSpec{Name: "field-slice.{index}"}
-	typ, err = fieldSpec.GetArgsTypeField(dataType)
-	assert.Nil(t, err)
-	assert.Equal(t, reflect.TypeOf(float32(1)), typ, "%s is not float32", typ.Name())
-
-	fieldSpec = ArgSpec{Name: "field-map.{key}"}
-	typ, err = fieldSpec.GetArgsTypeField(dataType)
-	assert.Nil(t, err)
-	assert.Equal(t, reflect.TypeOf(true), typ, "%s is not bool", typ.Name())
-}

internal/core/reflect.go

@@ -109,32 +109,3 @@ func getValuesForFieldByName(value reflect.Value, parts []string) (values []refl
 
 	return nil, fmt.Errorf("case is not handled")
 }
-
-// getTypeForFieldByName recursively search for fields in an ArgsType
-// The search is based on the name of the field.
-func getTypeForFieldByName(value reflect.Type, parts []string) (reflect.Type, error) {
-	if len(parts) == 0 {
-		return value, nil
-	}
-
-	switch value.Kind() {
-	case reflect.Ptr:
-		return getTypeForFieldByName(value.Elem(), parts)
-
-	case reflect.Slice:
-		return getTypeForFieldByName(value.Elem(), parts[1:])
-
-	case reflect.Map:
-		return getTypeForFieldByName(value.Elem(), parts[1:])
-
-	case reflect.Struct:
-		fieldName := strcase.ToPublicGoName(parts[0])
-		field, hasField := value.FieldByName(fieldName)
-		if !hasField {
-			return nil, fmt.Errorf("field %v does not exist for %v", fieldName, value.Name())
-		}
-		return getTypeForFieldByName(field.Type, parts[1:])
-	}
-
-	return nil, fmt.Errorf("type kind %s is not handled", value.Kind().String())
-}

internal/qa/argspec.go

@@ -0,0 +1,74 @@
+package qa
+
+import (
+	"fmt"
+	"reflect"
+
+	"github.com/scaleway/scaleway-cli/v2/internal/args"
+	"github.com/scaleway/scaleway-cli/v2/internal/core"
+)
+
+type ArgSpecInvalidError struct {
+	Command    *core.Command
+	argSpec    *core.ArgSpec
+	innerError error
+}
+
+func (err ArgSpecInvalidError) Error() string {
+	return fmt.Sprintf("command has invalid argspecs '%s' '%s' '%s'",
+		err.Command.GetCommandLine("scw"),
+		err.argSpec.Name,
+		err.innerError,
+	)
+}
+
+// testArgSpecInvalidError tests that all argspecs have a corresponding in their command's argstype.
+func testArgSpecInvalidError(commands *core.Commands) []error {
+	errors := []error(nil)
+
+	for _, command := range commands.GetAll() {
+		for _, arg := range command.ArgSpecs {
+			_, err := args.GetArgType(command.ArgsType, arg.Name)
+			if err != nil {
+				errors = append(errors, &ArgSpecInvalidError{Command: command, argSpec: arg, innerError: err})
+				continue
+			}
+		}
+	}
+
+	return errors
+}
+
+type ArgSpecMissingError struct {
+	Command *core.Command
+	argName string
+}
+
+func (err ArgSpecMissingError) Error() string {
+	return fmt.Sprintf("command has a missing argspec '%s' '%s'",
+		err.Command.GetCommandLine("scw"),
+		err.argName,
+	)
+}
+
+// testArgSpecInvalidError tests that all argstype fields have a corresponding argspec.
+func testArgSpecMissingError(commands *core.Commands) []error {
+	errors := []error(nil)
+
+	// Check all commands
+	for _, command := range commands.GetAll() {
+		if command.ArgsType == nil || command.ArgsType == reflect.TypeOf(args.RawArgs{}) {
+			continue
+		}
+
+		supposedArgSpecs := args.ListArgTypeFields(command.ArgsType)
+
+		for _, argSpecName := range supposedArgSpecs {
+			if command.ArgSpecs.GetByName(argSpecName) == nil {
+				errors = append(errors, &ArgSpecMissingError{Command: command, argName: argSpecName})
+			}
+		}
+	}
+
+	return errors
+}

internal/qa/qa.go

@@ -19,6 +19,8 @@ func LintCommands(commands *core.Commands) []error {
 	errors = append(errors, testDifferentLocalizationForNamespaceError(commands)...)
 	errors = append(errors, testDuplicatedCommandError(commands)...)
 	errors = append(errors, testAtLeastOneExampleIsPresentError(commands)...)
+	errors = append(errors, testArgSpecInvalidError(commands)...)
+	errors = append(errors, testArgSpecMissingError(commands)...)
 
 	errors = filterIgnore(errors)