From 5433818fa9e642cd45c6b712d156185bb3493c4c Mon Sep 17 00:00:00 2001 From: Duologic Date: Fri, 15 May 2026 00:51:38 +0200 Subject: [PATCH] fix(examples): flatten SchemaTypeObject blocks in generated example manifests The HCL-to-JSON scraper wraps all Terraform blocks in arrays per HCL spec. For NestingModeSingle (SchemaTypeObject) fields, upjet generates CRD types as embedded objects (pointer-to-struct), but the generated example manifests retained array syntax, causing unknown field errors. ApplyAPIConverters only handled TypeList/TypeSet with MaxItems=1 via SingletonListEmbedder. SchemaTypeObject fields were not covered because they use a different schema type (ValueType 9) and their paths lack [*] wildcards. Add a post-processing step that traverses each resource schema, collects SchemaTypeObject CRD paths, and flattens single-element arrays at those paths. Uses lexical ordering (parents before children) because without wildcards, child paths are not resolvable while the parent is still an array. Fixes #656 Signed-off-by: Duologic --- .../conversion/example_conversions.go | 140 +++++- .../conversion/example_conversions_test.go | 409 ++++++++++++++++++ 2 files changed, 535 insertions(+), 14 deletions(-) create mode 100644 pkg/examples/conversion/example_conversions_test.go diff --git a/pkg/examples/conversion/example_conversions.go b/pkg/examples/conversion/example_conversions.go index a722c8ad..ac11380c 100644 --- a/pkg/examples/conversion/example_conversions.go +++ b/pkg/examples/conversion/example_conversions.go @@ -11,8 +11,10 @@ import ( "log" "os" "path/filepath" + "sort" "strings" + "github.com/crossplane/crossplane-runtime/v2/pkg/fieldpath" "github.com/pkg/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" k8sschema "k8s.io/apimachinery/pkg/runtime/schema" @@ -21,8 +23,100 @@ import ( "github.com/crossplane/upjet/v2/pkg/config" "github.com/crossplane/upjet/v2/pkg/config/conversion" + "github.com/crossplane/upjet/v2/pkg/schema/traverser" + "github.com/crossplane/upjet/v2/pkg/types/conversion/tfjson" ) +// schemaTypeObjectCollector is a schema traverser that collects the CRD field +// paths for SchemaTypeObject (NestingModeSingle) fields. These fields are +// represented as embedded objects (pointer-to-struct) in the CRD but may appear +// as single-element arrays in scraped Terraform example configurations. +type schemaTypeObjectCollector struct { + traverser.NoopTraverser + paths []string +} + +func (c *schemaTypeObjectCollector) VisitResource(r *traverser.ResourceNode) error { + if r.Schema.Type == tfjson.SchemaTypeObject { + c.paths = append(c.paths, traverser.FieldPathWithWildcard(r.CRDPath)) + } + return nil +} + +// collectSchemaTypeObjectCRDPaths returns the CRD field paths for all +// SchemaTypeObject fields in the given resource's Terraform schema. +func collectSchemaTypeObjectCRDPaths(r *config.Resource) ([]string, error) { + collector := &schemaTypeObjectCollector{} + if err := traverser.Traverse(r.Name, r.TerraformResource, collector); err != nil { + return nil, errors.Wrapf(err, "failed to traverse the schema of resource %s", r.Name) + } + return collector.paths, nil +} + +// flattenSchemaTypeObjectExamples flattens single-element arrays at the given +// field paths to plain objects in the provided unstructured object. +// Unlike conversion.Convert, this function is lenient: it silently skips paths +// that don't exist or whose values are not single-element arrays. +// This is necessary because conversion.Convert returns an error for non-slice +// values, which is correct for runtime conversions but too strict for example +// manifests where the value may not be present or may already be an object. +func flattenSchemaTypeObjectExamples(obj map[string]any, paths []string) { + if len(paths) == 0 { + return + } + // Sort in lexical order so parents are flattened before children. + // This is necessary because SchemaTypeObject paths do NOT use [*] + // wildcards (unlike TypeList/TypeSet paths). Without wildcards, + // child paths like "outer.inner" can't be resolved while "outer" is + // still an array. Flattening parents first converts them to objects, + // making child paths accessible. + sortedPaths := append([]string(nil), paths...) + sort.Strings(sortedPaths) + pv := fieldpath.Pave(obj) + for _, fp := range sortedPaths { + exp, err := pv.ExpandWildcards(fp) + if err != nil { + // Path doesn't exist in the object, skip. + continue + } + for _, e := range exp { + flattenSingleElementArray(pv, e) + } + } +} + +// flattenSingleElementArray replaces a single-element array at the given +// concrete field path with its sole element. It is a no-op if the value +// does not exist, is not a []any, or does not have exactly one element. +func flattenSingleElementArray(pv *fieldpath.Paved, path string) { + v, err := pv.GetValue(path) + if err != nil { + return + } + s, ok := v.([]any) + if !ok || len(s) != 1 { + return + } + // Set the value to the single element by accessing the parent map + // directly. This avoids type coercion that fieldpath.Paved.SetValue + // might perform. + segments := strings.Split(path, ".") + key := segments[len(segments)-1] + var parentVal any = pv.UnstructuredContent() + if len(segments) > 1 { + parentPath := strings.Join(segments[:len(segments)-1], ".") + parentVal, err = pv.GetValue(parentPath) + if err != nil { + return + } + } + parent, ok := parentVal.(map[string]any) + if !ok { + return + } + parent[key] = s[0] +} + // ApplyAPIConverters applies the registered converters to generated // example manifests under the given root directory. // All (generated) manifests under the `startPath` are scanned and the @@ -67,23 +161,41 @@ func ApplyAPIConverters(pc *config.Provider, startPath, licenseHeaderPath string annotationValue := strings.ToLower(fmt.Sprintf("%s/%s/%s", rootResource.ShortGroup, rootResource.Version, rootResource.Kind)) for _, e := range examples { if resource, ok := resourceRegistry[fmt.Sprintf("%s/%s", e.GroupVersionKind().Kind, e.GroupVersionKind().Group)]; ok { - conversionPaths := resource.CRDListConversionPaths() - // if the latest version has conversions, run the conversions on the - // example manifest. - // Please note that only the version being generated (latest version) - // is processed. - if conversionPaths != nil && e.GroupVersionKind().Version == resource.Version { - for i, cp := range conversionPaths { - // Here, for the manifests to be converted, only `forProvider - // is converted, assuming the `initProvider` field is empty in the - // spec. - conversionPaths[i] = "spec.forProvider." + cp + if e.GroupVersionKind().Version == resource.Version { + conversionPaths := resource.CRDListConversionPaths() + // if the latest version has conversions, run the conversions on the + // example manifest. + // Please note that only the version being generated (latest version) + // is processed. + if conversionPaths != nil { + for i, cp := range conversionPaths { + // Here, for the manifests to be converted, only `forProvider + // is converted, assuming the `initProvider` field is empty in the + // spec. + conversionPaths[i] = "spec.forProvider." + cp + } + converted, err := conversion.Convert(e.Object, conversionPaths, conversion.ToEmbeddedObject, nil) + if err != nil { + return errors.Wrapf(err, "failed to convert example to embedded object in manifest %s", path) + } + e.Object = converted } - converted, err := conversion.Convert(e.Object, conversionPaths, conversion.ToEmbeddedObject, nil) + + // Also flatten SchemaTypeObject (NestingModeSingle) fields. + // These fields are generated as embedded objects in the CRD + // but the HCL-to-JSON scraper wraps them in single-element + // arrays per HCL spec. + objectPaths, err := collectSchemaTypeObjectCRDPaths(resource) if err != nil { - return errors.Wrapf(err, "failed to convert example to embedded object in manifest %s", path) + return errors.Wrapf(err, "failed to collect SchemaTypeObject paths for resource %s", resource.Name) + } + if len(objectPaths) > 0 { + for i, op := range objectPaths { + objectPaths[i] = "spec.forProvider." + op + } + flattenSchemaTypeObjectExamples(e.Object, objectPaths) } - e.Object = converted + e.SetGroupVersionKind(k8sschema.GroupVersionKind{ Group: e.GroupVersionKind().Group, Version: resource.Version, diff --git a/pkg/examples/conversion/example_conversions_test.go b/pkg/examples/conversion/example_conversions_test.go new file mode 100644 index 00000000..7d6e94a7 --- /dev/null +++ b/pkg/examples/conversion/example_conversions_test.go @@ -0,0 +1,409 @@ +// SPDX-FileCopyrightText: 2024 The Crossplane Authors +// +// SPDX-License-Identifier: Apache-2.0 + +package conversion + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + jsoniter "github.com/json-iterator/go" + + "github.com/crossplane/upjet/v2/pkg/config" + "github.com/crossplane/upjet/v2/pkg/types/conversion/tfjson" +) + +func TestCollectSchemaTypeObjectCRDPaths(t *testing.T) { + type args struct { + tfResource *schema.Resource + name string + } + type want struct { + paths []string + err error + } + tests := map[string]struct { + reason string + args args + want want + }{ + "RootLevelSchemaTypeObject": { + reason: "Should collect a root-level SchemaTypeObject field path.", + args: args{ + tfResource: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "object_block": { + Type: tfjson.SchemaTypeObject, + MaxItems: 1, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "inner_field": { + Type: schema.TypeString, + }, + }, + }, + }, + }, + }, + name: "test_resource", + }, + want: want{ + paths: []string{"objectBlock"}, + }, + }, + "NoSchemaTypeObjectFields": { + reason: "Should return nil when there are no SchemaTypeObject fields.", + args: args{ + tfResource: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "regular_list": { + Type: schema.TypeList, + MaxItems: 1, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "element": { + Type: schema.TypeString, + }, + }, + }, + }, + }, + }, + name: "test_resource", + }, + want: want{ + paths: nil, + }, + }, + "NestedSchemaTypeObjectInsideList": { + reason: "Should collect a SchemaTypeObject nested inside a TypeList with wildcard in path.", + args: args{ + tfResource: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "parent_list": { + Type: schema.TypeList, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "nested_object": { + Type: tfjson.SchemaTypeObject, + MaxItems: 1, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "value": { + Type: schema.TypeString, + }, + }, + }, + }, + }, + }, + }, + }, + }, + name: "test_resource", + }, + want: want{ + paths: []string{"parentList[*].nestedObject"}, + }, + }, + "MultipleSchemaTypeObjectFields": { + reason: "Should collect multiple SchemaTypeObject field paths.", + args: args{ + tfResource: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "block_a": { + Type: tfjson.SchemaTypeObject, + MaxItems: 1, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "field": { + Type: schema.TypeString, + }, + }, + }, + }, + "block_b": { + Type: tfjson.SchemaTypeObject, + MaxItems: 1, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "field": { + Type: schema.TypeString, + }, + }, + }, + }, + }, + }, + name: "test_resource", + }, + want: want{ + // Order is non-deterministic due to map iteration + paths: []string{"blockA", "blockB"}, + }, + }, + "NestedSchemaTypeObjectInsideSchemaTypeObject": { + reason: "Should collect nested SchemaTypeObject fields without wildcards between them.", + args: args{ + tfResource: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "outer_object": { + Type: tfjson.SchemaTypeObject, + MaxItems: 1, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "inner_object": { + Type: tfjson.SchemaTypeObject, + MaxItems: 1, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "value": { + Type: schema.TypeString, + }, + }, + }, + }, + }, + }, + }, + }, + }, + name: "test_resource", + }, + want: want{ + paths: []string{"outerObject", "outerObject.innerObject"}, + }, + }, + } + for n, tt := range tests { + t.Run(n, func(t *testing.T) { + r := config.DefaultResource(tt.args.name, tt.args.tfResource, nil, nil) + got, err := collectSchemaTypeObjectCRDPaths(r) + if diff := cmp.Diff(tt.want.err, err); diff != "" { + t.Fatalf("\n%s\ncollectSchemaTypeObjectCRDPaths(): -wantErr, +gotErr:\n%s", tt.reason, diff) + } + if diff := cmp.Diff(tt.want.paths, got, cmp.Transformer("sort", sortStrings)); diff != "" { + t.Errorf("\n%s\ncollectSchemaTypeObjectCRDPaths(): -want, +got:\n%s", tt.reason, diff) + } + }) + } +} + +func TestFlattenSchemaTypeObjectExamples(t *testing.T) { + type args struct { + obj map[string]any + paths []string + } + type want struct { + obj map[string]any + } + tests := map[string]struct { + reason string + args args + want want + }{ + "NilPaths": { + reason: "Flattening with nil paths should be a no-op.", + args: args{ + obj: map[string]any{"a": "b"}, + }, + want: want{ + obj: map[string]any{"a": "b"}, + }, + }, + "EmptyPaths": { + reason: "Flattening with empty paths should be a no-op.", + args: args{ + obj: map[string]any{"a": "b"}, + paths: []string{}, + }, + want: want{ + obj: map[string]any{"a": "b"}, + }, + }, + "FlattenRootLevelSingletonArray": { + reason: "Should flatten a single-element array at the root level to a plain object.", + args: args{ + obj: map[string]any{ + "spec": map[string]any{ + "forProvider": map[string]any{ + "block": []any{ + map[string]any{"key": "value"}, + }, + }, + }, + }, + paths: []string{"spec.forProvider.block"}, + }, + want: want{ + obj: map[string]any{ + "spec": map[string]any{ + "forProvider": map[string]any{ + "block": map[string]any{"key": "value"}, + }, + }, + }, + }, + }, + "SkipNonExistentPath": { + reason: "Should skip paths that don't exist in the object.", + args: args{ + obj: map[string]any{ + "spec": map[string]any{ + "forProvider": map[string]any{ + "existing": "value", + }, + }, + }, + paths: []string{"spec.forProvider.nonExistent"}, + }, + want: want{ + obj: map[string]any{ + "spec": map[string]any{ + "forProvider": map[string]any{ + "existing": "value", + }, + }, + }, + }, + }, + "SkipAlreadyFlattenedValue": { + reason: "Should skip values that are already objects (not arrays).", + args: args{ + obj: map[string]any{ + "spec": map[string]any{ + "forProvider": map[string]any{ + "block": map[string]any{"key": "value"}, + }, + }, + }, + paths: []string{"spec.forProvider.block"}, + }, + want: want{ + obj: map[string]any{ + "spec": map[string]any{ + "forProvider": map[string]any{ + "block": map[string]any{"key": "value"}, + }, + }, + }, + }, + }, + "FlattenNestedInsideList": { + reason: "Should flatten SchemaTypeObject fields nested inside list elements using wildcards.", + args: args{ + obj: map[string]any{ + "spec": map[string]any{ + "forProvider": map[string]any{ + "items": []any{ + map[string]any{ + "nested": []any{ + map[string]any{"key": "v1"}, + }, + }, + map[string]any{ + "nested": []any{ + map[string]any{"key": "v2"}, + }, + }, + }, + }, + }, + }, + paths: []string{"spec.forProvider.items[*].nested"}, + }, + want: want{ + obj: map[string]any{ + "spec": map[string]any{ + "forProvider": map[string]any{ + "items": []any{ + map[string]any{ + "nested": map[string]any{"key": "v1"}, + }, + map[string]any{ + "nested": map[string]any{"key": "v2"}, + }, + }, + }, + }, + }, + }, + }, + "FlattenNestedParentAndChild": { + reason: "Should flatten both parent and child SchemaTypeObject fields, processing children first.", + args: args{ + obj: map[string]any{ + "spec": map[string]any{ + "forProvider": map[string]any{ + "outer": []any{ + map[string]any{ + "inner": []any{ + map[string]any{"key": "value"}, + }, + }, + }, + }, + }, + }, + paths: []string{"spec.forProvider.outer", "spec.forProvider.outer.inner"}, + }, + want: want{ + obj: map[string]any{ + "spec": map[string]any{ + "forProvider": map[string]any{ + "outer": map[string]any{ + "inner": map[string]any{"key": "value"}, + }, + }, + }, + }, + }, + }, + } + for n, tt := range tests { + t.Run(n, func(t *testing.T) { + obj, err := roundTrip(tt.args.obj) + if err != nil { + t.Fatalf("Failed to preprocess tt.args.obj: %v", err) + } + wantObj, err := roundTrip(tt.want.obj) + if err != nil { + t.Fatalf("Failed to preprocess tt.want.obj: %v", err) + } + flattenSchemaTypeObjectExamples(obj, tt.args.paths) + if diff := cmp.Diff(wantObj, obj); diff != "" { + t.Errorf("\n%s\nflattenSchemaTypeObjectExamples(): -want, +got:\n%s", tt.reason, diff) + } + }) + } +} + +func sortStrings(s []string) []string { + if s == nil { + return nil + } + result := make([]string, len(s)) + copy(result, s) + for i := range result { + for j := i + 1; j < len(result); j++ { + if result[i] > result[j] { + result[i], result[j] = result[j], result[i] + } + } + } + return result +} + +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) +}