SetAttribute on Missing Attribute Path Silently Fails Without Update

[bflad]

Module version

v0.3.0

Description

Given a schema like:

Schema{
	Attributes: map[string]Attribute{
		"id": {
			Type:     types.StringType,
			Computed: true,
		},
	},
},

And example "empty" State assembly:

resourceType, _ := s.getResourceType(ctx, req.TypeName)
resourceSchema, _ := resourceType.GetSchema(ctx)
state := State{
	Raw:    tftypes.NewValue(resourceSchema.TerraformType(ctx), nil),
	Schema: resourceSchema,
}

With the call:

diags := state.SetAttribute(ctx, tftypes.NewAttributePath().WithAttributeName("id"), "example")
// len(diags) == 0
// before state == after state

Expected Behavior

(Plan).SetAttribute() and (State).SetAttribute() should provide feedback about not updating the state properly or the behavior is documented in the Go documentation for those methods.

Actual Behavior

Calling (State).SetAttribute() will silently fail, but the state will not be updated. This is because the transform function has no paths to match in this scenario and therefore p.Equal(path) will never match.

This was found while initially investigating how import support could work, but the same implications could apply to other places where new attribute paths are being introduced, such as (DataSource).Read() methods not calling Set() first or potentially adding elements to an existing attribute (although I have not verified either of these myself).

[kmoe]

Definitely a bug.

The original design for SetAttribute intended that the attribute should always be set for valid attribute paths, whether or not the path already exists in the state value.

There are some cases in which this is non-trivial. For example (via @paddycarver), consider a case in which a "list_of_things" has four elements, then the following is run:

SetAttribute(ctx, tftypes.NewAttributePath().WithAttributeName("list_of_things").WithElementKeyInt(12), "value")

In this case, the practitioner probably does not expect that elements 5-11 should be inserted into the list in order that element 12 be added. This call should therefore error.

There are likely other edge cases to discuss below.

This suggests to me that SetAttribute should do two things:

1. Validate the attribute path (including type constraints such as length of list)
2. Attempt to set the attribute

Either of these steps can return an error.

[bflad]

My proposal (hopefully matching everyone else's thoughts) is the following behaviors for SetAttribute():

• Errors if the attribute path or value is not valid with the current schema definition
• Path-specific rules that error otherwise:
  • WithElementKeyInt (List)
    • Allow writing key 0, creating list if non-existent
    • Allow writing key (length+1), except if list is non-existent
    • Allow overwriting any existing key
  • WithElementKeyString (Map)
    • Allow (over)writing any key, creating map if non-existent
  • WithElementKeyValue (Set)
    • Allow (over)writing any key, creating set if non-existent

These should probably be explicitly documented on the method. 😄

[paddycarver]

To capture context verbalised elsewhere, I think we have three potential approaches here:

Strict Success

We always make this succeed (as long as the path is valid for the schema), creating whatever values we need to to set the attribute that was specified. If that means inserting extra items in lists, so be it.

Strict Failure

We always make this fail when the value being pointed at by the path doesn't exist yet. When modifying a list, set, or map the entire list, set, or map must be replaced, it cannot be modified piecemeal.

Least Surprise

We do our best to do what a user would expect: create the value when it doesn't exist at the specified path, but in the case of lists, we throw an error instead of creating empty values. The only values that it's valid to create are: any element of a map, any element of a set, and only the next element in a list.

[paddycarver]

I believe Brian's proposal matches least surprise, and I think that's what I'm leaning towards. I haven't thought of any other edge cases besides adding elements to a list.

[bflad]

Another edge case is if the parent path (recursive) beyond just the List/Set/Map is missing.

For example with:

tftypes.NewAttributePath().WithAttributeName("this").WithAttributeName("list_attr")
// or nested further...
tftypes.NewAttributePath().WithAttributeName("this").WithAttributeName("list_attr").WithElementKeyInt(0)

And the this object not being present means that we would need to synthesize that value as well. I think we should also error in this case, following the same principal that this method should not "guess" the right implementation at antecedent path levels.

[paddycarver]

Hmm, I dunno if I agree on that being a surprising edge case. I think as a provider developer, I would expect tftypes.NewAttributePath().WithAttributeName("top_list").WithElementKeyInt("top_list_item").WithElementKeyInt("inner_list_item") when "top_list_item" is null to create top_list_item and set inner_let_item on it, because otherwise before I do a SetAttribute call I need to check every level with an IsNull or risk an error. I think this is meaningfully different from the example of inserting an item into the list at the not-next-position because I can reasonably see situations in which provider devs would need to populate the inside of a null attribute, but I can't really see a situation in which a provider developer would mean to create a bunch of null elements in a list just so they could set something at a particular index. So I'm okay making the latter harder to do (you need to insert them all manually) but making the former harder worries me that we're just going to be annoying providers with "are you sure?" kinda busy work on tasks that I think may be common.

Otherwise, I think we're back at lists, sets, objects, and maps pretty much always need to be set as atomic attributes, instead of being able to update specific elements/attributes in them.

[github-actions]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.