Strategy for testing and erroring on "deep unknowns" in `ValidateConfig()`

Here’s a little pretend sample from a ValidateConfig() method where the schema includes a SetNestedAttribute:

	// lets just pretend there's a real object set pulled from the config here
	myObjectSet := types.SetNull(types.ObjectType{AttrTypes: map[string]attr.Type{
		"name":  types.StringType,
		"attr1": types.StringType,
	}})

	type myObject struct {
		Name  types.String `tfsdk:"name"`
		Attr1 types.String `tfsdk:"attr1"`
	}

	// cannot validate with unknown data
	if myObjectSet.IsUnknown() {
		return
	}

	// let's call this "loop 1"
	for _, val := range myObjectSet.Elements() {
		// val is an attr.Value
		if val.IsUnknown() {
			return
		}
	}

	// extract objects from the config
	var myObjectSlice []myObject
	resp.Diagnostics.Append(myObjectSet.ElementsAs(ctx, &myObjectSlice, false)...)
	if resp.Diagnostics.HasError() {
		return
	}

	// map to test for name unique-ness
	nameMap := make(map[string]bool)

	// let's call this "loop 2"
	for _, obj := range myObjectSlice {
		if obj.Name.IsUnknown() {
			return // cannot validate with unknown data
		}
		if nameMap[obj.Name.ValueString()] {
			resp.Diagnostics.AddError("name collision", "two objects cannot use the same name")
		} else {
			nameMap[obj.Name.ValueString()] = true
		}
	}

There’s two things I don’t like about this code:

  1. loop_1 and loop_2 are looping over the same data, but in different format (attr.value in loop_1 and myObject in loop_2)

  2. I’d prefer to use AddAttributeError() in loop_2, but building the attribute path with AtSetValue() requires an attr.Value, which is the format I have in loop_1.

So… Questions:

  1. Is this layer-by-layer testing for Unknown() the right way to go about it? I’ve created some contrived situations with badly delayed computed strings, so it seems like I need to be this careful, but I’d be delighted to be wrong about this.

  2. Should I move the loop_2 checks into loop_1 and use val.(types.Object).As() to get ahold of a myObject? I’m mostly sure this type assertion is safe…

  3. Any other criticisms or comments are welcome!

Hey there @hQnVyLRx :wave:,

Going through each of your questions:

  1. Is this layer-by-layer testing for Unknown() the right way to go about it? I’ve created some contrived situations with badly delayed computed strings, so it seems like I need to be this careful, but I’d be delighted to be wrong about this.

Yeah that’s the right way to go about it. It’s always possible for config values to be unknown so returning early is the right call. We do have that mentioned in the validation documentation as well: Plugin Development - Framework: Validation | Terraform | HashiCorp Developer (can’t link the header, but it’s the first blue Note section)

  1. Should I move the loop_2 checks into loop_1 and use val.(types.Object).As() to get ahold of a myObject? I’m mostly sure this type assertion is safe…

You’re correct that the type assertion is safe in your current use-case, as it’s operating on a SetNestedAttribute, which always is represented by a types.Set with a types.Object element: Plugin Development - Framework: Set Nested Attribute | Terraform | HashiCorp Developer

Technically that assertion is not true of all types.Set, as the element can be any type: Plugin Development - Framework: Set Type | Terraform | HashiCorp Developer

So the short answer, is yes in your case, that type assertion is valid. Our team uses a linter that only allows testing the assertion, to avoid panics, so you could always return a diagnostic/return early if you want to be really safe.

objVal, ok := val.(types.Object)
if !ok {
   return
}

Putting all that feedback together, you’d get a validation function that might look like this:

Schema + validate function:

func (r *thingResource) Schema(ctx context.Context, req resource.SchemaRequest, resp *resource.SchemaResponse) {
	resp.Schema = schema.Schema{
		Attributes: map[string]schema.Attribute{
			"configure_set": schema.SetNestedAttribute{
				Required: true,
				NestedObject: schema.NestedAttributeObject{
					Attributes: map[string]schema.Attribute{
						"name": schema.StringAttribute{
							Required: true,
						},
						"attr1": schema.StringAttribute{
							Required: true,
						},
					},
				},
			},
		},
	}
}

type thingResourceModel struct {
	ConfigureSet types.Set `tfsdk:"configure_set"` // myObject
}
type myObject struct {
	Name  types.String `tfsdk:"name"`
	Attr1 types.String `tfsdk:"attr1"`
}

func (o *myObject) AttrTypes() map[string]attr.Type {
	return map[string]attr.Type{
		"name":  types.StringType,
		"attr1": types.StringType,
	}
}

func (r *thingResource) ValidateConfig(ctx context.Context, req resource.ValidateConfigRequest, resp *resource.ValidateConfigResponse) {
	var data thingResourceModel

	resp.Diagnostics.Append(req.Config.Get(ctx, &data)...)
	if resp.Diagnostics.HasError() {
		return
	}

	if data.ConfigureSet.IsUnknown() {
		return
	}

	nameMap := make(map[string]bool)

	for _, val := range data.ConfigureSet.Elements() {
		if val.IsUnknown() {
			return
		}

		objVal, ok := val.(types.Object)
		if !ok {
			return
		}

		var obj myObject
		resp.Diagnostics.Append(objVal.As(ctx, &obj, basetypes.ObjectAsOptions{})...)
		if resp.Diagnostics.HasError() {
			return
		}

		if obj.Name.IsUnknown() {
			return
		}
		if nameMap[obj.Name.ValueString()] {
			resp.Diagnostics.AddAttributeError(
				path.Root("configure_set").AtSetValue(val),
				"Name Collision",
				"Two objects cannot use the same name",
			)
		} else {
			nameMap[obj.Name.ValueString()] = true
		}
	}
}

Config

resource "examplecloud_thing" "this" {
  configure_set = [
    {
      name  = "abc",
      attr1 = "123"
    },
    {
      name  = "abc",
      attr1 = "456"
    }
  ]
}

Output from terraform validate

 $ terraform validate
╷
│ Error: Name Collision
│ 
│   with examplecloud_thing.this,
│   on resource.tf line 9, in resource "examplecloud_thing" "this":
│    9:   configure_set = [
│   10:     {
│   11:       name  = "abc",
│   12:       attr1 = "123"
│   13:     },
│   14:     {
│   15:       name  = "abc",
│   16:       attr1 = "456"
│   17:     }
│   18:   ]
│ 
│ Two objects cannot use the same name

Side note: AtSetValue should probably be pointing the diagnostic to val mentioned in the diagnostic, but I found an open bug in Terraform core preventing that: Config validation errors not citing the expected configuration attribute/line · Issue #33491 · hashicorp/terraform · GitHub.

  1. Any other criticisms or comments are welcome!

Missed this :thinking:

You may already know this, but if re-use is a concern, you could also implement Attribute Validation for a set using the validator.Set interface.

You could even make it generic with the comparable type constraint from Go 1.20 if you really want to get crazy (not official guidance, just speculating). This is a “napkin math” comparable example I whipped up:

Validator

func AttributeUniqueValidator[T comparable](attrName string, getCompareKey func(attr.Value) T) validator.Set {
	return attributeConflictValidator[T]{
		attrName:      attrName,
		getCompareKey: getCompareKey,
	}
}

var _ validator.Set = attributeConflictValidator[string]{}

type attributeConflictValidator[T comparable] struct {
	attrName      string
	getCompareKey func(attr.Value) T
}

func (v attributeConflictValidator[T]) Description(ctx context.Context) string {
	return ""
}

func (v attributeConflictValidator[T]) MarkdownDescription(ctx context.Context) string {
	return v.Description(ctx)
}

func (v attributeConflictValidator[T]) ValidateSet(ctx context.Context, req validator.SetRequest, resp *validator.SetResponse) {
	if req.ConfigValue.IsNull() || req.ConfigValue.IsUnknown() {
		return
	}

	_, ok := req.ConfigValue.ElementType(ctx).(basetypes.ObjectTypable)

	if !ok {
		resp.Diagnostics.AddAttributeError(
			req.Path,
			"Invalid Validator for Element Type",
			"While performing schema-based validation, an unexpected error occurred. "+
				"The attribute declares a Object values validator, however its values do not implement types.ObjectType or the types.ObjectTypable interface for custom Object types. "+
				"Use the appropriate values validator that matches the element type. "+
				"This is always an issue with the provider and should be reported to the provider developers.\n\n"+
				fmt.Sprintf("Path: %s\n", req.Path.String())+
				fmt.Sprintf("Element Type: %T\n", req.ConfigValue.ElementType(ctx)),
		)

		return
	}

	nameMap := make(map[T]bool)

	for _, element := range req.ConfigValue.Elements() {
		objectPath := req.Path.AtSetValue(element)

		objectValuable, ok := element.(basetypes.ObjectValuable)

		if !ok {
			resp.Diagnostics.AddAttributeError(
				req.Path,
				"Invalid Validator for Element Value",
				"While performing schema-based validation, an unexpected error occurred. "+
					"The attribute declares a Object values validator, however its values do not implement types.ObjectType or the types.ObjectTypable interface for custom Object types. "+
					"This is likely an issue with terraform-plugin-framework and should be reported to the provider developers.\n\n"+
					fmt.Sprintf("Path: %s\n", req.Path.String())+
					fmt.Sprintf("Element Type: %T\n", req.ConfigValue.ElementType(ctx))+
					fmt.Sprintf("Element Value Type: %T\n", element),
			)

			return
		}

		objectValue, diags := objectValuable.ToObjectValue(ctx)

		resp.Diagnostics.Append(diags...)
		if diags.HasError() {
			return
		}

		for name, attr := range objectValue.Attributes() {
			if name != v.attrName || attr.IsUnknown() {
				continue
			}

			compareKey := v.getCompareKey(attr)
			if nameMap[compareKey] {
				resp.Diagnostics.AddAttributeError(
					objectPath,
					fmt.Sprintf("%s collision", v.attrName),
					fmt.Sprintf("Two objects cannot use the same %s", v.attrName),
				)
			} else {
				nameMap[compareKey] = true
			}
			break
		}
	}
}

Schema

func (r *thingResource) Schema(ctx context.Context, req resource.SchemaRequest, resp *resource.SchemaResponse) {
	resp.Schema = schema.Schema{
		Attributes: map[string]schema.Attribute{
			"configure_set": schema.SetNestedAttribute{
				Required: true,
				Validators: []validator.Set{
					AttributeUniqueValidator("name", func(v attr.Value) string {
						strValue := v.(types.String)
						return strValue.ValueString()
					}),
					AttributeUniqueValidator("number", func(v attr.Value) int64 {
						strValue := v.(types.Int64)
						return strValue.ValueInt64()
					}),
				},
				NestedObject: schema.NestedAttributeObject{
					Attributes: map[string]schema.Attribute{
						"name": schema.StringAttribute{
							Required: true,
						},
						"number": schema.Int64Attribute{
							Required: true,
						},
					},
				},
			},
		},
	}
}

Sample validation errors

│ Error: number collision
│ 
│   with examplecloud_thing.this,
│   on resource.tf line 9, in resource "examplecloud_thing" "this":
│    9:   configure_set = [
│   10:     {
│   11:       name   = "abc",
│   12:       number = 123
│   13:     },
│   14:     {
│   15:       name   = "def",
│   16:       number = 123
│   17:     }
│   18:   ]
│ 
│ Two objects cannot use the same number

│ Error: name collision
│ 
│   with examplecloud_thing.this,
│   on resource.tf line 9, in resource "examplecloud_thing" "this":
│    9:   configure_set = [
│   10:     {
│   11:       name   = "abc",
│   12:       number = 123
│   13:     },
│   14:     {
│   15:       name   = "abc",
│   16:       number = 456
│   17:     }
│   18:   ]
│ 
│ Two objects cannot use the same name

That can probably be cleaned up immensely, but just some random thoughts since you asked :smile: and hoping it’ll spur up a better idea

Thank you @austin.valle.

I was pretty sure the type assertion was going to be okay. I’m going to go ahead (without the assertion check) with it.

Looping over the data twice (Elements() and ElementsAs()) felt terrible.

I’m aware of the error path to nested objects bug. Absent specific advice from folks on the terraform side of the protocol, I’ve decided to write my error paths as precisely/correctly as I can manage. I hope something changes here.

A set validator which looks for name collisions is a good/interesting idea. I’ve written a bunch of not-very-reusable validators which I kinda regret. This one looks like it might be worth doing, so I appreciate the suggestion.

I’ve taken a second (closer!) look at @austin.valle’s collision detection validator.

I no longer think it “might be worth doing”. I love it. Definitely implementing something like this.

1 Like