eschutho commented on code in PR #43842:
URL: https://github.com/apache/superset/pull/43842#discussion_r4030589754
##########
tests/unit_tests/initialization_test.py:
##########
@@ -133,6 +133,72 @@ def test_init_app_in_ctx_calls_sync_config_to_db(self,
mock_logger):
# Assert that sync_config_to_db was called on the app
mock_app.sync_config_to_db.assert_called_once()
+
@patch("superset.core.mcp.core_mcp_injection.initialize_core_mcp_host_tools")
+
@patch("superset.core.mcp.core_mcp_injection.initialize_core_mcp_decorators")
+
@patch("superset.core.api.core_api_injection.initialize_core_api_dependencies")
+ def test_init_core_dependencies_registers_host_tools_by_default(
+ self,
+ mock_init_api,
+ mock_init_decorators,
+ mock_init_host_tools,
+ ):
+ """Default config (flag absent): decorators and host tools both
register."""
+ mock_app = MagicMock()
+ mock_app.config = {}
+ app_initializer = SupersetAppInitializer(mock_app)
+
+ app_initializer.init_core_dependencies()
+
+ mock_init_api.assert_called_once()
+ # Decorators are always registered so extensions using @tool/@prompt at
+ # import time keep working in every process.
+ mock_init_decorators.assert_called_once()
+ # Host tools default to on.
+ mock_init_host_tools.assert_called_once()
+
+
@patch("superset.core.mcp.core_mcp_injection.initialize_core_mcp_host_tools")
+
@patch("superset.core.mcp.core_mcp_injection.initialize_core_mcp_decorators")
+
@patch("superset.core.api.core_api_injection.initialize_core_api_dependencies")
+ def test_init_core_dependencies_registers_host_tools_when_enabled(
+ self,
+ mock_init_api,
+ mock_init_decorators,
+ mock_init_host_tools,
+ ):
+ """Flag True: decorators and host tools both register (unchanged
behavior)."""
+ mock_app = MagicMock()
+ mock_app.config = {"CORE_MCP_HOST_TOOLS_ENABLED": True}
+ app_initializer = SupersetAppInitializer(mock_app)
+
+ app_initializer.init_core_dependencies()
+
+ mock_init_api.assert_called_once()
+ mock_init_decorators.assert_called_once()
+ mock_init_host_tools.assert_called_once()
+
+
@patch("superset.core.mcp.core_mcp_injection.initialize_core_mcp_host_tools")
+
@patch("superset.core.mcp.core_mcp_injection.initialize_core_mcp_decorators")
+
@patch("superset.core.api.core_api_injection.initialize_core_api_dependencies")
+ def test_init_core_dependencies_skips_host_tools_when_disabled(
+ self,
+ mock_init_api,
+ mock_init_decorators,
+ mock_init_host_tools,
+ ):
+ """Flag False: host-tool registration is skipped, but core init and the
+ decorator swap still run (so @tool/@prompt extensions don't break)."""
+ mock_app = MagicMock()
+ mock_app.config = {"CORE_MCP_HOST_TOOLS_ENABLED": False}
+ app_initializer = SupersetAppInitializer(mock_app)
+
+ app_initializer.init_core_dependencies()
+
+ # Core API deps and the decorator swap still run.
+ mock_init_api.assert_called_once()
+ mock_init_decorators.assert_called_once()
+ # The heavy MCP service app import is skipped.
+ mock_init_host_tools.assert_not_called()
Review Comment:
Good catch — you were right. The concrete `@tool`/`@prompt` decorators
(`create_tool_decorator` / `create_prompt_decorator`) imported
`superset.mcp_service.app` inside their *inner* `decorator(func)` —
`core_mcp_injection.py:101` and `:211` — so a real extension applying
`@tool`/`@prompt` at import time pulled in the whole host-tool stack even in a
process that opted out via `CORE_MCP_HOST_TOOLS_ENABLED=False`, defeating the
worker memory benefit. It wasn't just a test gap.
Fix (d45a8ae): the concrete decorators now short-circuit to a no-op that
returns the original function when `superset.mcp_service.app` isn't already
loaded in the process. Every MCP-serving process imports it via
`initialize_core_mcp_host_tools()` before any extension applies a decorator, so
registration still works there; a disabled worker never imports it and the
decoration stays inert.
Also added a non-mocked regression test
(`test_disabled_worker_decorators_do_not_import_service_app`) that exercises
the concrete decorators directly with the service app unloaded and asserts
`superset.mcp_service.app` stays out of `sys.modules` (and that the decorated
functions pass through unchanged). Verified it fails against the old code and
passes with the fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
