Skip to content

Commit 55bd0f1

Browse files
austinvalleaustinvalle
committed
add tests for fwserver
1 parent ce3618e commit 55bd0f1

File tree

3 files changed

+178
-16
lines changed

3 files changed

+178
-16
lines changed

‎internal/fwserver/server_importresourcestate.go‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -193,10 +193,8 @@ func (s *Server) ImportResourceState(ctx context.Context, req *ImportResourceSta
193193

194194
importResp.State.Raw = modifiedState
195195

196-
// TODO:ResourceIdentity: Now you can reasonably import using just the identity field and no state. However this still feels like not a great idea, because it's possible
197-
// to import via ID with a client that doesn't support identity, so the "Read" logic for providers will always have to account for both scenarios.
198-
//
199-
// A potential improvement on this could be to check if identity AND state are empty. That is the only true error state because then no data would be transferred from import to read.
196+
// If we are importing by ID, we should ensure that something in the import stub state has been populated,
197+
// otherwise the resource doesn't actually support import, which is a provider issue.
200198
if req.ID != "" && importResp.State.Raw.Equal(req.EmptyState.Raw) {
201199
resp.Diagnostics.AddError(
202200
"Missing Resource Import State",

‎internal/fwserver/server_importresourcestate_test.go‎

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,10 @@ import (
1818
"github.com/hashicorp/terraform-plugin-framework/path"
1919
"github.com/hashicorp/terraform-plugin-framework/provider"
2020
"github.com/hashicorp/terraform-plugin-framework/resource"
21+
"github.com/hashicorp/terraform-plugin-framework/resource/identityschema"
2122
"github.com/hashicorp/terraform-plugin-framework/resource/schema"
2223
"github.com/hashicorp/terraform-plugin-framework/tfsdk"
24+
"github.com/hashicorp/terraform-plugin-framework/types"
2325
)
2426

2527
func TestServerImportResourceState(t *testing.T) {
@@ -33,6 +35,13 @@ func TestServerImportResourceState(t *testing.T) {
3335
},
3436
}
3537

38+
testIdentityType := tftypes.Object{
39+
AttributeTypes: map[string]tftypes.Type{
40+
"test_id": tftypes.String,
41+
"other_test_id": tftypes.String,
42+
},
43+
}
44+
3645
testTypeWriteOnly := tftypes.Object{
3746
AttributeTypes: map[string]tftypes.Type{
3847
"id": tftypes.String,
@@ -61,6 +70,16 @@ func TestServerImportResourceState(t *testing.T) {
6170
"required": tftypes.NewValue(tftypes.String, nil),
6271
})
6372

73+
testRequestIdentityValue := tftypes.NewValue(testIdentityType, map[string]tftypes.Value{
74+
"test_id": tftypes.NewValue(tftypes.String, "id-123"),
75+
"other_test_id": tftypes.NewValue(tftypes.String, nil),
76+
})
77+
78+
testImportedResourceIdentityValue := tftypes.NewValue(testIdentityType, map[string]tftypes.Value{
79+
"test_id": tftypes.NewValue(tftypes.String, "id-123"),
80+
"other_test_id": tftypes.NewValue(tftypes.String, "new-value-123"),
81+
})
82+
6483
testSchema := schema.Schema{
6584
Attributes: map[string]schema.Attribute{
6685
"id": schema.StringAttribute{
@@ -75,6 +94,17 @@ func TestServerImportResourceState(t *testing.T) {
7594
},
7695
}
7796

97+
testIdentitySchema := identityschema.Schema{
98+
Attributes: map[string]identityschema.Attribute{
99+
"test_id": identityschema.StringAttribute{
100+
RequiredForImport: true,
101+
},
102+
"other_test_id": identityschema.StringAttribute{
103+
OptionalForImport: true,
104+
},
105+
},
106+
}
107+
78108
testSchemaWriteOnly := schema.Schema{
79109
Attributes: map[string]schema.Attribute{
80110
"id": schema.StringAttribute{
@@ -105,11 +135,21 @@ func TestServerImportResourceState(t *testing.T) {
105135
Schema: testSchema,
106136
}
107137

138+
testRequestIdentity := &tfsdk.ResourceIdentity{
139+
Raw: testRequestIdentityValue,
140+
Schema: testIdentitySchema,
141+
}
142+
108143
testState := &tfsdk.State{
109144
Raw: testStateValue,
110145
Schema: testSchema,
111146
}
112147

148+
testImportedResourceIdentity := &tfsdk.ResourceIdentity{
149+
Raw: testImportedResourceIdentityValue,
150+
Schema: testIdentitySchema,
151+
}
152+
113153
testProviderKeyValue := privatestate.MustMarshalToJson(map[string][]byte{
114154
"providerKeyOne": []byte(`{"pKeyOne": {"k0": "zero", "k1": 1}}`),
115155
})
@@ -202,6 +242,47 @@ func TestServerImportResourceState(t *testing.T) {
202242
},
203243
},
204244
},
245+
"request-identity": {
246+
server: &fwserver.Server{
247+
Provider: &testprovider.Provider{},
248+
},
249+
request: &fwserver.ImportResourceStateRequest{
250+
EmptyState: *testEmptyState,
251+
Identity: testRequestIdentity,
252+
IdentitySchema: testIdentitySchema,
253+
Resource: &testprovider.ResourceWithIdentityAndImportState{
254+
Resource: &testprovider.Resource{},
255+
ImportStateMethod: func(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) {
256+
var identityData struct {
257+
TestID types.String `tfsdk:"test_id"`
258+
OtherTestID types.String `tfsdk:"other_test_id"`
259+
}
260+
261+
resp.Diagnostics.Append(req.Identity.Get(ctx, &identityData)...)
262+
263+
if identityData.TestID.ValueString() != "id-123" {
264+
resp.Diagnostics.AddError("unexpected req.Identity value: %s", identityData.TestID.ValueString())
265+
}
266+
267+
resource.ImportStatePassthroughID(ctx, path.Root("id"), req, resp)
268+
},
269+
IdentitySchemaMethod: func(ctx context.Context, req resource.IdentitySchemaRequest, resp *resource.IdentitySchemaResponse) {
270+
resp.IdentitySchema = testIdentitySchema
271+
},
272+
},
273+
TypeName: "test_resource",
274+
},
275+
expectedResponse: &fwserver.ImportResourceStateResponse{
276+
ImportedResources: []fwserver.ImportedResource{
277+
{
278+
State: *testEmptyState,
279+
Identity: testRequestIdentity,
280+
TypeName: "test_resource",
281+
Private: testEmptyPrivate,
282+
},
283+
},
284+
},
285+
},
205286
"request-resourcetype-importstate-not-implemented": {
206287
server: &fwserver.Server{
207288
Provider: &testprovider.Provider{},
@@ -323,6 +404,97 @@ func TestServerImportResourceState(t *testing.T) {
323404
},
324405
},
325406
},
407+
"response-importedresources-identity": {
408+
server: &fwserver.Server{
409+
Provider: &testprovider.Provider{},
410+
},
411+
request: &fwserver.ImportResourceStateRequest{
412+
EmptyState: *testEmptyState,
413+
Identity: testRequestIdentity,
414+
IdentitySchema: testIdentitySchema,
415+
Resource: &testprovider.ResourceWithIdentityAndImportState{
416+
Resource: &testprovider.Resource{},
417+
ImportStateMethod: func(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) {
418+
resp.Diagnostics.Append(resp.Identity.SetAttribute(ctx, path.Root("other_test_id"), types.StringValue("new-value-123"))...)
419+
420+
resource.ImportStatePassthroughID(ctx, path.Root("id"), req, resp)
421+
},
422+
IdentitySchemaMethod: func(ctx context.Context, req resource.IdentitySchemaRequest, resp *resource.IdentitySchemaResponse) {
423+
resp.IdentitySchema = testIdentitySchema
424+
},
425+
},
426+
TypeName: "test_resource",
427+
},
428+
expectedResponse: &fwserver.ImportResourceStateResponse{
429+
ImportedResources: []fwserver.ImportedResource{
430+
{
431+
State: *testEmptyState,
432+
Identity: testImportedResourceIdentity,
433+
TypeName: "test_resource",
434+
Private: testEmptyPrivate,
435+
},
436+
},
437+
},
438+
},
439+
"response-importedresources-identity-supported": {
440+
server: &fwserver.Server{
441+
Provider: &testprovider.Provider{},
442+
},
443+
request: &fwserver.ImportResourceStateRequest{
444+
EmptyState: *testEmptyState,
445+
ID: "test-id",
446+
IdentitySchema: testIdentitySchema,
447+
Resource: &testprovider.ResourceWithImportState{
448+
Resource: &testprovider.Resource{},
449+
ImportStateMethod: func(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) {
450+
resp.Diagnostics.Append(resp.Identity.SetAttribute(ctx, path.Root("test_id"), types.StringValue("id-123"))...)
451+
resp.Diagnostics.Append(resp.Identity.SetAttribute(ctx, path.Root("other_test_id"), types.StringValue("new-value-123"))...)
452+
resource.ImportStatePassthroughID(ctx, path.Root("id"), req, resp)
453+
},
454+
},
455+
TypeName: "test_resource",
456+
},
457+
expectedResponse: &fwserver.ImportResourceStateResponse{
458+
ImportedResources: []fwserver.ImportedResource{
459+
{
460+
State: *testState,
461+
Identity: testImportedResourceIdentity,
462+
TypeName: "test_resource",
463+
Private: testEmptyPrivate,
464+
},
465+
},
466+
},
467+
},
468+
"response-importedresources-invalid-identity": {
469+
server: &fwserver.Server{
470+
Provider: &testprovider.Provider{},
471+
},
472+
request: &fwserver.ImportResourceStateRequest{
473+
EmptyState: *testEmptyState,
474+
ID: "test-id",
475+
Resource: &testprovider.ResourceWithImportState{
476+
Resource: &testprovider.Resource{},
477+
ImportStateMethod: func(ctx context.Context, req resource.ImportStateRequest, resp *resource.ImportStateResponse) {
478+
// This resource doesn't indicate identity support (via a schema), so this should raise a diagnostic.
479+
resp.Identity = &tfsdk.ResourceIdentity{
480+
Raw: testImportedResourceIdentityValue,
481+
Schema: testIdentitySchema,
482+
}
483+
resource.ImportStatePassthroughID(ctx, path.Root("id"), req, resp)
484+
},
485+
},
486+
TypeName: "test_resource",
487+
},
488+
expectedResponse: &fwserver.ImportResourceStateResponse{
489+
Diagnostics: diag.Diagnostics{
490+
diag.NewErrorDiagnostic(
491+
"Unexpected ImportState Response",
492+
"An unexpected error was encountered when creating the import response. New identity data was returned by the provider import operation, but the resource does not indicate identity support.\n\n"+
493+
"This is always a problem with the provider and should be reported to the provider developer.",
494+
),
495+
},
496+
},
497+
},
326498
"response-importedresources-deferral-automatic": {
327499
server: &fwserver.Server{
328500
Provider: &testprovider.Provider{

‎resource/import_state.go‎

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -95,9 +95,10 @@ type ImportStateResponse struct {
9595
// identifier to a given state attribute path. The attribute must accept a
9696
// string value.
9797
//
98-
// This method will also automatically pass through the Identity field if provided. (Terraform 1.12+ and later)
99-
// In this scenario where identity is provided instead of the string ID, the state field defined at `attrPath` will
100-
// be set to null.
98+
// This method will also automatically pass through the Identity field if imported by
99+
// the identity attribute of a import config block (Terraform 1.12+ and later). In this
100+
// scenario where identity is provided instead of the string ID, the state field defined
101+
// at `attrPath` will be set to null.
101102
func ImportStatePassthroughID(ctx context.Context, attrPath path.Path, req ImportStateRequest, resp *ImportStateResponse) {
102103
if attrPath.Equal(path.Empty()) {
103104
resp.Diagnostics.AddError(
@@ -112,13 +113,4 @@ func ImportStatePassthroughID(ctx context.Context, attrPath path.Path, req Impor
112113
if req.ID != "" {
113114
resp.Diagnostics.Append(resp.State.SetAttribute(ctx, attrPath, req.ID)...)
114115
}
115-
116-
// TODO:ResourceIdentity: Should we implement another pass-through? called like ImportStatePassthrough that would work "better" with both ID and Identity
117-
//
118-
// We need to decide how we want to handle "existing id" string fields that need to be set to state when an identity is provided.
119-
// Perhaps we can look at the identity schema and automatically use those values to set state? If we did that, we should probably rename this function
120-
// since it's doing much more than just passing through ID. We wouldn't rename it, maybe just deprecate/create a new function.
121-
//
122-
// Since providers will likely be supporting versions of Terraform that don't support identity, they will likely won't want to write multiple ways of reading
123-
// (read with identity, read with state "id" field)
124116
}

0 commit comments

Comments
 (0)