Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run plan modifiers from bottom to top for nested objects #974

Open
nickexported opened this issue Apr 3, 2024 · 4 comments
Open

Run plan modifiers from bottom to top for nested objects #974

nickexported opened this issue Apr 3, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@nickexported
Copy link

nickexported commented Apr 3, 2024

Module version

v1.7.0

Use-cases

Consider following schema.

	resp.Schema = schema.Schema{
		Attributes: map[string]schema.Attribute{
			"id": schema.StringAttribute{
				Computed:      true,
				PlanModifiers: []planmodifier.String{stringplanmodifier.UseStateForUnknown()},
			},
			"name": schema.StringAttribute{
				Required: true,
			},
			"cities": schema.SingleNestedAttribute{
				Required:      true,
				PlanModifiers: []planmodifier.Object{objectplanmodifier.UseStateForUnknown(), objectplanmodifier.RequiresReplace()},
				Attributes: map[string]schema.Attribute{
					"season": schema.StringAttribute{
						Required: true,
					},
					"computed": schema.StringAttribute{
						PlanModifiers: []planmodifier.String{stringplanmodifier.UseStateForUnknown()},
						Computed: true,
					},
				},
			},
		},
	}

After apply of following config

resource "copperfield_tour" "smth" {
  name = "john"
  cities = {
      season = "spring"
  }
}

I get state:

resource "copperfield_tour" "smth" {
  id = "123"
  name = "john"
  cities = {
      season = "spring"
      computed = "true"
  }
}

Now I want to update resource with following config:

resource "copperfield_tour" "smth" {
  name = "bob"
  cities = {
      season = "spring"
  }
}

And now terraform wants to recreate this reource because of unknown value for computed, however I configured field computed to use state for unknow. That is a bit frustrating and unexpected behaviour.
The key is in ths function https://github.com/hashicorp/terraform-plugin-framework/blob/main/internal/fwserver/attribute_plan_modification.go#L85 What it does is that it runs plan modifier for top level object and only after that for nested object, but at that time objectplanmodifier.RequiresReplace() has already marked resource for recreate.

Attempted Solutions

I would suggest to modify function AttributeModifyPlan so it checks if this object has nested objects and if true applies plan modifiers for nested objects, and only after that proceeds to it's own plan modifiers.

Proposal

Basically I suggest to put switch case after nested attribute object

func AttributeModifyPlan(ctx context.Context, a fwschema.Attribute, req ModifyAttributePlanRequest, resp *ModifyAttributePlanResponse) {
	ctx = logging.FrameworkWithAttributePath(ctx, req.AttributePath.String())

	if req.Private != nil {
		resp.Private = req.Private
	}

	// Null and unknown values should not have nested schema to modify.
	if nestedAttribute, ok := a.(fwschema.NestedAttribute); ok && !resp.AttributePlan.IsNull() && !resp.AttributePlan.IsUnknown() {

		nestedAttributeObject := nestedAttribute.GetNestedObject()

		nm := nestedAttribute.GetNestingMode()
		switch nm {
		case fwschema.NestingModeList:
			configList, diags := coerceListValue(ctx, req.AttributePath, req.AttributeConfig)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			// Use response as the planned value may have been modified with list
			// plan modifiers.
			planListValuable, diags := coerceListValuable(ctx, req.AttributePath, resp.AttributePlan)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			typable, diags := coerceListTypable(ctx, req.AttributePath, planListValuable)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			planList, diags := planListValuable.ToListValue(ctx)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			stateList, diags := coerceListValue(ctx, req.AttributePath, req.AttributeState)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			planElements := planList.Elements()

			for idx, planElem := range planElements {
				attrPath := req.AttributePath.AtListIndex(idx)

				configObject, diags := listElemObject(ctx, attrPath, configList, idx, fwschemadata.DataDescriptionConfiguration)

				resp.Diagnostics.Append(diags...)

				if resp.Diagnostics.HasError() {
					return
				}

				planObject, diags := coerceObjectValue(ctx, attrPath, planElem)

				resp.Diagnostics.Append(diags...)

				if resp.Diagnostics.HasError() {
					return
				}

				planObjectValuable, diags := coerceObjectValuable(ctx, attrPath, planElem)

				resp.Diagnostics.Append(diags...)

				if resp.Diagnostics.HasError() {
					return
				}

				typable, diags := coerceObjectTypable(ctx, attrPath, planObjectValuable)

				resp.Diagnostics.Append(diags...)

				if resp.Diagnostics.HasError() {
					return
				}

				stateObject, diags := listElemObject(ctx, attrPath, stateList, idx, fwschemadata.DataDescriptionState)

				resp.Diagnostics.Append(diags...)

				if resp.Diagnostics.HasError() {
					return
				}

				objectReq := planmodifier.ObjectRequest{
					Config:         req.Config,
					ConfigValue:    configObject,
					Path:           attrPath,
					PathExpression: attrPath.Expression(),
					Plan:           req.Plan,
					PlanValue:      planObject,
					Private:        resp.Private,
					State:          req.State,
					StateValue:     stateObject,
				}
				objectResp := &ModifyAttributePlanResponse{
					AttributePlan: objectReq.PlanValue,
					Private:       objectReq.Private,
				}

				NestedAttributeObjectPlanModify(ctx, nestedAttributeObject, objectReq, objectResp)

				respValue, diags := coerceObjectValue(ctx, attrPath, objectResp.AttributePlan)

				resp.Diagnostics.Append(diags...)

				if resp.Diagnostics.HasError() {
					return
				}

				// A custom value type must be returned in the final response to prevent
				// later correctness errors.
				// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/821
				respValuable, diags := typable.ValueFromObject(ctx, respValue)

				resp.Diagnostics.Append(diags...)

				if resp.Diagnostics.HasError() {
					return
				}

				planElements[idx] = respValuable
				resp.Diagnostics.Append(objectResp.Diagnostics...)
				resp.Private = objectResp.Private
				resp.RequiresReplace.Append(objectResp.RequiresReplace...)
			}

			respValue, diags := types.ListValue(planList.ElementType(ctx), planElements)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			// A custom value type must be returned in the final response to prevent
			// later correctness errors.
			// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/754
			respValuable, diags := typable.ValueFromList(ctx, respValue)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			resp.AttributePlan = respValuable
		case fwschema.NestingModeSet:
			configSet, diags := coerceSetValue(ctx, req.AttributePath, req.AttributeConfig)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			// Use response as the planned value may have been modified with set
			// plan modifiers.
			planSetValuable, diags := coerceSetValuable(ctx, req.AttributePath, resp.AttributePlan)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			typable, diags := coerceSetTypable(ctx, req.AttributePath, planSetValuable)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			planSet, diags := planSetValuable.ToSetValue(ctx)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			stateSet, diags := coerceSetValue(ctx, req.AttributePath, req.AttributeState)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			planElements := planSet.Elements()

			for idx, planElem := range planElements {
				attrPath := req.AttributePath.AtSetValue(planElem)

				configObject, diags := setElemObject(ctx, attrPath, configSet, idx, fwschemadata.DataDescriptionConfiguration)

				resp.Diagnostics.Append(diags...)

				if resp.Diagnostics.HasError() {
					return
				}

				planObject, diags := coerceObjectValue(ctx, attrPath, planElem)

				resp.Diagnostics.Append(diags...)

				if resp.Diagnostics.HasError() {
					return
				}

				planObjectValuable, diags := coerceObjectValuable(ctx, attrPath, planElem)

				resp.Diagnostics.Append(diags...)

				if resp.Diagnostics.HasError() {
					return
				}

				typable, diags := coerceObjectTypable(ctx, attrPath, planObjectValuable)

				resp.Diagnostics.Append(diags...)

				if resp.Diagnostics.HasError() {
					return
				}

				stateObject, diags := setElemObject(ctx, attrPath, stateSet, idx, fwschemadata.DataDescriptionState)

				resp.Diagnostics.Append(diags...)

				if resp.Diagnostics.HasError() {
					return
				}

				objectReq := planmodifier.ObjectRequest{
					Config:         req.Config,
					ConfigValue:    configObject,
					Path:           attrPath,
					PathExpression: attrPath.Expression(),
					Plan:           req.Plan,
					PlanValue:      planObject,
					Private:        resp.Private,
					State:          req.State,
					StateValue:     stateObject,
				}
				objectResp := &ModifyAttributePlanResponse{
					AttributePlan: objectReq.PlanValue,
					Private:       objectReq.Private,
				}

				NestedAttributeObjectPlanModify(ctx, nestedAttributeObject, objectReq, objectResp)

				respValue, diags := coerceObjectValue(ctx, attrPath, objectResp.AttributePlan)

				resp.Diagnostics.Append(diags...)

				if resp.Diagnostics.HasError() {
					return
				}

				// A custom value type must be returned in the final response to prevent
				// later correctness errors.
				// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/821
				respValuable, diags := typable.ValueFromObject(ctx, respValue)

				resp.Diagnostics.Append(diags...)

				if resp.Diagnostics.HasError() {
					return
				}

				planElements[idx] = respValuable
				resp.Diagnostics.Append(objectResp.Diagnostics...)
				resp.Private = objectResp.Private
				resp.RequiresReplace.Append(objectResp.RequiresReplace...)
			}

			respValue, diags := types.SetValue(planSet.ElementType(ctx), planElements)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			// A custom value type must be returned in the final response to prevent
			// later correctness errors.
			// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/754
			respValuable, diags := typable.ValueFromSet(ctx, respValue)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			resp.AttributePlan = respValuable
		case fwschema.NestingModeMap:
			configMap, diags := coerceMapValue(ctx, req.AttributePath, req.AttributeConfig)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			// Use response as the planned value may have been modified with map
			// plan modifiers.
			planMapValuable, diags := coerceMapValuable(ctx, req.AttributePath, resp.AttributePlan)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			typable, diags := coerceMapTypable(ctx, req.AttributePath, planMapValuable)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			planMap, diags := planMapValuable.ToMapValue(ctx)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			stateMap, diags := coerceMapValue(ctx, req.AttributePath, req.AttributeState)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			planElements := planMap.Elements()

			for key, planElem := range planElements {
				attrPath := req.AttributePath.AtMapKey(key)

				configObject, diags := mapElemObject(ctx, attrPath, configMap, key, fwschemadata.DataDescriptionConfiguration)

				resp.Diagnostics.Append(diags...)

				if resp.Diagnostics.HasError() {
					return
				}

				planObject, diags := coerceObjectValue(ctx, attrPath, planElem)

				resp.Diagnostics.Append(diags...)

				if resp.Diagnostics.HasError() {
					return
				}

				planObjectValuable, diags := coerceObjectValuable(ctx, attrPath, planElem)

				resp.Diagnostics.Append(diags...)

				if resp.Diagnostics.HasError() {
					return
				}

				typable, diags := coerceObjectTypable(ctx, attrPath, planObjectValuable)

				resp.Diagnostics.Append(diags...)

				if resp.Diagnostics.HasError() {
					return
				}

				stateObject, diags := mapElemObject(ctx, attrPath, stateMap, key, fwschemadata.DataDescriptionState)

				resp.Diagnostics.Append(diags...)

				if resp.Diagnostics.HasError() {
					return
				}

				objectReq := planmodifier.ObjectRequest{
					Config:         req.Config,
					ConfigValue:    configObject,
					Path:           attrPath,
					PathExpression: attrPath.Expression(),
					Plan:           req.Plan,
					PlanValue:      planObject,
					Private:        resp.Private,
					State:          req.State,
					StateValue:     stateObject,
				}
				objectResp := &ModifyAttributePlanResponse{
					AttributePlan: objectReq.PlanValue,
					Private:       objectReq.Private,
				}

				NestedAttributeObjectPlanModify(ctx, nestedAttributeObject, objectReq, objectResp)

				respValue, diags := coerceObjectValue(ctx, attrPath, objectResp.AttributePlan)

				resp.Diagnostics.Append(diags...)

				if resp.Diagnostics.HasError() {
					return
				}

				// A custom value type must be returned in the final response to prevent
				// later correctness errors.
				// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/821
				respValuable, diags := typable.ValueFromObject(ctx, respValue)

				resp.Diagnostics.Append(diags...)

				if resp.Diagnostics.HasError() {
					return
				}

				planElements[key] = respValuable
				resp.Diagnostics.Append(objectResp.Diagnostics...)
				resp.Private = objectResp.Private
				resp.RequiresReplace.Append(objectResp.RequiresReplace...)
			}

			respValue, diags := types.MapValue(planMap.ElementType(ctx), planElements)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			// A custom value type must be returned in the final response to prevent
			// later correctness errors.
			// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/754
			respValuable, diags := typable.ValueFromMap(ctx, respValue)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			resp.AttributePlan = respValuable
		case fwschema.NestingModeSingle:
			configObject, diags := coerceObjectValue(ctx, req.AttributePath, req.AttributeConfig)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			// Use response as the planned value may have been modified with object
			// plan modifiers.
			planObjectValuable, diags := coerceObjectValuable(ctx, req.AttributePath, resp.AttributePlan)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			typable, diags := coerceObjectTypable(ctx, req.AttributePath, planObjectValuable)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			planObject, diags := planObjectValuable.ToObjectValue(ctx)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			stateObject, diags := coerceObjectValue(ctx, req.AttributePath, req.AttributeState)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			objectReq := planmodifier.ObjectRequest{
				Config:         req.Config,
				ConfigValue:    configObject,
				Path:           req.AttributePath,
				PathExpression: req.AttributePathExpression,
				Plan:           req.Plan,
				PlanValue:      planObject,
				Private:        resp.Private,
				State:          req.State,
				StateValue:     stateObject,
			}
			objectResp := &ModifyAttributePlanResponse{
				AttributePlan: objectReq.PlanValue,
				Private:       objectReq.Private,
			}

			NestedAttributeObjectPlanModify(ctx, nestedAttributeObject, objectReq, objectResp)

			resp.Diagnostics.Append(objectResp.Diagnostics...)
			resp.Private = objectResp.Private
			resp.RequiresReplace.Append(objectResp.RequiresReplace...)

			respValue, diags := coerceObjectValue(ctx, req.AttributePath, objectResp.AttributePlan)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			// A custom value type must be returned in the final response to prevent
			// later correctness errors.
			// Reference: https://github.com/hashicorp/terraform-plugin-framework/issues/754
			respValuable, diags := typable.ValueFromObject(ctx, respValue)

			resp.Diagnostics.Append(diags...)

			if resp.Diagnostics.HasError() {
				return
			}

			resp.AttributePlan = respValuable
		default:
			err := fmt.Errorf("unknown attribute nesting mode (%T: %v) at path: %s", nm, nm, req.AttributePath)
			resp.Diagnostics.AddAttributeError(
				req.AttributePath,
				"Attribute Plan Modification Error",
				"Attribute plan modifier cannot walk schema. Report this to the provider developer:\n\n"+err.Error(),
			)

			return
		}
	}

	switch attributeWithPlanModifiers := a.(type) {
	case fwxschema.AttributeWithBoolPlanModifiers:
		AttributePlanModifyBool(ctx, attributeWithPlanModifiers, req, resp)
	case fwxschema.AttributeWithFloat64PlanModifiers:
		AttributePlanModifyFloat64(ctx, attributeWithPlanModifiers, req, resp)
	case fwxschema.AttributeWithInt64PlanModifiers:
		AttributePlanModifyInt64(ctx, attributeWithPlanModifiers, req, resp)
	case fwxschema.AttributeWithListPlanModifiers:
		AttributePlanModifyList(ctx, attributeWithPlanModifiers, req, resp)
	case fwxschema.AttributeWithMapPlanModifiers:
		AttributePlanModifyMap(ctx, attributeWithPlanModifiers, req, resp)
	case fwxschema.AttributeWithNumberPlanModifiers:
		AttributePlanModifyNumber(ctx, attributeWithPlanModifiers, req, resp)
	case fwxschema.AttributeWithObjectPlanModifiers:
		AttributePlanModifyObject(ctx, attributeWithPlanModifiers, req, resp)
	case fwxschema.AttributeWithSetPlanModifiers:
		AttributePlanModifySet(ctx, attributeWithPlanModifiers, req, resp)
	case fwxschema.AttributeWithStringPlanModifiers:
		AttributePlanModifyString(ctx, attributeWithPlanModifiers, req, resp)
	case fwxschema.AttributeWithDynamicPlanModifiers:
		AttributePlanModifyDynamic(ctx, attributeWithPlanModifiers, req, resp)
	}

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

References

@nickexported nickexported added the enhancement New feature or request label Apr 3, 2024
@nickexported
Copy link
Author

I would like to know if there is a reason that it is done this way and how I can implement my schema that I define top level attribute as RequiresReplace but it doesn't affect computed fields.

@chrismarget
Copy link

Hey @nickexported,

I can't help with your problem, but I'm tickled to see the copperfield provider show up here :)

@austinvalle
Copy link
Member

austinvalle commented Apr 4, 2024

Hey there @nickexported 👋🏻 , thanks for reporting the bug and sorry you're running into trouble here.

I've been trying to recreate this issue with the schema provided and a small sandbox example using it, see:

I'm not able to recreate the behavior you're describing, so I might need more information about your resource code. (CRUD methods, any other plan modification at the resource level, etc.) Perhaps you can take a look at the sandbox example I provided and see if I'm missing anything there.

Your interpretation of the framework plan modification logic is correct, the object in this scenario will be marked as RequiresReplace = true, however I believe Terraform should be ignoring this since the framework logic will still apply plan modification to the rest of the object, resulting in a planned value that has no changes when it gets back to Terraform (due to the UseStateForUnknown plan modifiers).

Can you also share what version of Terraform you're running into this issue with? Perhaps there is a bug somewhere else down the line.... I've been using v1.8.0-rc1 and v1.7.5

@austinvalle austinvalle added bug Something isn't working waiting-response Issues or pull requests waiting for an external response and removed enhancement New feature or request labels Apr 4, 2024
@shiyuhang0
Copy link

shiyuhang0 commented Apr 10, 2024

I think I have a similar problem when using

  • github.com/hashicorp/terraform-plugin-framework v1.6.0
  • go 1.21

schema

	resp.Schema = schema.Schema{
		Attributes: map[string]schema.Attribute{
			"id": schema.StringAttribute{
				Required:            true,
			},
			"name": schema.StringAttribute{
				Required:            true,
			},
			"status": schema.SingleNestedAttribute{
				Computed:            true,
				PlanModifiers: []planmodifier.Object{
					objectplanmodifier.UseStateForUnknown(),
				},
				Attributes: map[string]schema.Attribute{
					"version": schema.StringAttribute{
						Computed:            true,
						PlanModifiers: []planmodifier.String{
							stringplanmodifier.UseStateForUnknown(),
						},
					},
					"cluster_status": schema.StringAttribute{
						Computed:            true,
					},
				},
			},
		},
	}

In my case. I find the cluster_status will apply the UseStateForUnknown modifier. (it does not show know after apply when I update) I think it should not apply the UseStateForUnknown because I do not define it at all.

When I remove the objectplanmodifier.UseStateForUnknown() in the status attribute. Both version and cluster_status show know after apply when I update.

It seems the PlanModifiers defined in the netsed attributes do not work.

@github-actions github-actions bot removed the waiting-response Issues or pull requests waiting for an external response label Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants