-
Notifications
You must be signed in to change notification settings - Fork 698
feat: add resource handler middleware capability #569
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add resource handler middleware capability #569
Conversation
This is a fairly direct adaption of the existing tool handler middleware to also allow support for resource middlewares. Use case: I'm working on an MCP server that manages an API client that is used for both tool and resource calls. The tool handler middleware provides a nice pattern to wrap tool calls that fits some use cases better than the before/after tool call hooks. It would be helpful to have first party support for this pattern in the library so I don't need to work around it with custom closures etc. Notes: - There are currently (that I can find) that exercise the existing tool handler middleware logic, so I did not add tests for the resource handler middleware logic. - Existing docs, specifically those for the streamable HTTP transport, reference some middleware functions (for both tools and resources) that don't exist (ex: `s.AddToolMiddleware` does not, `s.AddResourceMiddleware` does not exist, etc). It seems they may be out of date. Happy to discuss updates to docs in a separate PR. Signed-off-by: TJ Hoplock <[email protected]>
The existing `WithRecovery()` ServerOption is tool oriented, this adds a corresponding recovery handler for resources. This will be especially useful if Resource middlewares are used, where things may possibly/need to panic. Signed-off-by: TJ Hoplock <[email protected]>
WalkthroughIntroduces resource-specific middleware in server/server.go, adding ResourceHandlerMiddleware, storage on MCPServer, WithResourceHandlerMiddleware and WithResourceRecovery options, initialization wiring, and wrapping of direct resource handlers with a middleware chain (reverse order) under lock. Recovery middleware converts panics to typed errors containing the resource URI. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/server.go (1)
872-887
: Recovery/middleware not applied to template-based resource handlers.
Direct handlers are wrapped; template handlers are not. Panics in template handlers will bypass WithResourceRecovery and can crash the server. Align behavior by wrapping template handlers with the same chain (use an adapter because function types differ).Apply this diff:
@@ s.resourcesMu.RUnlock() - if matched { - contents, err := matchedHandler(ctx, request) + if matched { + // Adapt and wrap template handler with resource middlewares + finalTemplateHandler := matchedHandler + s.middlewareMu.RLock() + mw := s.resourceHandlerMiddlewares + for i := len(mw) - 1; i >= 0; i-- { + next := finalTemplateHandler + wrapped := mw[i](func(ctx context.Context, req mcp.ReadResourceRequest) ([]mcp.ResourceContents, error) { + return next(ctx, req) + }) + finalTemplateHandler = func(ctx context.Context, req mcp.ReadResourceRequest) ([]mcp.ResourceContents, error) { + return wrapped(ctx, req) + } + } + s.middlewareMu.RUnlock() + + contents, err := finalTemplateHandler(ctx, request) if err != nil { return nil, &requestError{ id: id, code: mcp.INTERNAL_ERROR, err: err, } } return &mcp.ReadResourceResult{Contents: contents}, nil }Also applies to: 917-926
🧹 Nitpick comments (2)
server/server.go (2)
230-241
: Guard against nil middlewares and document ordering.
A nil middleware would panic when invoked. Also, note reverse-application in the doc for clarity.Apply this diff:
// WithResourceHandlerMiddleware allows adding a middleware for the // resource handler call chain. func WithResourceHandlerMiddleware( resourceHandlerMiddleware ResourceHandlerMiddleware, ) ServerOption { return func(s *MCPServer) { - s.middlewareMu.Lock() - s.resourceHandlerMiddlewares = append(s.resourceHandlerMiddlewares, resourceHandlerMiddleware) - s.middlewareMu.Unlock() + if resourceHandlerMiddleware == nil { + return // ignore nil middleware to avoid panics + } + s.middlewareMu.Lock() + // Note: middlewares run in reverse registration order (last added runs first). + s.resourceHandlerMiddlewares = append(s.resourceHandlerMiddlewares, resourceHandlerMiddleware) + s.middlewareMu.Unlock() } }
242-259
: Include stack traces in recovery for better diagnostics.
Parity with tool recovery is fine, but a stack greatly accelerates incident triage.Apply this diff:
func WithResourceRecovery() ServerOption { return WithResourceHandlerMiddleware(func(next ResourceHandlerFunc) ResourceHandlerFunc { return func(ctx context.Context, request mcp.ReadResourceRequest) (result []mcp.ResourceContents, err error) { defer func() { if r := recover(); r != nil { - err = fmt.Errorf( - "panic recovered in %s resource handler: %v", - request.Params.URI, - r, - ) + stack := debug.Stack() + err = fmt.Errorf("panic recovered in %s resource handler: %v\n%s", request.Params.URI, r, stack) } }() return next(ctx, request) } }) }Add the import:
// at top of file import "runtime/debug"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
server/server.go
(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-23T11:10:42.948Z
Learnt from: floatingIce91
PR: mark3labs/mcp-go#401
File: server/server.go:1082-1092
Timestamp: 2025-06-23T11:10:42.948Z
Learning: In Go MCP server, ServerTool.Tool field is only used for tool listing and indexing, not for tool execution or middleware. During handleToolCall, only the Handler field is used, so dynamic tools don't need the Tool field populated.
Applied to files:
server/server.go
🧬 Code graph analysis (1)
server/server.go (2)
server/hooks.go (1)
Hooks
(94-121)mcp/types.go (3)
ReadResourceRequest
(589-593)ResourceContents
(714-716)Params
(177-177)
🔇 Additional comments (3)
server/server.go (3)
46-48
: LGTM: Resource middleware type mirrors tool middleware cleanly.
Consistent shape with ToolHandlerMiddleware; no concerns.
157-173
: LGTM: Server state extended for resource middlewares.
Thread-safe access via middlewareMu matches existing pattern for tool middleware.
338-347
: LGTM: Proper initialization of new maps/slices.
Zero-length slices avoid nil checks; consistent with tool middleware init.
I noticed that mark3labs#569 was merged (thanks!) and looked at the diff again with fresh eyes and noticed that I reused the existing mutex for tool middlewares within the resource middlewares. This means that, at least while processing middlewares, it's possible a resource call could be blocked waiting on a lock because of a tool call or vice-versa. Since there's a separate mutex for tools, resources, etc, it seems there's a desire to not block each other. This commit renames the existing middleware mutex to better clarify it's specifically for tool middlewares, and adds a new mutex for use specifically with resource middlewares. Signed-off-by: TJ Hoplock <[email protected]>
I noticed that #569 was merged (thanks!) and looked at the diff again with fresh eyes and noticed that I reused the existing mutex for tool middlewares within the resource middlewares. This means that, at least while processing middlewares, it's possible a resource call could be blocked waiting on a lock because of a tool call or vice-versa. Since there's a separate mutex for tools, resources, etc, it seems there's a desire to not block each other. This commit renames the existing middleware mutex to better clarify it's specifically for tool middlewares, and adds a new mutex for use specifically with resource middlewares. Signed-off-by: TJ Hoplock <[email protected]>
This is a fairly direct adaption of the existing tool handler middleware
to also allow support for resource middlewares.
Use case: I'm working on an MCP server that manages an API client that
is used for both tool and resource calls. The tool handler middleware
provides a nice pattern to wrap tool calls that fits some use cases
better than the before/after tool call hooks. It would be helpful to
have first party support for this pattern in the library so I don't need
to work around it with custom closures etc.
Notes:
handler middleware logic, so I did not add tests for the resource
handler middleware logic.
reference some middleware functions (for both tools and resources)
that don't exist (ex:
s.AddToolMiddleware
does not,s.AddResourceMiddleware
does not exist, etc). It seems they may be outof date. Happy to discuss updates to docs in a separate PR.