Fix import with optional identity attributes (#36887)

* tfdiags: Support null values when printing objects

* Use type with optional fields when decoding identity config

* Use provider returned identity when importing

* Add test case for import with optional identity attrs

Author: dbanck

internal/configs/configschema/implied_type.go

@@ -107,6 +107,14 @@ func (o *Object) ImpliedType() cty.Type {
 	return o.specType().WithoutOptionalAttributesDeep()
 }
 
+// ConfigType returns a cty.Type that can be used to decode a configuration
+// object using the receiving block schema.
+//
+// ConfigType will preserve optional attributes
+func (o *Object) ConfigType() cty.Type {
+	return o.specType()
+}
+
 // specType returns the cty.Type used for decoding a NestedType Attribute using
 // the receiving block schema.
 func (o *Object) specType() cty.Type {

internal/terraform/context_plan_import_test.go

@@ -1961,6 +1961,9 @@ func TestContext2Plan_importIdentityModule(t *testing.T) {
 				State: cty.ObjectVal(map[string]cty.Value{
 					"id": cty.StringVal("foo"),
 				}),
+				Identity: cty.ObjectVal(map[string]cty.Value{
+					"name": cty.StringVal("bar"),
+				}),
 			},
 		},
 	}
@@ -2138,3 +2141,79 @@ import {
 		}
 	})
 }
+
+func TestContext2Plan_importIdentityModuleWithOptional(t *testing.T) {
+	p := testProvider("aws")
+	m := testModule(t, "import-identity-module")
+
+	p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&providerSchema{
+		ResourceTypes: map[string]*configschema.Block{
+			"aws_lb": {
+				Attributes: map[string]*configschema.Attribute{
+					"id": {
+						Type:     cty.String,
+						Computed: true,
+					},
+				},
+			},
+		},
+		IdentityTypes: map[string]*configschema.Object{
+			"aws_lb": {
+				Attributes: map[string]*configschema.Attribute{
+					"name": {
+						Type:     cty.String,
+						Required: true,
+					},
+					"something": {
+						Type:     cty.Number,
+						Optional: true,
+					},
+				},
+				Nesting: configschema.NestingSingle,
+			},
+		},
+	})
+	wantIdentity := cty.ObjectVal(map[string]cty.Value{
+		"name":      cty.StringVal("bar"),
+		"something": cty.NumberIntVal(42),
+	})
+	p.ImportResourceStateResponse = &providers.ImportResourceStateResponse{
+		ImportedResources: []providers.ImportedResource{
+			{
+				TypeName: "aws_lb",
+				State: cty.ObjectVal(map[string]cty.Value{
+					"id": cty.StringVal("foo"),
+				}),
+				Identity: wantIdentity,
+			},
+		},
+	}
+	ctx := testContext2(t, &ContextOpts{
+		Providers: map[addrs.Provider]providers.Factory{
+			addrs.NewDefaultProvider("aws"): testProviderFuncFixed(p),
+		},
+	})
+
+	diags := ctx.Validate(m, &ValidateOpts{})
+	if diags.HasErrors() {
+		t.Fatalf("unexpected errors\n%s", diags.Err().Error())
+	}
+
+	plan, diags := ctx.Plan(m, states.NewState(), DefaultPlanOpts)
+	if diags.HasErrors() {
+		t.Fatalf("unexpected errors: %s", diags.Err())
+	}
+
+	addr := mustResourceInstanceAddr("aws_lb.foo")
+	instPlan := plan.Changes.ResourceInstance(addr)
+	if instPlan == nil {
+		t.Fatalf("no plan for %s at all", addr)
+	}
+
+	identityMatches := instPlan.Importing.Identity.Equals(wantIdentity)
+	if !identityMatches.True() {
+		t.Errorf("identity does not match\ngot:  %s\nwant: %s",
+			tfdiags.ObjectToString(instPlan.Importing.Identity),
+			tfdiags.ObjectToString(wantIdentity))
+	}
+}

internal/terraform/eval_import.go

@@ -88,7 +88,7 @@ func evaluateImportIdentityExpression(expr hcl.Expression, identity *configschem
 	// that context.
 	ctx = evalContextForModuleInstance(ctx, addrs.RootModuleInstance)
 	scope := ctx.EvaluationScope(nil, nil, keyData)
-	importIdentityVal, evalDiags := scope.EvalExpr(expr, identity.ImpliedType())
+	importIdentityVal, evalDiags := scope.EvalExpr(expr, identity.ConfigType())
 	if evalDiags.HasErrors() {
 		// TODO? Do we need to improve the error message?
 		return cty.NilVal, evalDiags

internal/terraform/node_resource_plan_instance.go

@@ -386,7 +386,15 @@ func (n *NodePlannableResourceInstance) managedResourceExecute(ctx EvalContext)
 		}
 
 		if importing {
-			change.Importing = &plans.Importing{Target: n.importTarget}
+			// There is a subtle difference between the import by identity
+			// and the import by ID. When importing by identity, we need to
+			// make sure to use the complete identity return by the provider
+			// instead of the (potential) incomplete one from the configuration.
+			if n.importTarget.Type().IsObjectType() {
+				change.Importing = &plans.Importing{Target: instanceRefreshState.Identity}
+			} else {
+				change.Importing = &plans.Importing{Target: n.importTarget}
+			}
 		}
 
 		// FIXME: here we udpate the change to reflect the reason for

internal/tfdiags/object.go

@@ -34,6 +34,11 @@ func ObjectToString(obj cty.Value) string {
 				result += ","
 			}
 
+			if val.IsNull() {
+				result += fmt.Sprintf("%s=<null>", keyStr)
+				continue
+			}
+
 			switch val.Type() {
 			case cty.Bool:
 				result += fmt.Sprintf("%s=%t", keyStr, val.True())

internal/tfdiags/object_test.go

@@ -50,6 +50,14 @@ func Test_ObjectToString(t *testing.T) {
 			}),
 			expected: "list=[a,b,c],string=hello",
 		},
+		{
+			name: "with null value",
+			value: cty.ObjectVal(map[string]cty.Value{
+				"string": cty.StringVal("hello"),
+				"null":   cty.NullVal(cty.String),
+			}),
+			expected: "null=<null>,string=hello",
+		},
 	}
 
 	for _, tc := range testCases {