Attribute plan modification

[kmoe]

Closes #34

Design doc: #64

Needs #107

This PR implements schema.Attribute.PlanModifiers and the RequiresReplace and RequiresReplaceIf helpers, as described in the design doc.

Some more design points that arose during implementation:

1. When multiple AttributePlanModifiers are specified, should the req.AttributePlan value supplied to each be the resp.AttributePlan value from the previous modifier? This seems the intuitive behaviour (see chained_modifiers test for example).
2. If an error diagnostic is returned at some point during attribute plan modification, should RequiresReplace and the planned state still be populated in the server response? This PR assumes so, and that it is Core's responsibility to decide what to do in this case. This also makes it easier to write deterministic tests, as there is otherwise a race condition between early return on certain errors, and RequiresReplace being populated.

[paddycarver]

When multiple AttributePlanModifiers are specified, should the req.AttributePlan value supplied to each be the resp.AttributePlan value from the previous modifier? This seems the intuitive behaviour (see chained_modifiers test for example).

This matches my expectations. I'm not sure how we'd let them all operate on the request; what if two or more modified the same field in different ways? Which result would we use?

If an error diagnostic is returned at some point during attribute plan modification, should RequiresReplace and the planned state still be populated in the server response? This PR assumes so, and that it is Core's responsibility to decide what to do in this case. This also makes it easier to write deterministic tests, as there is otherwise a race condition between early return on certain errors, and RequiresReplace being populated.

This also matches my expectations, and my mental model of how plans works tells me it doesn't matter. Unlike applies, plans shouldn't get persisted in the face of an error, I believe, so if an error diagnostic is being returned, I think the plan is just going to get discarded, anyways. So this seems like reasonable behavior that won't have any external negative impacts, as far as I know.

[kmoe]

what if two or more modified the same field in different ways? Which result would we use?

Attribute.PlanModifiers is a slice of AttributePlanModifiers, executed in order. Each one can modify only the planned value for that attribute. Given this I think it's clear that if several plan modifiers on one attribute modify its value, the last modified value should be used.

Some complexity comes with nested attributes - currently it's possible to define PlanModifiers at several levels of the nested attribute hierarchy, e.g.:

Schema{
	Attributes: map[string]Attribute{
		"disks": {
			Optional: true,
			Computed: true,
			PlanModifiers: ..., // modifies "disks"
			Attributes: ListNestedAttributes(map[string]Attribute{
				"name": {
					Required: true,
					Type:     types.StringType,
					PlanModifiers: ..., // modifies "disks[0].name" etc
				},
				"size_gb": {
					Required: true,
					Type:     types.NumberType,
				},
				"boot": {
					Required: true,
					Type:     types.BoolType,
				},
			}, ListNestedAttributesOptions{}),
		},
	},
}

Currently, the top-level PlanModifiers are run first, followed by any modifiers on nested attributes.

This at least needs to be noted in the GoDoc and a test case added, so I'll do that.

If we like we could also prohibit PlanModifiers on Attributes without a Type, but I don't see a need to do this as long as the order is unambiguous and clearly documented.

[paddycarver]

A few comments, mostly about some testing stuff and about recursion.

[paddycarver]

Hmm, rather than new modifiers on cmp.Diff, does implementing an Equal method allow us to make this work without the cmpopts?

[kmoe]

Yes, that would work. We could do one of the following:

1. Define a type RequiresReplacePaths []*tftypes.AttributePath in the framework with an Equal method that ignores ordering
2. Do something similar but in plugin-go/tftypes itself.

However, I think it might be better just to have the framework server sort them so the response is deterministic, rather than ignoring the change of order during testing. Added 583f552 and a8991d7 for this purpose.

[paddycarver]

Also, some merge conflicts to resolve, I believe.

[paddycarver]

Attribute.PlanModifiers is a slice of AttributePlanModifiers, executed in order. Each one can modify only the planned value for that attribute. Given this I think it's clear that if several plan modifiers on one attribute modify its value, the last modified value should be used.

Yeah, sorry, I think we're getting at the same thing here. I was trying to say I don't know how it would work if we didn't take that approach.

Some complexity comes with nested attributes - currently it's possible to define PlanModifiers at several levels of the nested attribute hierarchy, e.g.:

[snip]

Currently, the top-level PlanModifiers are run first, followed by any modifiers on nested attributes.

This at least needs to be noted in the GoDoc and a test case added, so I'll do that.

If we like we could also prohibit PlanModifiers on Attributes without a Type, but I don't see a need to do this as long as the order is unambiguous and clearly documented.

Yeah, I'm fine with just "top-level goes first, nested ones follow", that seems like a really reasonable approach and I think the complexity is worth the extra power.

[appilon]

Would this feature allow a provider to specify defaults for optional fields?

[kmoe]

@appilon Yes, thanks for raising this. Adding a test case to show this working as expected.

A helper func like this makes sense I think:

Schema{
	Attributes: map[string]Attribute{
		"size": {
			Optional: true,
			PlanModifiers: []AttributePlanModifier{AttributeDefault(val)},
		},
	},
}

where we have either func AttributeDefault(v interface{}) AttributePlanModifier or func AttributeDefault(v attr.Value) AttributePlanModifier. Similar helper makes sense for a DefaultFunc equivalent.

Will follow up with a further PR for any such helpers. Also considering putting RequiresReplace and any other helpers in their own package so they are namespaced.

[kmoe]

Implementation of full recursion of nested attributes works, but the tests are currently running into #112 as tftypes.WalkAttributePath (called in Plan.terraformValueAtPath()) cannot handle a nil tftypes.Object. I suspect this is a plugin-go fix and will write it up there.

[tckb]

+1 waiting for this PR merged - I have the same issue as mentioned in #112

[kmoe]

In fact I think this doesn't require a plugin-go change (see comment on #112).

Changes since review:

• fixed error when a SingleNestedAttributes attribute has a nil plan value
• fixed recursion, so attribute plan modifiers run for arbitrarily nested attributes
• added sorting and deduplication to resp.RequiresReplace slice so it is entirely deterministic
• added test case for setting default attribute values
• added early return after first plan modifier that errors, instead of continuing with possibly invalid plan values

Ready for review.

[paddycarver]

Awesome work! This looks great to me.

[paddycarver]

nitpick: if we're going to have a constructor, I feel like the underlying type could be unexported to limit our compatibility surface. If we're going to export the underlying type, I don't know that we need a constructor. 🤔

(This should not block merge, we can iterate later.)

[bflad]

Is this last line expected? I think its just extraneous since they should point to the same address, but it could be confusing as it looks like it might be removing SetAttribute diagnostics.

[github-actions]

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