mcp-go icon indicating copy to clipboard operation
mcp-go copied to clipboard

Extract Params from CallToolRequest into a Named Struct

Open basebandit opened this issue 8 months ago • 4 comments

Currently, CallToolRequest uses an anonymous inline struct for the Params field, which makes it cumbersome to initialize in tests or client code. This is particularly painful when mocking or setting up test cases, as it requires verbose and brittle inline struct literals.

Example

In this test, the anonymous struct makes the code harder to read and reuse:

request := mcp.CallToolRequest{
	Params: struct {
		Name      string                 `json:"name"`
		Arguments map[string]interface{} `json:"arguments,omitempty"`
		Meta      *struct {
			ProgressToken mcp.ProgressToken `json:"progressToken,omitempty"`
		} `json:"_meta,omitempty"`
	}{
		Arguments: map[string]interface{}{
			"all_namespaces": true,
		},
	},
}

This verbosity could be avoided if Params (and optionally _meta) were defined as named types.

Suggested Improvement

Refactor CallToolRequest to use a named ToolParameters struct:

type ToolParameters struct {
	Name      string                 `json:"name"`
	Arguments map[string]interface{} `json:"arguments,omitempty"`
	Meta      *ToolParametersMeta    `json:"_meta,omitempty"`
}

type ToolParametersMeta struct {
	ProgressToken ProgressToken `json:"progressToken,omitempty"`
}

type CallToolRequest struct {
	Request
	Params ToolParameters `json:"params"`
}

This would greatly improve testability, readability, and ease of use.

basebandit avatar Apr 04 '25 16:04 basebandit

This issue with anonymous inline structs isn't limited to CallToolRequest. It exists in many other types under types.go, including:

InitializeRequest, Request, JSONRPCError, CancelledNotification, ServerCapabilities, ProgressNotification, PaginatedRequest, ReadResourceRequest, SubscribeRequest, UnsubscribeRequest, ResourceUpdatedNotification, SetLevelRequest, LoggingMessageNotification, CreateMessageRequest, CompleteRequest.

Having inline anonymous structs in all these cases makes it unnecessarily verbose in client code. It also affects reusability.

Would be happy to help with a refactor PR if there's interest.

yinebebt avatar May 01 '25 10:05 yinebebt

Looks like part of this issue i.e Meta anonymous struct was addressed by this PR. Thanks to @anuraaga.

basebandit avatar May 20 '25 15:05 basebandit

Yeah I extracted a type there mostly to be able to implement the custom JSON marshaling. But I agree with this issue for all the other types too that generally avoiding anonymous structs across the board will make things easier to use. Admittedly a particular minor use case but it was very hard to write transport middleware. For anyone interested in Go trivia, the json tags are required for it to work 😂

// SendRequest implements transport.Interface.
func (c *contextTransport) SendRequest(ctx context.Context, request transport.JSONRPCRequest) (*transport.JSONRPCResponse, error) {
	if params, ok := request.Params.(struct {
		Name      string         `json:"name"`
		Arguments map[string]any `json:"arguments,omitempty"`
		Meta      *mcp.Meta      `json:"_meta,omitempty"`
	}); ok {
		meta := params.Meta
		if meta == nil {
			meta = &mcp.Meta{}
			params.Meta = meta
		}
		if meta.AdditionalFields == nil {
			meta.AdditionalFields = make(map[string]any)
		}
		otel.GetTextMapPropagator().Inject(ctx, additionalFieldsCarrier{meta.AdditionalFields})
		request.Params = params
	}
	return c.delegate.SendRequest(ctx, request)
}

anuraaga avatar May 21 '25 10:05 anuraaga

Some fixes were made here: https://github.com/mark3labs/mcp-go/pull/333

Ideally all anonymous structs must be changed.

pottekkat avatar May 29 '25 06:05 pottekkat