Copilot commented on code in PR #3684:
URL: https://github.com/apache/dubbo-go/pull/3684#discussion_r3812662622
##########
protocol/invocation/rpcinvocation.go:
##########
@@ -84,10 +84,15 @@ func (r *RPCInvocation) MethodName() string {
// ActualMethodName gets actual invocation method name. It returns the method
name been called if it's a generic call
func (r *RPCInvocation) ActualMethodName() string {
if r.IsGenericInvocation() {
- return r.Arguments()[0].(string)
- } else {
- return r.MethodName()
+ // IsGenericInvocation only guarantees len(Arguments)==3, not
the element types;
+ // guard the assertion so a malformed $invoke falls back to
MethodName() instead of panicking.
+ mtdName, ok := r.Arguments()[0].(string)
+ if !ok {
+ return r.MethodName()
+ }
+ return mtdName
Review Comment:
`r.Arguments()` is called inline during the assertion; consider assigning
`args := r.Arguments()` once and then using `args[0]`. This avoids repeated
method calls if `Arguments()` ever becomes non-trivial and makes the code
easier to read.
##########
protocol/invocation/rpcinvocation_test.go:
##########
@@ -164,6 +164,33 @@ func TestRPCInvocation_ActualMethodName(t *testing.T) {
assert.Equal(t, "actualAsyncMethod", invocation.ActualMethodName())
}
+// TestRPCInvocation_ActualMethodName_MalformedArgs ensures a malformed $invoke
+// (arg[0] not a string) falls back to MethodName() instead of panicking.
+// See issue #3683.
+func TestRPCInvocation_ActualMethodName_MalformedArgs(t *testing.T) {
+ cases := []struct {
+ name string
+ method string
+ args []any
+ want string
+ }{
+ {name: "invoke-arg0-int", method: constant.Generic, args:
[]any{123, []string{}, []any{}}, want: constant.Generic},
+ {name: "invoke-arg0-nil", method: constant.Generic, args:
[]any{nil, []string{}, []any{}}, want: constant.Generic},
+ {name: "invokeAsync-arg0-int", method: constant.GenericAsync,
args: []any{123, []string{}, []any{}}, want: constant.GenericAsync},
+ {name: "invokeAsync-arg0-nil", method: constant.GenericAsync,
args: []any{nil, []string{}, []any{}}, want: constant.GenericAsync},
+ }
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ inv := NewRPCInvocationWithOptions(
+ WithMethodName(tc.method),
+ WithArguments(tc.args),
+ )
+ // Should fall back to MethodName() without panicking.
+ assert.Equal(t, tc.want, inv.ActualMethodName())
Review Comment:
This test will fail on panic implicitly, but it’s clearer and more
intention-revealing to assert explicitly that `ActualMethodName()` does not
panic (e.g., via an `assert.NotPanics`/`require.NotPanics` wrapper) and then
assert the return value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
