Merge pull request #14263 from Luap99/completion

shell completion --format: various improvements

cmd/podman/common/completion.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"os"
 	"reflect"
+	"strconv"
 	"strings"
 
 	"github.com/containers/common/libnetwork/types"
@@ -963,9 +964,22 @@ func AutocompleteNetworkFlag(cmd *cobra.Command, args []string, toComplete strin
 	return append(networks, suggestions...), dir
 }
 
+type formatSuggestion struct {
+	fieldname string
+	suffix    string
+}
+
+func convertFormatSuggestions(suggestions []formatSuggestion) []string {
+	completions := make([]string, 0, len(suggestions))
+	for _, f := range suggestions {
+		completions = append(completions, f.fieldname+f.suffix)
+	}
+	return completions
+}
+
 // AutocompleteFormat - Autocomplete json or a given struct to use for a go template.
 // The input can be nil, In this case only json will be autocompleted.
-// This function will only work for structs other types are not supported.
+// This function will only work for pointer to structs other types are not supported.
 // When "{{." is typed the field and method names of the given struct will be completed.
 // This also works recursive for nested structs.
 func AutocompleteFormat(o interface{}) func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
@@ -994,6 +1008,12 @@ func AutocompleteFormat(o interface{}) func(cmd *cobra.Command, args []string, t
 		// split this into it struct field names
 		fields := strings.Split(field[len(field)-1], ".")
 		f := reflect.ValueOf(o)
+		if f.Kind() != reflect.Ptr {
+			// We panic here to make sure that all callers pass the value by reference.
+			// If someone passes a by value then all podman commands will panic since
+			// this function is run at init time.
+			panic("AutocompleteFormat: passed value must be a pointer to a struct")
+		}
 		for i := 1; i < len(fields); i++ {
 			// last field get all names to suggest
 			if i == len(fields)-1 {
@@ -1002,61 +1022,83 @@ func AutocompleteFormat(o interface{}) func(cmd *cobra.Command, args []string, t
 				toCompArr := strings.Split(toComplete, ".")
 				toCompArr[len(toCompArr)-1] = ""
 				toComplete = strings.Join(toCompArr, ".")
-				return prefixSlice(toComplete, suggestions), cobra.ShellCompDirectiveNoSpace | cobra.ShellCompDirectiveNoFileComp
+				return prefixSlice(toComplete, convertFormatSuggestions(suggestions)), cobra.ShellCompDirectiveNoSpace | cobra.ShellCompDirectiveNoFileComp
 			}
 
-			val := getActualStructType(f)
-			if val == nil {
-				// no struct return nothing to complete
+			// first follow pointer and create element when it is nil
+			f = actualReflectValue(f)
+			switch f.Kind() {
+			case reflect.Struct:
+				for j := 0; j < f.NumField(); j++ {
+					field := f.Type().Field(j)
+					// ok this is a bit weird but when we have an embedded nil struct
+					// calling FieldByName on a name which is present on this struct will panic
+					// Therefore we have to init them (non nil ptr), https://github.com/containers/podman/issues/14223
+					if field.Anonymous && f.Field(j).Type().Kind() == reflect.Ptr {
+						f.Field(j).Set(reflect.New(f.Field(j).Type().Elem()))
+					}
+				}
+				// set the next struct field
+				f = f.FieldByName(fields[i])
+			case reflect.Map:
+				rtype := f.Type().Elem()
+				if rtype.Kind() == reflect.Ptr {
+					rtype = rtype.Elem()
+				}
+				f = reflect.New(rtype)
+			case reflect.Func:
+				if f.Type().NumOut() != 1 {
+					// unsupported type return nothing
+					return nil, cobra.ShellCompDirectiveNoFileComp
+				}
+				f = reflect.New(f.Type().Out(0))
+			default:
+				// unsupported type return nothing
 				return nil, cobra.ShellCompDirectiveNoFileComp
 			}
-			f = *val
-
-			// set the next struct field
-			f = f.FieldByName(fields[i])
 		}
 		return nil, cobra.ShellCompDirectiveNoFileComp
 	}
 }
 
-// getActualStructType take the value and check if it is a struct,
+// actualReflectValue takes the value,
 // if it is pointer it will dereference it and when it is nil,
-// it will create a new value from it to get the actual struct
-// returns nil when type is not a struct
-func getActualStructType(f reflect.Value) *reflect.Value {
+// it will create a new value from it
+func actualReflectValue(f reflect.Value) reflect.Value {
 	// follow the pointer first
 	if f.Kind() == reflect.Ptr {
 		// if the pointer is nil we create a new value from the elements type
-		// this allows us to follow nil pointers and get the actual struct fields
+		// this allows us to follow nil pointers and get the actual type
 		if f.IsNil() {
 			f = reflect.New(f.Type().Elem())
 		}
 		f = f.Elem()
 	}
-	// we only support structs
-	if f.Kind() != reflect.Struct {
-		return nil
-	}
-	return &f
+	return f
 }
 
 // getStructFields reads all struct field names and method names and returns them.
-func getStructFields(f reflect.Value, prefix string) []string {
-	var suggestions []string
+func getStructFields(f reflect.Value, prefix string) []formatSuggestion {
+	var suggestions []formatSuggestion
 	if f.IsValid() {
 		suggestions = append(suggestions, getMethodNames(f, prefix)...)
 	}
 
-	val := getActualStructType(f)
-	if val == nil {
-		// no struct return nothing to complete
+	f = actualReflectValue(f)
+	// we only support structs
+	if f.Kind() != reflect.Struct {
 		return suggestions
 	}
-	f = *val
 
+	var anonymous []formatSuggestion
 	// loop over all field names
 	for j := 0; j < f.NumField(); j++ {
 		field := f.Type().Field(j)
+		// check if struct field is not exported, templates only use exported fields
+		// PkgPath is always empty for exported fields
+		if field.PkgPath != "" {
+			continue
+		}
 		fname := field.Name
 		suffix := "}}"
 		kind := field.Type.Kind()
@@ -1065,27 +1107,63 @@ func getStructFields(f reflect.Value, prefix string) []string {
 			kind = field.Type.Elem().Kind()
 		}
 		// when we have a nested struct do not append braces instead append a dot
-		if kind == reflect.Struct {
+		if kind == reflect.Struct || kind == reflect.Map {
 			suffix = "."
 		}
 		// if field is anonymous add the child fields as well
 		if field.Anonymous {
-			suggestions = append(suggestions, getStructFields(f.Field(j), prefix)...)
-		} else if strings.HasPrefix(fname, prefix) {
+			anonymous = append(anonymous, getStructFields(f.Field(j), prefix)...)
+		}
+		if strings.HasPrefix(fname, prefix) {
 			// add field name with suffix
-			suggestions = append(suggestions, fname+suffix)
+			suggestions = append(suggestions, formatSuggestion{fieldname: fname, suffix: suffix})
+		}
+	}
+outer:
+	for _, ano := range anonymous {
+		// we should only add anonymous child fields if they are not already present.
+		for _, sug := range suggestions {
+			if ano.fieldname == sug.fieldname {
+				continue outer
+			}
 		}
+		suggestions = append(suggestions, ano)
 	}
 	return suggestions
 }
 
-func getMethodNames(f reflect.Value, prefix string) []string {
-	suggestions := make([]string, 0, f.NumMethod())
+func getMethodNames(f reflect.Value, prefix string) []formatSuggestion {
+	suggestions := make([]formatSuggestion, 0, f.NumMethod())
 	for j := 0; j < f.NumMethod(); j++ {
-		fname := f.Type().Method(j).Name
+		method := f.Type().Method(j)
+		// in a template we can only run functions with one return value
+		if method.Func.Type().NumOut() != 1 {
+			continue
+		}
+		// when we have a nested struct do not append braces instead append a dot
+		kind := method.Func.Type().Out(0).Kind()
+		suffix := "}}"
+		if kind == reflect.Struct || kind == reflect.Map {
+			suffix = "."
+		}
+		// From a template users POV it is not importent when the use a struct field or method.
+		// They only notice the difference when the function requires arguments.
+		// So lets be nice and let the user know that this method requires arguments via the help text.
+		// Note since this is actually a method on a type the first argument is always fix so we should skip it.
+		num := method.Func.Type().NumIn() - 1
+		if num > 0 {
+			// everything after tab will the completion scripts show as help when enabled
+			// overwrite the suffix because it expects the args
+			suffix = "\tThis is a function and requires " + strconv.Itoa(num) + " argument"
+			if num > 1 {
+				// add plural s
+				suffix += "s"
+			}
+		}
+		fname := method.Name
 		if strings.HasPrefix(fname, prefix) {
 			// add method name with closing braces
-			suggestions = append(suggestions, fname+"}}")
+			suggestions = append(suggestions, formatSuggestion{fieldname: fname, suffix: suffix})
 		}
 	}
 	return suggestions

cmd/podman/common/completion_test.go

@@ -14,11 +14,29 @@ type Car struct {
 		HP           *int
 		Displacement int
 	}
-	Extras map[string]string
+	Extras map[string]Extra
+	// also ensure it will work with pointers
+	Extras2 map[string]*Extra
+}
+
+type Extra struct {
+	Name1 string
+	Name2 string
 }
 
 type Anonymous struct {
 	Hello string
+	// The name should match the testStruct Name below. This is used to make
+	// sure the logic uses the actual struct fields before the embedded ones.
+	Name struct {
+		Suffix string
+		Prefix string
+	}
+}
+
+// The name should match the testStruct Age name below.
+func (a Anonymous) Age() int {
+	return 0
 }
 
 func (c Car) Type() string {
@@ -31,17 +49,31 @@ func (c *Car) Color() string {
 	return ""
 }
 
+// This is for reflect testing required.
+// nolint:unused
+func (c Car) internal() int {
+	return 0
+}
+
+func (c Car) TwoOut() (string, string) {
+	return "", ""
+}
+
+func (c Car) Struct() Car {
+	return Car{}
+}
+
 func TestAutocompleteFormat(t *testing.T) {
 	testStruct := struct {
 		Name string
 		Age  int
 		Car  *Car
 		Car2 *Car
 		*Anonymous
+		private int
 	}{}
 
 	testStruct.Car = &Car{}
-	testStruct.Car.Extras = map[string]string{"test": "1"}
 
 	tests := []struct {
 		name       string
@@ -76,17 +108,17 @@ func TestAutocompleteFormat(t *testing.T) {
 		{
 			"invalid completion",
 			"{{  ..",
-			nil,
+			[]string{},
 		},
 		{
 			"fist level struct field name",
 			"{{.",
-			[]string{"{{.Name}}", "{{.Age}}", "{{.Car.", "{{.Car2.", "{{.Hello}}"},
+			[]string{"{{.Name}}", "{{.Age}}", "{{.Car.", "{{.Car2.", "{{.Anonymous.", "{{.Hello}}"},
 		},
 		{
 			"fist level struct field name",
 			"{{ .",
-			[]string{"{{ .Name}}", "{{ .Age}}", "{{ .Car.", "{{ .Car2.", "{{ .Hello}}"},
+			[]string{"{{ .Name}}", "{{ .Age}}", "{{ .Car.", "{{ .Car2.", "{{ .Anonymous.", "{{ .Hello}}"},
 		},
 		{
 			"fist level struct field name",
@@ -96,7 +128,7 @@ func TestAutocompleteFormat(t *testing.T) {
 		{
 			"second level struct field name",
 			"{{ .Car.",
-			[]string{"{{ .Car.Color}}", "{{ .Car.Type}}", "{{ .Car.Brand}}", "{{ .Car.Stats.", "{{ .Car.Extras}}"},
+			[]string{"{{ .Car.Color}}", "{{ .Car.Struct.", "{{ .Car.Type}}", "{{ .Car.Brand}}", "{{ .Car.Stats.", "{{ .Car.Extras.", "{{ .Car.Extras2."},
 		},
 		{
 			"second level struct field name",
@@ -106,7 +138,7 @@ func TestAutocompleteFormat(t *testing.T) {
 		{
 			"second level nil struct field name",
 			"{{ .Car2.",
-			[]string{"{{ .Car2.Color}}", "{{ .Car2.Type}}", "{{ .Car2.Brand}}", "{{ .Car2.Stats.", "{{ .Car2.Extras}}"},
+			[]string{"{{ .Car2.Color}}", "{{ .Car2.Struct.", "{{ .Car2.Type}}", "{{ .Car2.Brand}}", "{{ .Car2.Stats.", "{{ .Car2.Extras.", "{{ .Car2.Extras2."},
 		},
 		{
 			"three level struct field name",
@@ -126,28 +158,44 @@ func TestAutocompleteFormat(t *testing.T) {
 		{
 			"invalid field name",
 			"{{ .Ca.B",
-			nil,
+			[]string{},
 		},
 		{
 			"map key names don't work",
 			"{{ .Car.Extras.",
-			nil,
+			[]string{},
+		},
+		{
+			"map values work",
+			"{{ .Car.Extras.somekey.",
+			[]string{"{{ .Car.Extras.somekey.Name1}}", "{{ .Car.Extras.somekey.Name2}}"},
+		},
+		{
+			"map values work with ptr",
+			"{{ .Car.Extras2.somekey.",
+			[]string{"{{ .Car.Extras2.somekey.Name1}}", "{{ .Car.Extras2.somekey.Name2}}"},
 		},
 		{
 			"two variables struct field name",
 			"{{ .Car.Brand }} {{ .Car.",
-			[]string{"{{ .Car.Brand }} {{ .Car.Color}}", "{{ .Car.Brand }} {{ .Car.Type}}", "{{ .Car.Brand }} {{ .Car.Brand}}",
-				"{{ .Car.Brand }} {{ .Car.Stats.", "{{ .Car.Brand }} {{ .Car.Extras}}"},
+			[]string{"{{ .Car.Brand }} {{ .Car.Color}}", "{{ .Car.Brand }} {{ .Car.Struct.", "{{ .Car.Brand }} {{ .Car.Type}}",
+				"{{ .Car.Brand }} {{ .Car.Brand}}", "{{ .Car.Brand }} {{ .Car.Stats.", "{{ .Car.Brand }} {{ .Car.Extras.",
+				"{{ .Car.Brand }} {{ .Car.Extras2."},
 		},
 		{
 			"only dot without variable",
 			".",
 			nil,
 		},
+		{
+			"access embedded nil struct field",
+			"{{.Hello.",
+			[]string{},
+		},
 	}
 
 	for _, test := range tests {
-		completion, directive := common.AutocompleteFormat(testStruct)(nil, nil, test.toComplete)
+		completion, directive := common.AutocompleteFormat(&testStruct)(nil, nil, test.toComplete)
 		// directive should always be greater than ShellCompDirectiveNoFileComp
 		assert.GreaterOrEqual(t, directive, cobra.ShellCompDirectiveNoFileComp, "unexpected ShellCompDirective")
 		assert.Equal(t, test.expected, completion, test.name)