Skip to content

Commit 7d8104c

Browse files
bbasatabbasata
authored
ListResource: revisit handling of null ListResult fields (#1167)
* ListResource: tidy diagnostic messages * ListResource: null handling * Add utility functions for list results stream errors * Add utility functions for list results stream errors * list: doc comments & tidying
1 parent 812bad4 commit 7d8104c

File tree

6 files changed

+140
-109
lines changed

6 files changed

+140
-109
lines changed

‎internal/fwserver/server_listresource.go

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -127,18 +127,22 @@ func processListResult(req list.ListRequest, result list.ListResult) ListResult
127127
return ListResult(result)
128128
}
129129

130-
if result.Identity == nil { // TODO: is result.Identity.Raw.IsNull() a practical concern?
130+
if result.Identity == nil || result.Identity.Raw.IsNull() {
131131
return ListResultError(
132132
"Incomplete List Result",
133-
"The provider did not populate the Identity field in the ListResourceResult. This may be due to an error in the provider's implementation.",
133+
"When listing resources, an implementation issue was found. "+
134+
"This is always a problem with the provider. Please report this to the provider developers.\n\n"+
135+
"The \"Identity\" field is nil.\n\n",
134136
)
135137
}
136138

137139
if req.IncludeResource {
138-
if result.Resource == nil { // TODO: is result.Resource.Raw.IsNull() a practical concern?
140+
if result.Resource == nil || result.Resource.Raw.IsNull() {
139141
result.Diagnostics.AddWarning(
140142
"Incomplete List Result",
141-
"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.",
143+
"When listing resources, an implementation issue was found. "+
144+
"This is always a problem with the provider. Please report this to the provider developers.\n\n"+
145+
"The \"IncludeResource\" field in the ListRequest is true, but the \"Resource\" field in the ListResult is nil.\n\n",
142146
)
143147
}
144148
}

‎internal/fwserver/server_listresource_test.go

Lines changed: 79 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -206,10 +206,36 @@ func TestServerListResource(t *testing.T) {
206206
expectedStreamEvents: []fwserver.ListResult{
207207
{
208208
Diagnostics: diag.Diagnostics{
209-
diag.NewErrorDiagnostic(
210-
"Incomplete List Result",
211-
"The provider did not populate the Identity field in the ListResourceResult. This may be due to an error in the provider's implementation.",
212-
),
209+
diag.NewErrorDiagnostic("Incomplete List Result", "..."),
210+
},
211+
},
212+
},
213+
},
214+
"error-on-null-resource-identity": {
215+
server: &fwserver.Server{
216+
Provider: &testprovider.Provider{},
217+
},
218+
request: &fwserver.ListRequest{
219+
Config: &tfsdk.Config{},
220+
ListResource: &testprovider.ListResource{
221+
ListMethod: func(ctx context.Context, req list.ListRequest, resp *list.ListResultsStream) {
222+
resp.Results = slices.Values([]list.ListResult{
223+
{
224+
Identity: &tfsdk.ResourceIdentity{},
225+
Resource: &tfsdk.Resource{
226+
Schema: testSchema,
227+
Raw: testResourceValue1,
228+
},
229+
DisplayName: "Test Resource 1",
230+
},
231+
})
232+
},
233+
},
234+
},
235+
expectedStreamEvents: []fwserver.ListResult{
236+
{
237+
Diagnostics: diag.Diagnostics{
238+
diag.NewErrorDiagnostic("Incomplete List Result", "..."),
213239
},
214240
},
215241
},
@@ -244,10 +270,43 @@ func TestServerListResource(t *testing.T) {
244270
},
245271
DisplayName: "Test Resource 1",
246272
Diagnostics: diag.Diagnostics{
247-
diag.NewWarningDiagnostic(
248-
"Incomplete List Result",
249-
"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.",
250-
),
273+
diag.NewWarningDiagnostic("Incomplete List Result", "..."),
274+
},
275+
},
276+
},
277+
},
278+
"warning-on-null-resource": {
279+
server: &fwserver.Server{
280+
Provider: &testprovider.Provider{},
281+
},
282+
request: &fwserver.ListRequest{
283+
Config: &tfsdk.Config{},
284+
IncludeResource: true,
285+
ListResource: &testprovider.ListResource{
286+
ListMethod: func(ctx context.Context, req list.ListRequest, resp *list.ListResultsStream) {
287+
resp.Results = slices.Values([]list.ListResult{
288+
{
289+
Identity: &tfsdk.ResourceIdentity{
290+
Schema: testIdentitySchema,
291+
Raw: testIdentityValue1,
292+
},
293+
Resource: &tfsdk.Resource{},
294+
DisplayName: "Test Resource 1",
295+
},
296+
})
297+
},
298+
},
299+
},
300+
expectedStreamEvents: []fwserver.ListResult{
301+
{
302+
Identity: &tfsdk.ResourceIdentity{
303+
Schema: testIdentitySchema,
304+
Raw: testIdentityValue1,
305+
},
306+
Resource: &tfsdk.Resource{},
307+
DisplayName: "Test Resource 1",
308+
Diagnostics: diag.Diagnostics{
309+
diag.NewWarningDiagnostic("Incomplete List Result", "..."),
251310
},
252311
},
253312
},
@@ -264,8 +323,19 @@ func TestServerListResource(t *testing.T) {
264323
t.Fatalf("unexpected error: %s", err)
265324
}
266325

326+
opts := cmp.Options{
327+
cmp.Comparer(func(a, b diag.Diagnostics) bool {
328+
// Differences in Detail() are not relevant to correctness of logic
329+
for i := range a {
330+
if a[i].Severity() != b[i].Severity() || a[i].Summary() != b[i].Summary() {
331+
return false
332+
}
333+
}
334+
return true
335+
}),
336+
}
267337
events := slices.AppendSeq([]fwserver.ListResult{}, response.Results)
268-
if diff := cmp.Diff(events, testCase.expectedStreamEvents); diff != "" {
338+
if diff := cmp.Diff(events, testCase.expectedStreamEvents, opts); diff != "" {
269339
t.Errorf("unexpected difference: %s", diff)
270340
}
271341
})

‎internal/proto5server/server_listresource_test.go

Lines changed: 9 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ func TestServerListResource(t *testing.T) {
3131
}
3232

3333
type ThingResource struct {
34-
// TODO: how do we feel about this?
3534
ThingResourceIdentity
3635
Name string `tfsdk:"name"`
3736
}
@@ -106,7 +105,15 @@ func TestServerListResource(t *testing.T) {
106105
continue
107106
}
108107

109-
result := req.ToListResult(ctx, resources[name].ThingResourceIdentity, resources[name], name)
108+
result := req.NewListResult()
109+
result.DisplayName = name
110+
111+
diags = result.Identity.Set(ctx, resources[name].ThingResourceIdentity)
112+
result.Diagnostics.Append(diags...)
113+
114+
diags = result.Resource.Set(ctx, resources[name])
115+
result.Diagnostics.Append(diags...)
116+
110117
results = append(results, result)
111118
}
112119
resp.Results = slices.Values(results)
@@ -117,21 +124,6 @@ func TestServerListResource(t *testing.T) {
117124
}
118125
}
119126

120-
listResourceThatDoesNotPopulateResource := func() list.ListResource {
121-
r, ok := listResource().(*testprovider.ListResource)
122-
if !ok {
123-
t.Fatal("listResourceThatDoesNotPopulateResource must be a testprovider.ListResource")
124-
}
125-
126-
r.ListMethod = func(ctx context.Context, req list.ListRequest, resp *list.ListResultsStream) {
127-
result := req.ToListResult(ctx, resources["plateau"].ThingResourceIdentity, nil, "plateau")
128-
129-
resp.Results = slices.Values([]list.ListResult{result})
130-
}
131-
132-
return r
133-
}
134-
135127
managedResource := func() resource.Resource {
136128
return &testprovider.ResourceWithIdentity{
137129
IdentitySchemaMethod: func(ctx context.Context, req resource.IdentitySchemaRequest, resp *resource.IdentitySchemaResponse) {
@@ -251,29 +243,6 @@ func TestServerListResource(t *testing.T) {
251243
},
252244
},
253245
},
254-
"result-with-include-resource-warning": {
255-
server: server(listResourceThatDoesNotPopulateResource, managedResource),
256-
request: &tfprotov5.ListResourceRequest{
257-
TypeName: "test_resource",
258-
Config: plateau,
259-
IncludeResource: true,
260-
},
261-
expectedError: nil,
262-
expectedDiagnostics: diag.Diagnostics{},
263-
expectedResults: []tfprotov5.ListResourceResult{
264-
{
265-
DisplayName: "plateau",
266-
Identity: expectedResourceIdentities["plateau"],
267-
Diagnostics: []*tfprotov5.Diagnostic{
268-
{
269-
Severity: tfprotov5.DiagnosticSeverityWarning,
270-
Summary: "Incomplete List Result",
271-
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.",
272-
},
273-
},
274-
},
275-
},
276-
},
277246
}
278247

279248
for name, testCase := range testCases {

‎internal/proto6server/server_listresource_test.go

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ func TestServerListResource(t *testing.T) {
3131
}
3232

3333
type ThingResource struct {
34-
// TODO: how do we feel about this?
3534
ThingResourceIdentity
3635
Name string `tfsdk:"name"`
3736
}
@@ -106,7 +105,15 @@ func TestServerListResource(t *testing.T) {
106105
continue
107106
}
108107

109-
result := req.ToListResult(ctx, resources[name].ThingResourceIdentity, resources[name], name)
108+
result := req.NewListResult()
109+
result.DisplayName = name
110+
111+
diags = result.Identity.Set(ctx, resources[name].ThingResourceIdentity)
112+
result.Diagnostics = append(result.Diagnostics, diags...)
113+
114+
diags = result.Resource.Set(ctx, resources[name])
115+
result.Diagnostics = append(result.Diagnostics, diags...)
116+
110117
results = append(results, result)
111118
}
112119
resp.Results = slices.Values(results)
@@ -124,9 +131,14 @@ func TestServerListResource(t *testing.T) {
124131
}
125132

126133
r.ListMethod = func(ctx context.Context, req list.ListRequest, resp *list.ListResultsStream) {
127-
result := req.ToListResult(ctx, resources["plateau"].ThingResourceIdentity, nil, "plateau")
134+
result := req.NewListResult()
135+
result.DisplayName = "plateau"
136+
137+
diags := result.Identity.Set(ctx, resources["plateau"].ThingResourceIdentity)
138+
result.Diagnostics = append(result.Diagnostics, diags...)
128139

129-
resp.Results = slices.Values([]list.ListResult{result})
140+
results := []list.ListResult{result}
141+
resp.Results = slices.Values(results)
130142
}
131143

132144
return r
@@ -264,6 +276,7 @@ func TestServerListResource(t *testing.T) {
264276
{
265277
DisplayName: "plateau",
266278
Identity: expectedResourceIdentities["plateau"],
279+
Resource: &tfprotov6.DynamicValue{MsgPack: []uint8{0xc0}},
267280
Diagnostics: []*tfprotov6.Diagnostic{
268281
{
269282
Severity: tfprotov6.DiagnosticSeverityWarning,

‎list/list_resource.go

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,20 @@ type ListRequest struct {
103103
ResourceIdentitySchema fwschema.Schema
104104
}
105105

106+
// NewListResult creates a new [ListResult] with convenient defaults
107+
// for each field.
108+
func (r ListRequest) NewListResult() ListResult {
109+
identity := &tfsdk.ResourceIdentity{Schema: r.ResourceIdentitySchema}
110+
resource := &tfsdk.Resource{Schema: r.ResourceSchema}
111+
112+
return ListResult{
113+
DisplayName: "",
114+
Resource: resource,
115+
Identity: identity,
116+
Diagnostics: diag.Diagnostics{},
117+
}
118+
}
119+
106120
// ListResultsStream represents a streaming response to a [ListRequest]. An
107121
// instance of this struct is supplied as an argument to the provider's
108122
// [ListResource.List] function. The provider should set a Results iterator
@@ -120,9 +134,20 @@ type ListResultsStream struct {
120134
}
121135

122136
// NoListResults is an iterator that pushes zero results.
123-
var NoListResults = func(func(ListResult) bool) {}
137+
var NoListResults = func(push func(ListResult) bool) {}
138+
139+
// ListResultsStreamDiagnostics returns a function that yields a single
140+
// [ListResult] with the given Diagnostics
141+
func ListResultsStreamDiagnostics(diags diag.Diagnostics) iter.Seq[ListResult] {
142+
return func(push func(ListResult) bool) {
143+
if !push(ListResult{Diagnostics: diags}) {
144+
return
145+
}
146+
}
147+
}
124148

125-
// ListResult represents a listed managed resource instance.
149+
// ListResult represents a listed managed resource instance. For convenience,
150+
// create new values using [NewListResult] instead of struct literals.
126151
type ListResult struct {
127152
// Identity is the identity of the managed resource instance.
128153
//

‎list/tosdk.go

Lines changed: 0 additions & 50 deletions
This file was deleted.

0 commit comments

Comments
 (0)