-
-
Notifications
You must be signed in to change notification settings - Fork 916
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
Schema builder fixes; add schema validation of subscription fields #3867
Conversation
@@ -111,14 +111,19 @@ private Schema BuildSchemaFrom(GraphQLDocument document) | |||
|
|||
var directives = new List<Directive>(); | |||
|
|||
// process the schema definition first | |||
foreach (var def in document.Definitions) | |||
{ | |||
if (def is GraphQLSchemaDefinition schemaDef) | |||
{ | |||
_schemaDef = schemaDef; |
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.
_schemaDef
must be set first or subscription resolvers don't get set properly. Who knows; maybe it affects more stuff.
@@ -74,7 +74,7 @@ public void Include(Type type) | |||
/// </summary> | |||
public void Include(string name, Type type) | |||
{ | |||
_typeConfigurations[name].Type = type; | |||
For(name).Type = type; |
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.
ForAll
only triggers when using For
, but seems to clearly indicate that it should operate on all types. So I've changed Include
to call For
and thereby execute any configured ForAll
code.
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #3867 +/- ##
==========================================
- Coverage 84.66% 84.66% -0.01%
==========================================
Files 419 419
Lines 19333 19344 +11
Branches 3033 3037 +4
==========================================
+ Hits 16369 16377 +8
- Misses 2251 2253 +2
- Partials 713 714 +1 ☔ View full report in Codecov by Sentry. |
Fixes #3580
Adds schema validation to ensure that
StreamResolver
is set for fields of root subscription types. This is a non-breaking change because without the validation, attempting to resolve these fields would fail at runtime.Trying to write a fix for #3580 I found two bugs in the schema-first SchemaBuilder (as far as I can tell).
Include
does not include configurations viaForAll
schema
definition is listed after the subscription definition