diff --git a/internal/args/args.go b/internal/args/args.go index 595e3ac15f..abd818bc1c 100644 --- a/internal/args/args.go +++ b/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 diff --git a/internal/args/args_test.go b/internal/args/args_test.go index c05f734550..1ff49f3520 100644 --- a/internal/args/args_test.go +++ b/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 +} diff --git a/internal/args/errors.go b/internal/args/errors.go index 5bb245a487..b824c0a237 100644 --- a/internal/args/errors.go +++ b/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" diff --git a/internal/args/marshal.go b/internal/args/marshal.go index 42c3b6c2f9..e70e460f53 100644 --- a/internal/args/marshal.go +++ b/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++ { diff --git a/internal/args/marshal_test.go b/internal/args/marshal_test.go index eb589ebe09..12c416983c 100644 --- a/internal/args/marshal_test.go +++ b/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,6 +96,9 @@ 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", @@ -101,6 +106,17 @@ func TestMarshal(t *testing.T) { }, })) + 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", diff --git a/internal/args/unmarshal.go b/internal/args/unmarshal.go index 7dec290a38..a4ea6fd3fd 100644 --- a/internal/args/unmarshal.go +++ b/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]} diff --git a/internal/args/unmarshal_test.go b/internal/args/unmarshal_test.go index e34e56cacc..3a4d1a45b4 100644 --- a/internal/args/unmarshal_test.go +++ b/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