-
Notifications
You must be signed in to change notification settings - Fork 81
[Server] Refactor protocol pipeline to separate request and notification handling #106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Server] Refactor protocol pipeline to separate request and notification handling #106
Conversation
|
a bit funny to see that because having notification and request handlers separate was exactly how i started: i treated both similar, that why i brought it up one level and unified. buuut, the context here is different - so i'm in for doing it 👍 |
Co-authored-by: Christopher Hertel <mail@christopher-hertel.de>
| public function testNotificationMethodUsedAsRequest(): void | ||
| { | ||
| $json = '{"jsonrpc": "2.0", "result": {"status": "ok"}}'; | ||
| // When a notification method is used with an id, it should still create the notification | ||
| // The fromArray validation will handle any issues | ||
| $json = '{"jsonrpc": "2.0", "method": "notifications/initialized", "id": 1}'; | ||
|
|
||
| $results = $this->factory->create($json); | ||
|
|
||
| $this->assertCount(1, $results); | ||
| // The notification class will reject the id in fromArray validation | ||
| $this->assertInstanceOf(InvalidInputMessageException::class, $results[0]); | ||
| $this->assertStringContainsString('id', $results[0]->getMessage()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra mile 😎 👍
This PR introduces a new
Protocollayer, reshapes message parsing, and redefines how custom handlers plug into the server. TheProtocolexposeshandleRequest,handleNotification, and a placeholderhandleResponse, along withsendRequest,sendResponse, andsendNotification. Only response sending is wired for now, but the other direction-facing methods lay the foundation for server-initiated messaging.Motivation and Context
JsonRpcHandler/MethodHandlerInterfaceblended request and notification semantics, making it impossible to enforce the JSON‑RPC contract (requests must yieldResponse|Error, notifications return nothing).initialize→connect→listen→close) while the protocol owns all transport callbacks (onMessage,onSessionEnd, and any future hooks) and output routing.What’s Changed
JsonRpcHandlertoProtocoland made it the transport-facing façade that registers callbacks, parses all JSON‑RPC message types, dispatches request/notification handlers, and emits responses.Server::run()into a small lifecycle orchestrator: it initializes the transport, connects the protocol, runslisten(), and guaranteesclose(). No more callback wiring in Server.MethodHandlerInterfacewithRequestHandlerInterfaceandNotificationHandlerInterface, enforcing that requests always returnResponse|Errorand notifications remain fire-and-forget.MessageFactoryto emit typedRequest,Notification,Response, andErrorobjects (orInvalidInputMessageException), laying the groundwork for bidirectional messaging.addRequestHandler(s)andaddNotificationHandler(s), and updated docs, examples, and tests to use the new abstractions.Breaking Changes
Builder::addMethodHandler(s)andMethodHandlerInterfacehave been removed.addRequestHandler(s)/RequestHandlerInterfacefor JSON‑RPC requests andaddNotificationHandler(s)/NotificationHandlerInterfacefor notifications.