Add ListResource RPC #1157

bbasata · 2025-06-05T14:13:12Z

Description

This change adds the ListResource RPC.

This is ~80% ready, so I feel okay moving it out of draft mode. I plan to make a round of code review comments, as a guided walkthrough.

Rollback Plan

If a change needs to be reverted, we will roll out an update to the code within 7 days.

Changes to Security Controls

None

bbasata · 2025-06-05T15:59:25Z

internal/fwserver/server_listresources.go

@@ -68,7 +80,8 @@ func (s *Server) ListResourceFuncs(ctx context.Context) (map[string]func() list.
 			continue
 		}

-		if _, ok := s.resourceFuncs[typeName]; !ok {
+		resourceFuncs, _ := s.ResourceFuncs(ctx)


Cross-ref: #1152 (comment)

This feels more robust. And removes the need for "call X before Y" comments.

bbasata · 2025-06-13T20:09:04Z

internal/fwserver/server_listresource.go

+	Results iter.Seq[ListResult]
+}
+
+func ListResultError(summary string, detail string) ListResult {


TODO: add a doc comment

Since it's not a "publicly" exported field, no need to add a doc comment unless you feel this helper needs more explanation for maintainers.

Although I'm wondering if this function even needs to be exported (or even exist, it's only used once ATM, maybe it will be used more later?)

internal/fwserver/server_listresource.go

internal/fwserver/server_listresources.go

internal/proto5server/server_validatelistresourceconfig.go

internal/proto6server/server_validatelistresourceconfig.go

internal/toproto5/list_resource_result.go

bbasata · 2025-06-14T03:35:23Z

internal/fwserver/server_listresources.go

@@ -69,7 +85,8 @@ func (s *Server) ListResourceFuncs(ctx context.Context) (map[string]func() list.
 			continue
 		}

-		if _, ok := s.resourceFuncs[typeName]; !ok {
+		resourceFuncs, _ := s.ResourceFuncs(ctx)
+		if _, ok := resourceFuncs[typeName]; !ok {
 			s.listResourceFuncsDiags.AddError(
 				"ListResource Type Defined without a Matching Managed Resource Type",
 				fmt.Sprintf("The %s ListResource type name was returned, but no matching managed Resource type was defined. ", typeName)+


TODO, after this PR: we want the flexibility for a list resource in Framework to work with a managed resource type in SDKv2. So we'll revisit this logic.

bbasata · 2025-06-14T03:35:46Z

internal/proto5server/server_listresource.go

+		return ListRequestErrorDiagnostics(ctx, allDiags...)
+	}
+
+	resourceSchema, diags := s.FrameworkServer.ResourceSchema(ctx, protoReq.TypeName)


TODO, after this PR: we want the flexibility for a list resource in Framework to work with a managed resource type in SDKv2. So we'll revisit this logic.

Yeah I'd think since fwserver doesn't need these values, then this would just be something could be a nil all the way through to the List call. Where the provider developer would then just construct tfsdk.Resource themselves? (which needs to happen anyways?)

I think this is the fun part. As in #1157 (comment), good enough for this PR while under construction for SDKv2 considerations.

As I piece this together, I see that a provider developer typically writes:

resp.State.Set(ctx, &data)

and this works because fwserver has pre-set resp.State.Schema:

terraform-plugin-framework/internal/fwserver/server_readresource.go

Lines 99 to 104 in a7f8748

readResp := resource.ReadResponse{

State: tfsdk.State{

Schema: req.CurrentState.Schema,

Raw: req.CurrentState.Raw.Copy(),

},

}

This is the one reason that fwserver.ListResource is interested in the resource schema and resource identity schema.

And then the "pre-set" mechanics are a little different from resp.State because List returns multiple entities instead of a single entity, as you've referred to in #1157 (comment). We definitely have room to improve on that.

In the next form of this, I'm hoping we can unexport the *Schema struct fields ✅

bbasata · 2025-06-14T03:35:52Z

internal/proto5server/server_listresource.go

+		return ListRequestErrorDiagnostics(ctx, allDiags...)
+	}
+
+	identitySchema, diags := s.FrameworkServer.ResourceIdentitySchema(ctx, protoReq.TypeName)


TODO, after this PR: we want the flexibility for a list resource in Framework to work with a managed resource type in SDKv2. So we'll revisit this logic.

bbasata · 2025-06-14T03:36:29Z

list/tosdk.go

+	return identity, diags
+}
+
+func (r ListRequest) ToListResult(ctx context.Context, identityVal any, resourceVal any, displayName string) ListResult {


TODO, after this PR: we want the flexibility for a list resource in Framework to work with a managed resource type in SDKv2. So we'll revisit this helper method.

austinvalle

Overall this LGTM, I left some general comments and thoughts

go.mod

internal/proto5server/serve.go

internal/proto6server/serve.go

austinvalle · 2025-06-17T16:21:42Z

internal/fwserver/server_listresource.go

+	if fwReq.Config == nil {
+		return errors.New("Invalid ListResource request: Config cannot be nil")
+	}


Is there a reason we aren't using response diagnostics here?

There's no functional reason. I was on the fence re: error or diagnostics. I learned that Terraform can work with either one, so error struck me as simpler for a request-level nil check.

Now that we have a 👍 on "push a single event with an error diagnostic," we could totally use diagnostics instead. I'm open to that. Any preference?

Most of framework (like the type system for example) uses diagnostics, which are just a summary/detail, so I'd say we should stick to diagnostics unless we can't for whatever reason 👍🏻

austinvalle · 2025-06-17T16:31:14Z

internal/fwserver/server_listresource.go

+		return ListResult(result)
+	}
+
+	if result.Identity == nil { // TODO: is result.Identity.Raw.IsNull() a practical concern?


I guess the question would: does Terraform core care if the provider returns a null identity? I would say we probably shouldn't return a null identity unless core says that's acceptable.

In which case we should just check both:

result.Identity == nil being the provider likely didn't set anything

result.Identity.Raw.IsNull() being the provider explicitly set a null value

does Terraform core care if the provider returns a null identity? I would say we probably shouldn't return a null identity unless core says that's acceptable.

cc: @dbanck @dsa0x for input

Terraform currently raises an error when a list result has no identity

tfsdk/resource.go

austinvalle · 2025-06-17T17:13:20Z

list/tosdk.go

This is probably the area we'd want the most feedback on from provider developers right? One thought might be to store the identity/resource schemas on ListResultsStream rather then on the request (also do they need to be exported? 🤔 )

Would a variant of this for multiple list results also we worth doing?

For example, today we have a lot of resp.Set which internally use schema data, maybe this would end up being a result.Append variation?

✅ #1157 (comment)

internal/proto6server/server_listresource_test.go

…aaa40dc65a33a10f6af

bbasata · 2025-06-18T15:31:33Z

Ready for re-review and auto-merge.

TODO items for follow-up:

Perhaps future PR, but if this is a warning that practitioners are likely to see often, maybe we should tailor it specifically to them (i.e. not using the Go types since they have no reference of what these fields mean and won't be reporting to provider developers)

// TODO: is result.Resource.Raw.IsNull() a practical concern? / does Terraform core care if the provider returns a null identity? I would say we probably shouldn't return a null identity unless core says that's acceptable.

Now that we have a 👍 on "push a single event with an error diagnostic," we could totally use diagnostics instead. I'm open to that. Any preference?

Interesting... I'm not really sure what to do about this. We've never just returned the schema in the request like this before, they are typically used to pre-populate a response object and in the case of them potentially not existing (like during upgrade), we just don't populate the response object.

Here, the results stream won't be pre-populated, but provider developers are required to pass along the same schema/identity schema with each result field. Maybe that implementation detail can be placed somewhere in framework?

Yes, I consider this good-enough for this PR, while still under construction. When we look at handling non-Framework schema types, I think that adds new design information that will be useful.

austinvalle

LGTM 🚀

bbasata mentioned this pull request Jun 5, 2025

fwserver: add ListResource method #1153

Closed

1 task

bbasata changed the base branch from main to add-list-to-fwserver-part-ii June 5, 2025 14:17

bbasata added this to the v1.16.0 milestone Jun 5, 2025

Base automatically changed from add-list-to-fwserver-part-ii to main June 5, 2025 14:53

list: tidy the doc comments and tests

d07f61a

bbasata force-pushed the add-listresource-rpc branch from 6414d9d to ed02781 Compare June 13, 2025 18:56

bbasata changed the title ~~Add listresource rpc~~ Add ListResource RPC Jun 13, 2025

bbasata force-pushed the add-listresource-rpc branch 2 times, most recently from 56424fa to d5d9cb5 Compare June 13, 2025 19:55

bbasata changed the base branch from main to revisit-list-package June 13, 2025 19:59

bbasata force-pushed the add-listresource-rpc branch 2 times, most recently from af2d8f7 to 380b706 Compare June 13, 2025 20:05

bbasata marked this pull request as ready for review June 13, 2025 20:07

bbasata requested a review from a team as a code owner June 13, 2025 20:07

Add ListResource RPC

ce0467f

bbasata force-pushed the add-listresource-rpc branch from 380b706 to ce0467f Compare June 13, 2025 20:22

bbasata commented Jun 13, 2025

View reviewed changes

bbasata added 7 commits June 13, 2025 22:47

Remove unused error type

fe9a7fa

Tidy doc comments

a193b8d

More correct processListResult()

a5a3d3f

Tidy diagnostics handling in list_resource_result.go

3ff5bfa

fixup! More correct processListResult()

eecfd76

fwserver.ListResourceSchema++

7f3191d

Tidy comments

f6b4307

Base automatically changed from revisit-list-package to main June 16, 2025 14:38

bbasata commented Jun 16, 2025

View reviewed changes

Merge branch 'main' into add-listresource-rpc

38660df

austinvalle previously approved these changes Jun 17, 2025

View reviewed changes

Tidy

d18edd0

bbasata dismissed austinvalle’s stale review via d18edd0 June 18, 2025 14:30

bbasata added 2 commits June 18, 2025 10:32

fwserver: add a test

7e41955

go get github.com/hashicorp/terraform-plugin-go@a19df43120ead10fa3e9d…

9590b11

…aaa40dc65a33a10f6af

bbasata enabled auto-merge (squash) June 18, 2025 15:30

bbasata requested a review from austinvalle June 18, 2025 15:31

austinvalle approved these changes Jun 18, 2025

View reviewed changes

bbasata merged commit 9cafd72 into main Jun 18, 2025
35 checks passed

bbasata deleted the add-listresource-rpc branch June 18, 2025 18:55

	readResp := resource.ReadResponse{
	State: tfsdk.State{
	Schema: req.CurrentState.Schema,
	Raw: req.CurrentState.Raw.Copy(),
	},
	}

Add ListResource RPC #1157

Add ListResource RPC #1157

Uh oh!

Conversation

bbasata commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Rollback Plan

Changes to Security Controls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

austinvalle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bbasata commented Jun 18, 2025

Uh oh!

austinvalle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bbasata commented Jun 5, 2025 •

edited

Loading