feat: add validateFullSchema and assertValidFullSchema#4798
Conversation
validateSchema validates a source schema and rejects any name beginning with "__", which is reserved by GraphQL introspection. Tools that work with a full schema (for example one reconstructed from introspection) may legitimately encounter reserved names, including ones added by a newer version of GraphQL than this library implements. Add validateFullSchema and assertValidFullSchema, which run the same checks as validateSchema except that reserved names are permitted. The two modes use separate validation caches so their results never collide. Closes graphql#4415
|
@vishwakt is attempting to deploy a commit to the The GraphQL Foundation Team on Vercel. A member of the Team first needs to authorize it. |
|
|
martinbonnin
left a comment
There was a problem hiding this comment.
LGTM although not a graphql-js expert.
Might be worth marking this function as experimental until we collectivelly converge on a bigger test suite?
Per review feedback, rename validateFullSchema and assertValidFullSchema to experimentalValidateFullSchema and experimentalAssertValidFullSchema, matching the existing experimental* naming convention used for experimentalExecuteIncrementally. This signals the API may change while the behavior and test coverage for full schemas converge.
|
Good idea, thanks. I marked both functions as experimental by adopting the existing |
martinbonnin
left a comment
There was a problem hiding this comment.
LGTM. Thanks for addressing the feedback.
Pinging @yaacovCR @benjie @jerelmiller for more expert eyes.
benjie
left a comment
There was a problem hiding this comment.
I'm not sure this is the right solution. For example, the schema:
scalar __S
type __Q {
__i: __S
}
schema {
query: __Q
}should be invalid.
Similarly, the less ridiculous schema:
type Query {
int: __Type
}should also be invalid.
I understand what we're trying to achieve here, but I don't think simply turning off the "no double underscore" validation rule is sufficient, there's a lot of assumptions baked into that rule that if we turn them off we'll need to re-apply other ways (e.g. "starting at the root operation types and without traversing or including metafields, no __-prefixed field, argument or type should be reachable").
I think we need to invest a lot of thought into what a "full schema" means for GraphQL.js
A simpler solution might be for GraphiQL to accept an option that tells it to assume the schema is valid.
Address review feedback: rather than disabling the reserved-name rule outright, a full schema now rejects a reserved (__-prefixed) name only when it is reachable from the root operation types through ordinary fields, arguments, and their types. The introspection types and other implementation-provided definitions are only reachable through the __schema, __type, and __typename meta-fields, which are never part of a type's fields, so they are not traversed and remain valid. A user type or field exposing an introspection type, or a reserved root operation type, is still rejected.
|
Thanks @benjie, that's a good catch and I agree the blanket approach was wrong. I've reworked it to implement the rule you described. Instead of disabling the reserved-name check, a full schema now reports a reserved ( Both of your examples are now rejected: scalar __S
type __Q { __i: __S }
schema { query: __Q }type Query { int: __Type }while a reserved definition that is not part of the queryable surface (e.g. the Added tests for both of your examples plus reachable reserved field/argument names, unreachable reserved definitions, and independent caching. This still doesn't settle the larger "what is a full schema" design question, but it should at least make the validation sound rather than simply permissive. Happy to keep iterating. |
Motivation
Closes #4415.
validateSchema()validates a source schema and rejects any name beginning with__, since those are reserved by GraphQL introspection. However, tools that work with a full schema (for example one reconstructed from an introspection result, as in GraphiQL or codegen) may legitimately encounter reserved names, including ones added by a newer version of GraphQL than the running version of graphql-js implements.The reproducer from the issue fails today purely on the reserved-name rule, even though the SDL is otherwise valid:
See the Full Schemas RFC for the broader context on source vs. full schemas.
Changes
experimentalValidateFullSchema(schema)andexperimentalAssertValidFullSchema(schema), mirroringvalidateSchema/assertValidSchema.__) restriction, so a full schema validates without spurious errors.allowReservedNamesflag threaded throughSchemaValidationContext.__validationErrorsvs.__fullSchemaValidationErrors) so their results never collide regardless of call order.experimentalname prefix (matchingexperimentalExecuteIncrementally) to signal the API may change while behavior and test coverage for full schemas converge.Tests
Added coverage for: the issue's reproducer, reserved names on types/fields/args/enum values/input fields, that non-name errors are still reported when validating a full schema, independent caching, and the assert variant.
npm run check,npm run lint, and the full test suite pass locally.