diff --git a/.changes/unreleased/BUG FIXES-20250922-091942.yaml b/.changes/unreleased/BUG FIXES-20250922-091942.yaml new file mode 100644 index 00000000000..81afb7fcc5e --- /dev/null +++ b/.changes/unreleased/BUG FIXES-20250922-091942.yaml @@ -0,0 +1,5 @@ +kind: BUG FIXES +body: 'all: Prevent identity change validation from raising an error when prior identity is empty (all attributes are null)' +time: 2025-09-22T09:19:42.075962-04:00 +custom: + Issue: "1527" diff --git a/helper/schema/grpc_provider.go b/helper/schema/grpc_provider.go index 3202c504cae..2e02fde7dff 100644 --- a/helper/schema/grpc_provider.go +++ b/helper/schema/grpc_provider.go @@ -968,15 +968,7 @@ func (s *GRPCProviderServer) ReadResource(ctx context.Context, req *tfprotov5.Re return resp, nil } - isFullyNull := true - for _, v := range newIdentityVal.AsValueMap() { - if !v.IsNull() { - isFullyNull = false - break - } - } - - if isFullyNull { + if isCtyObjectNullOrEmpty(newIdentityVal) { resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf( "Missing Resource Identity After Read: The Terraform provider unexpectedly returned no resource identity after having no errors in the resource read. "+ "This is always a problem with the provider and should be reported to the provider developer", @@ -985,7 +977,7 @@ func (s *GRPCProviderServer) ReadResource(ctx context.Context, req *tfprotov5.Re } // If we're refreshing the resource state (excluding a recently imported resource), validate that the new identity isn't changing - if !res.ResourceBehavior.MutableIdentity && !readFollowingImport && !currentIdentityVal.IsNull() && !currentIdentityVal.RawEquals(newIdentityVal) { + if !res.ResourceBehavior.MutableIdentity && !readFollowingImport && !isCtyObjectNullOrEmpty(currentIdentityVal) && !currentIdentityVal.RawEquals(newIdentityVal) { resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf("Unexpected Identity Change: %s", "During the read operation, the Terraform Provider unexpectedly returned a different identity then the previously stored one.\n\n"+ "This is always a problem with the provider and should be reported to the provider developer.\n\n"+ fmt.Sprintf("Current Identity: %s\n\n", currentIdentityVal.GoString())+ @@ -1327,7 +1319,7 @@ func (s *GRPCProviderServer) PlanResourceChange(ctx context.Context, req *tfprot } // If we're updating or deleting and we already have an identity stored, validate that the planned identity isn't changing - if !res.ResourceBehavior.MutableIdentity && !create && !priorIdentityVal.IsNull() && !priorIdentityVal.RawEquals(plannedIdentityVal) { + if !res.ResourceBehavior.MutableIdentity && !create && !isCtyObjectNullOrEmpty(priorIdentityVal) && !priorIdentityVal.RawEquals(plannedIdentityVal) { resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf( "Unexpected Identity Change: During the planning operation, the Terraform Provider unexpectedly returned a different identity than the previously stored one.\n\n"+ "This is always a problem with the provider and should be reported to the provider developer.\n\n"+ @@ -1567,15 +1559,7 @@ func (s *GRPCProviderServer) ApplyResourceChange(ctx context.Context, req *tfpro return resp, nil } - isFullyNull := true - for _, v := range newIdentityVal.AsValueMap() { - if !v.IsNull() { - isFullyNull = false - break - } - } - - if isFullyNull { + if isCtyObjectNullOrEmpty(newIdentityVal) { op := "Create" if !create { op = "Update" @@ -1589,7 +1573,7 @@ func (s *GRPCProviderServer) ApplyResourceChange(ctx context.Context, req *tfpro return resp, nil } - if !res.ResourceBehavior.MutableIdentity && !create && !plannedIdentityVal.IsNull() && !plannedIdentityVal.RawEquals(newIdentityVal) { + if !res.ResourceBehavior.MutableIdentity && !create && !isCtyObjectNullOrEmpty(plannedIdentityVal) && !plannedIdentityVal.RawEquals(newIdentityVal) { resp.Diagnostics = convert.AppendProtoDiag(ctx, resp.Diagnostics, fmt.Errorf( "Unexpected Identity Change: During the update operation, the Terraform Provider unexpectedly returned a different identity than the previously stored one.\n\n"+ "This is always a problem with the provider and should be reported to the provider developer.\n\n"+ @@ -2494,3 +2478,18 @@ func (s *GRPCProviderServer) upgradeJSONIdentity(ctx context.Context, version in return m, nil } + +// isCtyObjectNullOrEmpty is a helper function that checks if a given cty object is null or if all it's immediate children are null (empty) +func isCtyObjectNullOrEmpty(val cty.Value) bool { + if val.IsNull() { + return true + } + + for _, v := range val.AsValueMap() { + if !v.IsNull() { + return false + } + } + + return true +} diff --git a/helper/schema/grpc_provider_test.go b/helper/schema/grpc_provider_test.go index df822c2b8ad..dc4530aaa95 100644 --- a/helper/schema/grpc_provider_test.go +++ b/helper/schema/grpc_provider_test.go @@ -6001,6 +6001,112 @@ New Identity: cty.ObjectVal(map[string]cty.Value{"identity":cty.StringVal("chang }, }, }, + "update-resource-identity-with-empty-prior-identity-identity-may-change": { + server: NewGRPCProviderServer(&Provider{ + ResourcesMap: map[string]*Resource{ + "test": { + SchemaVersion: 1, + Schema: map[string]*Schema{ + "id": { + Type: TypeString, + Required: true, + }, + "test": { + Type: TypeString, + }, + }, + Identity: &ResourceIdentity{ + Version: 1, + SchemaFunc: func() map[string]*Schema { + return map[string]*Schema{ + "identity_attr_a": { + Type: TypeString, + RequiredForImport: true, + }, + "identity_attr_b": { + Type: TypeInt, + OptionalForImport: true, + }, + } + }, + }, + ReadContext: func(ctx context.Context, d *ResourceData, meta interface{}) diag.Diagnostics { + identity, err := d.Identity() + if err != nil { + return diag.FromErr(err) + } + err = identity.Set("identity_attr_a", "changed") + if err != nil { + return diag.FromErr(err) + } + err = identity.Set("identity_attr_b", 20) + if err != nil { + return diag.FromErr(err) + } + + return nil + }, + }, + }, + }), + req: &tfprotov5.ReadResourceRequest{ + TypeName: "test", + CurrentIdentity: &tfprotov5.ResourceIdentityData{ + IdentityData: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "identity_attr_a": cty.String, + "identity_attr_b": cty.Number, + }), + cty.ObjectVal(map[string]cty.Value{ + "identity_attr_a": cty.NullVal(cty.String), + "identity_attr_b": cty.NullVal(cty.Number), + }), + ), + }, + }, + CurrentState: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "test": cty.String, + "id": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "test": cty.StringVal("hello"), + "id": cty.StringVal("initial"), + }), + ), + }, + }, + expected: &tfprotov5.ReadResourceResponse{ + NewState: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "id": cty.String, + "test": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("initial"), + "test": cty.StringVal("hello"), + }), + ), + }, + NewIdentity: &tfprotov5.ResourceIdentityData{ + IdentityData: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "identity_attr_a": cty.String, + "identity_attr_b": cty.Number, + }), + cty.ObjectVal(map[string]cty.Value{ + "identity_attr_a": cty.StringVal("changed"), + "identity_attr_b": cty.NumberIntVal(20), + }), + ), + }, + }, + }, + }, "does-not-remove-user-data-from-private": { server: NewGRPCProviderServer(&Provider{ ResourcesMap: map[string]*Resource{ @@ -7664,6 +7770,140 @@ Planned Identity: cty.ObjectVal(map[string]cty.Value{"identity":cty.StringVal("c }, }, }, + "update-resource-identity-with-empty-prior-identity-identity-may-change": { + server: NewGRPCProviderServer(&Provider{ + ResourcesMap: map[string]*Resource{ + "test": { + SchemaVersion: 1, + Schema: map[string]*Schema{ + "id": { + Type: TypeString, + Required: true, + }, + "test": { + Type: TypeString, + }, + }, + Identity: &ResourceIdentity{ + Version: 1, + SchemaFunc: func() map[string]*Schema { + return map[string]*Schema{ + "identity_attr_a": { + Type: TypeString, + RequiredForImport: true, + }, + "identity_attr_b": { + Type: TypeInt, + OptionalForImport: true, + }, + } + }, + }, + CustomizeDiff: func(ctx context.Context, d *ResourceDiff, meta interface{}) error { + identity, err := d.Identity() + if err != nil { + return err + } + err = identity.Set("identity_attr_a", "changed") + if err != nil { + return err + } + err = identity.Set("identity_attr_b", 20) + if err != nil { + return err + } + return nil + }, + }, + }, + }), + req: &tfprotov5.PlanResourceChangeRequest{ + TypeName: "test", + PriorState: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "id": cty.String, + "test": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("initial"), + "test": cty.StringVal("initial"), + }), + ), + }, + PriorIdentity: &tfprotov5.ResourceIdentityData{ + IdentityData: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "identity_attr_a": cty.String, + "identity_attr_b": cty.Number, + }), + cty.ObjectVal(map[string]cty.Value{ + "identity_attr_a": cty.NullVal(cty.String), + "identity_attr_b": cty.NullVal(cty.Number), + }), + ), + }, + }, + ProposedNewState: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "id": cty.String, + "test": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + "test": cty.StringVal("initial"), + }), + ), + }, + Config: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "id": cty.String, + "test": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.NullVal(cty.String), + "test": cty.StringVal("initial"), + }), + ), + }, + }, + expected: &tfprotov5.PlanResourceChangeResponse{ + PlannedState: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "id": cty.String, + "test": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.UnknownVal(cty.String), + "test": cty.StringVal("initial"), + }), + ), + }, + RequiresReplace: []*tftypes.AttributePath{ + tftypes.NewAttributePath().WithAttributeName("id"), + }, + PlannedPrivate: []byte(`{"_new_extra_shim":{}}`), + UnsafeToUseLegacyTypeSystem: true, + PlannedIdentity: &tfprotov5.ResourceIdentityData{ + IdentityData: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "identity_attr_a": cty.String, + "identity_attr_b": cty.Number, + }), + cty.ObjectVal(map[string]cty.Value{ + "identity_attr_a": cty.StringVal("changed"), + "identity_attr_b": cty.NumberIntVal(20), + }), + ), + }, + }, + }, + }, "destroy-resource-identity-may-not-change": { server: NewGRPCProviderServer(&Provider{ ResourcesMap: map[string]*Resource{ @@ -8910,6 +9150,138 @@ New Identity: cty.ObjectVal(map[string]cty.Value{"identity":cty.StringVal("chang }, }, }, + "update-resource-identity-with-empty-prior-identity-identity-may-change": { + server: NewGRPCProviderServer(&Provider{ + ResourcesMap: map[string]*Resource{ + "test": { + SchemaVersion: 1, + Schema: map[string]*Schema{ + "id": { + Type: TypeString, + Required: true, + }, + "test": { + Type: TypeString, + }, + }, + Identity: &ResourceIdentity{ + Version: 1, + SchemaFunc: func() map[string]*Schema { + return map[string]*Schema{ + "identity_attr_a": { + Type: TypeString, + RequiredForImport: true, + }, + "identity_attr_b": { + Type: TypeInt, + OptionalForImport: true, + }, + } + }, + }, + UpdateContext: func(_ context.Context, rd *ResourceData, _ interface{}) diag.Diagnostics { + identity, err := rd.Identity() + if err != nil { + return diag.FromErr(err) + } + err = identity.Set("identity_attr_a", "changed") + if err != nil { + return diag.FromErr(err) + } + err = identity.Set("identity_attr_b", 20) + if err != nil { + return diag.FromErr(err) + } + rd.SetId("changed") + return nil + }, + }, + }, + }), + req: &tfprotov5.ApplyResourceChangeRequest{ + TypeName: "test", + PriorState: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "id": cty.String, + "test": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("initial"), + "test": cty.StringVal("initial"), + }), + ), + }, + PlannedState: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "id": cty.String, + "test": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("initial"), + "test": cty.StringVal("initial"), + }), + ), + }, + PlannedIdentity: &tfprotov5.ResourceIdentityData{ + IdentityData: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "identity_attr_a": cty.String, + "identity_attr_b": cty.Number, + }), + cty.ObjectVal(map[string]cty.Value{ + "identity_attr_a": cty.NullVal(cty.String), + "identity_attr_b": cty.NullVal(cty.Number), + }), + ), + }, + }, + Config: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "id": cty.String, + "test": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.NullVal(cty.String), + "test": cty.StringVal("initial"), + }), + ), + }, + }, + expected: &tfprotov5.ApplyResourceChangeResponse{ + NewState: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "id": cty.String, + "test": cty.String, + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("changed"), + "test": cty.StringVal("initial"), + }), + ), + }, + Private: []uint8(`{"schema_version":"1"}`), + UnsafeToUseLegacyTypeSystem: true, + NewIdentity: &tfprotov5.ResourceIdentityData{ + IdentityData: &tfprotov5.DynamicValue{ + MsgPack: mustMsgpackMarshal( + cty.Object(map[string]cty.Type{ + "identity_attr_a": cty.String, + "identity_attr_b": cty.Number, + }), + cty.ObjectVal(map[string]cty.Value{ + "identity_attr_a": cty.StringVal("changed"), + "identity_attr_b": cty.NumberIntVal(20), + }), + ), + }, + }, + }, + }, "destroy-resource-identity-may-change": { server: NewGRPCProviderServer(&Provider{ ResourcesMap: map[string]*Resource{