From 5bea6888b4c8b095f5815a216187cbb017728463 Mon Sep 17 00:00:00 2001 From: Will Wernert Date: Thu, 29 Jan 2026 12:20:57 -0500 Subject: [PATCH] fix(tfjson): Do not infer Computed for SchemaNestingModeSingle blocks When converting tfjson block types to SDK v2 schema, the Computed flag was being inferred as true when MinItems==0 && MaxItems==0. This heuristic works for SDK v2 collection types (List/Set/Map) but is incorrect for Plugin Framework resources using SchemaNestingModeSingle. Plugin Framework resources default MinItems/MaxItems to 0 for single blocks even when they contain user-configurable attributes. This caused blocks like 'metadata' and 'spec' to be incorrectly marked as Computed, excluding them from ForProvider/InitProvider parameters and only including them in Observation. This fix moves the Computed=true inference to only apply to collection nesting modes (List/Set/Map), not to SchemaNestingModeSingle. For single blocks, we now rely solely on hasRequiredChild() to determine Required/Optional, leaving Computed=false by default. This allows Plugin Framework resources (e.g., Grafana App Platform resources like AlertruleV0Alpha1) to properly expose their nested blocks in the CRD's forProvider schema. Also adds comprehensive unit tests for tfJSONBlockTypeToV2Schema and hasRequiredChild functions. Signed-off-by: Will Wernert --- pkg/types/conversion/tfjson/tfjson.go | 30 +- pkg/types/conversion/tfjson/tfjson_test.go | 543 +++++++++++++++++++++ 2 files changed, 566 insertions(+), 7 deletions(-) create mode 100644 pkg/types/conversion/tfjson/tfjson_test.go diff --git a/pkg/types/conversion/tfjson/tfjson.go b/pkg/types/conversion/tfjson/tfjson.go index 14261efb..d08cf7dc 100644 --- a/pkg/types/conversion/tfjson/tfjson.go +++ b/pkg/types/conversion/tfjson/tfjson.go @@ -150,23 +150,39 @@ func tfJSONBlockTypeToV2Schema(nb *tfjson.SchemaBlockType) *schemav2.Schema { // if nb.MinItems == 0 { v2sch.Optional = true } - if nb.MinItems == 0 && nb.MaxItems == 0 { - v2sch.Computed = true - } switch nb.NestingMode { //nolint:exhaustive case tfjson.SchemaNestingModeSet: v2sch.Type = schemav2.TypeSet + // For collection types (Set/List/Map), infer Computed when MinItems and + // MaxItems are both 0, following SDK v2 semantics. + if nb.MinItems == 0 && nb.MaxItems == 0 { + v2sch.Computed = true + } case tfjson.SchemaNestingModeList: v2sch.Type = schemav2.TypeList + // For collection types (Set/List/Map), infer Computed when MinItems and + // MaxItems are both 0, following SDK v2 semantics. + if nb.MinItems == 0 && nb.MaxItems == 0 { + v2sch.Computed = true + } case tfjson.SchemaNestingModeMap: v2sch.Type = schemav2.TypeMap - case tfjson.SchemaNestingModeSingle: + // For collection types (Set/List/Map), infer Computed when MinItems and + // MaxItems are both 0, following SDK v2 semantics. + if nb.MinItems == 0 && nb.MaxItems == 0 { + v2sch.Computed = true + } + case tfjson.SchemaNestingModeSingle, tfjson.SchemaNestingModeGroup: + // For SchemaNestingModeSingle and SchemaNestingModeGroup (Plugin + // Framework), we do NOT infer Computed=true from MinItems/MaxItems==0, + // because Plugin Framework resources default to 0 for these values even + // for user-configurable blocks. Instead, we determine Required/Optional + // based on whether the block has required children. + // Note: Group is similar to Single but guarantees non-null result even + // when the block is absent. v2sch.Type = schemav2.TypeList v2sch.MinItems = 0 - // TODO(erhan): not sure whether we need this - // the block itself can be optional, even if some child attribute - // or block is required v2sch.Required = hasRequiredChild(nb) v2sch.Optional = !v2sch.Required if v2sch.Required { diff --git a/pkg/types/conversion/tfjson/tfjson_test.go b/pkg/types/conversion/tfjson/tfjson_test.go new file mode 100644 index 00000000..edcae1b3 --- /dev/null +++ b/pkg/types/conversion/tfjson/tfjson_test.go @@ -0,0 +1,543 @@ +// SPDX-FileCopyrightText: 2023 The Crossplane Authors +// +// SPDX-License-Identifier: Apache-2.0 + +package tfjson + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + tfjson "github.com/hashicorp/terraform-json" + schemav2 "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" +) + +func TestTfJSONBlockTypeToV2Schema(t *testing.T) { + type args struct { + nb *tfjson.SchemaBlockType + } + type want struct { + schema *schemav2.Schema + } + cases := map[string]struct { + reason string + args + want + }{ + "SchemaNestingModeSingleWithRequiredChildren": { + reason: "Plugin Framework single block with required children should be Required=true, Optional=false, Computed=false.", + args: args{ + nb: &tfjson.SchemaBlockType{ + NestingMode: tfjson.SchemaNestingModeSingle, + MinItems: 0, + MaxItems: 0, + Block: &tfjson.SchemaBlock{ + Attributes: map[string]*tfjson.SchemaAttribute{ + "uid": { + Required: true, + }, + "folder_uid": { + Optional: true, + }, + }, + }, + }, + }, + want: want{ + schema: &schemav2.Schema{ + Type: schemav2.TypeList, + Required: true, + Optional: false, + Computed: false, + MinItems: 1, + MaxItems: 1, + Elem: &schemav2.Resource{ + Schema: map[string]*schemav2.Schema{ + "uid": { + Required: true, + }, + "folder_uid": { + Optional: true, + }, + }, + }, + }, + }, + }, + "SchemaNestingModeSingleWithOnlyOptionalChildren": { + reason: "Plugin Framework single block with only optional children should be Required=false, Optional=true, Computed=false. This was the bug - it was incorrectly marked Computed=true.", + args: args{ + nb: &tfjson.SchemaBlockType{ + NestingMode: tfjson.SchemaNestingModeSingle, + MinItems: 0, + MaxItems: 0, + Block: &tfjson.SchemaBlock{ + Attributes: map[string]*tfjson.SchemaAttribute{ + "overwrite": { + Optional: true, + }, + }, + }, + }, + }, + want: want{ + schema: &schemav2.Schema{ + Type: schemav2.TypeList, + Required: false, + Optional: true, + Computed: false, + MinItems: 0, + MaxItems: 1, + Elem: &schemav2.Resource{ + Schema: map[string]*schemav2.Schema{ + "overwrite": { + Optional: true, + }, + }, + }, + }, + }, + }, + "SchemaNestingModeSingleEmptyBlock": { + reason: "Single block with empty block definition should be Optional and not Computed.", + args: args{ + nb: &tfjson.SchemaBlockType{ + NestingMode: tfjson.SchemaNestingModeSingle, + MinItems: 0, + MaxItems: 0, + Block: &tfjson.SchemaBlock{}, + }, + }, + want: want{ + schema: &schemav2.Schema{ + Type: schemav2.TypeList, + Required: false, + Optional: true, + Computed: false, + MinItems: 0, + MaxItems: 1, + Elem: &schemav2.Resource{ + Schema: map[string]*schemav2.Schema{}, + }, + }, + }, + }, + "SchemaNestingModeSingleNilBlock": { + reason: "Single block with nil block definition should be Optional and not Computed.", + args: args{ + nb: &tfjson.SchemaBlockType{ + NestingMode: tfjson.SchemaNestingModeSingle, + MinItems: 0, + MaxItems: 0, + Block: nil, + }, + }, + want: want{ + schema: &schemav2.Schema{ + Type: schemav2.TypeList, + Required: false, + Optional: true, + Computed: false, + MinItems: 0, + MaxItems: 1, + }, + }, + }, + "SchemaNestingModeSingleNestedBlockWithRequiredChildren": { + reason: "Single block containing a nested block with required children should be Required because hasRequiredChild recurses.", + args: args{ + nb: &tfjson.SchemaBlockType{ + NestingMode: tfjson.SchemaNestingModeSingle, + MinItems: 0, + MaxItems: 0, + Block: &tfjson.SchemaBlock{ + Attributes: map[string]*tfjson.SchemaAttribute{ + "optional_attr": { + Optional: true, + }, + }, + NestedBlocks: map[string]*tfjson.SchemaBlockType{ + "nested": { + NestingMode: tfjson.SchemaNestingModeSingle, + MinItems: 0, + MaxItems: 0, + Block: &tfjson.SchemaBlock{ + Attributes: map[string]*tfjson.SchemaAttribute{ + "required_attr": { + Required: true, + }, + }, + }, + }, + }, + }, + }, + }, + want: want{ + schema: &schemav2.Schema{ + Type: schemav2.TypeList, + Required: true, + Optional: false, + Computed: false, + MinItems: 1, + MaxItems: 1, + Elem: &schemav2.Resource{ + Schema: map[string]*schemav2.Schema{ + "optional_attr": { + Optional: true, + }, + "nested": { + Type: schemav2.TypeList, + Required: true, + Optional: false, + Computed: false, + MinItems: 1, + MaxItems: 1, + Elem: &schemav2.Resource{ + Schema: map[string]*schemav2.Schema{ + "required_attr": { + Required: true, + }, + }, + }, + }, + }, + }, + }, + }, + }, + "SchemaNestingModeListMinMaxZero": { + reason: "SDK v2 list block with MinItems=0, MaxItems=0 should be Computed=true. This is the existing behavior we want to preserve.", + args: args{ + nb: &tfjson.SchemaBlockType{ + NestingMode: tfjson.SchemaNestingModeList, + MinItems: 0, + MaxItems: 0, + Block: &tfjson.SchemaBlock{ + Attributes: map[string]*tfjson.SchemaAttribute{ + "name": { + Optional: true, + }, + }, + }, + }, + }, + want: want{ + schema: &schemav2.Schema{ + Type: schemav2.TypeList, + Required: false, + Optional: true, + Computed: true, + MinItems: 0, + MaxItems: 0, + Elem: &schemav2.Resource{ + Schema: map[string]*schemav2.Schema{ + "name": { + Optional: true, + }, + }, + }, + }, + }, + }, + "SchemaNestingModeListWithMinItems": { + reason: "SDK v2 list block with MinItems=1 should not be Computed.", + args: args{ + nb: &tfjson.SchemaBlockType{ + NestingMode: tfjson.SchemaNestingModeList, + MinItems: 1, + MaxItems: 0, + Block: &tfjson.SchemaBlock{ + Attributes: map[string]*tfjson.SchemaAttribute{ + "name": { + Required: true, + }, + }, + }, + }, + }, + want: want{ + schema: &schemav2.Schema{ + Type: schemav2.TypeList, + Required: false, + Optional: false, + Computed: false, + MinItems: 1, + MaxItems: 0, + Elem: &schemav2.Resource{ + Schema: map[string]*schemav2.Schema{ + "name": { + Required: true, + }, + }, + }, + }, + }, + }, + "SchemaNestingModeSetMinMaxZero": { + reason: "SDK v2 set block with MinItems=0, MaxItems=0 should be Computed=true.", + args: args{ + nb: &tfjson.SchemaBlockType{ + NestingMode: tfjson.SchemaNestingModeSet, + MinItems: 0, + MaxItems: 0, + Block: &tfjson.SchemaBlock{ + Attributes: map[string]*tfjson.SchemaAttribute{ + "value": { + Optional: true, + }, + }, + }, + }, + }, + want: want{ + schema: &schemav2.Schema{ + Type: schemav2.TypeSet, + Required: false, + Optional: true, + Computed: true, + MinItems: 0, + MaxItems: 0, + Elem: &schemav2.Resource{ + Schema: map[string]*schemav2.Schema{ + "value": { + Optional: true, + }, + }, + }, + }, + }, + }, + "SchemaNestingModeMapMinMaxZero": { + reason: "SDK v2 map block with MinItems=0, MaxItems=0 should be Computed=true.", + args: args{ + nb: &tfjson.SchemaBlockType{ + NestingMode: tfjson.SchemaNestingModeMap, + MinItems: 0, + MaxItems: 0, + Block: &tfjson.SchemaBlock{ + Attributes: map[string]*tfjson.SchemaAttribute{ + "key": { + Optional: true, + }, + }, + }, + }, + }, + want: want{ + schema: &schemav2.Schema{ + Type: schemav2.TypeMap, + Required: false, + Optional: true, + Computed: true, + MinItems: 0, + MaxItems: 0, + Elem: &schemav2.Resource{ + Schema: map[string]*schemav2.Schema{ + "key": { + Optional: true, + }, + }, + }, + }, + }, + }, + "SchemaNestingModeGroupWithOptionalChildren": { + reason: "SchemaNestingModeGroup should be treated like SchemaNestingModeSingle - not Computed, and Optional when no required children.", + args: args{ + nb: &tfjson.SchemaBlockType{ + NestingMode: tfjson.SchemaNestingModeGroup, + MinItems: 0, + MaxItems: 0, + Block: &tfjson.SchemaBlock{ + Attributes: map[string]*tfjson.SchemaAttribute{ + "setting": { + Optional: true, + }, + }, + }, + }, + }, + want: want{ + schema: &schemav2.Schema{ + Type: schemav2.TypeList, + Required: false, + Optional: true, + Computed: false, + MinItems: 0, + MaxItems: 1, + Elem: &schemav2.Resource{ + Schema: map[string]*schemav2.Schema{ + "setting": { + Optional: true, + }, + }, + }, + }, + }, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got := tfJSONBlockTypeToV2Schema(tc.args.nb) + if diff := cmp.Diff(tc.want.schema, got); diff != "" { + t.Errorf("%s\ntfJSONBlockTypeToV2Schema(...): -want, +got:\n%s", tc.reason, diff) + } + }) + } +} + +func TestHasRequiredChild(t *testing.T) { + type args struct { + nb *tfjson.SchemaBlockType + } + type want struct { + result bool + } + cases := map[string]struct { + reason string + args + want + }{ + "NilBlock": { + reason: "A block type with nil Block should return false.", + args: args{ + nb: &tfjson.SchemaBlockType{Block: nil}, + }, + want: want{ + result: false, + }, + }, + "EmptyBlock": { + reason: "A block type with empty Block should return false.", + args: args{ + nb: &tfjson.SchemaBlockType{Block: &tfjson.SchemaBlock{}}, + }, + want: want{ + result: false, + }, + }, + "OnlyOptionalAttributes": { + reason: "A block with only optional attributes should return false.", + args: args{ + nb: &tfjson.SchemaBlockType{ + Block: &tfjson.SchemaBlock{ + Attributes: map[string]*tfjson.SchemaAttribute{ + "optional1": {Optional: true}, + "optional2": {Optional: true}, + }, + }, + }, + }, + want: want{ + result: false, + }, + }, + "HasRequiredAttribute": { + reason: "A block with at least one required attribute should return true.", + args: args{ + nb: &tfjson.SchemaBlockType{ + Block: &tfjson.SchemaBlock{ + Attributes: map[string]*tfjson.SchemaAttribute{ + "required": {Required: true}, + "optional": {Optional: true}, + }, + }, + }, + }, + want: want{ + result: true, + }, + }, + "NestedBlockWithRequiredChild": { + reason: "A block with a nested block that has required children should return true.", + args: args{ + nb: &tfjson.SchemaBlockType{ + Block: &tfjson.SchemaBlock{ + Attributes: map[string]*tfjson.SchemaAttribute{ + "optional": {Optional: true}, + }, + NestedBlocks: map[string]*tfjson.SchemaBlockType{ + "nested": { + Block: &tfjson.SchemaBlock{ + Attributes: map[string]*tfjson.SchemaAttribute{ + "required": {Required: true}, + }, + }, + }, + }, + }, + }, + }, + want: want{ + result: true, + }, + }, + "DeeplyNestedRequiredChild": { + reason: "A block with deeply nested required children should return true.", + args: args{ + nb: &tfjson.SchemaBlockType{ + Block: &tfjson.SchemaBlock{ + NestedBlocks: map[string]*tfjson.SchemaBlockType{ + "level1": { + Block: &tfjson.SchemaBlock{ + NestedBlocks: map[string]*tfjson.SchemaBlockType{ + "level2": { + Block: &tfjson.SchemaBlock{ + Attributes: map[string]*tfjson.SchemaAttribute{ + "required": {Required: true}, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + want: want{ + result: true, + }, + }, + "NilAttributeInMap": { + reason: "Nil attributes in the map should be safely skipped.", + args: args{ + nb: &tfjson.SchemaBlockType{ + Block: &tfjson.SchemaBlock{ + Attributes: map[string]*tfjson.SchemaAttribute{ + "nil_attr": nil, + "optional": {Optional: true}, + }, + }, + }, + }, + want: want{ + result: false, + }, + }, + "NilNestedBlockInMap": { + reason: "Nil nested blocks in the map should be safely skipped.", + args: args{ + nb: &tfjson.SchemaBlockType{ + Block: &tfjson.SchemaBlock{ + NestedBlocks: map[string]*tfjson.SchemaBlockType{ + "nil_block": nil, + }, + }, + }, + }, + want: want{ + result: false, + }, + }, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + got := hasRequiredChild(tc.args.nb) + if got != tc.want.result { + t.Errorf("%s\nhasRequiredChild(...) = %v, want %v", tc.reason, got, tc.want.result) + } + }) + } +}