Skip to content

fix: clone request state in response.To #315

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
immanuwell wants to merge 1 commit into
base: main
Choose a base branch
from
+54 −2
Open

fix: clone request state in response.To #315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions response/response.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"encoding/json"
"time"

"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/encoding/protojson"
"google.golang.org/protobuf/types/known/durationpb"
"google.golang.org/protobuf/types/known/structpb"
Expand All @@ -37,13 +38,23 @@ const DefaultTTL = 1 * time.Minute
// To bootstraps a response to the supplied request. It automatically copies the
// desired state from the request.
func To(req *v1.RunFunctionRequest, ttl time.Duration) *v1.RunFunctionResponse {
var desired *v1.State
if d := req.GetDesired(); d != nil {
desired = proto.Clone(d).(*v1.State)
}

var ctx *structpb.Struct
if c := req.GetContext(); c != nil {
ctx = proto.Clone(c).(*structpb.Struct)
}

return &v1.RunFunctionResponse{
Meta: &v1.ResponseMeta{
Tag: req.GetMeta().GetTag(),
Ttl: durationpb.New(ttl),
},
Desired: req.GetDesired(),
Context: req.GetContext(),
Desired: desired,
Context: ctx,
}
}

Expand Down
41 changes: 41 additions & 0 deletions response/response_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,57 @@ package response
import (
"encoding/json"
"testing"
"time"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"google.golang.org/protobuf/testing/protocmp"
"google.golang.org/protobuf/types/known/structpb"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"

v1 "github.com/crossplane/function-sdk-go/proto/v1"
"github.com/crossplane/function-sdk-go/resource"
)

func TestToClonesRequestState(t *testing.T) {
req := &v1.RunFunctionRequest{
Meta: &v1.RequestMeta{Tag: "original"},
Desired: &v1.State{
Resources: map[string]*v1.Resource{
"existing": {
Resource: resource.MustStructJSON(`{
"apiVersion": "example.org/v1",
"kind": "Widget"
}`),
},
},
},
Context: &structpb.Struct{
Fields: map[string]*structpb.Value{
"existing": structpb.NewStringValue("value"),
},
},
}

rsp := To(req, time.Minute)

SetContextKey(rsp, "new", structpb.NewStringValue("response-only"))
rsp.Desired.Resources["response-only"] = &v1.Resource{
Resource: resource.MustStructJSON(`{
"apiVersion": "example.org/v1",
"kind": "ExtraWidget"
}`),
}

if got, ok := req.GetContext().GetFields()["new"]; ok {
t.Fatalf("To(...) aliased request context: unexpected field %q", got.GetStringValue())
}

if _, ok := req.GetDesired().GetResources()["response-only"]; ok {
t.Fatal("To(...) aliased desired resources from the request")
}
}
Comment on lines +34 to +71

@coderabbitai coderabbitai Bot Jun 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Consider refactoring to table-driven test structure.

While this test effectively validates the cloning behavior, the coding guidelines require table-driven test structure with args/want pattern for consistency with the rest of the codebase. Looking at the other tests in this file (e.g., TestSetDesiredResources, TestOutput), they all follow the table-driven pattern.

You could structure this as:

func TestToClonesRequestState(t *testing.T) {
    type args struct {
        req *v1.RunFunctionRequest
        ttl time.Duration
    }
    type want struct {
        contextFieldsMatch  bool
        resourcesMatch      bool
    }
    cases := map[string]struct {
        reason string
        args   args
        mutate func(*v1.RunFunctionResponse)
        want   want
    }{
        "MutatingContextDoesNotAffectRequest": {
            reason: "Adding fields to response context should not modify request context",
            args: args{
                req: &v1.RunFunctionRequest{
                    Context: &structpb.Struct{
                        Fields: map[string]*structpb.Value{
                            "existing": structpb.NewStringValue("value"),
                        },
                    },
                },
                ttl: time.Minute,
            },
            mutate: func(rsp *v1.RunFunctionResponse) {
                SetContextKey(rsp, "new", structpb.NewStringValue("response-only"))
            },
            want: want{
                contextFieldsMatch: true,
                resourcesMatch:     true,
            },
        },
        // Additional cases for mutating Desired, both fields, etc.
    }
    // ... test implementation
}

This would align with the established pattern and make it easier to add more mutation scenarios in the future. What do you think about this approach?

As per coding guidelines, **/*_test.go: Enforce table-driven test structure: PascalCase test names (no underscores), args/want pattern.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@response/response_test.go` around lines 34 - 71, The test
TestToClonesRequestState should be refactored into a table-driven test using the
args/want pattern: define inner types args { req *v1.RunFunctionRequest; ttl
time.Duration } and want { contextFieldsMatch bool; resourcesMatch bool },
create a map[string]struct{ reason string; args args; mutate
func(*v1.RunFunctionResponse); want want } with cases (e.g., mutating context,
mutating desired resources), loop with t.Run(caseName, func(t *testing.T){ rsp
:= To(tc.args.req, tc.args.ttl); tc.mutate(rsp); assert that
req.GetContext().GetFields() and req.GetDesired().GetResources() are unchanged
compared to expectations }), and use SetContextKey and rsp.Desired mutation
inside mutate funcs to exercise behavior; ensure the test uses t.Run per case
and follows the existing PascalCase naming convention for the top-level test
function (TestToClonesRequestState) and case names.


func TestSetDesiredResources(t *testing.T) {
type args struct {
rsp *v1.RunFunctionResponse
Expand Down