[Server] Keep one unregistrable element from taking the whole registry down - #477
Conversation
| ): self { | ||
| if (!ResourceTemplate::isValidUriTemplate($uriTemplate)) { | ||
| throw new InvalidArgumentException(\sprintf('Invalid URI template "%s" for resource template handler %s: a template needs a scheme and at least one placeholder, e.g. "user://{userId}/profile". Use addResource() for a URI that addresses a single resource.', $uriTemplate, $this->describeHandler($handler))); | ||
| } |
There was a problem hiding this comment.
bringing the validation logic into Builder is not a real option to me. even tho you tried to mitigate with that new isValidUriTemplate method, this still is a bit leaky and wouldn't scale nice - think of bringing all those validation+exception paths to the Builder ... 😬
There was a problem hiding this comment.
Agreed, and dropped — Builder is untouched now, and ResourceTemplate::isValidUriTemplate() is gone with it.
Chasing your point further: the validation never belonged at registration at all, because the reported case is #[McpResourceTemplate], which never goes through addResourceTemplate(). The real defect is that ReflectedElementLoader rethrows a failed element as a ConfigurationException and takes Registry::load() down with it, before anything is registered — under lazy loading that happens mid-request, so one bad template answers tools/list and tools/call with the template's message on every request.
Discoverer already has the answer (Discoverer.php:300-310): catch per element, log, move on. The loader now does the same for all four element types. One bad element costs you that element.
69190d8 to
737ec3b
Compare
wachterjohannes
changed the title
737ec3b to
76fb3ec
Compare
…y down A placeholder-less resource template made the server serve nothing. `ReflectedElementLoader` wrapped the failure in a `ConfigurationException` and rethrew it, which aborted `Registry::load()` before any element was registered. Loading is lazy by default, so that ran while a request was being served: `tools/list` and `tools/call` answered `-32602` with the template's message, and since `load()` marks itself loaded only on success, every following request repeated it. The loader now logs the failure and skips that element, the way `Discoverer` already treats an attribute it cannot process. The bad template is the only thing missing; the rest of the registry stays servable, and a server-side configuration error stops being reported to the client as invalid params on unrelated calls. Applies to all four element types, which carried the same rethrow. The `ResourceTemplate` message now also says what to do about it: a URI without a placeholder addresses a single resource, so it belongs in `addResource()`. Fixes modelcontextprotocol#476.
76fb3ec to
12856e0
Compare
2 participants
Fixes #476.
A placeholder-less resource template made the server serve nothing.
ReflectedElementLoaderwrapped the failure in aConfigurationExceptionand rethrew it, which abortedRegistry::load()before any element was registered. Loading is lazy by default, so that ran while a request was being served:tools/listandtools/callanswered-32602with the template's message, and sinceload()marks itself loaded only on success, every following request repeated it.The loader now logs the failure and skips that element, the way
Discovereralready treats an attribute it cannot process (Discoverer.php:300-310). The bad template is the only thing missing; the rest of the registry stays servable, and a server-side configuration error stops being reported to the client as invalid params on unrelated calls. All four element types carried the same rethrow, so all four change.The
ResourceTemplatemessage now also says what to do about it: a URI without a placeholder addresses a single resource, so it belongs inaddResource().Changed from the first revision
The first take validated the URI template in
Builder::addResourceTemplate(). @chr-hertel pushed back on that, rightly: every schema invariant would end up mirrored in the builder alongside the value object that already owns it, and theResourceTemplate::isValidUriTemplate()accessor existed only to make that mirroring possible. Both are gone. Nothing inBuilderchanged, and this revision fixes the reported path — the#[McpResourceTemplate]blast radius — rather than one doorway into it.