Skip to content

Commit 994a2d4

Browse files
BabyChrist666claude
andcommitted
refactor: address review feedback on docstrings and test structure
- Trim JSONRPCNotification docstring to single-line style matching file convention - Trim validator docstring to single-line; move spec rationale to inline comment - Fix issue reference from :issue:`2057` to #2057 - Switch tests from class to module-level functions (matching tests/issues/ convention) - Reduce to 4 focused tests that verify the fix (drop redundant pre-existing behavior tests) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent a841b3c commit 994a2d4

File tree

2 files changed

+24
-82
lines changed

2 files changed

+24
-82
lines changed

src/mcp/types/jsonrpc.py

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -20,14 +20,7 @@ class JSONRPCRequest(BaseModel):
2020

2121

2222
class JSONRPCNotification(BaseModel):
23-
"""A JSON-RPC notification which does not expect a response.
24-
25-
Per JSON-RPC 2.0, a notification is a request without an ``id`` member.
26-
Messages that include ``id`` (even with a ``null`` value) are requests,
27-
not notifications. The validator below prevents Pydantic's union
28-
resolution from silently absorbing an invalid ``"id": null`` request
29-
as a notification (see :issue:`2057`).
30-
"""
23+
"""A JSON-RPC notification which does not expect a response."""
3124

3225
jsonrpc: Literal["2.0"]
3326
method: str
@@ -36,12 +29,10 @@ class JSONRPCNotification(BaseModel):
3629
@model_validator(mode="before")
3730
@classmethod
3831
def _reject_id_field(cls, data: dict[str, Any]) -> dict[str, Any]:
39-
"""Reject messages that contain an ``id`` field.
40-
41-
JSON-RPC notifications MUST NOT include an ``id`` member. If the
42-
raw payload contains ``"id"`` (regardless of its value), it is a
43-
malformed request — not a valid notification.
44-
"""
32+
"""Reject payloads containing an 'id' field (notifications must not have one)."""
33+
# Per JSON-RPC 2.0, notifications must not include an "id" member.
34+
# Without this check, Pydantic's union resolution silently absorbs
35+
# invalid "id": null requests as notifications (#2057).
4536
if "id" in data:
4637
raise ValueError(
4738
"Notifications must not include an 'id' field. "

tests/issues/test_2057_null_id_rejected.py

Lines changed: 19 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -14,81 +14,32 @@
1414

1515
from mcp.types import (
1616
JSONRPCNotification,
17-
JSONRPCRequest,
1817
jsonrpc_message_adapter,
1918
)
2019

2120

22-
class TestNullIdRejection:
23-
"""Verify that ``"id": null`` is never silently absorbed."""
21+
def test_notification_rejects_id_field() -> None:
22+
"""JSONRPCNotification must not accept messages with an 'id' field."""
23+
with pytest.raises(ValidationError, match="must not include an 'id' field"):
24+
JSONRPCNotification.model_validate({"jsonrpc": "2.0", "method": "initialize", "id": None})
2425

25-
def test_request_rejects_null_id(self) -> None:
26-
"""JSONRPCRequest correctly rejects null id."""
27-
with pytest.raises(ValidationError):
28-
JSONRPCRequest.model_validate({"jsonrpc": "2.0", "method": "initialize", "id": None})
2926

30-
def test_notification_rejects_id_field(self) -> None:
31-
"""JSONRPCNotification must not accept messages with an 'id' field."""
32-
with pytest.raises(ValidationError, match="must not include an 'id' field"):
33-
JSONRPCNotification.model_validate({"jsonrpc": "2.0", "method": "initialize", "id": None})
27+
@pytest.mark.parametrize("id_value", [None, 0, 1, "", "abc"])
28+
def test_notification_rejects_any_id_value(id_value: object) -> None:
29+
"""Notification rejects 'id' regardless of value — null, int, or str."""
30+
with pytest.raises(ValidationError):
31+
JSONRPCNotification.model_validate({"jsonrpc": "2.0", "method": "test", "id": id_value})
3432

35-
def test_notification_rejects_any_id_value(self) -> None:
36-
"""Notification rejects 'id' regardless of value — null, int, or str."""
37-
for id_value in [None, 0, 1, "", "abc"]:
38-
with pytest.raises(ValidationError):
39-
JSONRPCNotification.model_validate({"jsonrpc": "2.0", "method": "test", "id": id_value})
4033

41-
def test_message_adapter_rejects_null_id(self) -> None:
42-
"""JSONRPCMessage union must not accept ``"id": null``."""
43-
raw = {"jsonrpc": "2.0", "method": "initialize", "id": None}
44-
with pytest.raises(ValidationError):
45-
jsonrpc_message_adapter.validate_python(raw)
34+
def test_message_adapter_rejects_null_id() -> None:
35+
"""JSONRPCMessage union must not accept ``"id": null``."""
36+
raw = {"jsonrpc": "2.0", "method": "initialize", "id": None}
37+
with pytest.raises(ValidationError):
38+
jsonrpc_message_adapter.validate_python(raw)
4639

47-
def test_message_adapter_rejects_null_id_json(self) -> None:
48-
"""Same test but via validate_json (the path used by transports)."""
49-
raw_json = json.dumps({"jsonrpc": "2.0", "method": "initialize", "id": None})
50-
with pytest.raises(ValidationError):
51-
jsonrpc_message_adapter.validate_json(raw_json)
5240

53-
def test_valid_notification_still_works(self) -> None:
54-
"""A valid notification (no 'id' field at all) must still parse fine."""
55-
msg = JSONRPCNotification.model_validate({"jsonrpc": "2.0", "method": "notifications/initialized"})
56-
assert msg.method == "notifications/initialized"
57-
58-
def test_valid_notification_with_params(self) -> None:
59-
"""Notification with params but no 'id' should work."""
60-
msg = JSONRPCNotification.model_validate(
61-
{"jsonrpc": "2.0", "method": "notifications/progress", "params": {"progress": 50}}
62-
)
63-
assert msg.method == "notifications/progress"
64-
assert msg.params == {"progress": 50}
65-
66-
def test_valid_request_with_string_id(self) -> None:
67-
"""A valid request with a string id still works."""
68-
msg = JSONRPCRequest.model_validate({"jsonrpc": "2.0", "method": "initialize", "id": "abc-123"})
69-
assert msg.id == "abc-123"
70-
71-
def test_valid_request_with_int_id(self) -> None:
72-
"""A valid request with an integer id still works."""
73-
msg = JSONRPCRequest.model_validate({"jsonrpc": "2.0", "method": "initialize", "id": 42})
74-
assert msg.id == 42
75-
76-
def test_message_adapter_parses_valid_request(self) -> None:
77-
"""The union adapter correctly identifies a valid request."""
78-
raw = {"jsonrpc": "2.0", "method": "initialize", "id": 1}
79-
parsed = jsonrpc_message_adapter.validate_python(raw)
80-
assert isinstance(parsed, JSONRPCRequest)
81-
assert parsed.id == 1
82-
83-
def test_message_adapter_parses_valid_notification(self) -> None:
84-
"""The union adapter correctly identifies a valid notification."""
85-
raw = {"jsonrpc": "2.0", "method": "notifications/initialized"}
86-
parsed = jsonrpc_message_adapter.validate_python(raw)
87-
assert isinstance(parsed, JSONRPCNotification)
88-
assert parsed.method == "notifications/initialized"
89-
90-
def test_message_adapter_parses_notification_json(self) -> None:
91-
"""The union adapter correctly identifies a valid notification via JSON."""
92-
raw_json = json.dumps({"jsonrpc": "2.0", "method": "notifications/initialized"})
93-
parsed = jsonrpc_message_adapter.validate_json(raw_json)
94-
assert isinstance(parsed, JSONRPCNotification)
41+
def test_message_adapter_rejects_null_id_json() -> None:
42+
"""Same test but via validate_json (the path used by transports)."""
43+
raw_json = json.dumps({"jsonrpc": "2.0", "method": "initialize", "id": None})
44+
with pytest.raises(ValidationError):
45+
jsonrpc_message_adapter.validate_json(raw_json)

0 commit comments

Comments
 (0)