fix: validate subscription with conditional selections#963
fix: validate subscription with conditional selections#963goto-bus-stop merged 4 commits intomainfrom
Conversation
SimonSapin
left a comment
SimonSapin
left a comment
There was a problem hiding this comment.
Should this PR wait until graphql/graphql-spec#860 is merged?
| let has_conditional_field = fields | ||
| .iter() |
There was a problem hiding this comment.
This only considers field selections, but the spec’s For each {selection} in {selectionSet} suggests we should also check for @skip or @include on inline fragments or fragment spreads
|
| }); | ||
| if let Some(conditional_selection) = has_conditional_selection { | ||
| diagnostics.push( | ||
| operation.location(), |
There was a problem hiding this comment.
This is the location / source span for the entire operation all the way until }. As the snapshots below show, Ariadne renders it as:
1 │ ╭─▶ subscription ConditionalInlineSub($condition: Boolean = true) {
┆ ┆
5 │ ├─▶ }
│ │
│ ╰───────
I think it’d be more useful/precise to have the .location() of the conditional directive instead
| operation.location(), | ||
| executable::BuildError::SubscriptionUsesConditionalSelection { | ||
| name: operation.name.clone(), | ||
| selection: conditional_selection.clone(), |
There was a problem hiding this comment.
Printing an entire selection set can get verbose. I think with the more precise location (previous comment) this field can be removed entirely
|
|
||
| // first rule validates that we only have single selection | ||
| let has_conditional_selection = | ||
| operation.selection_set.selections.iter().find(|selection| { |
There was a problem hiding this comment.
Sorry for the back-and-forth, but now this lacks spec algorithm’s recursion and fails to catch something like subscription { ... { field @skip } }. I’ll move the check into expand_selections
Adds validation for subscription operations that specify `@skip`/`@include` conditionals on root selections. See: graphql/graphql-spec#860
* Recur into inline fragments and fragment spread * Point to the faulty directive * Skip printing an entire selection set
dariuszkuc
force-pushed
the
conditional_subscriptions
branch
from
be2f362 to
a11a530
Compare
| selection_set: &'doc executable::SelectionSet, | ||
| seen: &mut HashSet<&'doc Name>, | ||
| f: &mut dyn FnMut(&'doc executable::Selection), | ||
| ) -> Result<(), RecursionLimitError> { |
There was a problem hiding this comment.
The implementation here doesn't actually raise a RecursionLimitError right now, but that will be part of a follow-up PR
3 participants
Adds validation for subscription operations that specify
@skip/@includeconditionals on root selections.See: graphql/graphql-spec#860