feat: extend resource middlewares to resource templates #582
Conversation
The existing logic for resource middlewares was based heavily off of the prior art for tool middlewares. Tools, however, don't have the concept of templates. This change ensures that resource middlewares also get applied to resource templates. While they are defined as separate types, both `ResourceHandlerFunc` and `ResourceTemplateHandlerFunc` have the same function signature, and therefore satisfy the same underlying type. This seems intentional since resource templates are only actually executed within the same `handleReadResource` function, and only when A) no direct resources match and B) the resource uri matches the template -- executing in the same function and returning an `mcp.ReadResourceResult` regardless of direct resource or template. Since the uri needs to match in order for the template to be executed, we build the chain of middleware funcs after matching the uri to avoid wasted cycles. In order to apply the middlewares to the `ResourceTemplateHandlerFunc`, we explicitly cast to a `ResourceHandlerFunc`. This is safe, as they point to the same underlying type. If resource handler funcs ever diverge, this should also safely surface as a compile time warning, allowing ample time to address and not have a silent breakage slip through in builds. Signed-off-by: TJ Hoplock <t.hoplock@gmail.com>
WalkthroughAdds resource middleware execution to template-matched resource reads in server/server.go. When a URI matches a resource template, the matched handler is converted to a ResourceHandlerFunc, wrapped with resource middlewares (applied in reverse order) under resourceMiddlewareMu, and then invoked. Direct-resource handling was already using this path. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/server.go (2)
943-953: Minimize lock hold and DRY the wrapping logic.Copy the middleware slice under the read lock, release it, then build the chain; also consider a helper to avoid duplicating the wrapping in both code paths.
Apply this diff within this block:
- s.resourceMiddlewareMu.RLock() - finalHandler := ResourceHandlerFunc(matchedHandler) - mw := s.resourceHandlerMiddlewares - // Apply middlewares in reverse order - for i := len(mw) - 1; i >= 0; i-- { - finalHandler = mw[i](finalHandler) - } - s.resourceMiddlewareMu.RUnlock() + // Copy middlewares under lock, then build chain outside the lock + s.resourceMiddlewareMu.RLock() + mw := append([]ResourceHandlerMiddleware(nil), s.resourceHandlerMiddlewares...) + s.resourceMiddlewareMu.RUnlock() + finalHandler := ResourceHandlerFunc(matchedHandler) + for i := len(mw) - 1; i >= 0; i-- { + finalHandler = mw[i](finalHandler) + }Optionally, introduce a helper and call it here to remove duplication:
// place near other server methods func (s *MCPServer) wrapResourceHandler(h ResourceHandlerFunc) ResourceHandlerFunc { s.resourceMiddlewareMu.RLock() mw := append([]ResourceHandlerMiddleware(nil), s.resourceHandlerMiddlewares...) s.resourceMiddlewareMu.RUnlock() for i := len(mw)-1; i>=0; i-- { h = mw[i](h) } return h }Then this block becomes:
finalHandler := s.wrapResourceHandler(ResourceHandlerFunc(matchedHandler))And in the direct-resource path, replace the equivalent wrapping with:
finalHandler := s.wrapResourceHandler(handler)
943-953: Add a unit test to assert template handlers pass through middlewares.Test that a template-matched read triggers middlewares (order is reverse-registration) and that WithResourceRecovery recovers panics from a template handler.
I can draft a focused test that registers a template and a counter/panic middleware to verify execution and order. Want me to open one?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/server.go(1 hunks)
🔇 Additional comments (1)
server/server.go (1)
943-953: Middleware now applied to template handlers — parity achieved.Good call converting the matched template handler and running it through the same middleware chain as direct resources. This closes a real behavior gap.
The existing logic for resource middlewares was based heavily off of the
prior art for tool middlewares. Tools, however, don't have the concept
of templates. This change ensures that resource middlewares also get
applied to resource templates.
While they are defined as separate types, both
ResourceHandlerFuncandResourceTemplateHandlerFunchave the same function signature, andtherefore satisfy the same underlying type. This seems intentional since
resource templates are only actually executed within the same
handleReadResourcefunction, and only when A) no direct resourcesmatch and B) the resource uri matches the template -- executing in the
same function and returning an
mcp.ReadResourceResultregardless ofdirect resource or template.
Since the uri needs to match in order for the template to be executed,
we build the chain of middleware funcs after matching the uri to avoid
wasted cycles. In order to apply the middlewares to the
ResourceTemplateHandlerFunc, we explicitly cast to aResourceHandlerFunc. This is safe, as they point to the sameunderlying type. If resource handler funcs ever diverge, this should
also safely surface as a compile time warning, allowing ample time to
address and not have a silent breakage slip through in builds.
Signed-off-by: TJ Hoplock t.hoplock@gmail.com
Summary by CodeRabbit