From e7e301815f7d21235be0a5ba44c015be170f10a5 Mon Sep 17 00:00:00 2001
From: Jeremiah Lowin <153965+jlowin@users.noreply.github.com>
Date: Tue, 13 May 2025 14:33:11 -0400
Subject: [PATCH] Handle template errors

---
 src/fastmcp/resources/resource_manager.py  |  4 +++
 src/fastmcp/resources/template.py          | 31 +++++++++---------
 src/fastmcp/resources/types.py             | 37 +++++++++-------------
 tests/client/test_client.py                | 30 ++++++++++++++++++
 tests/resources/test_function_resources.py |  2 +-
 tests/resources/test_resource_manager.py   | 30 ++----------------
 tests/resources/test_resource_template.py  | 15 ---------
 7 files changed, 67 insertions(+), 82 deletions(-)

diff --git a/src/fastmcp/resources/resource_manager.py b/src/fastmcp/resources/resource_manager.py
index 2dd696be8..9d8a20d8e 100644
--- a/src/fastmcp/resources/resource_manager.py
+++ b/src/fastmcp/resources/resource_manager.py
@@ -244,7 +244,11 @@ class ResourceManager:
                         uri_str,
                         params=params,
                     )
+                except ResourceError as e:
+                    logger.error(f"Error creating resource from template: {e}")
+                    raise e
                 except Exception as e:
+                    logger.error(f"Error creating resource from template: {e}")
                     raise ValueError(f"Error creating resource from template: {e}")
 
         raise NotFoundError(f"Unknown resource: {uri_str}")
diff --git a/src/fastmcp/resources/template.py b/src/fastmcp/resources/template.py
index 7bae3554e..1335a2559 100644
--- a/src/fastmcp/resources/template.py
+++ b/src/fastmcp/resources/template.py
@@ -171,28 +171,27 @@ class ResourceTemplate(BaseModel):
         """Create a resource from the template with the given parameters."""
         from fastmcp.server.context import Context
 
-        try:
-            # Add context to parameters if needed
-            kwargs = params.copy()
-            context_kwarg = find_kwarg_by_type(self.fn, kwarg_type=Context)
-            if context_kwarg and context_kwarg not in kwargs:
-                kwargs[context_kwarg] = get_context()
+        # Add context to parameters if needed
+        kwargs = params.copy()
+        context_kwarg = find_kwarg_by_type(self.fn, kwarg_type=Context)
+        if context_kwarg and context_kwarg not in kwargs:
+            kwargs[context_kwarg] = get_context()
 
+        async def resource_read_fn() -> str | bytes:
             # Call function and check if result is a coroutine
             result = self.fn(**kwargs)
             if inspect.iscoroutine(result):
                 result = await result
+            return result
 
-            return FunctionResource(
-                uri=AnyUrl(uri),  # Explicitly convert to AnyUrl
-                name=self.name,
-                description=self.description,
-                mime_type=self.mime_type,
-                fn=lambda **kwargs: result,  # Capture result in closure
-                tags=self.tags,
-            )
-        except Exception as e:
-            raise ValueError(f"Error creating resource from template: {e}")
+        return FunctionResource(
+            uri=AnyUrl(uri),  # Explicitly convert to AnyUrl
+            name=self.name,
+            description=self.description,
+            mime_type=self.mime_type,
+            fn=resource_read_fn,
+            tags=self.tags,
+        )
 
     def __eq__(self, other: object) -> bool:
         if not isinstance(other, ResourceTemplate):
diff --git a/src/fastmcp/resources/types.py b/src/fastmcp/resources/types.py
index a52194c90..f1b9ff74d 100644
--- a/src/fastmcp/resources/types.py
+++ b/src/fastmcp/resources/types.py
@@ -63,30 +63,23 @@ class FunctionResource(Resource):
         """Read the resource by calling the wrapped function."""
         from fastmcp.server.context import Context
 
-        try:
-            kwargs = {}
-            context_kwarg = find_kwarg_by_type(self.fn, kwarg_type=Context)
-            if context_kwarg is not None:
-                kwargs[context_kwarg] = get_context()
+        kwargs = {}
+        context_kwarg = find_kwarg_by_type(self.fn, kwarg_type=Context)
+        if context_kwarg is not None:
+            kwargs[context_kwarg] = get_context()
 
-            result = self.fn(**kwargs)
-            if inspect.iscoroutinefunction(self.fn):
-                result = await result
+        result = self.fn(**kwargs)
+        if inspect.iscoroutinefunction(self.fn):
+            result = await result
 
-            if isinstance(result, Resource):
-                return await result.read()
-            elif isinstance(result, bytes):
-                return result
-            elif isinstance(result, str):
-                return result
-            else:
-                return pydantic_core.to_json(result, fallback=str, indent=2).decode()
-        except ResourceError as e:
-            logger.exception(f"Error reading resource {self.uri}: {e}")
-            raise e
-        except Exception as e:
-            logger.exception(f"Error reading resource {self.uri}: {e}")
-            raise ValueError(f"Error reading resource {self.uri}.") from e
+        if isinstance(result, Resource):
+            return await result.read()
+        elif isinstance(result, bytes):
+            return result
+        elif isinstance(result, str):
+            return result
+        else:
+            return pydantic_core.to_json(result, fallback=str, indent=2).decode()
 
 
 class FileResource(Resource):
diff --git a/tests/client/test_client.py b/tests/client/test_client.py
index d0200ff93..ff460b295 100644
--- a/tests/client/test_client.py
+++ b/tests/client/test_client.py
@@ -469,3 +469,33 @@ class TestErrorHandling:
             with pytest.raises(Exception) as excinfo:
                 await client.read_resource(AnyUrl("error://resource"))
             assert "This is a resource error (xyz)" in str(excinfo.value)
+
+    async def test_general_template_exceptions_are_masked(self):
+        mcp = FastMCP("TestServer")
+
+        @mcp.resource(uri="exception://resource/{id}")
+        async def exception_resource(id: str):
+            raise ValueError("This is an internal error (sensitive)")
+
+        client = Client(transport=FastMCPTransport(mcp))
+
+        async with client:
+            with pytest.raises(Exception) as excinfo:
+                await client.read_resource(AnyUrl("exception://resource/123"))
+            assert "Error reading resource" in str(excinfo.value)
+            assert "sensitive" not in str(excinfo.value)
+            assert "internal error" not in str(excinfo.value)
+
+    async def test_template_errors_are_sent_to_client(self):
+        mcp = FastMCP("TestServer")
+
+        @mcp.resource(uri="error://resource/{id}")
+        async def error_resource(id: str):
+            raise ResourceError("This is a resource error (xyz)")
+
+        client = Client(transport=FastMCPTransport(mcp))
+
+        async with client:
+            with pytest.raises(Exception) as excinfo:
+                await client.read_resource(AnyUrl("error://resource/123"))
+            assert "This is a resource error (xyz)" in str(excinfo.value)
diff --git a/tests/resources/test_function_resources.py b/tests/resources/test_function_resources.py
index 46ac320ae..8ebbe27c0 100644
--- a/tests/resources/test_function_resources.py
+++ b/tests/resources/test_function_resources.py
@@ -80,7 +80,7 @@ class TestFunctionResource:
             name="test",
             fn=failing_func,
         )
-        with pytest.raises(ValueError, match="Error reading resource function://test"):
+        with pytest.raises(ValueError, match="Test error"):
             await resource.read()
 
     async def test_basemodel_conversion(self):
diff --git a/tests/resources/test_resource_manager.py b/tests/resources/test_resource_manager.py
index f6739657c..006911c47 100644
--- a/tests/resources/test_resource_manager.py
+++ b/tests/resources/test_resource_manager.py
@@ -600,8 +600,7 @@ class TestResourceErrorHandling:
         )
         manager.add_template(template)
 
-        # ResourceErrors in templates are wrapped in ValueError
-        with pytest.raises(ValueError) as excinfo:
+        with pytest.raises(ResourceError) as excinfo:
             await manager.read_resource("error://test")
 
         # The original error message should be included in the ValueError
@@ -623,30 +622,5 @@ class TestResourceErrorHandling:
         manager.add_template(template)
 
         # First, the template creation will fail with ValueError
-        with pytest.raises(ValueError):
+        with pytest.raises(ResourceError, match="Error reading resource"):
             await manager.read_resource("buggy://test")
-
-        # Let's test with a template that returns a resource that fails
-        def create_failing_resource(param: str):
-            async def failing_resource():
-                raise ValueError(f"Resource from template fails with {param}")
-
-            return FunctionResource(
-                uri=AnyUrl(f"failing://{param}"),
-                name=f"failing_{param}",
-                fn=failing_resource,
-            )
-
-        template = ResourceTemplate.from_function(
-            fn=create_failing_resource,
-            uri_template="failing://{param}",
-            name="failing_template",
-        )
-        manager.add_template(template)
-
-        with pytest.raises(ResourceError) as excinfo:
-            await manager.read_resource("failing://test")
-
-        # Exception should contain resource URI but not internal details
-        assert "Error reading resource 'failing://test'" in str(excinfo.value)
-        assert "Resource from template fails with test" not in str(excinfo.value)
diff --git a/tests/resources/test_resource_template.py b/tests/resources/test_resource_template.py
index 00f2b01b6..089b4ab89 100644
--- a/tests/resources/test_resource_template.py
+++ b/tests/resources/test_resource_template.py
@@ -186,21 +186,6 @@ class TestResourceTemplate:
         data = json.loads(content)
         assert data == {"key": "foo", "value": 123}
 
-    async def test_template_error(self):
-        """Test error handling in template resource creation."""
-
-        def failing_func(x: str) -> str:
-            raise ValueError("Test error")
-
-        template = ResourceTemplate.from_function(
-            fn=failing_func,
-            uri_template="fail://{x}",
-            name="fail",
-        )
-
-        with pytest.raises(ValueError, match="Error creating resource from template"):
-            await template.create_resource("fail://test", {"x": "test"})
-
     async def test_async_text_resource(self):
         """Test creating a text resource from async function."""