Skip to content

Commit 5e03b50

Browse files
committed
fix: address review feedback on LFX standalone fallback
- Expand comment on `except ModuleNotFoundError` narrowing to explicitly document the ImportError-vs-ModuleNotFoundError distinction. - Bump fallback log from adebug to ainfo (mode switch is material). - Simplify `getattr(e, "name", None) or ""` to `e.name or ""`. - Add TODO(legacy-cleanup) marker flagging 800-line file debt. - Rename fallback tests to `should_[expected]_when_[condition]` convention. - Add regression tests: plain ImportError surfaces; lfx.services.deps missing surfaces (locks in prefix-check precedence). - Document why `patch("builtins.__import__")` is necessary in tests.
1 parent 36aa5e8 commit 5e03b50

2 files changed

Lines changed: 115 additions & 10 deletions

File tree

src/backend/tests/unit/components/models_and_agents/test_mcp_component.py

Lines changed: 98 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -733,8 +733,17 @@ async def test_rest_api_new_server_scenario(self, component):
733733

734734
# Connect should be called with API-provided config as fallback
735735

736+
# Tests below patch `builtins.__import__` to simulate import failures. This is a
737+
# heavyweight approach — the patch intercepts *every* import executed while active,
738+
# including unrelated library code pulled in by awaits, mocks, or assertions — but
739+
# targeted alternatives (sys.modules / importlib) do not reliably reproduce the
740+
# `ModuleNotFoundError` raised from an `import langflow.*` statement inside the
741+
# code under test. The `real_import` fallback forwards all non-matching names to
742+
# the real import machinery; the `name`-prefix guard in each fake_import is kept
743+
# as tight as possible so surrounding test plumbing is not affected.
744+
736745
@pytest.mark.asyncio
737-
async def test_lfx_standalone_falls_back_to_value_config(self, component):
746+
async def test_update_tool_list_falls_back_to_value_config_when_langflow_absent(self, component):
738747
"""Test LFX standalone mode falls back to value config when Langflow is unavailable.
739748
740749
Regression test: when lfx is run without the full Langflow package installed
@@ -780,7 +789,7 @@ def fake_import(name, import_globals=None, import_locals=None, fromlist=(), leve
780789
assert server_info["config"]["command"] == "uvx mcp-server-from-value"
781790

782791
@pytest.mark.asyncio
783-
async def test_transitive_import_error_surfaces(self, component):
792+
async def test_update_tool_list_surfaces_transitive_import_error_instead_of_falling_back(self, component):
784793
"""A transitive ModuleNotFoundError inside Langflow must NOT be silently swallowed.
785794
786795
If a Langflow dependency (e.g. sqlmodel) fails to import while loading
@@ -825,6 +834,93 @@ def fake_import(name, import_globals=None, import_locals=None, fromlist=(), leve
825834

826835
mock_update_tools.assert_not_called()
827836

837+
@pytest.mark.asyncio
838+
async def test_update_tool_list_surfaces_plain_import_error_instead_of_falling_back(self, component):
839+
"""A plain ImportError (attribute missing, not module missing) must surface.
840+
841+
Regression test for the intentional `except ModuleNotFoundError` narrowing: if
842+
an installed Langflow no longer exposes `get_server` or `get_user_by_id` (real
843+
API break), the resulting ImportError — which is NOT a ModuleNotFoundError —
844+
must NOT be swallowed as standalone mode. It must propagate to the outer
845+
handler and surface as a `ValueError("Error updating tool list: ...")`.
846+
"""
847+
import builtins
848+
849+
component.mcp_server = {
850+
"name": "broken_server",
851+
"config": {"command": "uvx mcp-server-from-value"},
852+
}
853+
component._user_id = "test_user_123"
854+
855+
real_import = builtins.__import__
856+
857+
def fake_import(name, import_globals=None, import_locals=None, fromlist=(), level=0):
858+
# The module imports fine, but a specific attribute is missing — this
859+
# raises plain ImportError (not ModuleNotFoundError) at the `from ... import`
860+
# line. Emulate that by raising ImportError when this module is requested
861+
# with a fromlist, just as `from langflow.api.v2.mcp import get_server` would.
862+
if name == "langflow.api.v2.mcp" and fromlist and "get_server" in fromlist:
863+
msg = "cannot import name 'get_server' from 'langflow.api.v2.mcp'"
864+
raise ImportError(msg, name="langflow.api.v2.mcp")
865+
return real_import(name, import_globals, import_locals, fromlist, level)
866+
867+
with (
868+
patch("builtins.__import__", side_effect=fake_import),
869+
patch("lfx.components.models_and_agents.mcp_component.update_tools") as mock_update_tools,
870+
):
871+
mock_update_tools.return_value = (None, [], {})
872+
873+
with pytest.raises(ValueError, match="Error updating tool list") as exc_info:
874+
await component.update_tool_list()
875+
876+
# The original ImportError should be preserved as __cause__ — it must not
877+
# have been silently converted into a fallback path.
878+
assert isinstance(exc_info.value.__cause__, ImportError)
879+
assert not isinstance(exc_info.value.__cause__, ModuleNotFoundError)
880+
881+
mock_update_tools.assert_not_called()
882+
883+
@pytest.mark.asyncio
884+
async def test_update_tool_list_surfaces_lfx_services_deps_missing(self, component):
885+
"""A ModuleNotFoundError for an lfx module (not langflow.*) must surface.
886+
887+
Edge-case regression test: if `lfx.services.deps` itself is missing (a
888+
packaging error inside lfx), the prefix check `missing_module == "langflow"
889+
or missing_module.startswith("langflow.")` correctly returns False and the
890+
error is re-raised. Locks in the intended precedence rule so a future rewrite
891+
of the prefix check cannot silently convert this into a standalone fallback.
892+
"""
893+
import builtins
894+
895+
component.mcp_server = {
896+
"name": "broken_server",
897+
"config": {"command": "uvx mcp-server-from-value"},
898+
}
899+
component._user_id = "test_user_123"
900+
901+
real_import = builtins.__import__
902+
903+
deps_missing_msg = "No module named 'lfx.services.deps'"
904+
905+
def fake_import(name, import_globals=None, import_locals=None, fromlist=(), level=0):
906+
if name == "lfx.services.deps" or name.startswith("lfx.services.deps."):
907+
raise ModuleNotFoundError(deps_missing_msg, name="lfx.services.deps")
908+
return real_import(name, import_globals, import_locals, fromlist, level)
909+
910+
with (
911+
patch("builtins.__import__", side_effect=fake_import),
912+
patch("lfx.components.models_and_agents.mcp_component.update_tools") as mock_update_tools,
913+
):
914+
mock_update_tools.return_value = (None, [], {})
915+
916+
with pytest.raises(ValueError, match="Error updating tool list") as exc_info:
917+
await component.update_tool_list()
918+
919+
assert isinstance(exc_info.value.__cause__, ModuleNotFoundError)
920+
assert exc_info.value.__cause__.name == "lfx.services.deps"
921+
922+
mock_update_tools.assert_not_called()
923+
828924

829925
# ============================================================================
830926
# Tests for resolve_mcp_config pure function

src/lfx/src/lfx/components/models_and_agents/mcp_component.py

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,9 @@ def resolve_mcp_config(
5050
return server_config_from_value
5151

5252

53+
# TODO(legacy-cleanup): This file is ~800 lines, over the 500-line guideline. Split
54+
# helper functions (resolve_mcp_config, connection resolution) from the MCPToolsComponent
55+
# orchestration logic in a follow-up PR.
5356
class MCPToolsComponent(ComponentWithCache):
5457
schema_inputs: list = []
5558
tools: list[StructuredTool] = []
@@ -240,17 +243,23 @@ async def update_tool_list(self, mcp_server_value=None):
240243

241244
from lfx.services.deps import get_settings_service
242245
except ModuleNotFoundError as e:
243-
# Only treat this as LFX standalone mode when one of the target Langflow
244-
# modules is itself missing. Transitive ModuleNotFoundError (e.g. a
245-
# dependency like sqlmodel failing to import inside langflow.*) indicates
246-
# a real bug in the full Langflow stack and must surface — otherwise we
247-
# would silently use a stale flow-embedded config when DB config should
248-
# have taken precedence.
249-
missing_module = getattr(e, "name", None) or ""
246+
# Deliberately `except ModuleNotFoundError` (not `except ImportError`): a
247+
# plain ImportError here means `get_server` / `get_user_by_id` was removed
248+
# from an installed Langflow — a real API break that should NOT be
249+
# swallowed as "standalone mode". ModuleNotFoundError alone covers the
250+
# "Langflow absent" case.
251+
#
252+
# Even within ModuleNotFoundError, only treat this as LFX standalone mode
253+
# when one of the target Langflow modules is itself missing. Transitive
254+
# ModuleNotFoundError (e.g. a dependency like sqlmodel failing to import
255+
# inside langflow.*) indicates a real bug in the full Langflow stack and
256+
# must surface — otherwise we would silently use a stale flow-embedded
257+
# config when DB config should have taken precedence.
258+
missing_module = e.name or ""
250259
is_langflow_standalone = missing_module == "langflow" or missing_module.startswith("langflow.")
251260
if not is_langflow_standalone:
252261
raise
253-
await logger.adebug(
262+
await logger.ainfo(
254263
"Langflow package not available; using MCP server config from flow value (LFX standalone mode)."
255264
)
256265
else:

0 commit comments

Comments
 (0)