From 8559eb39da67af5f622c204fa93a4e722f2cd74d Mon Sep 17 00:00:00 2001 From: Anton Tayanovskyy Date: Tue, 16 Jul 2024 12:45:07 -0400 Subject: [PATCH 1/3] Adjust documentation for Elem() Toward fixing #1828 Document current findings and add tests for PF representations. --- pf/tests/repr_test.go | 177 ++++++++++++++++++++++++++++++++++++++++++ pkg/tfshim/shim.go | 18 +++-- 2 files changed, 188 insertions(+), 7 deletions(-) create mode 100644 pf/tests/repr_test.go diff --git a/pf/tests/repr_test.go b/pf/tests/repr_test.go new file mode 100644 index 000000000..52f526518 --- /dev/null +++ b/pf/tests/repr_test.go @@ -0,0 +1,177 @@ +// Copyright 2016-2024, Pulumi Corporation. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package tfbridgetests + +import ( + "context" + "encoding/json" + "testing" + + "github.com/hashicorp/terraform-plugin-framework/provider" + "github.com/hashicorp/terraform-plugin-framework/resource/schema" + "github.com/hexops/autogold/v2" + "github.com/pulumi/pulumi-terraform-bridge/pf/internal/schemashim" + pb "github.com/pulumi/pulumi-terraform-bridge/pf/tests/internal/providerbuilder" + "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge" + "github.com/stretchr/testify/require" +) + +// Test how various PF-based schemata translate to the shim.Schema layer. +func TestRepresentations(t *testing.T) { + + type testCase struct { + name string + provider provider.Provider + expect autogold.Value + } + + testCases := []testCase{ + { + "single-nested-block", + &pb.Provider{ + AllResources: []pb.Resource{{ + ResourceSchema: schema.Schema{ + Blocks: map[string]schema.Block{ + "single_nested_block": schema.SingleNestedBlock{ + Attributes: map[string]schema.Attribute{ + "a1": schema.Float64Attribute{ + Optional: true, + }, + }, + }, + }, + }, + }}, + }, + autogold.Expect(`{ + "resources": { + "_": { + "single_nested_block": { + "element": { + "resource": { + "a1": { + "optional": true, + "type": 3 + } + } + }, + "optional": true, + "type": 6 + } + } + } +}`), + }, + { + "list-nested-block", + &pb.Provider{ + AllResources: []pb.Resource{{ + ResourceSchema: schema.Schema{ + Blocks: map[string]schema.Block{ + "list_nested_block": schema.ListNestedBlock{ + NestedObject: schema.NestedBlockObject{ + Attributes: map[string]schema.Attribute{ + "a1": schema.Float64Attribute{ + Optional: true, + }, + }, + }, + }, + }, + }, + }}, + }, + autogold.Expect(`{ + "resources": { + "_": { + "list_nested_block": { + "element": { + "resource": { + "a1": { + "optional": true, + "type": 3 + } + } + }, + "optional": true, + "type": 5 + } + } + } +}`), + }, + { + "map-nested-attribute", + &pb.Provider{ + AllResources: []pb.Resource{{ + ResourceSchema: schema.Schema{ + Attributes: map[string]schema.Attribute{ + "map_nested_attribute": schema.MapNestedAttribute{ + NestedObject: schema.NestedAttributeObject{ + Attributes: map[string]schema.Attribute{ + "a1": schema.StringAttribute{ + Optional: true, + }, + }, + }, + }, + }, + }, + }}, + }, + autogold.Expect(`{ + "resources": { + "_": { + "map_nested_attribute": { + "element": { + "schema": { + "element": { + "resource": { + "a1": { + "optional": true, + "type": 4 + } + } + }, + "type": 6 + } + }, + "type": 6 + } + } + } +}`), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + shimmedProvider := schemashim.ShimSchemaOnlyProvider(context.Background(), tc.provider) + + m := tfbridge.MarshalProvider(shimmedProvider) + bytes, err := json.Marshal(m) + require.NoError(t, err) + + var pretty map[string]any + err = json.Unmarshal(bytes, &pretty) + require.NoError(t, err) + + prettyBytes, err := json.MarshalIndent(pretty, "", " ") + require.NoError(t, err) + + tc.expect.Equal(t, string(prettyBytes)) + }) + } +} diff --git a/pkg/tfshim/shim.go b/pkg/tfshim/shim.go index 6fb38d34c..464eb7b9e 100644 --- a/pkg/tfshim/shim.go +++ b/pkg/tfshim/shim.go @@ -105,15 +105,16 @@ type Schema interface { // s.Elem() may return a nil, a Schema value, or a Resource value. // - // The design of Elem() follows Terraform Plugin SDK directly. Case analysis: - // // Case 1: s represents a compound type (s.Type() is one of TypeList, TypeSet or TypeMap), and s.Elem() // represents the element of this type as a Schema value. That is, if s ~ List[String] then s.Elem() ~ String. // // Case 2: s represents a single-nested Terraform block. Logically this is like s having an anonymous object // type such as s ~ {"x": Int, "y": String}. In this case s.Type() == TypeMap and s.Elem() is a Resource value. - // This value is not a real Resource and only implements the Schema field to enable inspecting s.Elem().Schema() - // to find out the names ("x", "y") and types (Int, String) of the block properties. + // This s.Elem() value is not a real Resource and only implements the Schema field to enable inspecting + // s.Elem().Schema() to find out the names ("x", "y") and types (Int, String) of the block properties. An + // additional distinction is that SDKv2 providers cannot represent single-nested blocks at all, so this case is + // only used for Plugin Framework providers. What SDKv2 providers typically do instead is declare a List-nested + // block with MaxItems=1. // // Case 3: s represents a list or set-nested Terraform block. That is, s ~ List[{"x": Int, "y": String}]. In // this case s.Type() is one of TypeList, TypeSet, and s.Elem() is a Resource that encodes the object type @@ -123,11 +124,14 @@ type Schema interface { // // Case 5: s.Elem() is nil but s.Type() is one of TypeList, TypeSet, TypeMap. The element type is unknown. // - // This encoding cannot support map-nested blocks or object types as it would introduce confusion with Case 2, - // because Map[String, {"x": Int}] and {"x": Int} both have s.Type() = TypeMap and s.Elem() being a Resource. - // Following the Terraform design, only set and list-nested blocks are supported. + // This encoding cannot support map-nested blocks but it does not need to as those are not expressible in TF. // // See also: https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/schema/schema.go#L231 + // + // There is the additional complication for SDKv2-based providers, case 2 should not be used but is tolerated + // for backwards-compatibility per [2], but is interpreted to mean a string-string map. + // + // [2]: https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/schema/core_schema_test.go#L220 Elem() interface{} MaxItems() int From 2fea5a630b4333d7652988c37892a48fd725a1a7 Mon Sep 17 00:00:00 2001 From: Anton Tayanovskyy Date: Tue, 16 Jul 2024 13:24:30 -0400 Subject: [PATCH 2/3] Test object attributes --- pf/tests/repr_test.go | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/pf/tests/repr_test.go b/pf/tests/repr_test.go index 52f526518..2a3e83a95 100644 --- a/pf/tests/repr_test.go +++ b/pf/tests/repr_test.go @@ -19,8 +19,10 @@ import ( "encoding/json" "testing" + "github.com/hashicorp/terraform-plugin-framework/attr" "github.com/hashicorp/terraform-plugin-framework/provider" "github.com/hashicorp/terraform-plugin-framework/resource/schema" + "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hexops/autogold/v2" "github.com/pulumi/pulumi-terraform-bridge/pf/internal/schemashim" pb "github.com/pulumi/pulumi-terraform-bridge/pf/tests/internal/providerbuilder" @@ -152,6 +154,38 @@ func TestRepresentations(t *testing.T) { } } } +}`), + }, + { + "object-attribute", + &pb.Provider{ + AllResources: []pb.Resource{{ + ResourceSchema: schema.Schema{ + Attributes: map[string]schema.Attribute{ + "object_attribute": schema.ObjectAttribute{ + AttributeTypes: map[string]attr.Type{ + "a1": types.StringType, + }, + }, + }, + }, + }}, + }, + autogold.Expect(`{ + "resources": { + "_": { + "object_attribute": { + "element": { + "resource": { + "a1": { + "type": 4 + } + } + }, + "type": 6 + } + } + } }`), }, } From f7785880158411b9c8a967981317a9e377f19ed4 Mon Sep 17 00:00:00 2001 From: Anton Tayanovskyy Date: Tue, 16 Jul 2024 16:58:48 -0400 Subject: [PATCH 3/3] PR feedback --- pf/tests/{repr_test.go => schemashim_test.go} | 2 +- pkg/tfshim/shim.go | 17 ++++++++--------- 2 files changed, 9 insertions(+), 10 deletions(-) rename pf/tests/{repr_test.go => schemashim_test.go} (98%) diff --git a/pf/tests/repr_test.go b/pf/tests/schemashim_test.go similarity index 98% rename from pf/tests/repr_test.go rename to pf/tests/schemashim_test.go index 2a3e83a95..43771ecc8 100644 --- a/pf/tests/repr_test.go +++ b/pf/tests/schemashim_test.go @@ -31,7 +31,7 @@ import ( ) // Test how various PF-based schemata translate to the shim.Schema layer. -func TestRepresentations(t *testing.T) { +func TestSchemaShimRepresentations(t *testing.T) { type testCase struct { name string diff --git a/pkg/tfshim/shim.go b/pkg/tfshim/shim.go index 464eb7b9e..3d0ca21e8 100644 --- a/pkg/tfshim/shim.go +++ b/pkg/tfshim/shim.go @@ -103,7 +103,7 @@ type Schema interface { ForceNew() bool StateFunc() SchemaStateFunc - // s.Elem() may return a nil, a Schema value, or a Resource value. + // s.Elem() may return a nil, a Schema value, or a Resource value [1]. // // Case 1: s represents a compound type (s.Type() is one of TypeList, TypeSet or TypeMap), and s.Elem() // represents the element of this type as a Schema value. That is, if s ~ List[String] then s.Elem() ~ String. @@ -111,10 +111,10 @@ type Schema interface { // Case 2: s represents a single-nested Terraform block. Logically this is like s having an anonymous object // type such as s ~ {"x": Int, "y": String}. In this case s.Type() == TypeMap and s.Elem() is a Resource value. // This s.Elem() value is not a real Resource and only implements the Schema field to enable inspecting - // s.Elem().Schema() to find out the names ("x", "y") and types (Int, String) of the block properties. An - // additional distinction is that SDKv2 providers cannot represent single-nested blocks at all, so this case is - // only used for Plugin Framework providers. What SDKv2 providers typically do instead is declare a List-nested - // block with MaxItems=1. + // s.Elem().Schema() to find out the names ("x", "y") and types (Int, String) of the block properties. SDKv2 + // providers cannot represent single-nested blocks; this case is only used for Plugin Framework providers. SDKv2 + // providers use a convention to declare a List-nested block with MaxItems=1 to model object types. Per [2] + // SDKv2 providers reinterpret case 2 as a string-string map for backwards compatibility. // // Case 3: s represents a list or set-nested Terraform block. That is, s ~ List[{"x": Int, "y": String}]. In // this case s.Type() is one of TypeList, TypeSet, and s.Elem() is a Resource that encodes the object type @@ -126,12 +126,11 @@ type Schema interface { // // This encoding cannot support map-nested blocks but it does not need to as those are not expressible in TF. // - // See also: https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/schema/schema.go#L231 - // - // There is the additional complication for SDKv2-based providers, case 2 should not be used but is tolerated - // for backwards-compatibility per [2], but is interpreted to mean a string-string map. + // A test suite [3] is provided to explore how Plugin Framework constructs map to Schema. // + // [1]: https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/schema/schema.go#L231 // [2]: https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/schema/core_schema_test.go#L220 + // [3]: https://github.com/pulumi/pulumi-terraform-bridge/blob/master/pf/tests/schemashim_test.go#L34 Elem() interface{} MaxItems() int