Added support for custom middleware being injected into the underlying MCP server instance#114
Conversation
…g MCP server instance
|
|
joe-clickhouse
left a comment
There was a problem hiding this comment.
Hi @andrei-katkov, thanks for the PR! This is a really nice addition. I left a few comments in the code and there are a few other points:
- Looks like there's a typo in the filename
mpc_middleware_hook.py. Should it bemcp_.... Notice the transposed "p" and "c". - The current tests are reproducing the logic of the implementation inside the test instead of importing and invoking the actual code, which means they'll always pass even if someone later changes the actual implementation which is problematic. To test the actual code we don't want to mock the
setup_middlewarefunction. For example, to test if it correctly handles no configuration, we'll want to do something like:
import os
from unittest.mock import Mock, patch
# Import the actual function
# assuming the filename is fixed to mcp... instead of mpc...
from mcp_clickhouse.mcp_middleware_hook import setup_middleware
class TestMiddleware:
def test_no_configuration(self):
"""Verify that if no env var is set nothing happens"""
mock_mcp = Mock()
with (
patch.dict(os.environ, {}, clear=True),
patch("importlib.import_module") as mock_import,
):
# Call the ACTUAL function we imported
setup_middleware(mock_mcp)
# Shouldn't attempt imports
mock_import.assert_not_called()Let me know if you have any questions about this or would like me to flesh this out further. Happy to help!
| logger.error(f"Failed to import middleware module '{middleware_module}': {e}") | ||
| except Exception as e: | ||
| logger.error(f"Failed to load middleware: {e}") | ||
| return mcp |
There was a problem hiding this comment.
Nit: The mcp object is modified in place, no need to return it as the caller ignores it.
There was a problem hiding this comment.
Indeed. This is now fixed in d36d32f. Thank you
|
Hello, @joe-clickhouse. Thank you for your review. I've addressed the following concerns of yours:
Can you please review again? Thank you |
|
Hi @andrei-katkov, I hope you don't mind but I added a few final small changes. Mostly my fault for not noticing on the first review. Take a look when you have a chance, but I think this'll take us over the finish line on this one:
|
|
Hello, @joe-clickhouse. Your additional commits make total sense, and I have no objections to them. Let me know if anything else needs to be improved before we can merge it. Thank you very much. |
This PR adds a new configuration option, which allows library users to inject their own middleware into the underlying FastMCP server instance. Middleware class is inferred by the module name provided in configuration.
The specified module is intended to provide a
setup_middlewaremethod that receives an instance of the MCP server.