fix: clone request state in response.To#315
Open
immanuwell wants to merge 1 commit into
Open
Conversation
Signed-off-by: immanuwell <pchpr.00@list.ru>
📝 WalkthroughWalkthroughThe PR fixes an aliasing bug where ChangesRequest State Cloning
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
response/response.go (2)
46-49: 💤 Low valueConsider safer type assertion for cloned Context.
Similar to the Desired state handling, the type assertion for Context could benefit from a checked assertion pattern. However, this is a minor defensive coding suggestion rather than a critical issue.
🤖 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.go` around lines 46 - 49, The cloned Context should use a checked type assertion to avoid panics: replace the direct assertion in the req.GetContext() block with a two-value assertion from proto.Clone(c) into a local variable (e.g., v, ok := proto.Clone(c).(*structpb.Struct)); if ok set ctx = v, otherwise leave ctx nil (or log/handle the unexpected type) so the code mirrors the safer pattern used for Desired state handling.
41-44: 💤 Low valueConsider safer type assertion for cloned Desired state.
The type assertion
proto.Clone(d).(*v1.State)could panic ifproto.Clonereturns an unexpected type. Whileproto.Cloneshould preserve the concrete type, using a checked type assertion would make this more defensive:var desired *v1.State if d := req.GetDesired(); d != nil { cloned, ok := proto.Clone(d).(*v1.State) if !ok { // This should never happen, but guards against unexpected proto.Clone behavior panic("proto.Clone returned unexpected type for Desired state") } desired = cloned }That said, if the current pattern is established elsewhere in the codebase and
proto.Cloneis guaranteed to preserve types, this may be acceptable as-is. What's the team's preference for type assertions with protobuf cloning?🤖 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.go` around lines 41 - 44, The unchecked type assertion proto.Clone(d).(*v1.State) can panic if the clone's concrete type differs; update the desired construction (around req.GetDesired and proto.Clone) to use a checked assertion: store proto.Clone(d) in a variable, perform a two-value type assertion to *v1.State, handle the !ok branch (either panic with a clear message or return an error) and only assign desired when ok is true to make the code defensive against unexpected proto.Clone results.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@response/response_test.go`:
- Around line 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.
---
Nitpick comments:
In `@response/response.go`:
- Around line 46-49: The cloned Context should use a checked type assertion to
avoid panics: replace the direct assertion in the req.GetContext() block with a
two-value assertion from proto.Clone(c) into a local variable (e.g., v, ok :=
proto.Clone(c).(*structpb.Struct)); if ok set ctx = v, otherwise leave ctx nil
(or log/handle the unexpected type) so the code mirrors the safer pattern used
for Desired state handling.
- Around line 41-44: The unchecked type assertion proto.Clone(d).(*v1.State) can
panic if the clone's concrete type differs; update the desired construction
(around req.GetDesired and proto.Clone) to use a checked assertion: store
proto.Clone(d) in a variable, perform a two-value type assertion to *v1.State,
handle the !ok branch (either panic with a clear message or return an error) and
only assign desired when ok is true to make the code defensive against
unexpected proto.Clone results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fb82b9b0-731c-4019-958e-52ed980562c4
📒 Files selected for processing (2)
response/response.goresponse/response_test.go
Comment on lines
+34
to
+71
| 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") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of your changes
Fixes #314
response.Tosaid it copied request state, but it was still sharingDesiredandContextwith the request. So a function could updaterspafterTo(...)and accidentally mutatereqtoo. Kinda sneaky bug.This clones both fields before returning, so
rspis its own thing now. nice and boring.I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Repro on old
main:DesiredandContext.rsp := response.To(req, time.Minute).rsp.reqchanges too. not ideal.Ran:
go test ./response -run TestToClonesRequestState -count=1go test ./...