Skip to content
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

Validate schema root types and directives #1124

Merged
merged 1 commit into from
Dec 8, 2017

Conversation

leebyron
Copy link
Contributor

@leebyron leebyron commented Dec 7, 2017

This moves validation out of GraphQLSchema's constructor (but not yet from other type constructors), which is responsible for root type validation and interface implementation checking.

Reduces time to construct GraphQLSchema significantly, shifting the time to validation.

This also allows for much looser rules within the schema builders, which implicitly validate while trying to adhere to flow types. Instead we use any casts to loosen the rules to defer that to validation where errors can be richer.

This also loosens the rule that a schema can only be constructed if it has a query type, moving that to validation as well. That makes flow typing slightly less nice, but allows for incremental schema building which is valuable

Improves #1079
Partial #1080
Closes #722

@leebyron
Copy link
Contributor Author

leebyron commented Dec 7, 2017

This builds atop @kassens's fed9610

operation,
]);
}
return queryType;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message suggests that it's normal to have schema without query and it will work fine if you don't send query operations against it. I think error message should say that query type is mandatory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree. I'll make that change

@IvanGoncharov
Copy link
Member

@leebyron Wow, you did a lot of work to search for correct error location both in astNode and extensionASTNodes 👍

Having queryType optional makes #817 more feasible and I will rebase it after this PR will be merged.

@leebyron leebyron force-pushed the validate-top-level-schema branch 4 times, most recently from faeec88 to 5f6690e Compare December 7, 2017 22:44
@kassens
Copy link
Contributor

kassens commented Dec 8, 2017

Really excited about this. The one thing that seemed not 100% ideal was the any casts, but I suppose the only way around that would to actually have different types for validated and unvalidated schema which would be a breaking change.

@leebyron
Copy link
Contributor Author

leebyron commented Dec 8, 2017

Yeah, casting to any makes me feel bad inside, but it's the right thing to do here. I'll add some comments there explaining what's happening.

This moves validation out of GraphQLSchema's constructor (but not yet from other type constructors), which is responsible for root type validation and interface implementation checking.

Reduces time to construct GraphQLSchema significantly, shifting the time to validation.

This also allows for much looser rules within the schema builders, which implicitly validate while trying to adhere to flow types. Instead we use any casts to loosen the rules to defer that to validation where errors can be richer.

This also loosens the rule that a schema can only be constructed if it has a query type, moving that to validation as well. That makes flow typing slightly less nice, but allows for incremental schema building which is valuable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants