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) + } + }) + } +}