From b742d27834e16ba639554755346b43bb5d5a2f95 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Fri, 23 Jun 2023 10:55:13 -0400 Subject: [PATCH] helper/schema: Add SchemaFunc field to Resource type Reference: https://github.com/hashicorp/terraform-plugin-sdk/issues/1217 This change introduces a new `SchemaFunc` field and `SchemaMap` method to the `Resource` type. Currently, the `Schema` field data of all `Resource` is held in memory for the lifecycle of the provider server, which is problematic for providers with many resources and/or larger schemas in resources. The new field enables provider developers to swap pieces of resident memory usage for the slight additional computation time of reconstituting the data when necessary. Callers directly referencing the exported `Schema` field should switch to referencing the `SchemaMap` method, which returns the result of `SchemaFunc` or `Schema` in that preference order. To ensure internal usage was migrated to the new method, this change was performed by temporarily commenting out the `Schema` field itself with broken references in non-testing code migrated to the method. The `Schema` field is not marked as deprecated via Go documentation comment as this would introduce a major ecosystem burden to migrate with generally little benefit for most use cases. The `Resource` type `InternalValidate` method has been updated to return an error if both `Schema` and `SchemaFunc` are defined. Provider developers are encouraged to migrate resources to the terraform-plugin-framework, as it already behaves in a manner similar to `SchemaFunc` by nature of resource schema data being behind a method call, amongst many of the other benefits outlined at https://developer.hashicorp.com/terraform/plugin/framework-benefits. --- .../ENHANCEMENTS-20230623-102412.yaml | 6 ++ .../unreleased/NOTES-20230623-102528.yaml | 7 ++ helper/schema/core_schema.go | 2 +- helper/schema/field_reader.go | 2 +- helper/schema/field_reader_config.go | 2 +- helper/schema/grpc_provider.go | 2 +- helper/schema/grpc_provider_test.go | 58 ++++++++++++++++ helper/schema/resource.go | 69 ++++++++++++++----- helper/schema/resource_test.go | 45 ++++++++++++ helper/schema/schema.go | 10 +-- helper/schema/serialize.go | 2 +- helper/schema/shims.go | 2 +- 12 files changed, 177 insertions(+), 30 deletions(-) create mode 100644 .changes/unreleased/ENHANCEMENTS-20230623-102412.yaml create mode 100644 .changes/unreleased/NOTES-20230623-102528.yaml diff --git a/.changes/unreleased/ENHANCEMENTS-20230623-102412.yaml b/.changes/unreleased/ENHANCEMENTS-20230623-102412.yaml new file mode 100644 index 00000000000..1469e815729 --- /dev/null +++ b/.changes/unreleased/ENHANCEMENTS-20230623-102412.yaml @@ -0,0 +1,6 @@ +kind: ENHANCEMENTS +body: 'helper/schema: Added `Resource` type `SchemaFunc` field and `SchemaMap` method, + which can reduce resident memory usage with large schemas' +time: 2023-06-23T10:24:12.025356-04:00 +custom: + Issue: "1217" diff --git a/.changes/unreleased/NOTES-20230623-102528.yaml b/.changes/unreleased/NOTES-20230623-102528.yaml new file mode 100644 index 00000000000..5f791f4ffd4 --- /dev/null +++ b/.changes/unreleased/NOTES-20230623-102528.yaml @@ -0,0 +1,7 @@ +kind: NOTES +body: 'helper/schema: Consumers directly referencing the `Resource` type `Schema` + field should switch to the `SchemaMap` method to ensure new `SchemaFunc` field + data is properly retrieved' +time: 2023-06-23T10:25:28.864812-04:00 +custom: + Issue: "1217" diff --git a/helper/schema/core_schema.go b/helper/schema/core_schema.go index 3ded6fc612b..736af218da2 100644 --- a/helper/schema/core_schema.go +++ b/helper/schema/core_schema.go @@ -367,5 +367,5 @@ func (r *Resource) CoreConfigSchema() *configschema.Block { } func (r *Resource) coreConfigSchema() *configschema.Block { - return schemaMap(r.Schema).CoreConfigSchema() + return schemaMap(r.SchemaMap()).CoreConfigSchema() } diff --git a/helper/schema/field_reader.go b/helper/schema/field_reader.go index 02defaacf62..6aae74d95b6 100644 --- a/helper/schema/field_reader.go +++ b/helper/schema/field_reader.go @@ -84,7 +84,7 @@ func addrToSchema(addr []string, schemaMap map[string]*Schema) []*Schema { case *Resource: current = &Schema{ Type: typeObject, - Elem: v.Schema, + Elem: v.SchemaMap(), } case *Schema: current = v diff --git a/helper/schema/field_reader_config.go b/helper/schema/field_reader_config.go index 2edd8e7f09f..df317c20bb0 100644 --- a/helper/schema/field_reader_config.go +++ b/helper/schema/field_reader_config.go @@ -303,7 +303,7 @@ func (r *ConfigFieldReader) hasComputedSubKeys(key string, schema *Schema) bool switch t := schema.Elem.(type) { case *Resource: - for k, schema := range t.Schema { + for k, schema := range t.SchemaMap() { if r.Config.IsComputed(prefix + k) { return true } diff --git a/helper/schema/grpc_provider.go b/helper/schema/grpc_provider.go index 6b632f8b402..831d78777af 100644 --- a/helper/schema/grpc_provider.go +++ b/helper/schema/grpc_provider.go @@ -1303,7 +1303,7 @@ func stripResourceModifiers(r *Resource) *Resource { newResource.CustomizeDiff = nil newResource.Schema = map[string]*Schema{} - for k, s := range r.Schema { + for k, s := range r.SchemaMap() { newResource.Schema[k] = stripSchema(s) } diff --git a/helper/schema/grpc_provider_test.go b/helper/schema/grpc_provider_test.go index 3aeb3377b78..82292689da5 100644 --- a/helper/schema/grpc_provider_test.go +++ b/helper/schema/grpc_provider_test.go @@ -807,6 +807,23 @@ func TestApplyResourceChange(t *testing.T) { }, }, }, + { + Description: "CreateContext_SchemaFunc", + TestResource: &Resource{ + SchemaFunc: func() map[string]*Schema { + return map[string]*Schema{ + "id": { + Type: TypeString, + Computed: true, + }, + } + }, + CreateContext: func(_ context.Context, rd *ResourceData, _ interface{}) diag.Diagnostics { + rd.SetId("bar") // expected in response + return nil + }, + }, + }, } for _, testCase := range testCases { @@ -1254,6 +1271,47 @@ func TestReadDataSource(t *testing.T) { }, }, }, + "SchemaFunc": { + server: NewGRPCProviderServer(&Provider{ + DataSourcesMap: map[string]*Resource{ + "test": { + SchemaFunc: func() map[string]*Schema { + return map[string]*Schema{ + "id": { + Type: TypeString, + Computed: true, + }, + } + }, + ReadContext: func(ctx context.Context, d *ResourceData, meta interface{}) diag.Diagnostics { + d.SetId("test-id") + return nil + }, + }, + }, + }), + req: &tfprotov5.ReadDataSourceRequest{ + TypeName: "test", + Config: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.EmptyObject, + cty.NullVal(cty.EmptyObject), + ), + }, + }, + expected: &tfprotov5.ReadDataSourceResponse{ + State: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "id": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("test-id"), + }), + ), + }, + }, + }, "null-object": { server: NewGRPCProviderServer(&Provider{ DataSourcesMap: map[string]*Resource{ diff --git a/helper/schema/resource.go b/helper/schema/resource.go index 42030b74858..8a1472e45d8 100644 --- a/helper/schema/resource.go +++ b/helper/schema/resource.go @@ -56,13 +56,25 @@ var ReservedResourceFields = []string{ // being implemented. type Resource struct { // Schema is the structure and type information for this component. This - // field is required for all Resource concepts. + // field, or SchemaFunc, is required for all Resource concepts. To prevent + // storing all schema information in memory for the lifecycle of a provider, + // use SchemaFunc instead. // // The keys of this map are the names used in a practitioner configuration, // such as the attribute or block name. The values describe the structure // and type information of that attribute or block. Schema map[string]*Schema + // SchemaFunc is the structure and type information for this component. This + // field, or Schema, is required for all Resource concepts. Use this field + // instead of Schema on top level Resource declarations to prevent storing + // all schema information in memory for the lifecycle of a provider. + // + // The keys of this map are the names used in a practitioner configuration, + // such as the attribute or block name. The values describe the structure + // and type information of that attribute or block. + SchemaFunc func() map[string]*Schema + // SchemaVersion is the version number for this resource's Schema // definition. This field is only valid when the Resource is a managed // resource. @@ -585,6 +597,17 @@ type Resource struct { UseJSONNumber bool } +// SchemaMap returns the schema information for this Resource whether it is +// defined via the SchemaFunc field or Schema field. The SchemaFunc field, if +// defined, takes precedence over the Schema field. +func (r *Resource) SchemaMap() map[string]*Schema { + if r.SchemaFunc != nil { + return r.SchemaFunc() + } + + return r.Schema +} + // ShimInstanceStateFromValue converts a cty.Value to a // terraform.InstanceState. func (r *Resource) ShimInstanceStateFromValue(state cty.Value) (*terraform.InstanceState, error) { @@ -594,7 +617,7 @@ func (r *Resource) ShimInstanceStateFromValue(state cty.Value) (*terraform.Insta // We now rebuild the state through the ResourceData, so that the set indexes // match what helper/schema expects. - data, err := schemaMap(r.Schema).Data(s, nil) + data, err := schemaMap(r.SchemaMap()).Data(s, nil) if err != nil { return nil, err } @@ -767,7 +790,8 @@ func (r *Resource) Apply( s *terraform.InstanceState, d *terraform.InstanceDiff, meta interface{}) (*terraform.InstanceState, diag.Diagnostics) { - data, err := schemaMap(r.Schema).Data(s, d) + schema := schemaMap(r.SchemaMap()) + data, err := schema.Data(s, d) if err != nil { return s, diag.FromErr(err) } @@ -824,7 +848,7 @@ func (r *Resource) Apply( } // Reset the data to be stateless since we just destroyed - data, err = schemaMap(r.Schema).Data(nil, d) + data, err = schema.Data(nil, d) if err != nil { return nil, append(diags, diag.FromErr(err)...) } @@ -868,7 +892,7 @@ func (r *Resource) Diff( return nil, fmt.Errorf("[ERR] Error decoding timeout: %s", err) } - instanceDiff, err := schemaMap(r.Schema).Diff(ctx, s, c, r.CustomizeDiff, meta, true) + instanceDiff, err := schemaMap(r.SchemaMap()).Diff(ctx, s, c, r.CustomizeDiff, meta, true) if err != nil { return instanceDiff, err } @@ -890,7 +914,7 @@ func (r *Resource) SimpleDiff( c *terraform.ResourceConfig, meta interface{}) (*terraform.InstanceDiff, error) { - instanceDiff, err := schemaMap(r.Schema).Diff(ctx, s, c, r.CustomizeDiff, meta, false) + instanceDiff, err := schemaMap(r.SchemaMap()).Diff(ctx, s, c, r.CustomizeDiff, meta, false) if err != nil { return instanceDiff, err } @@ -915,7 +939,7 @@ func (r *Resource) SimpleDiff( // Validate validates the resource configuration against the schema. func (r *Resource) Validate(c *terraform.ResourceConfig) diag.Diagnostics { - diags := schemaMap(r.Schema).Validate(c) + diags := schemaMap(r.SchemaMap()).Validate(c) if r.DeprecationMessage != "" { diags = append(diags, diag.Diagnostic{ @@ -937,7 +961,7 @@ func (r *Resource) ReadDataApply( ) (*terraform.InstanceState, diag.Diagnostics) { // Data sources are always built completely from scratch // on each read, so the source state is always nil. - data, err := schemaMap(r.Schema).Data(nil, d) + data, err := schemaMap(r.SchemaMap()).Data(nil, d) if err != nil { return nil, diag.FromErr(err) } @@ -978,10 +1002,12 @@ func (r *Resource) RefreshWithoutUpgrade( } } + schema := schemaMap(r.SchemaMap()) + if r.Exists != nil { // Make a copy of data so that if it is modified it doesn't // affect our Read later. - data, err := schemaMap(r.Schema).Data(s, nil) + data, err := schema.Data(s, nil) if err != nil { return s, diag.FromErr(err) } @@ -1004,7 +1030,7 @@ func (r *Resource) RefreshWithoutUpgrade( } } - data, err := schemaMap(r.Schema).Data(s, nil) + data, err := schema.Data(s, nil) if err != nil { return s, diag.FromErr(err) } @@ -1023,7 +1049,7 @@ func (r *Resource) RefreshWithoutUpgrade( state = nil } - schemaMap(r.Schema).handleDiffSuppressOnRefresh(ctx, s, state) + schema.handleDiffSuppressOnRefresh(ctx, s, state) return r.recordCurrentSchemaVersion(state), diags } @@ -1069,13 +1095,14 @@ func (r *Resource) InternalValidate(topSchemaMap schemaMap, writable bool) error } } + schema := schemaMap(r.SchemaMap()) tsm := topSchemaMap if r.isTopLevel() && writable { // All non-Computed attributes must be ForceNew if Update is not defined if !r.updateFuncSet() { nonForceNewAttrs := make([]string, 0) - for k, v := range r.Schema { + for k, v := range schema { if !v.ForceNew && !v.Computed { nonForceNewAttrs = append(nonForceNewAttrs, k) } @@ -1086,19 +1113,19 @@ func (r *Resource) InternalValidate(topSchemaMap schemaMap, writable bool) error } } else { nonUpdateableAttrs := make([]string, 0) - for k, v := range r.Schema { + for k, v := range schema { if v.ForceNew || v.Computed && !v.Optional { nonUpdateableAttrs = append(nonUpdateableAttrs, k) } } - updateableAttrs := len(r.Schema) - len(nonUpdateableAttrs) + updateableAttrs := len(schema) - len(nonUpdateableAttrs) if updateableAttrs == 0 { return fmt.Errorf( "All fields are ForceNew or Computed w/out Optional, Update is superfluous") } } - tsm = schemaMap(r.Schema) + tsm = schema // Destroy, and Read are required if !r.readFuncSet() { @@ -1157,7 +1184,7 @@ func (r *Resource) InternalValidate(topSchemaMap schemaMap, writable bool) error // Data source if r.isTopLevel() && !writable { - tsm = schemaMap(r.Schema) + tsm = schema for k := range tsm { if isReservedDataSourceFieldName(k) { return fmt.Errorf("%s is a reserved field name", k) @@ -1165,6 +1192,10 @@ func (r *Resource) InternalValidate(topSchemaMap schemaMap, writable bool) error } } + if r.SchemaFunc != nil && r.Schema != nil { + return fmt.Errorf("SchemaFunc and Schema should not both be set") + } + // check context funcs are not set alongside their nonctx counterparts if r.CreateContext != nil && r.Create != nil { return fmt.Errorf("CreateContext and Create should not both be set") @@ -1207,7 +1238,7 @@ func (r *Resource) InternalValidate(topSchemaMap schemaMap, writable bool) error return fmt.Errorf("Delete and DeleteWithoutTimeout should not both be set") } - return schemaMap(r.Schema).InternalValidate(tsm) + return schema.InternalValidate(tsm) } func isReservedDataSourceFieldName(name string) bool { @@ -1254,7 +1285,7 @@ func isReservedResourceFieldName(name string) bool { // // This function is useful for unit tests and ResourceImporter functions. func (r *Resource) Data(s *terraform.InstanceState) *ResourceData { - result, err := schemaMap(r.Schema).Data(s, nil) + result, err := schemaMap(r.SchemaMap()).Data(s, nil) if err != nil { // At the time of writing, this isn't possible (Data never returns // non-nil errors). We panic to find this in the future if we have to. @@ -1281,7 +1312,7 @@ func (r *Resource) Data(s *terraform.InstanceState) *ResourceData { // TODO: May be able to be removed with the above ResourceData function. func (r *Resource) TestResourceData() *ResourceData { return &ResourceData{ - schema: r.Schema, + schema: r.SchemaMap(), } } diff --git a/helper/schema/resource_test.go b/helper/schema/resource_test.go index debb55f08cf..5c9fd629ba9 100644 --- a/helper/schema/resource_test.go +++ b/helper/schema/resource_test.go @@ -1080,6 +1080,51 @@ func TestResourceInternalValidate(t *testing.T) { true, true, }, + 27: { // Non-Writable SchemaFunc and Schema should not both be set + In: &Resource{ + Schema: map[string]*Schema{ + "test": { + Type: TypeString, + Required: true, + }, + }, + SchemaFunc: func() map[string]*Schema { + return map[string]*Schema{ + "test": { + Type: TypeString, + Required: true, + }, + } + }, + Read: Noop, + }, + Writable: false, + Err: true, + }, + 28: { // Writable SchemaFunc and Schema should not both be set + In: &Resource{ + Schema: map[string]*Schema{ + "test": { + Type: TypeString, + Required: true, + }, + }, + SchemaFunc: func() map[string]*Schema { + return map[string]*Schema{ + "test": { + Type: TypeString, + Required: true, + }, + } + }, + Create: Noop, + Read: Noop, + Update: Noop, + Delete: Noop, + }, + Writable: true, + Err: true, + }, } for i, tc := range cases { diff --git a/helper/schema/schema.go b/helper/schema/schema.go index 61739a1bb1f..176288b0cd8 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -939,7 +939,7 @@ func (m schemaMap) internalValidate(topSchemaMap schemaMap, attrsOnly bool) erro case *Resource: attrsOnly := attrsOnly || v.ConfigMode == SchemaConfigModeAttr - if err := schemaMap(t.Schema).internalValidate(topSchemaMap, attrsOnly); err != nil { + if err := schemaMap(t.SchemaMap()).internalValidate(topSchemaMap, attrsOnly); err != nil { return err } case *Schema: @@ -1069,7 +1069,7 @@ func checkKeysAgainstSchemaFlags(k string, keys []string, topSchemaMap schemaMap return fmt.Errorf("%s configuration block reference (%s) can only be used with TypeList and MaxItems: 1 configuration blocks", k, key) } - sm = schemaMap(subResource.Schema) + sm = subResource.SchemaMap() } if target == nil { @@ -1259,7 +1259,7 @@ func (m schemaMap) diffList( case *Resource: // This is a complex resource for i := 0; i < maxLen; i++ { - for k2, schema := range t.Schema { + for k2, schema := range t.SchemaMap() { subK := fmt.Sprintf("%s.%d.%s", k, i, k2) err := m.diff(ctx, subK, schema, diff, d, all) if err != nil { @@ -1506,7 +1506,7 @@ func (m schemaMap) diffSet( switch t := schema.Elem.(type) { case *Resource: // This is a complex resource - for k2, schema := range t.Schema { + for k2, schema := range t.SchemaMap() { subK := fmt.Sprintf("%s.%s.%s", k, code, k2) err := m.diff(ctx, subK, schema, diff, d, true) if err != nil { @@ -1973,7 +1973,7 @@ func (m schemaMap) validateList( switch t := schema.Elem.(type) { case *Resource: // This is a sub-resource - diags = append(diags, m.validateObject(key, t.Schema, c, p)...) + diags = append(diags, m.validateObject(key, t.SchemaMap(), c, p)...) case *Schema: diags = append(diags, m.validateType(key, raw, t, c, p)...) } diff --git a/helper/schema/serialize.go b/helper/schema/serialize.go index 459b61314ed..d629240fd38 100644 --- a/helper/schema/serialize.go +++ b/helper/schema/serialize.go @@ -91,7 +91,7 @@ func SerializeResourceForHash(buf *bytes.Buffer, val interface{}, resource *Reso if val == nil { return } - sm := resource.Schema + sm := resource.SchemaMap() m := val.(map[string]interface{}) var keys []string allComputed := true diff --git a/helper/schema/shims.go b/helper/schema/shims.go index d6398d59cc9..e8baebd70cf 100644 --- a/helper/schema/shims.go +++ b/helper/schema/shims.go @@ -43,7 +43,7 @@ func diffFromValues(ctx context.Context, prior, planned, config cty.Value, res * removeConfigUnknowns(cfg.Config) removeConfigUnknowns(cfg.Raw) - diff, err := schemaMap(res.Schema).Diff(ctx, instanceState, cfg, cust, nil, false) + diff, err := schemaMap(res.SchemaMap()).Diff(ctx, instanceState, cfg, cust, nil, false) if err != nil { return nil, err }