Skip to content

Commit b55ad61

Browse files
author
PR-Contributor
committed
fix: address review feedback for remove_prompt/resource
- Fix remove_resource() to accept AnyUrl | str and normalize with str() - Add comprehensive tests for remove_resource() and remove_template() - Add tests for remove_prompt() - All tests cover: removing existing, removing non-existent (error path), removing from multiple, and verifying protocol-level errors
1 parent c1053d5 commit b55ad61

File tree

3 files changed

+235
-3
lines changed

3 files changed

+235
-3
lines changed

src/mcp/server/mcpserver/resources/resource_manager.py

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ def list_templates(self) -> list[ResourceTemplate]:
110110
logger.debug("Listing templates", extra={"count": len(self._templates)})
111111
return list(self._templates.values())
112112

113-
def remove_resource(self, uri: str) -> None:
113+
def remove_resource(self, uri: AnyUrl | str) -> None:
114114
"""Remove a resource by URI.
115115
116116
Args:
@@ -119,9 +119,10 @@ def remove_resource(self, uri: str) -> None:
119119
Raises:
120120
ResourceError: If the resource does not exist
121121
"""
122-
if uri not in self._resources:
122+
uri_str = str(uri)
123+
if uri_str not in self._resources:
123124
raise ResourceError(f"Unknown resource: {uri}")
124-
del self._resources[uri]
125+
del self._resources[uri_str]
125126

126127
def remove_template(self, uri_template: str) -> None:
127128
"""Remove a resource template by URI template.

tests/server/mcpserver/prompts/test_manager.py

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import pytest
22

33
from mcp.server.mcpserver import Context
4+
from mcp.server.mcpserver.exceptions import PromptError
45
from mcp.server.mcpserver.prompts.base import Prompt, UserMessage
56
from mcp.server.mcpserver.prompts.manager import PromptManager
67
from mcp.types import TextContent
@@ -108,3 +109,70 @@ def fn(name: str) -> str: # pragma: no cover
108109
manager.add_prompt(prompt)
109110
with pytest.raises(ValueError, match="Missing required arguments"):
110111
await manager.render_prompt("fn", None, Context())
112+
113+
114+
class TestRemovePrompt:
115+
"""Test PromptManager.remove_prompt() functionality."""
116+
117+
def test_remove_existing_prompt(self):
118+
"""Test removing an existing prompt."""
119+
120+
def fn() -> str: # pragma: no cover
121+
return "Hello, world!"
122+
123+
manager = PromptManager()
124+
prompt = Prompt.from_function(fn)
125+
manager.add_prompt(prompt)
126+
127+
# Verify prompt exists
128+
assert manager.get_prompt("fn") is not None
129+
assert len(manager.list_prompts()) == 1
130+
131+
# Remove the prompt - should not raise any exception
132+
manager.remove_prompt("fn")
133+
134+
# Verify prompt is removed
135+
assert manager.get_prompt("fn") is None
136+
assert len(manager.list_prompts()) == 0
137+
138+
def test_remove_nonexistent_prompt(self):
139+
"""Test removing a non-existent prompt raises error."""
140+
manager = PromptManager()
141+
142+
with pytest.raises(Exception, match="Unknown prompt: nonexistent"):
143+
manager.remove_prompt("nonexistent")
144+
145+
def test_remove_one_prompt_from_multiple(self):
146+
"""Test removing one prompt when multiple prompts exist."""
147+
148+
def fn1() -> str: # pragma: no cover
149+
return "Hello, world!"
150+
151+
def fn2() -> str: # pragma: no cover
152+
return "Goodbye, world!"
153+
154+
def fn3() -> str: # pragma: no cover
155+
return "How are you?"
156+
157+
manager = PromptManager()
158+
prompt1 = Prompt.from_function(fn1)
159+
prompt2 = Prompt.from_function(fn2)
160+
prompt3 = Prompt.from_function(fn3)
161+
manager.add_prompt(prompt1)
162+
manager.add_prompt(prompt2)
163+
manager.add_prompt(prompt3)
164+
165+
# Verify all prompts exist
166+
assert len(manager.list_prompts()) == 3
167+
assert manager.get_prompt("fn1") is not None
168+
assert manager.get_prompt("fn2") is not None
169+
assert manager.get_prompt("fn3") is not None
170+
171+
# Remove middle prompt
172+
manager.remove_prompt("fn2")
173+
174+
# Verify only fn2 is removed
175+
assert len(manager.list_prompts()) == 2
176+
assert manager.get_prompt("fn1") is not None
177+
assert manager.get_prompt("fn2") is None
178+
assert manager.get_prompt("fn3") is not None

tests/server/mcpserver/resources/test_resource_manager.py

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
from pydantic import AnyUrl
66

77
from mcp.server.mcpserver import Context
8+
from mcp.server.mcpserver.exceptions import ResourceError
89
from mcp.server.mcpserver.resources import FileResource, FunctionResource, ResourceManager, ResourceTemplate
910

1011

@@ -175,3 +176,165 @@ def get_item(id: str) -> str: # pragma: no cover
175176
)
176177

177178
assert template.meta is None
179+
180+
181+
class TestRemoveResource:
182+
"""Test ResourceManager.remove_resource() functionality."""
183+
184+
def test_remove_existing_resource(self, temp_file: Path):
185+
"""Test removing an existing resource."""
186+
manager = ResourceManager()
187+
resource = FileResource(
188+
uri=f"file://{temp_file}",
189+
name="test",
190+
path=temp_file,
191+
)
192+
manager.add_resource(resource)
193+
194+
# Verify resource exists
195+
assert len(manager.list_resources()) == 1
196+
197+
# Remove the resource - should not raise any exception
198+
manager.remove_resource(f"file://{temp_file}")
199+
200+
# Verify resource is removed
201+
assert len(manager.list_resources()) == 0
202+
203+
def test_remove_nonexistent_resource(self):
204+
"""Test removing a non-existent resource raises ResourceError."""
205+
manager = ResourceManager()
206+
207+
with pytest.raises(ResourceError, match="Unknown resource: nonexistent"):
208+
manager.remove_resource("nonexistent")
209+
210+
def test_remove_resource_with_anyurl(self, temp_file: Path):
211+
"""Test removing a resource using AnyUrl instead of string."""
212+
manager = ResourceManager()
213+
resource = FileResource(
214+
uri=f"file://{temp_file}",
215+
name="test",
216+
path=temp_file,
217+
)
218+
manager.add_resource(resource)
219+
220+
# Verify resource exists
221+
assert len(manager.list_resources()) == 1
222+
223+
# Remove using AnyUrl - should work
224+
manager.remove_resource(AnyUrl(f"file://{temp_file}"))
225+
226+
# Verify resource is removed
227+
assert len(manager.list_resources()) == 0
228+
229+
def test_remove_one_resource_from_multiple(self, temp_file: Path):
230+
"""Test removing one resource when multiple resources exist."""
231+
manager = ResourceManager()
232+
resource1 = FileResource(
233+
uri=f"file://{temp_file}1",
234+
name="test1",
235+
path=temp_file,
236+
)
237+
resource2 = FileResource(
238+
uri=f"file://{temp_file}2",
239+
name="test2",
240+
path=temp_file,
241+
)
242+
resource3 = FileResource(
243+
uri=f"file://{temp_file}3",
244+
name="test3",
245+
path=temp_file,
246+
)
247+
manager.add_resource(resource1)
248+
manager.add_resource(resource2)
249+
manager.add_resource(resource3)
250+
251+
# Verify all resources exist
252+
assert len(manager.list_resources()) == 3
253+
254+
# Remove middle resource
255+
manager.remove_resource(f"file://{temp_file}2")
256+
257+
# Verify only resource2 is removed
258+
assert len(manager.list_resources()) == 2
259+
assert manager.list_resources() == [resource1, resource3]
260+
261+
@pytest.mark.anyio
262+
async def test_call_removed_resource_raises_error(self, temp_file: Path):
263+
"""Test that calling a removed resource raises ValueError."""
264+
manager = ResourceManager()
265+
resource = FileResource(
266+
uri=f"file://{temp_file}",
267+
name="test",
268+
path=temp_file,
269+
)
270+
manager.add_resource(resource)
271+
272+
# Verify resource works before removal
273+
result = await manager.get_resource(resource.uri, Context())
274+
assert result == resource
275+
276+
# Remove the resource
277+
manager.remove_resource(f"file://{temp_file}")
278+
279+
# Verify getting removed resource raises error
280+
with pytest.raises(ValueError, match="Unknown resource"):
281+
await manager.get_resource(resource.uri, Context())
282+
283+
284+
class TestRemoveTemplate:
285+
"""Test ResourceManager.remove_template() functionality."""
286+
287+
def test_remove_existing_template(self):
288+
"""Test removing an existing template."""
289+
manager = ResourceManager()
290+
291+
def greet(name: str) -> str:
292+
return f"Hello, {name}!"
293+
294+
template = manager.add_template(
295+
fn=greet,
296+
uri_template="greet://{name}",
297+
)
298+
299+
# Verify template exists
300+
assert len(manager.list_templates()) == 1
301+
302+
# Remove the template
303+
manager.remove_template("greet://{name}")
304+
305+
# Verify template is removed
306+
assert len(manager.list_templates()) == 0
307+
308+
def test_remove_nonexistent_template(self):
309+
"""Test removing a non-existent template raises ResourceError."""
310+
manager = ResourceManager()
311+
312+
with pytest.raises(ResourceError, match="Unknown template: nonexistent"):
313+
manager.remove_template("nonexistent")
314+
315+
def test_remove_one_template_from_multiple(self):
316+
"""Test removing one template when multiple templates exist."""
317+
manager = ResourceManager()
318+
319+
def greet(name: str) -> str:
320+
return f"Hello, {name}!"
321+
322+
def farewell(name: str) -> str:
323+
return f"Goodbye, {name}!"
324+
325+
def ask(question: str) -> str:
326+
return f"What is {question}?"
327+
328+
template1 = manager.add_template(fn=greet, uri_template="greet://{name}")
329+
template2 = manager.add_template(fn=farewell, uri_template="farewell://{name}")
330+
template3 = manager.add_template(fn=ask, uri_template="ask://{question}")
331+
332+
# Verify all templates exist
333+
assert len(manager.list_templates()) == 3
334+
335+
# Remove middle template
336+
manager.remove_template("farewell://{name}")
337+
338+
# Verify only farewell template is removed
339+
assert len(manager.list_templates()) == 2
340+
assert manager.list_templates() == [template1, template3]

0 commit comments

Comments
 (0)