diff --git a/internal/fwserver/server_listresource.go b/internal/fwserver/server_listresource.go index 37adb9ff3..0acff71c1 100644 --- a/internal/fwserver/server_listresource.go +++ b/internal/fwserver/server_listresource.go @@ -127,18 +127,22 @@ func processListResult(req list.ListRequest, result list.ListResult) ListResult return ListResult(result) } - if result.Identity == nil { // TODO: is result.Identity.Raw.IsNull() a practical concern? + if result.Identity == nil || result.Identity.Raw.IsNull() { return ListResultError( "Incomplete List Result", - "The provider did not populate the Identity field in the ListResourceResult. This may be due to an error in the provider's implementation.", + "When listing resources, an implementation issue was found. "+ + "This is always a problem with the provider. Please report this to the provider developers.\n\n"+ + "The \"Identity\" field is nil.\n\n", ) } if req.IncludeResource { - if result.Resource == nil { // TODO: is result.Resource.Raw.IsNull() a practical concern? + if result.Resource == nil || result.Resource.Raw.IsNull() { result.Diagnostics.AddWarning( "Incomplete List Result", - "The provider did not populate the Resource field in the ListResourceResult. This may be due to the provider not supporting this functionality or an error in the provider's implementation.", + "When listing resources, an implementation issue was found. "+ + "This is always a problem with the provider. Please report this to the provider developers.\n\n"+ + "The \"IncludeResource\" field in the ListRequest is true, but the \"Resource\" field in the ListResult is nil.\n\n", ) } } diff --git a/internal/fwserver/server_listresource_test.go b/internal/fwserver/server_listresource_test.go index f404d29fa..55cefe779 100644 --- a/internal/fwserver/server_listresource_test.go +++ b/internal/fwserver/server_listresource_test.go @@ -206,10 +206,36 @@ func TestServerListResource(t *testing.T) { expectedStreamEvents: []fwserver.ListResult{ { Diagnostics: diag.Diagnostics{ - diag.NewErrorDiagnostic( - "Incomplete List Result", - "The provider did not populate the Identity field in the ListResourceResult. This may be due to an error in the provider's implementation.", - ), + diag.NewErrorDiagnostic("Incomplete List Result", "..."), + }, + }, + }, + }, + "error-on-null-resource-identity": { + server: &fwserver.Server{ + Provider: &testprovider.Provider{}, + }, + request: &fwserver.ListRequest{ + Config: &tfsdk.Config{}, + ListResource: &testprovider.ListResource{ + ListMethod: func(ctx context.Context, req list.ListRequest, resp *list.ListResultsStream) { + resp.Results = slices.Values([]list.ListResult{ + { + Identity: &tfsdk.ResourceIdentity{}, + Resource: &tfsdk.Resource{ + Schema: testSchema, + Raw: testResourceValue1, + }, + DisplayName: "Test Resource 1", + }, + }) + }, + }, + }, + expectedStreamEvents: []fwserver.ListResult{ + { + Diagnostics: diag.Diagnostics{ + diag.NewErrorDiagnostic("Incomplete List Result", "..."), }, }, }, @@ -244,10 +270,43 @@ func TestServerListResource(t *testing.T) { }, DisplayName: "Test Resource 1", Diagnostics: diag.Diagnostics{ - diag.NewWarningDiagnostic( - "Incomplete List Result", - "The provider did not populate the Resource field in the ListResourceResult. This may be due to the provider not supporting this functionality or an error in the provider's implementation.", - ), + diag.NewWarningDiagnostic("Incomplete List Result", "..."), + }, + }, + }, + }, + "warning-on-null-resource": { + server: &fwserver.Server{ + Provider: &testprovider.Provider{}, + }, + request: &fwserver.ListRequest{ + Config: &tfsdk.Config{}, + IncludeResource: true, + ListResource: &testprovider.ListResource{ + ListMethod: func(ctx context.Context, req list.ListRequest, resp *list.ListResultsStream) { + resp.Results = slices.Values([]list.ListResult{ + { + Identity: &tfsdk.ResourceIdentity{ + Schema: testIdentitySchema, + Raw: testIdentityValue1, + }, + Resource: &tfsdk.Resource{}, + DisplayName: "Test Resource 1", + }, + }) + }, + }, + }, + expectedStreamEvents: []fwserver.ListResult{ + { + Identity: &tfsdk.ResourceIdentity{ + Schema: testIdentitySchema, + Raw: testIdentityValue1, + }, + Resource: &tfsdk.Resource{}, + DisplayName: "Test Resource 1", + Diagnostics: diag.Diagnostics{ + diag.NewWarningDiagnostic("Incomplete List Result", "..."), }, }, }, @@ -264,8 +323,19 @@ func TestServerListResource(t *testing.T) { t.Fatalf("unexpected error: %s", err) } + opts := cmp.Options{ + cmp.Comparer(func(a, b diag.Diagnostics) bool { + // Differences in Detail() are not relevant to correctness of logic + for i := range a { + if a[i].Severity() != b[i].Severity() || a[i].Summary() != b[i].Summary() { + return false + } + } + return true + }), + } events := slices.AppendSeq([]fwserver.ListResult{}, response.Results) - if diff := cmp.Diff(events, testCase.expectedStreamEvents); diff != "" { + if diff := cmp.Diff(events, testCase.expectedStreamEvents, opts); diff != "" { t.Errorf("unexpected difference: %s", diff) } }) diff --git a/internal/proto5server/server_listresource_test.go b/internal/proto5server/server_listresource_test.go index 69cc206c9..1b68ec8c3 100644 --- a/internal/proto5server/server_listresource_test.go +++ b/internal/proto5server/server_listresource_test.go @@ -31,7 +31,6 @@ func TestServerListResource(t *testing.T) { } type ThingResource struct { - // TODO: how do we feel about this? ThingResourceIdentity Name string `tfsdk:"name"` } @@ -106,7 +105,15 @@ func TestServerListResource(t *testing.T) { continue } - result := req.ToListResult(ctx, resources[name].ThingResourceIdentity, resources[name], name) + result := req.NewListResult() + result.DisplayName = name + + diags = result.Identity.Set(ctx, resources[name].ThingResourceIdentity) + result.Diagnostics.Append(diags...) + + diags = result.Resource.Set(ctx, resources[name]) + result.Diagnostics.Append(diags...) + results = append(results, result) } resp.Results = slices.Values(results) @@ -117,21 +124,6 @@ func TestServerListResource(t *testing.T) { } } - listResourceThatDoesNotPopulateResource := func() list.ListResource { - r, ok := listResource().(*testprovider.ListResource) - if !ok { - t.Fatal("listResourceThatDoesNotPopulateResource must be a testprovider.ListResource") - } - - r.ListMethod = func(ctx context.Context, req list.ListRequest, resp *list.ListResultsStream) { - result := req.ToListResult(ctx, resources["plateau"].ThingResourceIdentity, nil, "plateau") - - resp.Results = slices.Values([]list.ListResult{result}) - } - - return r - } - managedResource := func() resource.Resource { return &testprovider.ResourceWithIdentity{ IdentitySchemaMethod: func(ctx context.Context, req resource.IdentitySchemaRequest, resp *resource.IdentitySchemaResponse) { @@ -251,29 +243,6 @@ func TestServerListResource(t *testing.T) { }, }, }, - "result-with-include-resource-warning": { - server: server(listResourceThatDoesNotPopulateResource, managedResource), - request: &tfprotov5.ListResourceRequest{ - TypeName: "test_resource", - Config: plateau, - IncludeResource: true, - }, - expectedError: nil, - expectedDiagnostics: diag.Diagnostics{}, - expectedResults: []tfprotov5.ListResourceResult{ - { - DisplayName: "plateau", - Identity: expectedResourceIdentities["plateau"], - Diagnostics: []*tfprotov5.Diagnostic{ - { - Severity: tfprotov5.DiagnosticSeverityWarning, - Summary: "Incomplete List Result", - Detail: "The provider did not populate the Resource field in the ListResourceResult. This may be due to the provider not supporting this functionality or an error in the provider's implementation.", - }, - }, - }, - }, - }, } for name, testCase := range testCases { diff --git a/internal/proto6server/server_listresource_test.go b/internal/proto6server/server_listresource_test.go index 162666484..831f6e4a6 100644 --- a/internal/proto6server/server_listresource_test.go +++ b/internal/proto6server/server_listresource_test.go @@ -31,7 +31,6 @@ func TestServerListResource(t *testing.T) { } type ThingResource struct { - // TODO: how do we feel about this? ThingResourceIdentity Name string `tfsdk:"name"` } @@ -106,7 +105,15 @@ func TestServerListResource(t *testing.T) { continue } - result := req.ToListResult(ctx, resources[name].ThingResourceIdentity, resources[name], name) + result := req.NewListResult() + result.DisplayName = name + + diags = result.Identity.Set(ctx, resources[name].ThingResourceIdentity) + result.Diagnostics = append(result.Diagnostics, diags...) + + diags = result.Resource.Set(ctx, resources[name]) + result.Diagnostics = append(result.Diagnostics, diags...) + results = append(results, result) } resp.Results = slices.Values(results) @@ -124,9 +131,14 @@ func TestServerListResource(t *testing.T) { } r.ListMethod = func(ctx context.Context, req list.ListRequest, resp *list.ListResultsStream) { - result := req.ToListResult(ctx, resources["plateau"].ThingResourceIdentity, nil, "plateau") + result := req.NewListResult() + result.DisplayName = "plateau" + + diags := result.Identity.Set(ctx, resources["plateau"].ThingResourceIdentity) + result.Diagnostics = append(result.Diagnostics, diags...) - resp.Results = slices.Values([]list.ListResult{result}) + results := []list.ListResult{result} + resp.Results = slices.Values(results) } return r @@ -264,6 +276,7 @@ func TestServerListResource(t *testing.T) { { DisplayName: "plateau", Identity: expectedResourceIdentities["plateau"], + Resource: &tfprotov6.DynamicValue{MsgPack: []uint8{0xc0}}, Diagnostics: []*tfprotov6.Diagnostic{ { Severity: tfprotov6.DiagnosticSeverityWarning, diff --git a/list/list_resource.go b/list/list_resource.go index baee106cf..75fdbca07 100644 --- a/list/list_resource.go +++ b/list/list_resource.go @@ -103,6 +103,20 @@ type ListRequest struct { ResourceIdentitySchema fwschema.Schema } +// NewListResult creates a new [ListResult] with convenient defaults +// for each field. +func (r ListRequest) NewListResult() ListResult { + identity := &tfsdk.ResourceIdentity{Schema: r.ResourceIdentitySchema} + resource := &tfsdk.Resource{Schema: r.ResourceSchema} + + return ListResult{ + DisplayName: "", + Resource: resource, + Identity: identity, + Diagnostics: diag.Diagnostics{}, + } +} + // ListResultsStream represents a streaming response to a [ListRequest]. An // instance of this struct is supplied as an argument to the provider's // [ListResource.List] function. The provider should set a Results iterator @@ -120,9 +134,20 @@ type ListResultsStream struct { } // NoListResults is an iterator that pushes zero results. -var NoListResults = func(func(ListResult) bool) {} +var NoListResults = func(push func(ListResult) bool) {} + +// ListResultsStreamDiagnostics returns a function that yields a single +// [ListResult] with the given Diagnostics +func ListResultsStreamDiagnostics(diags diag.Diagnostics) iter.Seq[ListResult] { + return func(push func(ListResult) bool) { + if !push(ListResult{Diagnostics: diags}) { + return + } + } +} -// ListResult represents a listed managed resource instance. +// ListResult represents a listed managed resource instance. For convenience, +// create new values using [NewListResult] instead of struct literals. type ListResult struct { // Identity is the identity of the managed resource instance. // diff --git a/list/tosdk.go b/list/tosdk.go deleted file mode 100644 index cabf10bf9..000000000 --- a/list/tosdk.go +++ /dev/null @@ -1,50 +0,0 @@ -// Copyright (c) HashiCorp, Inc. -// SPDX-License-Identifier: MPL-2.0 - -package list - -import ( - "context" - - "github.com/hashicorp/terraform-plugin-framework/diag" - "github.com/hashicorp/terraform-plugin-framework/tfsdk" -) - -func (r ListRequest) ToResource(ctx context.Context, val any) (*tfsdk.Resource, diag.Diagnostics) { - resource := &tfsdk.Resource{Schema: r.ResourceSchema} - diags := resource.Set(ctx, val) - return resource, diags -} - -func (r ListRequest) ToIdentity(ctx context.Context, val any) (*tfsdk.ResourceIdentity, diag.Diagnostics) { - identity := &tfsdk.ResourceIdentity{Schema: r.ResourceIdentitySchema} - diags := identity.Set(ctx, val) - - return identity, diags -} - -func (r ListRequest) ToListResult(ctx context.Context, identityVal any, resourceVal any, displayName string) ListResult { - allDiags := diag.Diagnostics{} - - identity, diags := r.ToIdentity(ctx, identityVal) - allDiags.Append(diags...) - if diags.HasError() { - return ListResult{Diagnostics: allDiags} - } - - var resource *tfsdk.Resource - if r.IncludeResource && resourceVal != nil { - resource, diags = r.ToResource(ctx, resourceVal) - allDiags.Append(diags...) - if diags.HasError() { - return ListResult{Diagnostics: allDiags} - } - } - - return ListResult{ - DisplayName: displayName, - Resource: resource, - Identity: identity, - Diagnostics: allDiags, - } -}