Replace FileResource.is_binary with an encoding field (#3171)
This commit is contained in:
Max
GitHub
parent
814072c94d
commit
11934c90ae
4 changed files with 240 additions and 98 deletions
|
|
@ -842,6 +842,37 @@ Reading a missing resource now returns JSON-RPC error code `-32602` (invalid par
|
|||
|
||||
The underlying lookups now raise typed exceptions instead of `ValueError`. `ResourceManager.get_resource()` raises `ResourceNotFoundError` when no resource or template matches the URI, and `ResourceTemplate.create_resource()` raises `ResourceError` when the template function fails. Neither subclasses `ValueError`, so callers catching `ValueError` should switch to `ResourceNotFoundError` / `ResourceError` (both importable from `mcp.server.mcpserver.exceptions`; `ResourceNotFoundError` subclasses `ResourceError`).
|
||||
|
||||
### `Resource` classes reject unknown keyword arguments
|
||||
|
||||
The `Resource` base class now sets `extra="forbid"`, so every resource class — `TextResource`, `BinaryResource`, `FunctionResource`, `FileResource`, `HttpResource`, `DirectoryResource`, and your own subclasses — raises `ValidationError` on an unrecognised keyword argument instead of silently dropping it. Previously a typo'd or since-removed parameter (such as `FileResource(is_binary=...)`, below) was accepted and ignored. Remove any stray keyword arguments; if a subclass needs to accept arbitrary extras, set its own `model_config = ConfigDict(extra="allow")`.
|
||||
|
||||
### `FileResource.is_binary` replaced by `encoding`
|
||||
|
||||
`FileResource` used to take `is_binary: bool` and guess its default from `mime_type` (`text/*` → text, anything else → bytes). Two problems fell out of that: `is_binary=False` could not actually be set — `False` doubled as the "not given" sentinel, so `mime_type="application/json"` always came back as a base64 blob — and text reads used `Path.read_text()` with no encoding, i.e. the platform locale (cp1252 on Windows).
|
||||
|
||||
The field is now `encoding: str | None`. A string means "decode with this encoding and serve as text"; `None` means "read bytes and serve as a blob". When omitted it defaults to the `charset` declared in `mime_type` if there is one, otherwise `"utf-8-sig"` for textual mime types (`text/*`, `application/json`, `application/xml`, and any `+json`/`+xml` suffix) and `None` for everything else, so JSON and XML files are now served as text without any configuration. `utf-8-sig` is plain UTF-8 that also drops a byte-order mark if the file has one; a declared `charset=` is used as-is.
|
||||
|
||||
Passing the removed `is_binary=` argument now raises a `ValidationError` at construction (see the section above) rather than being silently ignored. A misspelled `encoding` also fails at construction rather than on the first read.
|
||||
|
||||
Two edge cases to check. A non-UTF-8 file with a *newly* textual mime type (say a UTF-16 `application/xml`) previously shipped byte-exact as a blob and now fails to decode. And an existing `text/*` file that was only readable through your platform's locale encoding (v1 decoded these with the locale, not UTF-8) now fails too. In both cases set `encoding` to the file's real encoding, or `encoding=None` to serve the bytes as a blob.
|
||||
|
||||
**Before (v1):**
|
||||
|
||||
```python
|
||||
FileResource(uri="file:///logo.png", path=logo, mime_type="image/png", is_binary=True)
|
||||
FileResource(uri="file:///notes.txt", path=notes) # text, decoded with the locale encoding
|
||||
```
|
||||
|
||||
**After (v2):**
|
||||
|
||||
```python
|
||||
FileResource(uri="file:///logo.png", path=logo, mime_type="image/png") # bytes, from mime_type
|
||||
FileResource(uri="file:///notes.txt", path=notes) # text, decoded as UTF-8 (BOM tolerated)
|
||||
FileResource(uri="file:///data.json", path=data, mime_type="application/json") # now text, not a blob
|
||||
```
|
||||
|
||||
Pass `encoding=None` to force a blob, or `encoding="latin-1"` (etc.) to decode a text file that isn't UTF-8.
|
||||
|
||||
### Resource templates: matching behavior changes
|
||||
|
||||
Resource template matching has been rewritten with [RFC 6570](https://datatracker.ietf.org/doc/html/rfc6570) support.
|
||||
|
|
|
|||
|
|
@ -16,7 +16,7 @@ from pydantic import (
|
|||
class Resource(BaseModel, abc.ABC):
|
||||
"""Base class for all resources."""
|
||||
|
||||
model_config = ConfigDict(validate_default=True)
|
||||
model_config = ConfigDict(validate_default=True, extra="forbid")
|
||||
|
||||
uri: str = Field(default=..., description="URI of the resource")
|
||||
name: str | None = Field(description="Name of the resource", default=None)
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@ from __future__ import annotations
|
|||
|
||||
import json
|
||||
from collections.abc import Callable
|
||||
from functools import partial
|
||||
from pathlib import Path
|
||||
from typing import Any
|
||||
|
||||
|
|
@ -13,12 +14,34 @@ import httpx2
|
|||
import pydantic
|
||||
import pydantic_core
|
||||
from mcp_types import Annotations, Icon, InputRequiredResult
|
||||
from pydantic import Field, ValidationInfo, validate_call
|
||||
from pydantic import Field, validate_call
|
||||
|
||||
from mcp.server.mcpserver.resources.base import Resource
|
||||
from mcp.shared._callable_inspection import is_async_callable
|
||||
from mcp.shared.exceptions import MCPError
|
||||
|
||||
# `application/*` types that are textual but predate the `+json`/`+xml`
|
||||
# structured-syntax suffixes, so the suffix rule below can't catch them.
|
||||
_TEXTUAL_APPLICATION_TYPES = frozenset({"application/json", "application/xml"})
|
||||
|
||||
|
||||
def _default_file_encoding(mime_type: str) -> str | None:
|
||||
"""The encoding a file of this mime type is decoded with by default.
|
||||
|
||||
A declared `charset=` parameter wins. Otherwise textual types (`text/*`, JSON,
|
||||
XML) are `utf-8-sig` — UTF-8 that also tolerates a byte-order mark — and
|
||||
everything else is bytes (None).
|
||||
"""
|
||||
essence, *params = (part.strip() for part in mime_type.split(";"))
|
||||
for param in params:
|
||||
name, _, value = param.partition("=")
|
||||
if name.strip().lower() == "charset" and value:
|
||||
return value.strip().strip('"')
|
||||
essence = essence.lower()
|
||||
if essence.startswith("text/") or essence.endswith(("+json", "+xml")) or essence in _TEXTUAL_APPLICATION_TYPES:
|
||||
return "utf-8-sig"
|
||||
return None
|
||||
|
||||
|
||||
class TextResource(Resource):
|
||||
"""A resource that reads from a string."""
|
||||
|
|
@ -122,17 +145,17 @@ class FunctionResource(Resource):
|
|||
class FileResource(Resource):
|
||||
"""A resource that reads from a file.
|
||||
|
||||
Set is_binary=True to read the file as binary data instead of text.
|
||||
The file is decoded with `encoding` and served as text, or read as bytes and
|
||||
served as a base64 blob when `encoding` is None. When `encoding` is omitted it
|
||||
defaults to the `charset` declared in `mime_type`, else `"utf-8-sig"` for
|
||||
textual mime types (`text/*`, JSON, XML) and None for everything else; pass
|
||||
it explicitly to override either way.
|
||||
"""
|
||||
|
||||
path: Path = Field(description="Path to the file")
|
||||
is_binary: bool = Field(
|
||||
default=False,
|
||||
description="Whether to read the file as binary data",
|
||||
)
|
||||
mime_type: str = Field(
|
||||
default="text/plain",
|
||||
description="MIME type of the resource content",
|
||||
encoding: str | None = Field(
|
||||
default_factory=lambda data: _default_file_encoding(data["mime_type"]),
|
||||
description="Text encoding used to decode the file, or None to serve its bytes as a blob",
|
||||
)
|
||||
|
||||
@pydantic.field_validator("path")
|
||||
|
|
@ -143,21 +166,27 @@ class FileResource(Resource):
|
|||
raise ValueError("Path must be absolute")
|
||||
return path
|
||||
|
||||
@pydantic.field_validator("is_binary")
|
||||
@pydantic.field_validator("encoding")
|
||||
@classmethod
|
||||
def set_binary_from_mime_type(cls, is_binary: bool, info: ValidationInfo) -> bool:
|
||||
"""Set is_binary based on mime_type if not explicitly set."""
|
||||
if is_binary:
|
||||
return True
|
||||
mime_type = info.data.get("mime_type", "text/plain")
|
||||
return not mime_type.startswith("text/")
|
||||
def validate_text_encoding(cls, encoding: str | None) -> str | None:
|
||||
"""Ensure the encoding names a usable text codec, so a mistake fails at construction not at read."""
|
||||
if encoding is not None:
|
||||
# Decoding a probe byte rejects both unknown names and codecs that
|
||||
# aren't text encodings (base64_codec, rot13, ...) via LookupError.
|
||||
try:
|
||||
b"x".decode(encoding)
|
||||
except LookupError as e:
|
||||
raise ValueError(str(e)) from e
|
||||
except UnicodeError:
|
||||
pass # a real text encoding; the probe byte just doesn't decode in it
|
||||
return encoding
|
||||
|
||||
async def read(self) -> str | bytes:
|
||||
"""Read the file content."""
|
||||
try:
|
||||
if self.is_binary:
|
||||
if self.encoding is None:
|
||||
return await anyio.to_thread.run_sync(self.path.read_bytes)
|
||||
return await anyio.to_thread.run_sync(self.path.read_text)
|
||||
return await anyio.to_thread.run_sync(partial(self.path.read_text, encoding=self.encoding))
|
||||
except Exception as e:
|
||||
raise ValueError(f"Error reading file {self.path}: {e}")
|
||||
|
||||
|
|
|
|||
|
|
@ -1,8 +1,10 @@
|
|||
import codecs
|
||||
import os
|
||||
from pathlib import Path
|
||||
from tempfile import NamedTemporaryFile
|
||||
|
||||
import pytest
|
||||
from pydantic import ValidationError
|
||||
|
||||
from mcp.server.mcpserver.resources import FileResource
|
||||
|
||||
|
|
@ -24,93 +26,173 @@ def temp_file():
|
|||
pass # File was already deleted by the test
|
||||
|
||||
|
||||
class TestFileResource:
|
||||
"""Test FileResource functionality."""
|
||||
def test_file_resource_creation(temp_file: Path):
|
||||
resource = FileResource(
|
||||
uri=temp_file.as_uri(),
|
||||
name="test",
|
||||
description="test file",
|
||||
path=temp_file,
|
||||
)
|
||||
assert str(resource.uri) == temp_file.as_uri()
|
||||
assert resource.name == "test"
|
||||
assert resource.description == "test file"
|
||||
assert resource.mime_type == "text/plain"
|
||||
assert resource.path == temp_file
|
||||
assert resource.encoding == "utf-8-sig"
|
||||
|
||||
def test_file_resource_creation(self, temp_file: Path):
|
||||
"""Test creating a FileResource."""
|
||||
|
||||
def test_file_resource_str_path_conversion(temp_file: Path):
|
||||
resource = FileResource(
|
||||
uri=f"file://{temp_file}",
|
||||
name="test",
|
||||
path=Path(str(temp_file)),
|
||||
)
|
||||
assert isinstance(resource.path, Path)
|
||||
assert resource.path.is_absolute()
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_read_text_file(temp_file: Path):
|
||||
resource = FileResource(
|
||||
uri=f"file://{temp_file}",
|
||||
name="test",
|
||||
path=temp_file,
|
||||
)
|
||||
content = await resource.read()
|
||||
assert content == "test content"
|
||||
assert resource.mime_type == "text/plain"
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_encoding_none_reads_bytes(temp_file: Path):
|
||||
resource = FileResource(
|
||||
uri=f"file://{temp_file}",
|
||||
name="test",
|
||||
path=temp_file,
|
||||
encoding=None,
|
||||
)
|
||||
content = await resource.read()
|
||||
assert isinstance(content, bytes)
|
||||
assert content == b"test content"
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"mime_type",
|
||||
[
|
||||
"text/plain",
|
||||
"text/html",
|
||||
"application/json",
|
||||
"application/xml",
|
||||
"application/vnd.api+json",
|
||||
"image/svg+xml",
|
||||
],
|
||||
)
|
||||
def test_textual_mime_types_default_to_utf8_sig(temp_file: Path, mime_type: str):
|
||||
resource = FileResource(uri=temp_file.as_uri(), path=temp_file, mime_type=mime_type)
|
||||
assert resource.encoding == "utf-8-sig"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("mime_type", ["image/png", "application/octet-stream", "application/pdf"])
|
||||
def test_binary_mime_types_default_to_no_encoding(temp_file: Path, mime_type: str):
|
||||
resource = FileResource(uri=temp_file.as_uri(), path=temp_file, mime_type=mime_type)
|
||||
assert resource.encoding is None
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"mime_type",
|
||||
["text/plain; charset=iso-8859-1", 'text/plain; format=flowed; charset="iso-8859-1"'],
|
||||
)
|
||||
def test_declared_charset_becomes_default_encoding(temp_file: Path, mime_type: str):
|
||||
resource = FileResource(uri=temp_file.as_uri(), path=temp_file, mime_type=mime_type)
|
||||
assert resource.encoding == "iso-8859-1"
|
||||
|
||||
|
||||
def test_removed_is_binary_kwarg_is_rejected(temp_file: Path):
|
||||
"""The v1 `is_binary` parameter fails loudly at construction rather than being ignored."""
|
||||
with pytest.raises(ValidationError, match="is_binary"):
|
||||
FileResource.model_validate({"uri": temp_file.as_uri(), "path": temp_file, "is_binary": True})
|
||||
|
||||
|
||||
def test_unknown_encoding_is_rejected(temp_file: Path):
|
||||
"""A codec typo fails at construction rather than on the first read."""
|
||||
with pytest.raises(ValidationError, match="unknown encoding: not-a-codec"):
|
||||
FileResource(uri=temp_file.as_uri(), path=temp_file, encoding="not-a-codec")
|
||||
|
||||
|
||||
def test_unknown_declared_charset_is_rejected(temp_file: Path):
|
||||
with pytest.raises(ValidationError, match="unknown encoding"):
|
||||
FileResource(uri=temp_file.as_uri(), path=temp_file, mime_type="text/plain; charset=not-a-codec")
|
||||
|
||||
|
||||
def test_multibyte_encoding_is_accepted(temp_file: Path):
|
||||
"""UTF-16 can't decode a lone probe byte but is still a valid text encoding."""
|
||||
resource = FileResource(uri=temp_file.as_uri(), path=temp_file, encoding="utf-16")
|
||||
assert resource.encoding == "utf-16"
|
||||
|
||||
|
||||
def test_non_text_codec_is_rejected(temp_file: Path):
|
||||
"""A registered codec that isn't a text encoding (bytes-to-bytes) is not a usable encoding."""
|
||||
with pytest.raises(ValidationError, match="not a text encoding"):
|
||||
FileResource(uri=temp_file.as_uri(), path=temp_file, encoding="base64_codec")
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_json_file_is_served_as_text_by_default(temp_file: Path):
|
||||
"""The mime type that motivated the encoding field: JSON must not become a base64 blob."""
|
||||
temp_file.write_text('{"a": 1}', encoding="utf-8")
|
||||
resource = FileResource(uri=temp_file.as_uri(), path=temp_file, mime_type="application/json")
|
||||
assert await resource.read() == '{"a": 1}'
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_utf8_bom_is_stripped_by_default(temp_file: Path):
|
||||
"""The default utf-8-sig decoding drops a byte-order mark that would otherwise break JSON parsers."""
|
||||
temp_file.write_bytes(codecs.BOM_UTF8 + b'{"a": 1}')
|
||||
resource = FileResource(uri=temp_file.as_uri(), path=temp_file, mime_type="application/json")
|
||||
assert await resource.read() == '{"a": 1}'
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_explicit_encoding_overrides_default(temp_file: Path):
|
||||
"""An explicit encoding wins over the mime-type default and is what decodes the file."""
|
||||
temp_file.write_bytes("naïve".encode("latin-1"))
|
||||
resource = FileResource(uri=temp_file.as_uri(), path=temp_file, mime_type="image/png", encoding="latin-1")
|
||||
assert resource.encoding == "latin-1"
|
||||
assert await resource.read() == "naïve"
|
||||
|
||||
|
||||
def test_relative_path_error():
|
||||
with pytest.raises(ValueError, match="Path must be absolute"):
|
||||
FileResource(
|
||||
uri="file:///test.txt",
|
||||
name="test",
|
||||
path=Path("test.txt"),
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_missing_file_error(temp_file: Path):
|
||||
missing = temp_file.parent / "missing.txt"
|
||||
resource = FileResource(
|
||||
uri="file:///missing.txt",
|
||||
name="test",
|
||||
path=missing,
|
||||
)
|
||||
with pytest.raises(ValueError, match="Error reading file"):
|
||||
await resource.read()
|
||||
|
||||
|
||||
@pytest.mark.skipif(os.name == "nt", reason="File permissions behave differently on Windows")
|
||||
@pytest.mark.anyio
|
||||
async def test_permission_error(temp_file: Path): # pragma: lax no cover
|
||||
temp_file.chmod(0o000) # Remove all permissions
|
||||
try:
|
||||
resource = FileResource(
|
||||
uri=temp_file.as_uri(),
|
||||
name="test",
|
||||
description="test file",
|
||||
path=temp_file,
|
||||
)
|
||||
assert str(resource.uri) == temp_file.as_uri()
|
||||
assert resource.name == "test"
|
||||
assert resource.description == "test file"
|
||||
assert resource.mime_type == "text/plain" # default
|
||||
assert resource.path == temp_file
|
||||
assert resource.is_binary is False # default
|
||||
|
||||
def test_file_resource_str_path_conversion(self, temp_file: Path):
|
||||
"""Test FileResource handles string paths."""
|
||||
resource = FileResource(
|
||||
uri=f"file://{temp_file}",
|
||||
name="test",
|
||||
path=Path(str(temp_file)),
|
||||
)
|
||||
assert isinstance(resource.path, Path)
|
||||
assert resource.path.is_absolute()
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_read_text_file(self, temp_file: Path):
|
||||
"""Test reading a text file."""
|
||||
resource = FileResource(
|
||||
uri=f"file://{temp_file}",
|
||||
name="test",
|
||||
path=temp_file,
|
||||
)
|
||||
content = await resource.read()
|
||||
assert content == "test content"
|
||||
assert resource.mime_type == "text/plain"
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_read_binary_file(self, temp_file: Path):
|
||||
"""Test reading a file as binary."""
|
||||
resource = FileResource(
|
||||
uri=f"file://{temp_file}",
|
||||
name="test",
|
||||
path=temp_file,
|
||||
is_binary=True,
|
||||
)
|
||||
content = await resource.read()
|
||||
assert isinstance(content, bytes)
|
||||
assert content == b"test content"
|
||||
|
||||
def test_relative_path_error(self):
|
||||
"""Test error on relative path."""
|
||||
with pytest.raises(ValueError, match="Path must be absolute"):
|
||||
FileResource(
|
||||
uri="file:///test.txt",
|
||||
name="test",
|
||||
path=Path("test.txt"),
|
||||
)
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_missing_file_error(self, temp_file: Path):
|
||||
"""Test error when file doesn't exist."""
|
||||
# Create path to non-existent file
|
||||
missing = temp_file.parent / "missing.txt"
|
||||
resource = FileResource(
|
||||
uri="file:///missing.txt",
|
||||
name="test",
|
||||
path=missing,
|
||||
)
|
||||
with pytest.raises(ValueError, match="Error reading file"):
|
||||
await resource.read()
|
||||
|
||||
@pytest.mark.skipif(os.name == "nt", reason="File permissions behave differently on Windows")
|
||||
@pytest.mark.anyio
|
||||
async def test_permission_error(self, temp_file: Path): # pragma: lax no cover
|
||||
"""Test reading a file without permissions."""
|
||||
temp_file.chmod(0o000) # Remove all permissions
|
||||
try:
|
||||
resource = FileResource(
|
||||
uri=temp_file.as_uri(),
|
||||
name="test",
|
||||
path=temp_file,
|
||||
)
|
||||
with pytest.raises(ValueError, match="Error reading file"):
|
||||
await resource.read()
|
||||
finally:
|
||||
temp_file.chmod(0o644) # Restore permissions
|
||||
finally:
|
||||
temp_file.chmod(0o644) # Restore permissions
|
||||
|
|
|
|||
Loading…
Reference in a new issue