Consider raising error when a `List` (or `Set`) attribute defines both `ElementType` and `CustomType`

ewbankkit commented 6 months ago

Module version

% go list -m github.com/hashicorp/terraform-plugin-framework/...
github.com/hashicorp/terraform-plugin-framework v1.6.1

Use-cases

If the schema definition of a List (or Set) attribute contains both ElementType and CustomType then ElementType is not effective. Consider reporting such a case during schema verification.

References

Relates https://github.com/hashicorp/terraform-plugin-docs/issues/342. Relates https://github.com/hashicorp/terraform-plugin-docs/issues/343.

bendbennett commented 6 months ago

Schema definitions which use a custom type for attributes which embed a collection type (e.g., basetypes.ListType), and do not specify the ElementType of the embedded type will generate unexpected output.

The following two examples are taken from https://github.com/hashicorp/terraform-plugin-docs/issues/342, and https://github.com/hashicorp/terraform-plugin-docs/issues/343, respectively:

"health_check_arns": schema.ListAttribute{
    ElementType: types.StringType,
    CustomType:  cctypes.MultisetType, // `MultisetType embeds `ListType` 
        /* ... */
},

"framework_controls": schema.SetNestedAttribute{
    NestedObject: schema.NestedAttributeObject{ 
        Attributes: map[string]schema.Attribute{ 
            "control_scope": schema.SingleNestedAttribute{
                Attributes: map[string]schema.Attribute{ 
                    "compliance_resource_ids": schema.ListAttribute{
                        ElementType: types.StringType,
                        CustomType:  cctypes.MultisetType, // MultisetType embeds `ListType`
                        /* ... */
                    },
                    /* ... */

In both instances, the CustomType specified (i.e., MultisetType), has an embedded ListType which has an ElementType which is nil as the ElementType defined directly on the attribute is not used by the CustomType by default.

As a consequence, this results in the GetProviderSchema RPC returning an element type of DynamicPseudoType, for instance:

    block: {
      attributes: {
        name: "framework_controls"
        nested_type: {
          attributes: {
            name: "control_scope"
            nested_type: {
              attributes: {
                name: "compliance_resource_ids"
                type: "[\"list\",\"dynamic\"]"
                description: "The ID of the only AWS resource that you want your control scope to contain."
                optional: true
                computed: true
              }

The usage of DynamicePseudoType arises because during the execution of the GetProviderSchema RPC, there is a call to SchemaAttribute(), which calls a.GetType().TerraformType(ctx), and subsequently ElementType():

func (l ListType) ElementType() attr.Type {
    if l.ElemType == nil {
        return missingType{}
    }

    return l.ElemType
}

The TerraformType() call on missingType{} returns the following:

func (t missingType) TerraformType(_ context.Context) tftypes.Type {
    return tftypes.DynamicPseudoType
}

Depending upon the where in the schema this occurs, this can give rise to unexpected results (e.g., Adding CustomType to a ListAttribute changes generated description to List of Dynamic), or errors (e.g., Unexpected error NestingSet blocks may not contain attributes of cty.DynamicPseudoType). In the latter case, the error is raised in Terraform core due to a validation failure.

bflad commented 6 months ago

Drive-by note: I see this issue two ways --

One, we could do what is proposed here since it reflects the current framework implementation. I would classify this more as a bug fix in the sense of not notifying provider developers more upfront of the errant implementation and needing to unfortunately discover/triage the issue other ways.

Two, this could potentially be turned around as a framework feature request that attributes could automatically do what you tried to do there with both fields. There is the attr.TypeWithElementType interface already available that could potentially help in this situation if the CustomType implemented it and the framework attribute GetType() logic checked for it. While it would likely solve this exact situation, I'm not sure off the top of my head whether just adjusting that GetType() logic would fully enable correct type handling everywhere else in the framework though. We would need to verify. 😄

Maybe its worth treating this issue as-is as a bug fix and creating a separate feature request to see if about the feasibility of using the additional type system interface if its detected on the custom type. When it comes to more gnarly schema definitions like collection-based nested attributes, needing to effectively repeat all the type information of a NestedObject, which already knows its underlying type information, is certainly a hassle.

hashicorp / terraform-plugin-framework