diff --git a/pkg/config/conversion/conversions_test.go b/pkg/config/conversion/conversions_test.go index 03de9080..fb428001 100644 --- a/pkg/config/conversion/conversions_test.go +++ b/pkg/config/conversion/conversions_test.go @@ -1,4 +1,4 @@ -// SPDX-FileCopyrightText: 2023 The Crossplane Authors +// SPDX-FileCopyrightText: 2024 The Crossplane Authors // // SPDX-License-Identifier: Apache-2.0 diff --git a/pkg/config/conversion/runtime_conversion.go b/pkg/config/conversion/runtime_conversion.go index d3b22016..1d224167 100644 --- a/pkg/config/conversion/runtime_conversion.go +++ b/pkg/config/conversion/runtime_conversion.go @@ -5,6 +5,7 @@ package conversion import ( + "reflect" "slices" "sort" "strings" @@ -29,6 +30,11 @@ const ( ToSingletonList ) +const ( + errFmtMultiItemList = "singleton list, at the field path %s, must have a length of at most 1 but it has a length of %d" + errFmtNonSlice = "value at the field path %s must be []any, not %q" +) + // String returns a string representation of the conversion mode. func (m Mode) String() string { switch m { @@ -104,9 +110,12 @@ func Convert(params map[string]any, paths []string, mode Mode) (map[string]any, if v != nil { newVal = map[string]any{} s, ok := v.([]any) - if !ok || len(s) > 1 { - // if len(s) is 0, then it's not a slice - return nil, errors.Errorf("singleton list, at the field path %s, must have a length of at most 1 but it has a length of %d", e, len(s)) + if !ok { + // then it's not a slice + return nil, errors.Errorf(errFmtNonSlice, e, reflect.TypeOf(v)) + } + if len(s) > 1 { + return nil, errors.Errorf(errFmtMultiItemList, e, len(s)) } if len(s) > 0 { newVal = s[0] diff --git a/pkg/config/conversion/runtime_conversion_test.go b/pkg/config/conversion/runtime_conversion_test.go new file mode 100644 index 00000000..1af9e31b --- /dev/null +++ b/pkg/config/conversion/runtime_conversion_test.go @@ -0,0 +1,332 @@ +// SPDX-FileCopyrightText: 2024 The Crossplane Authors +// +// SPDX-License-Identifier: Apache-2.0 + +package conversion + +import ( + "reflect" + "testing" + + "github.com/crossplane/crossplane-runtime/pkg/test" + "github.com/google/go-cmp/cmp" + jsoniter "github.com/json-iterator/go" + "github.com/pkg/errors" +) + +func TestConvert(t *testing.T) { + type args struct { + params map[string]any + paths []string + mode Mode + } + type want struct { + err error + params map[string]any + } + tests := map[string]struct { + reason string + args args + want want + }{ + "NilParamsAndPaths": { + reason: "Conversion on an nil map should not fail.", + args: args{}, + }, + "EmptyPaths": { + reason: "Empty conversion on a map should be an identity function.", + args: args{ + params: map[string]any{"a": "b"}, + }, + want: want{ + params: map[string]any{"a": "b"}, + }, + }, + "SingletonListToEmbeddedObject": { + reason: "Should successfully convert a singleton list at the root level to an embedded object.", + args: args{ + params: map[string]any{ + "l": []map[string]any{ + { + "k": "v", + }, + }, + }, + paths: []string{"l"}, + mode: ToEmbeddedObject, + }, + want: want{ + params: map[string]any{ + "l": map[string]any{ + "k": "v", + }, + }, + }, + }, + "NestedSingletonListsToEmbeddedObjectsPathsInLexicalOrder": { + reason: "Should successfully convert the parent & nested singleton lists to embedded objects. Paths specified in lexical order.", + args: args{ + params: map[string]any{ + "parent": []map[string]any{ + { + "child": []map[string]any{ + { + "k": "v", + }, + }, + }, + }, + }, + paths: []string{"parent", "parent[*].child"}, + mode: ToEmbeddedObject, + }, + want: want{ + params: map[string]any{ + "parent": map[string]any{ + "child": map[string]any{ + "k": "v", + }, + }, + }, + }, + }, + "NestedSingletonListsToEmbeddedObjectsPathsInReverseLexicalOrder": { + reason: "Should successfully convert the parent & nested singleton lists to embedded objects. Paths specified in reverse-lexical order.", + args: args{ + params: map[string]any{ + "parent": []map[string]any{ + { + "child": []map[string]any{ + { + "k": "v", + }, + }, + }, + }, + }, + paths: []string{"parent[*].child", "parent"}, + mode: ToEmbeddedObject, + }, + want: want{ + params: map[string]any{ + "parent": map[string]any{ + "child": map[string]any{ + "k": "v", + }, + }, + }, + }, + }, + "EmbeddedObjectToSingletonList": { + reason: "Should successfully convert an embedded object at the root level to a singleton list.", + args: args{ + params: map[string]any{ + "l": map[string]any{ + "k": "v", + }, + }, + paths: []string{"l"}, + mode: ToSingletonList, + }, + want: want{ + params: map[string]any{ + "l": []map[string]any{ + { + "k": "v", + }, + }, + }, + }, + }, + "NestedEmbeddedObjectsToSingletonListInLexicalOrder": { + reason: "Should successfully convert the parent & nested embedded objects to singleton lists. Paths are specified in lexical order.", + args: args{ + params: map[string]any{ + "parent": map[string]any{ + "child": map[string]any{ + "k": "v", + }, + }, + }, + paths: []string{"parent", "parent[*].child"}, + mode: ToSingletonList, + }, + want: want{ + params: map[string]any{ + "parent": []map[string]any{ + { + "child": []map[string]any{ + { + "k": "v", + }, + }, + }, + }, + }, + }, + }, + "NestedEmbeddedObjectsToSingletonListInReverseLexicalOrder": { + reason: "Should successfully convert the parent & nested embedded objects to singleton lists. Paths are specified in reverse-lexical order.", + args: args{ + params: map[string]any{ + "parent": map[string]any{ + "child": map[string]any{ + "k": "v", + }, + }, + }, + paths: []string{"parent[*].child", "parent"}, + mode: ToSingletonList, + }, + want: want{ + params: map[string]any{ + "parent": []map[string]any{ + { + "child": []map[string]any{ + { + "k": "v", + }, + }, + }, + }, + }, + }, + }, + "FailConversionOfAMultiItemList": { + reason: `Conversion of a multi-item list in mode "ToEmbeddedObject" should fail.`, + args: args{ + params: map[string]any{ + "l": []map[string]any{ + { + "k1": "v1", + }, + { + "k2": "v2", + }, + }, + }, + paths: []string{"l"}, + mode: ToEmbeddedObject, + }, + want: want{ + err: errors.Errorf(errFmtMultiItemList, "l", 2), + }, + }, + "FailConversionOfNonSlice": { + reason: `Conversion of a non-slice value in mode "ToEmbeddedObject" should fail.`, + args: args{ + params: map[string]any{ + "l": map[string]any{ + "k": "v", + }, + }, + paths: []string{"l"}, + mode: ToEmbeddedObject, + }, + want: want{ + err: errors.Errorf(errFmtNonSlice, "l", reflect.TypeOf(map[string]any{})), + }, + }, + "ToSingletonListWithNonExistentPath": { + reason: `"ToSingletonList" mode conversions specifying only non-existent paths should be identity functions.`, + args: args{ + params: map[string]any{ + "l": map[string]any{ + "k": "v", + }, + }, + paths: []string{"nonexistent"}, + mode: ToSingletonList, + }, + want: want{ + params: map[string]any{ + "l": map[string]any{ + "k": "v", + }, + }, + }, + }, + "ToEmbeddedObjectWithNonExistentPath": { + reason: `"ToEmbeddedObject" mode conversions specifying only non-existent paths should be identity functions.`, + args: args{ + params: map[string]any{ + "l": []map[string]any{ + { + "k": "v", + }, + }, + }, + paths: []string{"nonexistent"}, + mode: ToEmbeddedObject, + }, + want: want{ + params: map[string]any{ + "l": []map[string]any{ + { + "k": "v", + }, + }, + }, + }, + }, + } + + for n, tt := range tests { + t.Run(n, func(t *testing.T) { + params, err := roundTrip(tt.args.params) + if err != nil { + t.Fatalf("Failed to preprocess tt.args.params: %v", err) + } + wantParams, err := roundTrip(tt.want.params) + if err != nil { + t.Fatalf("Failed to preprocess tt.want.params: %v", err) + } + got, err := Convert(params, tt.args.paths, tt.args.mode) + if diff := cmp.Diff(tt.want.err, err, test.EquateErrors()); diff != "" { + t.Fatalf("\n%s\nConvert(tt.args.params, tt.args.paths): -wantErr, +gotErr:\n%s", tt.reason, diff) + } + if diff := cmp.Diff(wantParams, got); diff != "" { + t.Errorf("\n%s\nConvert(tt.args.params, tt.args.paths): -wantConverted, +gotConverted:\n%s", tt.reason, diff) + } + }) + } +} + +func TestModeString(t *testing.T) { + tests := map[string]struct { + m Mode + want string + }{ + "ToSingletonList": { + m: ToSingletonList, + want: "toSingletonList", + }, + "ToEmbeddedObject": { + m: ToEmbeddedObject, + want: "toEmbeddedObject", + }, + "Unknown": { + m: ToSingletonList + 1, + want: "unknown", + }, + } + for n, tt := range tests { + t.Run(n, func(t *testing.T) { + if diff := cmp.Diff(tt.want, tt.m.String()); diff != "" { + t.Errorf("String(): -want, +got:\n%s", diff) + } + }) + } +} + +func roundTrip(m map[string]any) (map[string]any, error) { + if len(m) == 0 { + return m, nil + } + buff, err := jsoniter.ConfigCompatibleWithStandardLibrary.Marshal(m) + if err != nil { + return nil, err + } + var r map[string]any + return r, jsoniter.ConfigCompatibleWithStandardLibrary.Unmarshal(buff, &r) +} diff --git a/pkg/controller/conversion/functions.go b/pkg/controller/conversion/functions.go index e65d25b9..da4f46f5 100644 --- a/pkg/controller/conversion/functions.go +++ b/pkg/controller/conversion/functions.go @@ -13,6 +13,12 @@ import ( "github.com/crossplane/upjet/pkg/resource" ) +const ( + errFmtPrioritizedManagedConversion = "cannot apply the PrioritizedManagedConversion for the %q object" + errFmtPavedConversion = "cannot apply the PavedConversion for the %q object" + errFmtManagedConversion = "cannot apply the ManagedConversion for the %q object" +) + // RoundTrip round-trips from `src` to `dst` via an unstructured map[string]any // representation of the `src` object and applies the registered webhook // conversion functions of this registry. @@ -21,7 +27,7 @@ func (r *registry) RoundTrip(dst, src resource.Terraformed) error { //nolint:goc for _, c := range r.GetConversions(dst) { if pc, ok := c.(conversion.PrioritizedManagedConversion); ok { if _, err := pc.ConvertManaged(src, dst); err != nil { - return errors.Wrapf(err, "cannot apply the PrioritizedManagedConversion for the %q object", dst.GetTerraformResourceType()) + return errors.Wrapf(err, errFmtPrioritizedManagedConversion, dst.GetTerraformResourceType()) } } } @@ -41,7 +47,7 @@ func (r *registry) RoundTrip(dst, src resource.Terraformed) error { //nolint:goc for _, c := range r.GetConversions(dst) { if pc, ok := c.(conversion.PavedConversion); ok { if _, err := pc.ConvertPaved(srcPaved, dstPaved); err != nil { - return errors.Wrapf(err, "cannot apply the PavedConversion for the %q object", dst.GetTerraformResourceType()) + return errors.Wrapf(err, errFmtPavedConversion, dst.GetTerraformResourceType()) } } } @@ -58,7 +64,7 @@ func (r *registry) RoundTrip(dst, src resource.Terraformed) error { //nolint:goc continue // then already run in the first stage } if _, err := tc.ConvertManaged(src, dst); err != nil { - return errors.Wrapf(err, "cannot apply the ManagedConversion for the %q object", dst.GetTerraformResourceType()) + return errors.Wrapf(err, errFmtManagedConversion, dst.GetTerraformResourceType()) } } } diff --git a/pkg/controller/conversion/functions_test.go b/pkg/controller/conversion/functions_test.go index 2c1213e3..2df01aee 100644 --- a/pkg/controller/conversion/functions_test.go +++ b/pkg/controller/conversion/functions_test.go @@ -8,9 +8,12 @@ import ( "fmt" "testing" + "github.com/crossplane/crossplane-runtime/pkg/fieldpath" xpresource "github.com/crossplane/crossplane-runtime/pkg/resource" "github.com/crossplane/crossplane-runtime/pkg/test" "github.com/google/go-cmp/cmp" + "github.com/pkg/errors" + "k8s.io/apimachinery/pkg/runtime" "github.com/crossplane/upjet/pkg/config" "github.com/crossplane/upjet/pkg/config/conversion" @@ -25,6 +28,7 @@ const ( val2 = "val2" commonKey = "commonKey" commonVal = "commonVal" + errTest = "test error" ) func TestRoundTrip(t *testing.T) { @@ -76,6 +80,39 @@ func TestRoundTrip(t *testing.T) { dst: fake.NewTerraformed(fake.WithParameters(fake.NewMap(commonKey, commonVal, key2, val1))), }, }, + "RoundTripFailedPrioritizedConversion": { + reason: "Should return an error if a PrioritizedConversion fails.", + args: args{ + dst: fake.NewTerraformed(), + src: fake.NewTerraformed(), + conversions: []conversion.Conversion{failedPrioritizedConversion{}}, + }, + want: want{ + err: errors.Wrapf(errors.New(errTest), errFmtPrioritizedManagedConversion, ""), + }, + }, + "RoundTripFailedPavedConversion": { + reason: "Should return an error if a PavedConversion fails.", + args: args{ + dst: fake.NewTerraformed(), + src: fake.NewTerraformed(), + conversions: []conversion.Conversion{failedPavedConversion{}}, + }, + want: want{ + err: errors.Wrapf(errors.New(errTest), errFmtPavedConversion, ""), + }, + }, + "RoundTripFailedManagedConversion": { + reason: "Should return an error if a ManagedConversion fails.", + args: args{ + dst: fake.NewTerraformed(), + src: fake.NewTerraformed(), + conversions: []conversion.Conversion{failedManagedConversion{}}, + }, + want: want{ + err: errors.Wrapf(errors.New(errTest), errFmtManagedConversion, ""), + }, + }, "RoundTripWithExcludedFields": { reason: "Source object is successfully copied into the target object with certain fields excluded.", args: args{ @@ -114,3 +151,35 @@ func TestRoundTrip(t *testing.T) { }) } } + +type failedPrioritizedConversion struct{} + +func (failedPrioritizedConversion) Applicable(_, _ runtime.Object) bool { + return true +} + +func (failedPrioritizedConversion) ConvertManaged(_, _ xpresource.Managed) (bool, error) { + return false, errors.New(errTest) +} + +func (failedPrioritizedConversion) Prioritized() {} + +type failedPavedConversion struct{} + +func (failedPavedConversion) Applicable(_, _ runtime.Object) bool { + return true +} + +func (failedPavedConversion) ConvertPaved(_, _ *fieldpath.Paved) (bool, error) { + return false, errors.New(errTest) +} + +type failedManagedConversion struct{} + +func (failedManagedConversion) Applicable(_, _ runtime.Object) bool { + return true +} + +func (failedManagedConversion) ConvertManaged(_, _ xpresource.Managed) (bool, error) { + return false, errors.New(errTest) +}