mirror of
https://github.com/PrefectHQ/fastmcp.git
synced 2026-08-09 07:09:11 +02:00
fix(fs): isolate same-named package imports across providers (#4361)
This commit is contained in:
parent
ddcdf64813
commit
cf1d821129
2 changed files with 144 additions and 0 deletions
|
|
@ -13,6 +13,7 @@ import hashlib
|
||||||
import importlib.util
|
import importlib.util
|
||||||
import sys
|
import sys
|
||||||
from dataclasses import dataclass, field
|
from dataclasses import dataclass, field
|
||||||
|
from importlib.machinery import ModuleSpec
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
from types import ModuleType
|
from types import ModuleType
|
||||||
|
|
||||||
|
|
@ -116,6 +117,26 @@ def _compute_module_name(file_path: Path, package_root: Path) -> str:
|
||||||
return ".".join(parts)
|
return ".".join(parts)
|
||||||
|
|
||||||
|
|
||||||
|
def _package_path_matches(module: ModuleType, package_root: Path) -> bool:
|
||||||
|
"""Check whether a package module's __path__ points at package_root.
|
||||||
|
|
||||||
|
Used to tell whether a top-level package name already present in
|
||||||
|
sys.modules belongs to this provider (same directory) or to a different
|
||||||
|
provider that happens to share the package name.
|
||||||
|
"""
|
||||||
|
module_paths = getattr(module, "__path__", None)
|
||||||
|
if not module_paths:
|
||||||
|
return False
|
||||||
|
package_root = package_root.resolve()
|
||||||
|
return any(Path(p).resolve() == package_root for p in module_paths)
|
||||||
|
|
||||||
|
|
||||||
|
def _private_package_prefix(directory: Path) -> str:
|
||||||
|
"""Compute a collision-safe synthetic package name anchored at a directory."""
|
||||||
|
digest = hashlib.sha1(str(directory.resolve()).encode()).hexdigest()[:12]
|
||||||
|
return f"_fastmcp_pkg_{digest}"
|
||||||
|
|
||||||
|
|
||||||
def import_module_from_file(
|
def import_module_from_file(
|
||||||
file_path: Path, provider_root: Path | None = None
|
file_path: Path, provider_root: Path | None = None
|
||||||
) -> ModuleType:
|
) -> ModuleType:
|
||||||
|
|
@ -150,6 +171,35 @@ def import_module_from_file(
|
||||||
# Import as part of a package
|
# Import as part of a package
|
||||||
module_name = _compute_module_name(file_path, package_root)
|
module_name = _compute_module_name(file_path, package_root)
|
||||||
|
|
||||||
|
# If another provider has already registered this top-level package name
|
||||||
|
# from a different directory, importing normally would resolve against
|
||||||
|
# that provider's directory (wrong file, or ModuleNotFoundError for a
|
||||||
|
# sibling that only exists here). Isolate this provider's tree under a
|
||||||
|
# private anchor package so both providers coexist. The anchor is a
|
||||||
|
# namespace package whose __path__ points at this provider's tree, so
|
||||||
|
# importlib resolves every intermediate package and relative import
|
||||||
|
# normally beneath it.
|
||||||
|
top_name = module_name.split(".")[0]
|
||||||
|
existing_top = sys.modules.get(top_name)
|
||||||
|
if existing_top is not None and not _package_path_matches(
|
||||||
|
existing_top, package_root
|
||||||
|
):
|
||||||
|
anchor = _private_package_prefix(package_root.parent)
|
||||||
|
if anchor not in sys.modules:
|
||||||
|
spec = ModuleSpec(anchor, loader=None, is_package=True)
|
||||||
|
anchor_module = importlib.util.module_from_spec(spec)
|
||||||
|
anchor_module.__path__ = [str(package_root.parent)]
|
||||||
|
sys.modules[anchor] = anchor_module
|
||||||
|
private_name = f"{anchor}.{module_name}"
|
||||||
|
try:
|
||||||
|
if private_name in sys.modules:
|
||||||
|
return importlib.reload(sys.modules[private_name])
|
||||||
|
return importlib.import_module(private_name)
|
||||||
|
except ImportError as e:
|
||||||
|
raise ImportError(
|
||||||
|
f"Failed to import {module_name} from {file_path}: {e}"
|
||||||
|
) from e
|
||||||
|
|
||||||
# Temporarily add package root's parent to sys.path for the import
|
# Temporarily add package root's parent to sys.path for the import
|
||||||
package_parent = str(package_root.parent)
|
package_parent = str(package_root.parent)
|
||||||
path_added = package_parent not in sys.path
|
path_added = package_parent not in sys.path
|
||||||
|
|
|
||||||
|
|
@ -459,6 +459,100 @@ def charge(amount: float) -> str:
|
||||||
names = {t.name for t in tools_list}
|
names = {t.name for t in tools_list}
|
||||||
assert names == {"greet", "charge"}
|
assert names == {"greet", "charge"}
|
||||||
|
|
||||||
|
async def test_mounted_servers_with_same_package_and_module_name(
|
||||||
|
self, tmp_path: Path
|
||||||
|
):
|
||||||
|
"""Two providers sharing a package/module path should not collide.
|
||||||
|
|
||||||
|
Reproduces the exact scenario from the bug report: both providers have a
|
||||||
|
`components/tools.py` package. Without isolation the second provider
|
||||||
|
reuses the first's cached module and `list_tools` returns the first
|
||||||
|
server's tool twice.
|
||||||
|
"""
|
||||||
|
for server_name, tool_name in [
|
||||||
|
("server1", "server1_tool"),
|
||||||
|
("server2", "server2_tool"),
|
||||||
|
]:
|
||||||
|
components = tmp_path / server_name / "components"
|
||||||
|
components.mkdir(parents=True)
|
||||||
|
(components / "__init__.py").write_text("")
|
||||||
|
(components / "tools.py").write_text(
|
||||||
|
f"""\
|
||||||
|
from fastmcp.tools import tool
|
||||||
|
|
||||||
|
@tool
|
||||||
|
def {tool_name}() -> str:
|
||||||
|
return "{tool_name}"
|
||||||
|
"""
|
||||||
|
)
|
||||||
|
|
||||||
|
server1 = FastMCP(
|
||||||
|
"server1",
|
||||||
|
providers=[FileSystemProvider(tmp_path / "server1" / "components")],
|
||||||
|
)
|
||||||
|
server2 = FastMCP(
|
||||||
|
"server2",
|
||||||
|
providers=[FileSystemProvider(tmp_path / "server2" / "components")],
|
||||||
|
)
|
||||||
|
parent = FastMCP("parent")
|
||||||
|
parent.mount(server1)
|
||||||
|
parent.mount(server2)
|
||||||
|
|
||||||
|
tools = await parent.list_tools()
|
||||||
|
assert {t.name for t in tools} == {"server1_tool", "server2_tool"}
|
||||||
|
|
||||||
|
result = await parent.call_tool("server2_tool", {})
|
||||||
|
assert "server2_tool" in str(result)
|
||||||
|
|
||||||
|
async def test_mounted_servers_with_same_package_different_module(
|
||||||
|
self, tmp_path: Path
|
||||||
|
):
|
||||||
|
"""Same package name but different module filenames must coexist.
|
||||||
|
|
||||||
|
The package directory name (`components`) collides while the leaf module
|
||||||
|
differs (`a.py` vs `b.py`). The shared package must not resolve to the
|
||||||
|
first provider's directory, which would drop the second provider's tool.
|
||||||
|
A relative import inside each module exercises that the isolated tree is
|
||||||
|
a real package.
|
||||||
|
"""
|
||||||
|
for server_name, module_name, tool_name in [
|
||||||
|
("server1", "a", "server1_tool"),
|
||||||
|
("server2", "b", "server2_tool"),
|
||||||
|
]:
|
||||||
|
components = tmp_path / server_name / "components"
|
||||||
|
components.mkdir(parents=True)
|
||||||
|
(components / "__init__.py").write_text("")
|
||||||
|
(components / "_shared.py").write_text(f'LABEL = "{tool_name}"\n')
|
||||||
|
(components / f"{module_name}.py").write_text(
|
||||||
|
f"""\
|
||||||
|
from fastmcp.tools import tool
|
||||||
|
|
||||||
|
from ._shared import LABEL
|
||||||
|
|
||||||
|
@tool
|
||||||
|
def {tool_name}() -> str:
|
||||||
|
return LABEL
|
||||||
|
"""
|
||||||
|
)
|
||||||
|
|
||||||
|
server1 = FastMCP(
|
||||||
|
"server1",
|
||||||
|
providers=[FileSystemProvider(tmp_path / "server1" / "components")],
|
||||||
|
)
|
||||||
|
server2 = FastMCP(
|
||||||
|
"server2",
|
||||||
|
providers=[FileSystemProvider(tmp_path / "server2" / "components")],
|
||||||
|
)
|
||||||
|
parent = FastMCP("parent")
|
||||||
|
parent.mount(server1)
|
||||||
|
parent.mount(server2)
|
||||||
|
|
||||||
|
tools = await parent.list_tools()
|
||||||
|
assert {t.name for t in tools} == {"server1_tool", "server2_tool"}
|
||||||
|
|
||||||
|
result = await parent.call_tool("server2_tool", {})
|
||||||
|
assert "server2_tool" in str(result)
|
||||||
|
|
||||||
|
|
||||||
class TestFileSystemProviderVersioning:
|
class TestFileSystemProviderVersioning:
|
||||||
"""Tests for version propagation through FileSystemProvider."""
|
"""Tests for version propagation through FileSystemProvider."""
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue