Only root fields of subscriptions should set StreamResolver; only object output types should set Resolver #3574
Conversation
…put types should set Resolver
github-actions
bot
added
the
test
|
@Shane32 I need your help with grammar in messages. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -63,6 +63,9 @@ | |||
| throw new InvalidOperationException($"The field '{field.Name}' of an Object type '{type.Name}' must be an output type."); | |||
|
|
|||
| ValidateFieldArgumentsUniqueness(field, type); | |||
|
|
|||
| if (field.StreamResolver != null && type != schema.Subscription) | |||
Check warning
Code scanning / CodeQL
Reference equality test on System.Object Warning
Co-authored-by: Shane Krueger <shane@acdmail.com>
Co-authored-by: Shane Krueger <shane@acdmail.com>
Co-authored-by: Shane Krueger <shane@acdmail.com>
Co-authored-by: Shane Krueger <shane@acdmail.com>
sungam3r
changed the title
|
Do we have schema validation tests to ensure that |
|
I really like this PR. But, is it a breaking PR? It would seem so; notice the changes necessary to our own StarWars sample. |
I removed some resolvers to make it work. Does not it mean that changes are indeed breaking? |
|
For v8, I would also like to require that |
No. I can add it in new PR, don't want to mix one more check. |
Well, technically, I would say it is indeed breaking, as setting certain resolvers now throws an exception whereas before they were simply ignored. That's not always a bad thing though. So: I'm not worried about the fact that stream resolvers must not be set for non-root-subscription graph types. This should not occur in user code; it would be a 'bug' if it were so. It seems to me that having resolvers for interface types MAY be common practice. I do not know. It is clear that they should not be used/necessary with a properly configured schema. I have demonstrated in the past how they could be usefully used, although it does break GraphQL spec. It is also possible or perhaps likely that dynamically-built schemas would set these resolvers for interfaces, assuming they would be ignored, just as So, we have a few options:
I'm really not sure what I would choose. Probably option 2, but options 3 and 4 also has merit. Your thoughts? |
Sounds good. If this were not the case, the schema would be improperly configured, so this is not a breaking change. |
|
I prefer option 4 - merge as is and wait for feedback. If anyone will be affected, then push changes for option 3 in v7.5+ |
Co-authored-by: Shane Krueger <shane@acdmail.com>
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #3574 +/- ##
==========================================
+ Coverage 83.82% 83.89% +0.06%
==========================================
Files 381 381
Lines 16875 16884 +9
Branches 2704 2710 +6
==========================================
+ Hits 14146 14164 +18
+ Misses 2084 2073 -11
- Partials 645 647 +2
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
fixes #2994
also fixes #1176
@Shane32 I suggest to consider this as a "fix" for #1176. I do not believe that we will split
FieldTypefor inputs and outputs or redesign API in some other way. At least I'm sure that it will be opened for years without any progress. Schema validation allows to find all invalid usages during initialization, i.e. early thought in runtime, not at compile time, so I think this is a good compromise and we do not need to break the whole design.