Merge pull request #411 from ulucinar/fix-embedded-conversion

Add config.Resource.RemoveSingletonListConversion

Author: ulucinar

pkg/config/provider.go

@@ -373,7 +373,7 @@ func NewProvider(schema []byte, prefix string, modulePath string, metadata []byt
 		p.Resources[name].useTerraformPluginFrameworkClient = isPluginFrameworkResource
 		// traverse the Terraform resource schema to initialize the upjet Resource
 		// configurations
-		if err := traverseSchemas(name, terraformResource, p.Resources[name], p.schemaTraversers...); err != nil {
+		if err := TraverseSchemas(name, p.Resources[name], p.schemaTraversers...); err != nil {
 			panic(errors.Wrap(err, "failed to execute the Terraform schema traverser chain"))
 		}
 	}

pkg/config/resource.go

@@ -693,6 +693,28 @@ func (r *Resource) AddSingletonListConversion(tfPath, crdPath string) {
 	r.listConversionPaths[tfPath] = crdPath
 }
 
+// RemoveSingletonListConversion removes the singleton list conversion
+// for the specified Terraform configuration path. Also unsets the path's
+// embedding mode. The specified fieldpath expression must be a Terraform
+// field path with or without the wildcard segments. Returns true if
+// the path has already been registered for singleton list conversion.
+func (r *Resource) RemoveSingletonListConversion(tfPath string) bool {
+	nPath := strings.ReplaceAll(tfPath, "[*]", "")
+	nPath = strings.ReplaceAll(nPath, "[0]", "")
+	for p := range r.listConversionPaths {
+		n := strings.ReplaceAll(p, "[*]", "")
+		n = strings.ReplaceAll(n, "[0]", "")
+		if n == nPath {
+			delete(r.listConversionPaths, p)
+			if r.SchemaElementOptions[n] != nil {
+				r.SchemaElementOptions[n].EmbeddedObject = false
+			}
+			return true
+		}
+	}
+	return false
+}
+
 // SetEmbeddedObject sets the EmbeddedObject for the specified key.
 // The key is a Terraform field path without the wildcard segments.
 func (m SchemaElementOptions) SetEmbeddedObject(el string) {

pkg/config/resource_test.go

@@ -24,7 +24,7 @@ const (
 	provider = "ACoolProvider"
 )
 
-func TestTagger_Initialize(t *testing.T) {
+func TestTaggerInitialize(t *testing.T) {
 	errBoom := errors.New("boom")
 
 	type args struct {
@@ -112,3 +112,187 @@ func TestSetExternalTagsWithPaved(t *testing.T) {
 		})
 	}
 }
+
+func TestAddSingletonListConversion(t *testing.T) {
+	type args struct {
+		r       func() *Resource
+		tfPath  string
+		crdPath string
+	}
+	type want struct {
+		r func() *Resource
+	}
+	cases := map[string]struct {
+		reason string
+		args
+		want
+	}{
+		"AddNonWildcardTFPath": {
+			reason: "A non-wildcard TF path of a singleton list should successfully be configured to be converted into an embedded object.",
+			args: args{
+				tfPath:  "singleton_list",
+				crdPath: "singletonList",
+				r: func() *Resource {
+					r := DefaultResource("test_resource", nil, nil, nil)
+					r.AddSingletonListConversion("singleton_list", "singletonList")
+					return r
+				},
+			},
+			want: want{
+				r: func() *Resource {
+					r := DefaultResource("test_resource", nil, nil, nil)
+					r.SchemaElementOptions = SchemaElementOptions{}
+					r.SchemaElementOptions["singleton_list"] = &SchemaElementOption{
+						EmbeddedObject: true,
+					}
+					r.listConversionPaths["singleton_list"] = "singletonList"
+					return r
+				},
+			},
+		},
+		"AddWildcardTFPath": {
+			reason: "A wildcard TF path of a singleton list should successfully be configured to be converted into an embedded object.",
+			args: args{
+				tfPath:  "parent[*].singleton_list",
+				crdPath: "parent[*].singletonList",
+				r: func() *Resource {
+					r := DefaultResource("test_resource", nil, nil, nil)
+					r.AddSingletonListConversion("parent[*].singleton_list", "parent[*].singletonList")
+					return r
+				},
+			},
+			want: want{
+				r: func() *Resource {
+					r := DefaultResource("test_resource", nil, nil, nil)
+					r.SchemaElementOptions = SchemaElementOptions{}
+					r.SchemaElementOptions["parent.singleton_list"] = &SchemaElementOption{
+						EmbeddedObject: true,
+					}
+					r.listConversionPaths["parent[*].singleton_list"] = "parent[*].singletonList"
+					return r
+				},
+			},
+		},
+		"AddIndexedTFPath": {
+			reason: "An indexed TF path of a singleton list should successfully be configured to be converted into an embedded object.",
+			args: args{
+				tfPath:  "parent[0].singleton_list",
+				crdPath: "parent[0].singletonList",
+				r: func() *Resource {
+					r := DefaultResource("test_resource", nil, nil, nil)
+					r.AddSingletonListConversion("parent[0].singleton_list", "parent[0].singletonList")
+					return r
+				},
+			},
+			want: want{
+				r: func() *Resource {
+					r := DefaultResource("test_resource", nil, nil, nil)
+					r.SchemaElementOptions = SchemaElementOptions{}
+					r.SchemaElementOptions["parent.singleton_list"] = &SchemaElementOption{
+						EmbeddedObject: true,
+					}
+					r.listConversionPaths["parent[0].singleton_list"] = "parent[0].singletonList"
+					return r
+				},
+			},
+		},
+	}
+	for n, tc := range cases {
+		t.Run(n, func(t *testing.T) {
+			r := tc.args.r()
+			r.AddSingletonListConversion(tc.args.tfPath, tc.args.crdPath)
+			wantR := tc.want.r()
+			if diff := cmp.Diff(wantR.listConversionPaths, r.listConversionPaths); diff != "" {
+				t.Errorf("%s\nAddSingletonListConversion(tfPath): -wantConversionPaths, +gotConversionPaths: \n%s", tc.reason, diff)
+			}
+			if diff := cmp.Diff(wantR.SchemaElementOptions, r.SchemaElementOptions); diff != "" {
+				t.Errorf("%s\nAddSingletonListConversion(tfPath): -wantSchemaElementOptions, +gotSchemaElementOptions: \n%s", tc.reason, diff)
+			}
+		})
+	}
+}
+
+func TestRemoveSingletonListConversion(t *testing.T) {
+	type args struct {
+		r      func() *Resource
+		tfPath string
+	}
+	type want struct {
+		removed bool
+		r       func() *Resource
+	}
+	cases := map[string]struct {
+		reason string
+		args
+		want
+	}{
+		"RemoveWildcardListConversion": {
+			reason: "An existing wildcard list conversion can successfully be removed.",
+			args: args{
+				tfPath: "parent[*].singleton_list",
+				r: func() *Resource {
+					r := DefaultResource("test_resource", nil, nil, nil)
+					r.AddSingletonListConversion("parent[*].singleton_list", "parent[*].singletonList")
+					return r
+				},
+			},
+			want: want{
+				removed: true,
+				r: func() *Resource {
+					r := DefaultResource("test_resource", nil, nil, nil)
+					return r
+				},
+			},
+		},
+		"RemoveIndexedListConversion": {
+			reason: "An existing indexed list conversion can successfully be removed.",
+			args: args{
+				tfPath: "parent[0].singleton_list",
+				r: func() *Resource {
+					r := DefaultResource("test_resource", nil, nil, nil)
+					r.AddSingletonListConversion("parent[0].singleton_list", "parent[0].singletonList")
+					return r
+				},
+			},
+			want: want{
+				removed: true,
+				r: func() *Resource {
+					r := DefaultResource("test_resource", nil, nil, nil)
+					return r
+				},
+			},
+		},
+		"NonExistingListConversion": {
+			reason: "A list conversion path that does not exist cannot be removed.",
+			args: args{
+				tfPath: "non-existent",
+				r: func() *Resource {
+					r := DefaultResource("test_resource", nil, nil, nil)
+					r.AddSingletonListConversion("parent[*].singleton_list", "parent[*].singletonList")
+					return r
+				},
+			},
+			want: want{
+				removed: false,
+				r: func() *Resource {
+					r := DefaultResource("test_resource", nil, nil, nil)
+					r.AddSingletonListConversion("parent[*].singleton_list", "parent[*].singletonList")
+					return r
+				},
+			},
+		},
+	}
+	for n, tc := range cases {
+		t.Run(n, func(t *testing.T) {
+			r := tc.args.r()
+			got := r.RemoveSingletonListConversion(tc.args.tfPath)
+			if diff := cmp.Diff(tc.want.removed, got); diff != "" {
+				t.Errorf("%s\nRemoveSingletonListConversion(tfPath): -wantRemoved, +gotRemoved: \n%s", tc.reason, diff)
+			}
+
+			if diff := cmp.Diff(tc.want.r().listConversionPaths, r.listConversionPaths); diff != "" {
+				t.Errorf("%s\nRemoveSingletonListConversion(tfPath): -wantConversionPaths, +gotConversionPaths: \n%s", tc.reason, diff)
+			}
+		})
+	}
+}

pkg/config/schema_conversions.go

@@ -6,6 +6,7 @@ package config
 
 import (
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+	"github.com/pkg/errors"
 
 	"github.com/crossplane/upjet/pkg/schema/traverser"
 )
@@ -17,15 +18,34 @@ type ResourceSetter interface {
 	SetResource(r *Resource)
 }
 
-func traverseSchemas(tfName string, tfResource *schema.Resource, r *Resource, visitors ...traverser.SchemaTraverser) error {
+// ResourceSchema represents a provider's resource schema.
+type ResourceSchema map[string]*Resource
+
+// TraverseTFSchemas traverses the Terraform schemas of all the resources of
+// the Provider `p` using the specified visitors. Reports any errors
+// encountered.
+func (s ResourceSchema) TraverseTFSchemas(visitors ...traverser.SchemaTraverser) error {
+	for name, cfg := range s {
+		if err := TraverseSchemas(name, cfg, visitors...); err != nil {
+			return errors.Wrapf(err, "failed to traverse the schema of the Terraform resource with name %q", name)
+		}
+	}
+	return nil
+}
+
+// TraverseSchemas visits the specified schema belonging to the Terraform
+// resource with the given name and given upjet resource configuration using
+// the specified visitors. If any visitors report an error, traversal is
+// stopped and the error is reported to the caller.
+func TraverseSchemas(tfName string, r *Resource, visitors ...traverser.SchemaTraverser) error {
 	// set the upjet Resource configuration as context for the visitors that
 	// satisfy the ResourceSetter interface.
 	for _, v := range visitors {
 		if rs, ok := v.(ResourceSetter); ok {
 			rs.SetResource(r)
 		}
 	}
-	return traverser.Traverse(tfName, tfResource, visitors...)
+	return traverser.Traverse(tfName, r.TerraformResource, visitors...)
 }
 
 type resourceContext struct {

pkg/config/schema_conversions_test.go

@@ -155,7 +155,10 @@ func TestSingletonListEmbedder(t *testing.T) {
 		t.Run(n, func(t *testing.T) {
 			e := &SingletonListEmbedder{}
 			r := DefaultResource(tt.args.name, tt.args.resource, nil, nil)
-			err := traverseSchemas(tt.args.name, tt.args.resource, r, e)
+			s := ResourceSchema{
+				tt.args.name: r,
+			}
+			err := s.TraverseTFSchemas(e)
 			if diff := cmp.Diff(tt.want.err, err, test.EquateErrors()); diff != "" {
 				t.Fatalf("\n%s\ntraverseSchemas(name, schema, ...): -wantErr, +gotErr:\n%s", tt.reason, diff)
 			}

pkg/schema/traverser/access.go

@@ -0,0 +1,58 @@
+// SPDX-FileCopyrightText: 2024 The Crossplane Authors <https://crossplane.io>
+//
+// SPDX-License-Identifier: Apache-2.0
+
+package traverser
+
+import (
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+	"github.com/pkg/errors"
+)
+
+// SchemaAccessor is a function that accesses and potentially modifies a
+// Terraform schema.
+type SchemaAccessor func(*schema.Schema) error
+
+// AccessSchema accesses the schema element at the specified path and calls
+// the given schema accessors in order. Reports any errors encountered by
+// the accessors. The terminal node at the end of the specified path must be
+// a *schema.Resource or an error will be reported. The specified path must
+// have at least one component.
+func AccessSchema(sch any, path []string, accessors ...SchemaAccessor) error { //nolint:gocyclo // easier to follow the flow
+	if len(path) == 0 {
+		return errors.New("empty path specified while accessing the Terraform resource schema")
+	}
+	k := path[0]
+	path = path[1:]
+	switch s := sch.(type) {
+	case *schema.Schema:
+		if len(path) == 0 {
+			return errors.Errorf("terminal node at key %q is a *schema.Schema", k)
+		}
+		if k != wildcard {
+			return errors.Errorf("expecting a wildcard key but encountered the key %q", k)
+		}
+		if err := AccessSchema(s.Elem, path, accessors...); err != nil {
+			return err
+		}
+	case *schema.Resource:
+		c := s.Schema[k]
+		if c == nil {
+			return errors.Errorf("schema element for key %q is nil", k)
+		}
+		if len(path) == 0 {
+			for _, a := range accessors {
+				if err := a(c); err != nil {
+					return errors.Wrapf(err, "failed to call the accessor function on the schema element at key %q", k)
+				}
+			}
+			return nil
+		}
+		if err := AccessSchema(c, path, accessors...); err != nil {
+			return err
+		}
+	default:
+		return errors.Errorf("unknown schema element type %T at key %q", s, k)
+	}
+	return nil
+}

pkg/schema/traverser/maxitemssync.go

@@ -0,0 +1,47 @@
+// SPDX-FileCopyrightText: 2024 The Crossplane Authors <https://crossplane.io>
+//
+// SPDX-License-Identifier: Apache-2.0
+
+package traverser
+
+import (
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+	"github.com/pkg/errors"
+)
+
+// maxItemsSync is a visitor to sync the MaxItems constraints from the
+// Go schema to the JSON schema. We've observed that some MaxItems constraints
+// in the JSON schemas are not set where the corresponding MaxItems constraints
+// in the Go schemas are set to 1. This inconsistency results in some singleton
+// lists not being properly converted in the MR API whereas at runtime we may
+// try to invoke the corresponding Terraform conversion functions. This
+// traverser can mitigate such inconsistencies by syncing the MaxItems
+// constraints from the Go schema to the JSON schema.
+type maxItemsSync struct {
+	NoopTraverser
+
+	jsonSchema TFResourceSchema
+}
+
+// NewMaxItemsSync returns a new schema traverser capable of
+// syncing the MaxItems constraints from the Go schema to the specified JSON
+// schema. This will ensure the generation time and the runtime schema MaxItems
+// constraints will be consistent. This is needed only if the generation time
+// and runtime schemas differ.
+func NewMaxItemsSync(s TFResourceSchema) SchemaTraverser {
+	return &maxItemsSync{
+		jsonSchema: s,
+	}
+}
+
+func (m *maxItemsSync) VisitResource(r *ResourceNode) error {
+	// this visitor only works on singleton lists
+	if (r.Schema.Type != schema.TypeList && r.Schema.Type != schema.TypeSet) || r.Schema.MaxItems != 1 {
+		return nil
+	}
+	return errors.Wrapf(AccessSchema(m.jsonSchema[r.TFName], r.TFPath,
+		func(s *schema.Schema) error {
+			s.MaxItems = 1
+			return nil
+		}), "failed to access the schema element at path %v for resource %q", r.TFPath, r.TFName)
+}