From 7dce0b7f350f7d77d985cc799490d8f437b99740 Mon Sep 17 00:00:00 2001 From: Baraa Basata Date: Thu, 19 Jun 2025 15:24:09 -0400 Subject: [PATCH 1/5] ListResource: tidy diagnostic messages --- internal/fwserver/server_listresource.go | 8 ++++++-- internal/fwserver/server_listresource_test.go | 15 +++++++++++++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/internal/fwserver/server_listresource.go b/internal/fwserver/server_listresource.go index 37adb9ff3..8eddb270e 100644 --- a/internal/fwserver/server_listresource.go +++ b/internal/fwserver/server_listresource.go @@ -130,7 +130,9 @@ func processListResult(req list.ListRequest, result list.ListResult) ListResult if result.Identity == nil { // TODO: is result.Identity.Raw.IsNull() a practical concern? 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", ) } @@ -138,7 +140,9 @@ func processListResult(req list.ListRequest, result list.ListResult) ListResult if result.Resource == nil { // TODO: is result.Resource.Raw.IsNull() a practical concern? 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..2e67a8d6e 100644 --- a/internal/fwserver/server_listresource_test.go +++ b/internal/fwserver/server_listresource_test.go @@ -208,7 +208,7 @@ func TestServerListResource(t *testing.T) { 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.", + "The provider did not populate the Identity field in the ListResourceResult. This is always a problem with the provider. Please report this to the provider developer.", ), }, }, @@ -264,8 +264,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) } }) From 65651f6ca8a3ef429883b392f3d961d2a3e237c5 Mon Sep 17 00:00:00 2001 From: Baraa Basata Date: Wed, 18 Jun 2025 20:27:22 -0400 Subject: [PATCH 2/5] ListResource: null handling --- internal/fwserver/server_listresource.go | 4 +- internal/fwserver/server_listresource_test.go | 75 +++++++++++++++++-- .../proto5server/server_listresource_test.go | 45 ++--------- .../proto6server/server_listresource_test.go | 12 ++- list/list_resource.go | 12 +++ list/tosdk.go | 50 ------------- 6 files changed, 95 insertions(+), 103 deletions(-) delete mode 100644 list/tosdk.go diff --git a/internal/fwserver/server_listresource.go b/internal/fwserver/server_listresource.go index 8eddb270e..0acff71c1 100644 --- a/internal/fwserver/server_listresource.go +++ b/internal/fwserver/server_listresource.go @@ -127,7 +127,7 @@ 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", "When listing resources, an implementation issue was found. "+ @@ -137,7 +137,7 @@ func processListResult(req list.ListRequest, result list.ListResult) ListResult } 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", "When listing resources, an implementation issue was found. "+ diff --git a/internal/fwserver/server_listresource_test.go b/internal/fwserver/server_listresource_test.go index 2e67a8d6e..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 is always a problem with the provider. Please report this to the provider developer.", - ), + 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", "..."), }, }, }, diff --git a/internal/proto5server/server_listresource_test.go b/internal/proto5server/server_listresource_test.go index 69cc206c9..03172e783 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,11 @@ func TestServerListResource(t *testing.T) { continue } - result := req.ToListResult(ctx, resources[name].ThingResourceIdentity, resources[name], name) + result := req.NewListResult() + result.Identity.Set(ctx, resources[name].ThingResourceIdentity) + result.Resource.Set(ctx, resources[name]) + result.DisplayName = name + results = append(results, result) } resp.Results = slices.Values(results) @@ -117,21 +120,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 +239,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..094b2f452 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,11 @@ func TestServerListResource(t *testing.T) { continue } - result := req.ToListResult(ctx, resources[name].ThingResourceIdentity, resources[name], name) + result := req.NewListResult() + result.Identity.Set(ctx, resources[name].ThingResourceIdentity) + result.Resource.Set(ctx, resources[name]) + result.DisplayName = name + results = append(results, result) } resp.Results = slices.Values(results) @@ -124,7 +127,9 @@ 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.Identity.Set(ctx, resources["plateau"].ThingResourceIdentity) + result.DisplayName = "plateau" resp.Results = slices.Values([]list.ListResult{result}) } @@ -264,6 +269,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 8e793dc70..9779fcc47 100644 --- a/list/list_resource.go +++ b/list/list_resource.go @@ -103,6 +103,18 @@ type ListRequest struct { ResourceIdentitySchema fwschema.Schema } +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 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, - } -} From 3e5c9f7e0a16ecb63df078973afc609c478ec4f9 Mon Sep 17 00:00:00 2001 From: Baraa Basata Date: Mon, 23 Jun 2025 15:14:05 -0400 Subject: [PATCH 3/5] Add utility functions for list results stream errors --- list/list_resource.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/list/list_resource.go b/list/list_resource.go index 9779fcc47..3a2ded454 100644 --- a/list/list_resource.go +++ b/list/list_resource.go @@ -134,6 +134,25 @@ type ListResultsStream struct { // NoListResults is an iterator that pushes zero results. var NoListResults = func(func(ListResult) bool) {} +// ListResultsStreamError returns a function that yields a single +// [[ListResult]] with an error diagnostic +func ListResultsStreamError(summary string, detail string) iter.Seq[ListResult] { + return func(push func(ListResult) bool) { + if !push(ListResultError(summary, detail)) { + return + } + } +} + +// ListResultError constructs a [[ListResult]] with an error diagnostic +func ListResultError(summary string, detail string) ListResult { + return ListResult{ + Diagnostics: diag.Diagnostics{ + diag.NewErrorDiagnostic(summary, detail), + }, + } +} + // ListResult represents a listed managed resource instance. type ListResult struct { // Identity is the identity of the managed resource instance. From 9c0fecc9e9ba67a970346674ef31df0b2bc7a437 Mon Sep 17 00:00:00 2001 From: Baraa Basata Date: Mon, 23 Jun 2025 15:14:05 -0400 Subject: [PATCH 4/5] Add utility functions for list results stream errors --- list/list_resource.go | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/list/list_resource.go b/list/list_resource.go index 3a2ded454..3b856790f 100644 --- a/list/list_resource.go +++ b/list/list_resource.go @@ -134,25 +134,16 @@ type ListResultsStream struct { // NoListResults is an iterator that pushes zero results. var NoListResults = func(func(ListResult) bool) {} -// ListResultsStreamError returns a function that yields a single -// [[ListResult]] with an error diagnostic -func ListResultsStreamError(summary string, detail string) iter.Seq[ListResult] { +// 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(ListResultError(summary, detail)) { + if !push(ListResult{Diagnostics: diags}) { return } } } -// ListResultError constructs a [[ListResult]] with an error diagnostic -func ListResultError(summary string, detail string) ListResult { - return ListResult{ - Diagnostics: diag.Diagnostics{ - diag.NewErrorDiagnostic(summary, detail), - }, - } -} - // ListResult represents a listed managed resource instance. type ListResult struct { // Identity is the identity of the managed resource instance. From 93602b756b247ea72f302ee9a8e5482bb04f8305 Mon Sep 17 00:00:00 2001 From: Baraa Basata Date: Wed, 25 Jun 2025 08:37:46 -0400 Subject: [PATCH 5/5] list: doc comments & tidying --- internal/proto5server/server_listresource_test.go | 8 ++++++-- internal/proto6server/server_listresource_test.go | 15 +++++++++++---- list/list_resource.go | 9 ++++++--- 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/internal/proto5server/server_listresource_test.go b/internal/proto5server/server_listresource_test.go index 03172e783..1b68ec8c3 100644 --- a/internal/proto5server/server_listresource_test.go +++ b/internal/proto5server/server_listresource_test.go @@ -106,10 +106,14 @@ func TestServerListResource(t *testing.T) { } result := req.NewListResult() - result.Identity.Set(ctx, resources[name].ThingResourceIdentity) - result.Resource.Set(ctx, resources[name]) 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) diff --git a/internal/proto6server/server_listresource_test.go b/internal/proto6server/server_listresource_test.go index 094b2f452..831f6e4a6 100644 --- a/internal/proto6server/server_listresource_test.go +++ b/internal/proto6server/server_listresource_test.go @@ -106,10 +106,14 @@ func TestServerListResource(t *testing.T) { } result := req.NewListResult() - result.Identity.Set(ctx, resources[name].ThingResourceIdentity) - result.Resource.Set(ctx, resources[name]) 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) @@ -128,10 +132,13 @@ func TestServerListResource(t *testing.T) { r.ListMethod = func(ctx context.Context, req list.ListRequest, resp *list.ListResultsStream) { result := req.NewListResult() - result.Identity.Set(ctx, resources["plateau"].ThingResourceIdentity) result.DisplayName = "plateau" - resp.Results = slices.Values([]list.ListResult{result}) + diags := result.Identity.Set(ctx, resources["plateau"].ThingResourceIdentity) + result.Diagnostics = append(result.Diagnostics, diags...) + + results := []list.ListResult{result} + resp.Results = slices.Values(results) } return r diff --git a/list/list_resource.go b/list/list_resource.go index 3b856790f..92492d4bf 100644 --- a/list/list_resource.go +++ b/list/list_resource.go @@ -103,6 +103,8 @@ 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} @@ -132,10 +134,10 @@ 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 +// [ListResult] with the given Diagnostics func ListResultsStreamDiagnostics(diags diag.Diagnostics) iter.Seq[ListResult] { return func(push func(ListResult) bool) { if !push(ListResult{Diagnostics: diags}) { @@ -144,7 +146,8 @@ func ListResultsStreamDiagnostics(diags diag.Diagnostics) iter.Seq[ListResult] { } } -// 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. //