-
Notifications
You must be signed in to change notification settings - Fork 422
schema: Implemented simulated discriminated union for transports and arguments #630
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
base: main
Are you sure you want to change the base?
schema: Implemented simulated discriminated union for transports and arguments #630
Conversation
…avoid "anyOf" fan out schema validation errors (where all templates fail when none match)
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.
I am undecided about this. I am not used to this syntax in JSON schemas, and worry it potentially makes things more complex.
That said, I also see the benefits to users of having the schema validation give clearer errors.
Though for what its worth good validators could figure this out already, e.g. TypeScript generated off the JSON schema does already work.
I share the concern. I really tried to find a cleaner syntax that would work. For example, you would think this might work: {
"transport": {
"oneOf": [
{
"type": "object",
"properties": {
"type": { "const": "stdio" }
},
"required": ["type"],
"$ref": "#/definitions/StdioTransport"
},
{
"type": "object",
"properties": {
"type": { "const": "sse" }
},
"required": ["type"],
"$ref": "#/definitions/SseTransport"
},
{
"type": "object",
"properties": {
"type": { "const": "streamable-http" }
},
"required": ["type"],
"$ref": "#/definitions/StreamableHttpTransport"
}
]
}
} But it failed in the same way in both Go and Javascript validation. I also struggled picking through the detailed schema validation results on both platforms looking for a pattern that would help me filter out results that shouldn't apply (the only limitation being it had to be general purpose - I wasn't ready to special case the situations I happened to know about), and I really couldn't figure out a solution there either. This was literally my last resort and the only thing I could make work. I'm going to push for requiring schema validation, and for that to be acceptable I think the schema errors need to be reasonably clean, which is the only reason I'm pursuing this. Otherwise, I prefer the schema as-is. If you didn't care about re-using the transport types between packages and remotes, it's possible you could inline them all into one Transport type (for each) and make the discrimination on type work, but that has its own issues in terms of schema hygiene. |
There are some even more degenerate cases. I had a server with a named arg with an invalid format (an existing published server that uses "integer" instead of "number"). The schema validation produced the correct error (bad format on a named arg) but not before giving four errors for violating positional argument constraints (because of the oneOf and exhaustive validation without distinction). Number 10 below is the only semantically meaningful error. 6. [error] packages.0.packageArguments.0.format (schema)
value must be one of "string", "number", "boolean", "filepath"
Reference: #/definitions/Input/properties/format/enum from: [#/definitions/ServerDetail]/properties/packages/items/[#/definitions/Package]/properties/packageArguments/items/[#/definitions/Argument]/anyOf/0/[#/definitions/PositionalArgument]/allOf/0/[#/definitions/InputWithVariables]/allOf/0/[#/definitions/Input]/properties/format/enum
7. [error] packages.0.packageArguments.0.type (schema)
value must be "positional"
Reference: #/definitions/PositionalArgument/allOf/1/properties/type/enum from: [#/definitions/ServerDetail]/properties/packages/items/[#/definitions/Package]/properties/packageArguments/items/[#/definitions/Argument]/anyOf/0/[#/definitions/PositionalArgument]/allOf/1/properties/type/enum
8. [error] packages.0.packageArguments.0 (schema)
missing required fields: 'valueHint'
Reference: #/definitions/PositionalArgument/allOf/1/anyOf/0/required from: [#/definitions/ServerDetail]/properties/packages/items/[#/definitions/Package]/properties/packageArguments/items/[#/definitions/Argument]/anyOf/0/[#/definitions/PositionalArgument]/allOf/1/anyOf/0/required
9. [error] packages.0.packageArguments.0 (schema)
missing required fields: 'value'
Reference: #/definitions/PositionalArgument/allOf/1/anyOf/1/required from: [#/definitions/ServerDetail]/properties/packages/items/[#/definitions/Package]/properties/packageArguments/items/[#/definitions/Argument]/anyOf/0/[#/definitions/PositionalArgument]/allOf/1/anyOf/1/required
10. [error] packages.0.packageArguments.0.format (schema)
value must be one of "string", "number", "boolean", "filepath"
Reference: #/definitions/Input/properties/format/enum from: [#/definitions/ServerDetail]/properties/packages/items/[#/definitions/Package]/properties/packageArguments/items/[#/definitions/Argument]/anyOf/1/[#/definitions/NamedArgument]/allOf/0/[#/definitions/InputWithVariables]/allOf/0/[#/definitions/Input]/properties/format/enum Also, could we try to special-case our way around the confusing errors? Sure, I guess. But the schema really needs to be able to travel. Anyone should be able to use the schema with any validator they choose and get reasonable output. |
When using
anyOf
in schema7 and no element matches, schema errors from validations of all child elements are produced, which makes it hard to understand the actual schema error. This will be increasingly important when we move toward providing a schema validation command inmcp-publisher
and eventually enforcing schema compliance in/v0/publish
as I will put forward in a later PR.Motivation and Context
When validating schema and there is any failure in an
anyOf
element, the set of schema validation errors returned makes it difficult to understand the issue. For example, with the oldanyOf
when validating an SSE transport with nourl
you get these schema validation errors:[error] packages.0.transport.type (schema)
value must be "stdio"
Reference: #/definitions/StdioTransport/properties/type/enum
[error] packages.0.transport (schema)
missing required fields: 'url'
Reference: #/definitions/StreamableHttpTransport/required
[error] packages.0.transport.type (schema)
value must be "streamable-http"
Reference: #/definitions/StreamableHttpTransport/properties/type/enum
[error] packages.0.transport (schema)
missing required fields: 'url'
Reference: #/definitions/SseTransport/require
Schema7 lacks discriminated union support, but we can use if/then/else to achieve something similar (as I have done in this PR). When validating the same server.json as above using the if/then/else schema in this PR you get only the correct schema error:
missing required fields: 'url'
Reference: #/definitions/SseTransport/required
I tried a few variations of validating the
type
underanyOf
to make something a little more human readable, but I could not induce the correct behavior (even when "filtering" ontype
inside theanyOf
, allanyOf
elements were still evaluated and produced schema validation errors).How Has This Been Tested?
I validated using a new
validate
command ofmcp-publisher
(that is part of an upcoming PR), with specific test scenarios triggeringanyOf
failures (that validation used the Go lib in this project, santhosh-tekuri/jsonschema/v5).I also ran the same test scenario using my TypeScript MCP registry validator, which uses Ajv (the standard schema validator for Javascript).
Lastly I used my validator to run the new schema against all published servers and compared the results to the output using the previous schema.
Note: I also made changes to the
uri
format for the StreamableHttpTransport and SseTransport (required for currently published SSE servers that use tokens in theirurl
to pass schema validation). This will likely be replaced by a better solution in #570 but I think it's worth fixing this now while that is pending (given that it impacts currently published servers).I think this is the last of the schema changes I'm going to suggest based on my work evaluating existing published servers and implementing schema validation (aforementioned upcoming PR).
Breaking Changes
This schema is functionally identical the the version it replaces with respect to the transport and argument changes. The only difference is in the schema validation errors produced (currently no code in the project performs schema validation, so I don't consider that a breaking change).
This schema will also now catch a StreamableHttpTransport
uri
that doesn't start with an http scheme - but there are no such servers currently published (and if there were, they would be in error). And it will allow tokens in SseTransporturi
properties, allowing some existing published servers to pass that would have failed (but again, I don't think anyone is actually doing schema validation yet). I suppose if you really wanted it, you could call the failure to allow a non-http streamable uri a breaking change.Types of changes
Checklist