refactor(rpc): handlers.go - makeJSONRPCHandler() control flow refactor (#3716)

I am proposing a simpler way to write the makeJSONRPCHandler function.
There was a recent bug due to this function that turned lists of
requests with a single item to a object that is a response--but not a
list of responses. With this refactor, this function would be more
resilient to bugs because of its simplified control flow: it first tries
to unmarshal as a slice of requests, and returns an array of responses
if successful. It then tries to unmarshal it into a single request, and
returns a single response if successful. This refactor also abstracts
the logic that processes the requests out to `processRequests(r, request, funcMap, logger)` to see if they actually need to be sent (eg. notifications do not need to be sent). All of that logic is now in the processRequest function, and
improves on the previous implementation because now makeJSONRPCHandle is
much easier to read and understand. If both unmarshals fail, the
function returns an error "error unmarshalling request," same as before.
Please let me know what you think of the refactor. Thanks.

Author: masonmcbride

tm2/pkg/bft/rpc/lib/server/handlers.go

@@ -140,74 +140,84 @@ func makeJSONRPCHandler(funcMap map[string]*RPCFunc, logger *slog.Logger) http.H
 			return
 		}
 
-		// first try to unmarshal the incoming request as an array of RPC requests
-		var (
-			requests  types.RPCRequests
-			responses types.RPCResponses
-		)
-
-		// isRPCRequestArray is used to determine if the incoming payload is an array of requests.
-		// This flag helps decide whether to return an array of responses (for batch requests) or a single response.
-		isRPCRequestArray := true
+		// --- Branch 1: Attempt to Unmarshal as a Batch (Slice) of Requests ---
+		var requests types.RPCRequests
+		if err := json.Unmarshal(b, &requests); err == nil {
+			var responses types.RPCResponses
+			for _, req := range requests {
+				if resp := processRequest(r, req, funcMap, logger); resp != nil {
+					responses = append(responses, *resp)
+				}
+			}
 
-		if err := json.Unmarshal(b, &requests); err != nil {
-			// next, try to unmarshal as a single request
-			var request types.RPCRequest
-			if err := json.Unmarshal(b, &request); err != nil {
-				WriteRPCResponseHTTP(w, types.RPCParseError(types.JSONRPCStringID(""), errors.Wrap(err, "error unmarshalling request")))
+			if len(responses) > 0 {
+				WriteRPCResponseArrayHTTP(w, responses)
 				return
 			}
-			requests = []types.RPCRequest{request}
-			isRPCRequestArray = false
 		}
 
-		for _, request := range requests {
-			request := request
-			// A Notification is a Request object without an "id" member.
-			// The Server MUST NOT reply to a Notification, including those that are within a batch request.
-			if request.ID == types.JSONRPCStringID("") {
-				logger.Debug("HTTPJSONRPC received a notification, skipping... (please send a non-empty ID if you want to call a method)")
-				continue
-			}
-			if len(r.URL.Path) > 1 {
-				responses = append(responses, types.RPCInvalidRequestError(request.ID, errors.New("path %s is invalid", r.URL.Path)))
-				continue
-			}
-			rpcFunc, ok := funcMap[request.Method]
-			if !ok || rpcFunc.ws {
-				responses = append(responses, types.RPCMethodNotFoundError(request.ID))
-				continue
-			}
-			ctx := &types.Context{JSONReq: &request, HTTPReq: r}
-			args := []reflect.Value{reflect.ValueOf(ctx)}
-			if len(request.Params) > 0 {
-				fnArgs, err := jsonParamsToArgs(rpcFunc, request.Params)
-				if err != nil {
-					responses = append(responses, types.RPCInvalidParamsError(request.ID, errors.Wrap(err, "error converting json params to arguments")))
-					continue
-				}
-				args = append(args, fnArgs...)
-			}
-			returns := rpcFunc.f.Call(args)
-			logger.Info("HTTPJSONRPC", "method", request.Method, "args", args, "returns", returns)
-			result, err := unreflectResult(returns)
-			if err != nil {
-				responses = append(responses, types.RPCInternalError(request.ID, err))
-				continue
+		// --- Branch 2: Attempt to Unmarshal as a Single Request ---
+		var request types.RPCRequest
+		if err := json.Unmarshal(b, &request); err == nil {
+			if resp := processRequest(r, request, funcMap, logger); resp != nil {
+				WriteRPCResponseHTTP(w, *resp)
+				return
 			}
-			responses = append(responses, types.NewRPCSuccessResponse(request.ID, result))
-		}
-		if len(responses) == 0 {
+		} else {
+			WriteRPCResponseHTTP(w, types.RPCParseError(types.JSONRPCStringID(""), errors.Wrap(err, "error unmarshalling request")))
 			return
 		}
+	}
+}
 
-		if isRPCRequestArray {
-			WriteRPCResponseArrayHTTP(w, responses)
-			return
+// processRequest checks and processes a single JSON-RPC request.
+// If the request should produce a response, it returns a pointer to that response.
+// Otherwise (e.g. if the request is a notification or fails validation), it returns nil.
+func processRequest(r *http.Request, req types.RPCRequest, funcMap map[string]*RPCFunc, logger *slog.Logger) *types.RPCResponse {
+	// Skip notifications (an empty ID indicates no response should be sent)
+	if req.ID == types.JSONRPCStringID("") {
+		logger.Debug("Skipping notification (empty ID)")
+		return nil
+	}
+
+	// Check that the URL path is valid (assume only "/" is acceptable)
+	if len(r.URL.Path) > 1 {
+		resp := types.RPCInvalidRequestError(req.ID, fmt.Errorf("invalid path: %s", r.URL.Path))
+		return &resp
+	}
+
+	// Look up the requested method in the function map.
+	rpcFunc, ok := funcMap[req.Method]
+	if !ok || rpcFunc.ws {
+		resp := types.RPCMethodNotFoundError(req.ID)
+		return &resp
+	}
+
+	ctx := &types.Context{JSONReq: &req, HTTPReq: r}
+	args := []reflect.Value{reflect.ValueOf(ctx)}
+	if len(req.Params) > 0 {
+		fnArgs, err := jsonParamsToArgs(rpcFunc, req.Params)
+		if err != nil {
+			resp := types.RPCInvalidParamsError(req.ID, errors.Wrap(err, "error converting json params to arguments"))
+			return &resp
 		}
+		args = append(args, fnArgs...)
+	}
 
-		WriteRPCResponseHTTP(w, responses[0])
+	// Call the RPC function using reflection.
+	returns := rpcFunc.f.Call(args)
+	logger.Info("HTTPJSONRPC", "method", req.Method, "args", args, "returns", returns)
+
+	// Convert the reflection return values into a result value for JSON serialization.
+	result, err := unreflectResult(returns)
+	if err != nil {
+		resp := types.RPCInternalError(req.ID, err)
+		return &resp
 	}
+
+	// Build and return a successful response.
+	resp := types.NewRPCSuccessResponse(req.ID, result)
+	return &resp
 }
 
 func handleInvalidJSONRPCPaths(next http.HandlerFunc) http.HandlerFunc {