fix: validate subscription with conditional selections (#963)

* fix: validate subscription with conditional selections

Adds validation for subscription operations that specify `@skip`/`@include` conditionals on root selections.

See: graphql/graphql-spec#860

* check for all selections

* Subscription root validation:

* Recur into inline fragments and fragment spread
* Point to the faulty directive
* Skip printing an entire selection set

* Use a single special-purpose walk in subscription validation

---------

Co-authored-by: Simon Sapin <[email protected]>
Co-authored-by: Renée Kooi <[email protected]>

Author: 3 people

crates/apollo-compiler/src/executable/mod.rs

@@ -266,6 +266,14 @@ pub(crate) enum BuildError {
         /// Name of the introspection field
         field: Name,
     },
+    #[error(
+        "{} can not specify @skip or @include on root fields",
+        subscription_name_or_anonymous(name)
+    )]
+    SubscriptionUsesConditionalSelection {
+        /// Name of the operation
+        name: Option<Name>,
+    },
 
     #[error("{0}")]
     ConflictingFieldType(Box<ConflictingFieldType>),

crates/apollo-compiler/src/validation/mod.rs

@@ -1,11 +1,6 @@
 //! Supporting APIs for [GraphQL validation](https://spec.graphql.org/October2021/#sec-Validation)
 //! and other kinds of errors.
 
-use crate::coordinate::SchemaCoordinate;
-#[cfg(doc)]
-use crate::ExecutableDocument;
-use crate::Schema;
-
 pub(crate) mod argument;
 pub(crate) mod diagnostics;
 pub(crate) mod directive;
@@ -26,6 +21,7 @@ pub(crate) mod variable;
 use crate::collections::HashMap;
 use crate::collections::HashSet;
 use crate::collections::IndexSet;
+use crate::coordinate::SchemaCoordinate;
 use crate::diagnostic::CliReport;
 use crate::diagnostic::Diagnostic;
 use crate::diagnostic::ToCliReport;
@@ -41,6 +37,7 @@ use crate::schema::BuildError as SchemaBuildError;
 use crate::schema::Implementers;
 use crate::Name;
 use crate::Node;
+use crate::Schema;
 use std::fmt;
 use std::sync::Arc;
 use std::sync::OnceLock;
@@ -338,6 +335,9 @@ impl DiagnosticData {
                 ExecutableBuildError::SubscriptionUsesIntrospection { .. } => {
                     "SubscriptionUsesIntrospection"
                 }
+                ExecutableBuildError::SubscriptionUsesConditionalSelection { .. } => {
+                    "SubscriptionUsesConditionalSelection"
+                }
                 ExecutableBuildError::ConflictingFieldType(_) => "ConflictingFieldType",
                 ExecutableBuildError::ConflictingFieldName(_) => "ConflictingFieldName",
                 ExecutableBuildError::ConflictingFieldArgument(_) => "ConflictingFieldArgument",
@@ -586,6 +586,18 @@ impl DiagnosticData {
                             .to_string())
                     }
                 }
+                ExecutableBuildError::SubscriptionUsesConditionalSelection { name, .. } => {
+                    if let Some(name) = name {
+                        Some(format!(
+                            r#"Subscription "{name}" can not specify @skip or @include on root fields."#
+                        ))
+                    } else {
+                        Some(
+                            "Anonymous Subscription can not specify @skip or @include on root fields."
+                                .to_string(),
+                        )
+                    }
+                }
                 ExecutableBuildError::ConflictingFieldType(inner) => {
                     let ConflictingFieldType {
                         alias,
@@ -839,6 +851,9 @@ impl ToCliReport for DiagnosticData {
                         format_args!("{field} is an introspection field"),
                     );
                 }
+                ExecutableBuildError::SubscriptionUsesConditionalSelection { .. } => {
+                    report.with_label_opt(self.location, "conditional directive used here");
+                }
                 ExecutableBuildError::ConflictingFieldType(inner) => {
                     let ConflictingFieldType {
                         alias,

crates/apollo-compiler/src/validation/operation.rs

@@ -1,51 +1,117 @@
+use crate::collections::HashSet;
 use crate::executable;
+use crate::validation::diagnostics::DiagnosticData;
 use crate::validation::DiagnosticList;
 use crate::validation::ExecutableValidationContext;
+use crate::validation::RecursionLimitError;
 use crate::ExecutableDocument;
+use crate::Name;
 use crate::Node;
 
+/// Iterate all selections in the selection set.
+///
+/// This includes fields, fragment spreads, and inline fragments. For fragments, both the spread
+/// and the fragment's nested selections are reported.
+///
+/// Does not recurse into nested fields.
+fn walk_selections<'doc>(
+    document: &'doc ExecutableDocument,
+    selections: &'doc executable::SelectionSet,
+    mut f: impl FnMut(&'doc executable::Selection),
+) -> Result<(), RecursionLimitError> {
+    fn walk_selections_inner<'doc>(
+        document: &'doc ExecutableDocument,
+        selection_set: &'doc executable::SelectionSet,
+        seen: &mut HashSet<&'doc Name>,
+        f: &mut dyn FnMut(&'doc executable::Selection),
+    ) -> Result<(), RecursionLimitError> {
+        for selection in &selection_set.selections {
+            f(selection);
+            match selection {
+                executable::Selection::Field(_) => {
+                    // Nothing to do
+                }
+                executable::Selection::FragmentSpread(fragment) => {
+                    let new = seen.insert(&fragment.fragment_name);
+                    if !new {
+                        continue;
+                    }
+
+                    // If the fragment doesn't exist, that error is reported elsewhere.
+                    if let Some(fragment_definition) =
+                        document.fragments.get(&fragment.fragment_name)
+                    {
+                        walk_selections_inner(
+                            document,
+                            &fragment_definition.selection_set,
+                            seen,
+                            f,
+                        )?;
+                    }
+                }
+                executable::Selection::InlineFragment(fragment) => {
+                    walk_selections_inner(document, &fragment.selection_set, seen, f)?;
+                }
+            }
+        }
+        Ok(())
+    }
+
+    walk_selections_inner(document, selections, &mut HashSet::default(), &mut f)
+}
+
 pub(crate) fn validate_subscription(
     document: &executable::ExecutableDocument,
     operation: &Node<executable::Operation>,
     diagnostics: &mut DiagnosticList,
 ) {
-    if operation.is_subscription() {
-        let fields = super::selection::expand_selections(
-            &document.fragments,
-            std::iter::once(&operation.selection_set),
-        );
+    if !operation.is_subscription() {
+        return;
+    }
 
-        if fields.len() > 1 {
-            diagnostics.push(
-                operation.location(),
-                executable::BuildError::SubscriptionUsesMultipleFields {
-                    name: operation.name.clone(),
-                    fields: fields
-                        .iter()
-                        .map(|field| field.field.name.clone())
-                        .collect(),
-                },
-            );
+    let mut field_names = vec![];
+
+    let walked = walk_selections(document, &operation.selection_set, |selection| {
+        if let executable::Selection::Field(field) = selection {
+            field_names.push(field.name.clone());
+            if matches!(field.name.as_str(), "__type" | "__schema" | "__typename") {
+                diagnostics.push(
+                    field.location(),
+                    executable::BuildError::SubscriptionUsesIntrospection {
+                        name: operation.name.clone(),
+                        field: field.name.clone(),
+                    },
+                );
+            }
         }
 
-        let has_introspection_fields = fields
+        if let Some(conditional_directive) = selection
+            .directives()
             .iter()
-            .find(|field| {
-                matches!(
-                    field.field.name.as_str(),
-                    "__type" | "__schema" | "__typename"
-                )
-            })
-            .map(|field| &field.field);
-        if let Some(field) = has_introspection_fields {
+            .find(|d| matches!(d.name.as_str(), "skip" | "include"))
+        {
             diagnostics.push(
-                field.location(),
-                executable::BuildError::SubscriptionUsesIntrospection {
+                conditional_directive.location(),
+                executable::BuildError::SubscriptionUsesConditionalSelection {
                     name: operation.name.clone(),
-                    field: field.name.clone(),
                 },
             );
         }
+    });
+
+    if walked.is_err() {
+        diagnostics.push(None, DiagnosticData::RecursionError {});
+        return;
+    }
+
+    if field_names.len() > 1 {
+        diagnostics.push(
+            operation.location(),
+            executable::BuildError::SubscriptionUsesMultipleFields {
+                name: operation.name.clone(),
+                fields: field_names,
+            },
+        );
     }
 }
 

crates/apollo-compiler/src/validation/selection.rs

@@ -64,7 +64,7 @@ impl<'a> FieldSelection<'a> {
 }
 
 /// Expand one or more selection sets to a list of all fields selected.
-pub(crate) fn expand_selections<'doc>(
+fn expand_selections<'doc>(
     fragments: &'doc IndexMap<Name, Node<executable::Fragment>>,
     selection_sets: impl Iterator<Item = &'doc executable::SelectionSet>,
 ) -> Vec<FieldSelection<'doc>> {

crates/apollo-compiler/src/validation/variable.rs

@@ -80,7 +80,14 @@ pub(crate) fn validate_variable_definitions(
     }
 }
 
-/// Named fragments are "deduplicated": only visited once even if spread multiple times
+/// Call a function for every selection that is reachable from the given selection set.
+///
+/// This includes fields, fragment spreads, and inline fragments. For fragments, both the spread
+/// and the fragment's nested selections are reported. For fields, nested selections are also
+/// reported.
+///
+/// Named fragments are "deduplicated": only visited once even if spread multiple times *in
+/// different locations*. This is only appropriate for certain kinds of validations, so reuser beware.
 fn walk_selections_with_deduped_fragments<'doc>(
     document: &'doc ExecutableDocument,
     selections: &'doc executable::SelectionSet,

crates/apollo-compiler/test_data/diagnostics/0120_conditional_subscriptions.graphql

@@ -0,0 +1,11 @@
+subscription ConditionalSub($condition: Boolean = true) {
+    ticker @include(if: $condition)
+}
+
+type Query {
+    hello: String
+}
+
+type Subscription {
+    ticker: String
+}

crates/apollo-compiler/test_data/diagnostics/0120_conditional_subscriptions.txt

@@ -0,0 +1,8 @@
+Error: subscription `ConditionalSub` can not specify @skip or @include on root fields
+   ╭─[ 0120_conditional_subscriptions.graphql:2:12 ]
+   │
+ 2 │     ticker @include(if: $condition)
+   │            ────────────┬───────────  
+   │                        ╰───────────── conditional directive used here
+───╯
+

crates/apollo-compiler/test_data/diagnostics/0121_conditional_subscriptions_with_inline_fragment.graphql

@@ -0,0 +1,13 @@
+subscription ConditionalInlineSub($condition: Boolean = true) {
+    ... @include(if: $condition) {
+        ticker
+    }
+}
+
+type Query {
+    hello: String
+}
+
+type Subscription {
+    ticker: String
+}

crates/apollo-compiler/test_data/diagnostics/0121_conditional_subscriptions_with_inline_fragment.txt

@@ -0,0 +1,8 @@
+Error: subscription `ConditionalInlineSub` can not specify @skip or @include on root fields
+   ╭─[ 0121_conditional_subscriptions_with_inline_fragment.graphql:2:9 ]
+   │
+ 2 │     ... @include(if: $condition) {
+   │         ────────────┬───────────  
+   │                     ╰───────────── conditional directive used here
+───╯
+

crates/apollo-compiler/test_data/diagnostics/0122_conditional_subscriptions_with_named_fragment.graphql

@@ -0,0 +1,15 @@
+subscription ConditionalInlineSub($condition: Boolean = true) {
+    ...mySubscription @include(if: $condition)
+}
+
+fragment mySubscription on Subscription {
+    ticker
+}
+
+type Query {
+    hello: String
+}
+
+type Subscription {
+    ticker: String
+}