Optional SingleNestedAttribute not treated as optional if nested attributes are required

[kenny-statsig]

Module version

v1.13.0

Relevant provider source code

Schema generated from OpenAPI spec

			"active_review": schema.SingleNestedAttribute{
				Attributes: map[string]schema.Attribute{
					"description": schema.StringAttribute{
						Required: true,
					},
					"review_id": schema.StringAttribute{
						Required: true,
					},
					"review_status": schema.StringAttribute{
						Required: true,
					},
				},
				CustomType: ActiveReviewType{
					ObjectType: types.ObjectType{
						AttrTypes: ActiveReviewValue{}.AttributeTypes(ctx),
					},
				},
				Optional: true,
				Computed: true,
			},

Relevant part of OpenAPI spec

          "activeReview": {
            "type": "object",
            "properties": {
              "reviewID": {
                "type": "string"
              },
              "reviewStatus": {
                "type": "string"
              },
              "description": {
                "type": "string"
              }
            },
            "required": [
              "reviewID",
              "reviewStatus",
              "description"
            ]
          }
        },
        "required": [
          "id",
          "description",
          "lastModifierID",
          "lastModifiedTime",
          "lastModifierEmail",
          "lastModifierName",
          "createdTime",
          "creatorName",
          "checksPerHour",
          "status",
          "type",
          "typeReason",
          "isEnabled",
          "rules"
        ]

Terraform Configuration Files

resource "statsig_experiment" "full_experiment" {
  name        = "full_experiment"
  description = "A short description of what we are experimenting on."
  id_type     = "userID"
  allocation  = 12.3
  status      = "setup"
  hypothesis  = "Move some metrics"
  layer_id    = "a_layer"
  tags = [
    "test-tag-a",
    "test-tag-b"
  ]
  primary_metrics = [
    {
      name = "d1_retention_rate"
      type = "user"
    }
  ]
  primary_metric_tags = [
    "test-tag-a"
  ]
  secondary_metrics = [
    {
      name = "new_dau"
      type = "user"
    }
  ]
  secondary_metric_tags = [
    "test-tag-b"
  ]
  groups = [
    {
      name             = "Test A"
      size             = 33.3
      parameter_values = { "a_string" : "test_a" }
    },
    {
      name             = "Test B"
      size             = 33.3
      parameter_values = { "a_string" : "test_b" }
    },
    {
      name             = "Control"
      size             = 33.4
      parameter_values = { "a_string" : "control" }
    }
  ]
  default_confidence_interval = "80"
  bonferroni_correction       = true
  duration                    = 10
  launched_group_id           = ""
  targeting_gate_id           = "targeting_gate"
}

Debug Output

https://gist.github.com/kenny-statsig/37655ad084167fe70fd327da27bb72ae

Expected Behavior

I expected the attribute active_review to be treated as optional. And only if provided in the configuration, would the attributes description, review_id, review_status be required.

Actual Behavior

My acceptance test failed due to the missing attributes active_review.description, active_review.review_id, active_review.review_status supposedly marked as required by my provider.

Error: Missing Configuration for Required Attribute
        
          with statsig_experiment.full_experiment,
          on experiment_full.tf line 3, in resource "statsig_experiment" "full_experiment":
           3:   description = "A short description of what we are experimenting on."
        
        Must set a configuration value for the active_review.description attribute as
        the provider has marked it as required.

Steps to Reproduce

Provider is still in development so cannot be reproduced externally.
But it's a simple acceptance test

func TestAccExperimentFull_MUX(t *testing.T) {
	resource.Test(t, resource.TestCase{
		ProtoV6ProviderFactories: protoV6ProviderFactories(),
		PreCheck:                 func() { testAccPreCheck(t) },
		Steps: []resource.TestStep{
			{
				ConfigFile: config.StaticFile("test_resources/experiment_full.tf"),
				ConfigPlanChecks: resource.ConfigPlanChecks{
					PreApply: []plancheck.PlanCheck{
						plancheck.ExpectEmptyPlan(),
					},
				},
			},
		},
	})
}

References

[austinvalle]

Hey there @kenny-statsig 👋🏻 , thanks for reporting the issue and sorry you're running into trouble here.

This error message is being generated by terraform-plugin-framework and it's validation, which makes me wonder if the bug is being caused by the custom type ActiveReviewType.

Specifically, the code path that is called right before this error message is the ValueFromTerraform implementation in your ActiveReviewType code. If you could provide the source code to that custom type or even just the ValueFromTerraform function, I could tell you for sure if that's where the bug is originating from and try to advise on a fix.

My suspicion is that the (ActiveReviewType).ValueFromTerraform function is not returning a null value when the tftypes.Value being passed in is null. You can see similar handling is done in the base types of the framework (which if you embed the ObjectType in your custom type, you could just call that logic directly so you don't have to reimplement it):

• terraform-plugin-framework/types/basetypes/object_type.go

  Lines 72 to 83 in c126fe4

  	if in.Type() == nil {
  	return NewObjectNull(o.AttrTypes), nil
  	}
  	if !in.Type().Equal(o.TerraformType(ctx)) {
  	return nil, fmt.Errorf("expected %s, got %s", o.TerraformType(ctx), in.Type())
  	}
  	if !in.IsKnown() {
  	return NewObjectUnknown(o.AttrTypes), nil
  	}
  	if in.IsNull() {
  	return NewObjectNull(o.AttrTypes), nil
  	}

You can see the pattern of embedding in a custom type, then offloading the majority of the ValueFromTerraform logic, in an example custom type for IPv6 strings: https://github.com/hashicorp/terraform-plugin-framework-nettypes/blob/c3118e83502ea3e76969891e19d020570554bf53/iptypes/ipv6_address_type.go#L63-L83

[kenny-statsig]

@austinvalle Thanks for the response! Here is the full implementation of ActiveReviewType and ActiveReviewValue
https://gist.github.com/kenny-statsig/5d7ab5a43ebdc6315c01331c7c6c89d6

BTW these are all generated using the OpenAPI provider spec generator. But I do see similar null value handling implemented.

func (t ActiveReviewType) ValueFromTerraform(ctx context.Context, in tftypes.Value) (attr.Value, error) {
	if in.Type() == nil {
		return NewActiveReviewValueNull(), nil
	}

	if !in.Type().Equal(t.TerraformType(ctx)) {
		return nil, fmt.Errorf("expected %s, got %s", t.TerraformType(ctx), in.Type())
	}

	if !in.IsKnown() {
		return NewActiveReviewValueUnknown(), nil
	}

	if in.IsNull() {
		return NewActiveReviewValueNull(), nil
	}

	attributes := map[string]attr.Value{}

	val := map[string]tftypes.Value{}

	err := in.As(&val)

	if err != nil {
		return nil, err
	}

	for k, v := range val {
		a, err := t.AttrTypes[k].ValueFromTerraform(ctx, v)

		if err != nil {
			return nil, err
		}

		attributes[k] = a
	}

	return NewActiveReviewValueMust(ActiveReviewValue{}.AttributeTypes(ctx), attributes), nil
}

I did also find a similar issue but for SingleNestedBlocks here. However, i'm not sure if it's relevant to attributes as well.

[austinvalle]

Thanks for providing the full custom type code! I was able to recreate the bug and it's actually originating from (ActiveReviewValue).ToObjectValue implementation.

The current implementation of ToObjectValue is returning a known object with null values, despite the entire object being null:

func (v ActiveReviewValue) ToObjectValue(ctx context.Context) (basetypes.ObjectValue, diag.Diagnostics) {
	var diags diag.Diagnostics

	objVal, diags := types.ObjectValue(
		map[string]attr.Type{
			"description":   basetypes.StringType{},
			"review_id":     basetypes.StringType{},
			"review_status": basetypes.StringType{},
		},
		map[string]attr.Value{
			"description":   v.Description,
			"review_id":     v.ReviewId,
			"review_status": v.ReviewStatus,
		})
	
	// This always is a known object with null values, regardless of what `v` is
	return objVal, diags
}

The actual implementation should look something like:

func (v ActiveReviewValue) ToObjectValue(ctx context.Context) (basetypes.ObjectValue, diag.Diagnostics) {
	var diags diag.Diagnostics

	if v.IsNull() {
		return types.ObjectNull(v.AttributeTypes(ctx)), nil
	}

	if v.IsUnknown() {
		return types.ObjectUnknown(v.AttributeTypes(ctx)), nil
	}

	objVal, diags := types.ObjectValue(
		map[string]attr.Type{
			"description":   basetypes.StringType{},
			"review_id":     basetypes.StringType{},
			"review_status": basetypes.StringType{},
		},
		map[string]attr.Value{
			"description":   v.Description,
			"review_id":     v.ReviewId,
			"review_status": v.ReviewStatus,
		})

	return objVal, diags
}

[kenny-statsig]

@austinvalle Sorry for the slow response, is there an ETA for when this might get patched? Since this is a generated file from the tf plugin framework.

I also tested this change myself and I'm seeing this error that suggests there may be some other requirement

Error: Provider returned invalid result object after apply
        
        After the apply operation, the provider still indicated an unknown value for
        statsig_experiment.full_experiment.active_review. All values must be known
        after apply, so this is always a bug in the provider and should be reported
        in the provider's own repository. Terraform will still save the other known
        object values in the state.