Skip to content

Commit

Permalink
Fix positional optional params (#2421)
Browse files Browse the repository at this point in the history
* Fix positional optional params

* use required count instead of optional
  • Loading branch information
weiihann authored Feb 5, 2025
1 parent c646d21 commit daeb674
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 5 deletions.
19 changes: 18 additions & 1 deletion jsonrpc/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,10 @@ type Method struct {
// The method takes a context as its first parameter.
// Set upon successful registration.
needsContext bool

// The number of required parameters in the method.
// Set upon successful registration.
requiredParamCount int
}

type Server struct {
Expand Down Expand Up @@ -198,6 +202,14 @@ func (s *Server) registerMethod(method Method) error {
return errors.New("second return value must be a http.Header for 3 tuple handler")
}

requiredParamCount := 0
for _, param := range method.Params {
if !param.Optional {
requiredParamCount++
}
}
method.requiredParamCount = requiredParamCount

// The method is valid. Mutate the appropriate fields and register on the server.
s.methods[method.Name] = method

Expand Down Expand Up @@ -534,7 +546,8 @@ func (s *Server) buildArguments(ctx context.Context, params any, method Method)
case reflect.Slice:
paramsList := params.([]any)

if len(paramsList) != numArgs-addContext {
// Ensure that the number of provided parameters is between required and total parameters
if len(paramsList) < method.requiredParamCount || len(paramsList) > len(method.Params) {
return nil, errors.New("missing/unexpected params in list")
}

Expand All @@ -545,6 +558,10 @@ func (s *Server) buildArguments(ctx context.Context, params any, method Method)
}
args = append(args, v)
}
// Add remaining optional parameters if available
for i := addContext + len(paramsList); i < numArgs; i++ {
args = append(args, reflect.New(handlerType.In(i)).Elem())
}
case reflect.Map:
paramsMap := params.(map[string]any)

Expand Down
24 changes: 20 additions & 4 deletions jsonrpc/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,18 @@ func TestHandle(t *testing.T) {
return 0, nil
},
},
{
Name: "multipleOptionalParams",
Params: []jsonrpc.Parameter{
{Name: "param1"},
{Name: "param2"},
{Name: "param3", Optional: true},
{Name: "param4", Optional: true},
},
Handler: func(param1 *int, param2 []int, param3 *int, param4 []int) (int, *jsonrpc.Error) {
return 0, nil
},
},
}

listener := CountingEventListener{}
Expand Down Expand Up @@ -218,10 +230,6 @@ func TestHandle(t *testing.T) {
req: `{"jsonrpc" : "2.0", "method" : "method", "id" : 5}`,
res: `{"jsonrpc":"2.0","error":{"code":-32602,"message":"Invalid Params","data":"missing non-optional param field"},"id":5}`,
},
"missing param(s)": {
req: `{"jsonrpc" : "2.0", "method" : "method", "params" : [3, false] , "id" : 3}`,
res: `{"jsonrpc":"2.0","error":{"code":-32602,"message":"Invalid Params","data":"missing/unexpected params in list"},"id":3}`,
},
"too many params": {
req: `{"jsonrpc" : "2.0", "method" : "method", "params" : [3, false, "error message", "too many"] , "id" : 3}`,
res: `{"jsonrpc":"2.0","error":{"code":-32602,"message":"Invalid Params","data":"missing/unexpected params in list"},"id":3}`,
Expand Down Expand Up @@ -490,6 +498,14 @@ func TestHandle(t *testing.T) {
req: `{"jsonrpc": "2.0", "method": "singleOptionalParam", "id": 1}`,
res: `{"jsonrpc":"2.0","result":0,"id":1}`,
},
"empty multiple optional params": {
req: `{"jsonrpc": "2.0", "method": "multipleOptionalParams", "params": {"param1": 1, "param2": [2, 3]}, "id": 1}`,
res: `{"jsonrpc":"2.0","result":0,"id":1}`,
},
"empty multiple optional positional params": {
req: `{"jsonrpc": "2.0", "method": "multipleOptionalParams", "params": [1, [2, 3]], "id": 1}`,
res: `{"jsonrpc":"2.0","result":0,"id":1}`,
},
}

for desc, test := range tests {
Expand Down

0 comments on commit daeb674

Please sign in to comment.