From 261df01eec2befc83b4f256578efeb09df6b3d3e Mon Sep 17 00:00:00 2001 From: Ian Wahbe Date: Wed, 10 Apr 2024 15:37:53 +0200 Subject: [PATCH] Remove `rawNames` from `makeObjectTerraformInputs` It never makes sense to pass `rawNames=true` to `makeObjectTerraformInputs` since that implies we have an object type with properties, but that we should not do property name translations. This is never the case. This PR introduces a subtle change in behavior when walking values of unknown type: Before this PR, we assumed that an untyped `resource.PropertyMap` was an object (name translation is performed) but all nested `resource.PropertyMap` values were then assumed to be a map (name translation is not performed). After this PR, we treat all untyped `resource.PropertyMap` values as maps. IMO this makes more sense: the rule is we don't apply name translation unless we know a value is an object (and can thus generate naming tables for it). I don't expect that this change will break users since user's should never hit untyped values. If they do, neither before or after will work consistently. Addressing the test change: "object_property_value" was used to test object names but never given a type. In light of the above change, we needed to make it typed to get the expected name translation. --- pkg/tfbridge/schema.go | 84 +++++++++++++++---------------------- pkg/tfbridge/schema_test.go | 2 +- 2 files changed, 34 insertions(+), 52 deletions(-) diff --git a/pkg/tfbridge/schema.go b/pkg/tfbridge/schema.go index 54d786779..fd14cca49 100644 --- a/pkg/tfbridge/schema.go +++ b/pkg/tfbridge/schema.go @@ -479,51 +479,40 @@ func (ctx *conversionContext) makeTerraformInput( oldObject = old.ObjectValue() } - var input map[string]interface{} - if shimutil.IsOfTypeMap(tfs) { - var tfsElem shim.Schema - if tfs != nil { - if s, ok := tfs.Elem().(shim.Schema); ok { - tfsElem = s - } - } - var psElem *SchemaInfo - if ps != nil { - psElem = ps.Elem - } - var err error - input, err = ctx.makeMapTerraformInputs(oldObject, v.ObjectValue(), - tfsElem, psElem) - if err != nil { - return nil, err - } - } else { - var tfflds shim.SchemaMap - if tfs != nil { - if res, isres := tfs.Elem().(shim.Resource); isres { - tfflds = res.Schema() - } + var tfflds shim.SchemaMap + + // We cannot use [shimutil.CastToTypeObject] because we have machinery that constructs invalid + // resource objects, such as [elemSchemas]. + if tfs != nil { + if r, ok := tfs.Elem().(shim.Resource); ok { + tfflds = r.Schema() } + } + + if tfflds != nil { var psflds map[string]*SchemaInfo if ps != nil { psflds = ps.Fields } - var err error - input, err = ctx.makeObjectTerraformInputs(oldObject, v.ObjectValue(), - tfflds, psflds, rawNames) + obj, err := ctx.makeObjectTerraformInputs(oldObject, v.ObjectValue(), tfflds, psflds) if err != nil { return nil, err } - } - // If we have schema information that indicates that this value is being presented to a map-typed field whose - // Elem is a shim.Resource, wrap the value in an array in order to work around a bug in Terraform. - if tfs != nil && tfs.Type() == shim.TypeMap { - if _, hasResourceElem := tfs.Elem().(shim.Resource); hasResourceElem { - return []interface{}{input}, nil + if tfs.Type() == shim.TypeMap { + // If we have schema information that indicates that this value is being + // presented to a map-typed field whose Elem is a shim.Resource, wrap the + // value in an array in order to work around a bug in Terraform. + // + // TODO[pulumi/pulumi-terraform-bridge#1828]: This almost certainly stems from + // a bug in the bridge's implementation and not terraform's implementation. + return []interface{}{obj}, nil } + return obj, nil } - return input, nil + + etfs, eps := elemSchemas(tfs, ps) + return ctx.makeMapTerraformInputs(oldObject, v.ObjectValue(), etfs, eps) case v.IsComputed() || v.IsOutput(): // If any variables are unknown, we need to mark them in the inputs so the config map treats it right. This // requires the use of the special UnknownVariableValue sentinel in Terraform, which is how it internally stores @@ -533,7 +522,6 @@ func (ctx *conversionContext) makeTerraformInput( contract.Failf("Unexpected value marshaled: %v", v) return nil, nil } - } // makeTerraformInputs takes a property map plus custom schema info and does whatever is necessary @@ -545,7 +533,7 @@ func (ctx *conversionContext) makeTerraformInputs( tfs shim.SchemaMap, ps map[string]*SchemaInfo, ) (map[string]interface{}, error) { - return ctx.makeObjectTerraformInputs(olds, news, tfs, ps, false /*rawNames*/) + return ctx.makeObjectTerraformInputs(olds, news, tfs, ps) } // Should only be called from inside makeTerraformInputs. Variation for makeTerraformInputs used @@ -591,14 +579,14 @@ func (ctx *conversionContext) makeMapTerraformInputs( return result, nil } -// Should only be called from inside makeTerraformInputs. This variation should only be handling the -// case when an object type is expected, or else the schema is lost and the translation is not sure -// of the expected type. The case when map types are expected is handled by makeMapTerraformInputs. +// Should only be called from inside makeTerraformInputs. This variation should only be +// handling the case when an object type is expected. The case when map types are expected +// or the schema is lost and the translation is not sure of the expected type is handled +// by makeMapTerraformInputs. func (ctx *conversionContext) makeObjectTerraformInputs( olds, news resource.PropertyMap, tfs shim.SchemaMap, ps map[string]*SchemaInfo, - rawNames bool, ) (map[string]interface{}, error) { result := make(map[string]interface{}) tfAttributesToPulumiProperties := make(map[string]string) @@ -612,14 +600,8 @@ func (ctx *conversionContext) makeObjectTerraformInputs( } // First translate the Pulumi property name to a Terraform name. - name, tfi, psi := getInfoFromPulumiName(key, tfs, ps, rawNames) - - // rawNames=true indicate that we are processing a map[string,X], not an object type, and therefore - // should not be renaming terraform_style names to pulumiStyle names or assuming that the map keys are - // non-empty. - if !rawNames { - contract.Assertf(name != "", `name != ""`) - } + name, tfi, psi := getInfoFromPulumiName(key, tfs, ps, false) + contract.Assertf(name != "", `Object properties cannot be empty`) if _, duplicate := result[name]; duplicate { // If multiple Pulumi `key`s map to the same Terraform attribute `name`, then @@ -644,7 +626,7 @@ func (ctx *conversionContext) makeObjectTerraformInputs( } // And then translate the property value. - v, err := ctx.makeTerraformInput(name, old, value, tfi, psi, rawNames) + v, err := ctx.makeTerraformInput(name, old, value, tfi, psi, false) if err != nil { return nil, err } @@ -653,7 +635,7 @@ func (ctx *conversionContext) makeObjectTerraformInputs( } // Now enumerate and propagate defaults if the corresponding values are still missing. - if err := ctx.applyDefaults(result, olds, news, tfs, ps, rawNames); err != nil { + if err := ctx.applyDefaults(result, olds, news, tfs, ps, false); err != nil { return nil, err } @@ -661,7 +643,7 @@ func (ctx *conversionContext) makeObjectTerraformInputs( // Iterate over the TF schema and add an empty array for each nil MaxItemsOne property. tfs.Range(func(key string, value shim.Schema) bool { // First do a lookup of the name/info. - _, tfi, psi := getInfoFromTerraformName(key, tfs, ps, rawNames) + _, tfi, psi := getInfoFromTerraformName(key, tfs, ps, false) if IsMaxItemsOne(tfi, psi) && result[key] == nil { result[key] = []interface{}{} } diff --git a/pkg/tfbridge/schema_test.go b/pkg/tfbridge/schema_test.go index 0ca185d90..93313087c 100644 --- a/pkg/tfbridge/schema_test.go +++ b/pkg/tfbridge/schema_test.go @@ -3082,7 +3082,7 @@ func Test_makeTerraformInputsNoDefaults(t *testing.T) { }, }), //nolint:lll - expect: autogold.Expect(map[string]interface{}{"property_a": "a", "property_b": true, "property_c": map[string]interface{}{"nested_property_a": true}}), + expect: autogold.Expect(map[string]interface{}{"property_a": "a", "property_b": true, "property_c": map[string]interface{}{"nestedPropertyA": true}}), }, { testCaseName: "list_nested_block",