From 5bd3d807b0f801a994dd3192e93ac48dca741824 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 makes SchemaNestingModeSingle blocks always `Optional` and not `Computed`, so that they always generate a configurable spec field. `SchemaNestingModeSingle` is only present in Plugin Framework resource schemas, and FW resources never set `Min/MaxItems`. The heuristic does not make sense here. 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 | 33 +- pkg/types/conversion/tfjson/tfjson_test.go | 543 +++++++++++++++++++++ 2 files changed, 564 insertions(+), 12 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 b4e53047..6ad96aa1 100644 --- a/pkg/types/conversion/tfjson/tfjson.go +++ b/pkg/types/conversion/tfjson/tfjson.go @@ -147,6 +147,10 @@ func tfJSONBlockTypeToV2Schema(nb *tfjson.SchemaBlockType) *schemav2.Schema { // // https://github.com/hashicorp/terraform-plugin-sdk/blob/6461ac6e9044a44157c4e2c8aec0f1ab7efc2055/helper/schema/core_schema.go#L204 v2sch.Computed = false v2sch.Optional = false + if nb.MinItems > 0 { + v2sch.Required = true + v2sch.Optional = false + } if nb.MinItems == 0 { v2sch.Optional = true } @@ -154,24 +158,29 @@ func tfJSONBlockTypeToV2Schema(nb *tfjson.SchemaBlockType) *schemav2.Schema { // v2sch.Computed = true } - switch nb.NestingMode { //nolint:exhaustive + switch nb.NestingMode { case tfjson.SchemaNestingModeSet: v2sch.Type = schemav2.TypeSet case tfjson.SchemaNestingModeList: v2sch.Type = schemav2.TypeList case tfjson.SchemaNestingModeMap: v2sch.Type = schemav2.TypeMap - case tfjson.SchemaNestingModeSingle: - 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 { - v2sch.MinItems = 1 - } + case tfjson.SchemaNestingModeSingle, tfjson.SchemaNestingModeGroup: + // This is a Plugin Framework-only nesting mode, and + // FW schemas never get Min/MaxItems specified in their + // core schemas (tfjson schema). + // See https://github.com/hashicorp/terraform-plugin-framework/blob/a0219204842978493e5f7742b0c06d5c39951e73/internal/fwschema/block.go#L24 + // Therefore, the heuristics for determining Required/Optional/Computed + // does not make sense and inconclusive. Always make them optional, + // so that they generate a configurable spec field in the CRD. + // This only affects the CRD generation. Runtime schema validations + // are still valid and relevant. + // Provider developers can manually override these in their provider, + // if they want observation-only, or make them Required. + v2sch.Type = SchemaTypeObject + v2sch.Required = false + v2sch.Optional = true + v2sch.Computed = false v2sch.MaxItems = 1 default: panic("unhandled nesting mode: " + nb.NestingMode) diff --git a/pkg/types/conversion/tfjson/tfjson_test.go b/pkg/types/conversion/tfjson/tfjson_test.go new file mode 100644 index 00000000..f4606285 --- /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 always optional regardless, and Type=SchemaTypeObject.", + 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: SchemaTypeObject, + Required: false, + Optional: true, + Computed: false, + MinItems: 0, + MaxItems: 1, + Elem: &schemav2.Resource{ + Schema: map[string]*schemav2.Schema{ + "uid": { + Required: true, + }, + "folder_uid": { + Optional: true, + }, + }, + }, + }, + }, + }, + "SchemaNestingModeSingleWithOnlyOptionalChildren": { + reason: "Plugin Framework single block optional children should be always optional, and Type=SchemaTypeObject", + 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: SchemaTypeObject, + 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, not Computed, and Type=SchemaTypeObject.", + args: args{ + nb: &tfjson.SchemaBlockType{ + NestingMode: tfjson.SchemaNestingModeSingle, + MinItems: 0, + MaxItems: 0, + Block: &tfjson.SchemaBlock{}, + }, + }, + want: want{ + schema: &schemav2.Schema{ + Type: SchemaTypeObject, + 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, not Computed, and Type=SchemaTypeObject.", + args: args{ + nb: &tfjson.SchemaBlockType{ + NestingMode: tfjson.SchemaNestingModeSingle, + MinItems: 0, + MaxItems: 0, + Block: nil, + }, + }, + want: want{ + schema: &schemav2.Schema{ + Type: SchemaTypeObject, + Required: false, + Optional: true, + Computed: false, + MinItems: 0, + MaxItems: 1, + }, + }, + }, + "SchemaNestingModeSingleNestedBlockWithRequiredChildren": { + reason: "Single block containing a nested block with required children, block itself should still be Optional", + 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: SchemaTypeObject, + Required: false, + Optional: true, + Computed: false, + MinItems: 0, + MaxItems: 1, + Elem: &schemav2.Resource{ + Schema: map[string]*schemav2.Schema{ + "optional_attr": { + Optional: true, + }, + "nested": { + Type: SchemaTypeObject, + Required: false, + Optional: true, + Computed: false, + MinItems: 0, + 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: true, + 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 - always Optional and Type=SchemaTypeObject.", + 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: SchemaTypeObject, + 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) + } + }) + } +}