feat(args): add support for empty slice un/marshahaling (#692)

Author: jerome-quere

internal/args/args.go

@@ -9,6 +9,8 @@ import (
 // validArgNameRegex regex to check that args words are lower-case or digit starting and ending with a letter.
 var validArgNameRegex = regexp.MustCompile(`^([a-z][a-z0-9-]*)(\.[a-z0-9-]*)*$`)
 
+const emptySliceValue = "none"
+
 // RawArgs allows to retrieve a simple []string using UnmarshalStruct()
 type RawArgs []string
 

internal/args/args_test.go

@@ -25,6 +25,7 @@ type Basic struct {
 type Slice struct {
 	Strings    []string
 	StringsPtr []*string
+	SlicePtr   *[]string
 	Basics     []Basic
 }
 
@@ -129,3 +130,8 @@ func unmarshalHeight(value string, dest interface{}) error {
 	_, err := fmt.Sscanf(value, "%dcm", h)
 	return err
 }
+
+type SamePrefixArgName struct {
+	IP   string
+	IPv6 string
+}

internal/args/errors.go

@@ -161,6 +161,17 @@ func (e *CannotUnmarshalError) Error() string {
 	return fmt.Sprintf("%T is not unmarshalable: %s", e.Dest, e.Err)
 }
 
+// ConflictArgError is return when two args that are in conflict with each other are used together.
+// e.g cluster=prod cluster.name=test are conflicting args
+type ConflictArgError struct {
+	ArgName1 string
+	ArgName2 string
+}
+
+func (e *ConflictArgError) Error() string {
+	return fmt.Sprintf("arguments '%s' and '%s' cannot be used simultaneously", e.ArgName1, e.ArgName2)
+}
+
 // missingIndices returns a string of all the missing indices between index and length.
 // e.g.: missingIndices(index=5, length=0) should return "0,1,2,3"
 // e.g.: missingIndices(index=5, length=2) should return "2,3"

internal/args/marshal.go

@@ -118,11 +118,21 @@ func marshal(src reflect.Value, keys []string) (args []string, err error) {
 		if src.IsNil() {
 			return nil, nil
 		}
+
+		// When:
+		// - dest is a pointer to a slice
+		// - The slice is empty
+		// we return slice=none
+		if src.Elem().Kind() == reflect.Slice && src.Elem().Len() == 0 {
+			return append(args, marshalKeyValue(keys, emptySliceValue)), nil
+		}
+
 		// If type is a pointer we Marshal pointer.Elem()
 		return marshal(src.Elem(), keys)
 
 	case reflect.Slice:
 		// If type is a slice:
+
 		// We loop through all items and marshal them with key = key.0, key.1, ....
 		args := []string(nil)
 		for i := 0; i < src.Len(); i++ {

internal/args/marshal_test.go

@@ -15,6 +15,7 @@ func TestMarshal(t *testing.T) {
 	}
 
 	stringPtr := "test"
+	slicePtr := []string{"0", "1", "2"}
 
 	run := func(testCase TestCase) func(t *testing.T) {
 		return func(t *testing.T) {
@@ -76,6 +77,7 @@ func TestMarshal(t *testing.T) {
 		data: &Slice{
 			Strings:    []string{"1", "2", "", "3"},
 			StringsPtr: []*string{&stringPtr, &stringPtr},
+			SlicePtr:   &slicePtr,
 			Basics: []Basic{
 				{
 					String: "test",
@@ -94,13 +96,27 @@ func TestMarshal(t *testing.T) {
 			"strings.3=3",
 			"strings-ptr.0=test",
 			"strings-ptr.1=test",
+			"slice-ptr.0=0",
+			"slice-ptr.1=1",
+			"slice-ptr.2=2",
 			"basics.0.string=test",
 			"basics.0.int=42",
 			"basics.2.string=test",
 			"basics.2.int=42",
 		},
 	}))
 
+	t.Run("empty-slice", run(TestCase{
+		data: &Slice{
+			Strings:    []string{},
+			SlicePtr:   scw.StringsPtr(nil),
+			StringsPtr: []*string{},
+		},
+		expected: []string{
+			"slice-ptr=none",
+		},
+	}))
+
 	t.Run("well-known-types", run(TestCase{
 		data: &WellKnownTypes{
 			Size: 20 * scw.GB,
@@ -247,11 +263,16 @@ func TestMarshalValue(t *testing.T) {
 		expected: "",
 	}))
 
-	t.Run("typed-nil", run(TestCase{
+	t.Run("nil-slice", run(TestCase{
 		data:     []string(nil),
 		expected: "",
 	}))
 
+	t.Run("empty-slice", run(TestCase{
+		data:     []string{},
+		expected: "none",
+	}))
+
 	t.Run("custom-func", run(TestCase{
 		data:     height(42),
 		expected: "42cm",

internal/args/unmarshal.go

@@ -75,6 +75,7 @@ func UnmarshalStruct(args []string, data interface{}) error {
 	// Loop through all arguments
 	for _, kv := range argsSlice {
 		argName, argValue := kv[0], kv[1]
+		argNameWords := strings.Split(argName, ".")
 
 		// Make sure argument name is correct.
 		// We enforce this check to avoid not well formatted argument name to work by "accident"
@@ -102,10 +103,30 @@ func UnmarshalStruct(args []string, data interface{}) error {
 				Err:      &DuplicateArgError{},
 			}
 		}
+
+		// We check that we did not already handle an argument value set on a child or a parent
+		// Example `cluster=premium cluster.volume.size=12` cannot be valid as both args are in conflict.
+		// Example `cluster.volume.size=12 cluster=premium` should also be invalid.
+		for processedArgName := range processedArgNames {
+			// We put the longest argName in long and the shortest in short.
+			short, long := argName, processedArgName
+			if len(long) < len(short) {
+				short, long = long, short
+			}
+
+			// We check if the longest starts with short+"."
+			// If it does this mean we have a conflict.
+			if strings.HasPrefix(long, short+".") {
+				return &ConflictArgError{
+					ArgName1: processedArgName,
+					ArgName2: argName,
+				}
+			}
+		}
 		processedArgNames[argName] = true
 
 		// Set will recursively find the correct field to set.
-		err := set(dest, strings.Split(argName, "."), argValue)
+		err := set(dest, argNameWords, argValue)
 		if err != nil {
 			return &UnmarshalArgError{
 				ArgName:  argName,
@@ -172,19 +193,27 @@ func set(dest reflect.Value, argNameWords []string, value string) error {
 			dest.Set(reflect.New(dest.Type().Elem()))
 		}
 
+		// When:
+		// - dest is a pointer to a slice
+		// - there is no more argNameWords left
+		// - value == none
+		// we let the slice empty and return
+		if dest.Elem().Kind() == reflect.Slice && len(argNameWords) == 0 && value == emptySliceValue {
+			return nil
+		}
+
 		// Call set with the pointer.Elem()
 		return set(dest.Elem(), argNameWords, value)
 
 	case reflect.Slice:
 		// If type is a slice:
-		// We check if argNameWords[0] is an number to handle cases like keys.0.value=12
 
 		// We cannot handle slice without an index notation.
 		if len(argNameWords) == 0 {
 			return &MissingIndexOnArrayError{}
 		}
 
-		// Make sure index is a positive integer.
+		// We check if argNameWords[0] is a positive integer to handle cases like keys.0.value=12
 		index, err := strconv.ParseUint(argNameWords[0], 10, 64)
 		if err != nil {
 			return &InvalidIndexError{Index: argNameWords[0]}

internal/args/unmarshal_test.go

@@ -17,6 +17,7 @@ func TestUnmarshalStruct(t *testing.T) {
 	}
 
 	stringPtr := "test"
+	slicePtr := []string{"0", "1", "2"}
 
 	run := func(testCase TestCase) func(t *testing.T) {
 		return func(t *testing.T) {
@@ -164,6 +165,9 @@ func TestUnmarshalStruct(t *testing.T) {
 			"strings.3=test",
 			"strings-ptr.0=test",
 			"strings-ptr.1=test",
+			"slice-ptr.0=0",
+			"slice-ptr.1=1",
+			"slice-ptr.2=2",
 			"basics.0.string=test",
 			"basics.0.int=42",
 			"basics.1.string=test",
@@ -172,6 +176,7 @@ func TestUnmarshalStruct(t *testing.T) {
 		expected: &Slice{
 			Strings:    []string{"1", "2", "3", "test"},
 			StringsPtr: []*string{&stringPtr, &stringPtr},
+			SlicePtr:   &slicePtr,
 			Basics: []Basic{
 				{
 					String: "test",
@@ -185,6 +190,43 @@ func TestUnmarshalStruct(t *testing.T) {
 		},
 	}))
 
+	t.Run("empty-slice", run(TestCase{
+		args: []string{
+			"slice-ptr=none",
+		},
+		expected: &Slice{
+			Strings:    []string(nil),
+			SlicePtr:   scw.StringsPtr(nil),
+			StringsPtr: []*string(nil),
+		},
+	}))
+
+	t.Run("none-on-non-pointer-slice", run(TestCase{
+		args: []string{
+			"strings=none",
+		},
+		error: "cannot unmarshal arg 'strings=none': missing index on the array",
+		data:  &Slice{},
+	}))
+
+	t.Run("simple-parent-child-conflict", run(TestCase{
+		args: []string{
+			"slice-ptr=none",
+			"slice-ptr.0=none",
+		},
+		error: "arguments 'slice-ptr' and 'slice-ptr.0' cannot be used simultaneously",
+		data:  &Slice{},
+	}))
+
+	t.Run("simple-child-parent-conflict", run(TestCase{
+		args: []string{
+			"slice-ptr.0=none",
+			"slice-ptr=none",
+		},
+		error: "arguments 'slice-ptr.0' and 'slice-ptr' cannot be used simultaneously",
+		data:  &Slice{},
+	}))
+
 	t.Run("well-known-types", run(TestCase{
 		args: []string{
 			"size=20gb",
@@ -377,8 +419,18 @@ func TestUnmarshalStruct(t *testing.T) {
 			},
 		},
 	}))
-}
 
+	t.Run("common-prefix-args", run(TestCase{
+		args: []string{
+			"ip=ip",
+			"ipv6=ipv6",
+		},
+		expected: &SamePrefixArgName{
+			IP:   "ip",
+			IPv6: "ipv6",
+		},
+	}))
+}
 func TestIsUmarshalableValue(t *testing.T) {
 	type TestCase struct {
 		expected bool