Subscription root validation:

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

Author: SimonSapin

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

@@ -273,8 +273,6 @@ pub(crate) enum BuildError {
     SubscriptionUsesConditionalSelection {
         /// Name of the operation
         name: Option<Name>,
-        /// Selection that specify @skip or @include directives.
-        selection: Selection,
     },
 
     #[error("{0}")]

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

@@ -854,13 +854,8 @@ impl ToCliReport for DiagnosticData {
                         format_args!("{field} is an introspection field"),
                     );
                 }
-                ExecutableBuildError::SubscriptionUsesConditionalSelection {
-                    selection, ..
-                } => {
-                    report.with_label_opt(
-                        self.location,
-                        format_args!("{selection} specifies @skip or @include condition"),
-                    );
+                ExecutableBuildError::SubscriptionUsesConditionalSelection { .. } => {
+                    report.with_label_opt(self.location, "conditional directive used here");
                 }
                 ExecutableBuildError::ConflictingFieldType(inner) => {
                     let ConflictingFieldType {

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

@@ -13,6 +13,7 @@ pub(crate) fn validate_subscription(
         let fields = super::selection::expand_selections(
             &document.fragments,
             std::iter::once(&operation.selection_set),
+            Some((operation, diagnostics)),
         );
 
         if fields.len() > 1 {
@@ -46,24 +47,6 @@ pub(crate) fn validate_subscription(
                 },
             );
         }
-
-        // first rule validates that we only have single selection
-        let has_conditional_selection =
-            operation.selection_set.selections.iter().find(|selection| {
-                selection
-                    .directives()
-                    .iter()
-                    .any(|d| matches!(d.name.as_str(), "skip" | "include"))
-            });
-        if let Some(conditional_selection) = has_conditional_selection {
-            diagnostics.push(
-                operation.location(),
-                executable::BuildError::SubscriptionUsesConditionalSelection {
-                    name: operation.name.clone(),
-                    selection: conditional_selection.clone(),
-                },
-            );
-        }
     }
 }
 

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

@@ -67,13 +67,28 @@ impl<'a> FieldSelection<'a> {
 pub(crate) fn expand_selections<'doc>(
     fragments: &'doc IndexMap<Name, Node<executable::Fragment>>,
     selection_sets: impl Iterator<Item = &'doc executable::SelectionSet>,
+    mut for_subscription_top_level: Option<(&executable::Operation, &mut DiagnosticList)>,
 ) -> Vec<FieldSelection<'doc>> {
     let mut selections = vec![];
     let mut queue: VecDeque<&executable::SelectionSet> = selection_sets.collect();
     let mut seen_fragments = HashSet::with_hasher(ahash::RandomState::new());
 
     while let Some(next_set) = queue.pop_front() {
         for selection in &next_set.selections {
+            if let Some((operation, diagnostics)) = &mut for_subscription_top_level {
+                if let Some(conditional_directive) = selection
+                    .directives()
+                    .iter()
+                    .find(|d| matches!(d.name.as_str(), "skip" | "include"))
+                {
+                    diagnostics.push(
+                        conditional_directive.location(),
+                        executable::BuildError::SubscriptionUsesConditionalSelection {
+                            name: operation.name.clone(),
+                        },
+                    );
+                }
+            }
             match selection {
                 executable::Selection::Field(field) => {
                     selections.push(FieldSelection::new(&next_set.ty, field))
@@ -520,8 +535,11 @@ impl<'alloc, 's, 'doc> FieldsInSetCanMerge<'alloc, 's, 'doc> {
         &self,
         selection_sets: impl Iterator<Item = &'doc executable::SelectionSet>,
     ) -> &'alloc [FieldSelection<'doc>] {
-        self.alloc
-            .alloc(expand_selections(&self.document.fragments, selection_sets))
+        self.alloc.alloc(expand_selections(
+            &self.document.fragments,
+            selection_sets,
+            None,
+        ))
     }
 
     pub(crate) fn validate_operation(

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

@@ -1,10 +1,8 @@
 Error: subscription `ConditionalSub` can not specify @skip or @include on root fields
-   ╭─[ 0120_conditional_subscriptions.graphql:1:1 ]
+   ╭─[ 0120_conditional_subscriptions.graphql:2:12 ]
    │
- 1 │ ╭─▶ subscription ConditionalSub($condition: Boolean = true) {
-   ┆ ┆   
- 3 │ ├─▶ }
-   │ │       
-   │ ╰─────── ticker @include(if: $condition) specifies @skip or @include condition
+ 2 │     ticker @include(if: $condition)
+   │            ────────────┬───────────  
+   │                        ╰───────────── conditional directive used here
 ───╯
 

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

@@ -1,12 +1,8 @@
 Error: subscription `ConditionalInlineSub` can not specify @skip or @include on root fields
-   ╭─[ 0121_conditional_subscriptions_with_inline_fragment.graphql:1:1 ]
+   ╭─[ 0121_conditional_subscriptions_with_inline_fragment.graphql:2:9 ]
    │
- 1 │ ╭─▶ subscription ConditionalInlineSub($condition: Boolean = true) {
-   ┆ ┆   
- 5 │ ├─▶ }
-   │ │       
-   │ ╰─────── ... @include(if: $condition) {
-  ticker
-} specifies @skip or @include condition
+ 2 │     ... @include(if: $condition) {
+   │         ────────────┬───────────  
+   │                     ╰───────────── conditional directive used here
 ───╯
 

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

@@ -1,10 +1,8 @@
 Error: subscription `ConditionalInlineSub` can not specify @skip or @include on root fields
-   ╭─[ 0122_conditional_subscriptions_with_named_fragment.graphql:1:1 ]
+   ╭─[ 0122_conditional_subscriptions_with_named_fragment.graphql:2:23 ]
    │
- 1 │ ╭─▶ subscription ConditionalInlineSub($condition: Boolean = true) {
-   ┆ ┆   
- 3 │ ├─▶ }
-   │ │       
-   │ ╰─────── ...mySubscription @include(if: $condition) specifies @skip or @include condition
+ 2 │     ...mySubscription @include(if: $condition)
+   │                       ────────────┬───────────  
+   │                                   ╰───────────── conditional directive used here
 ───╯
 

crates/apollo-compiler/test_data/diagnostics/0123_conditional_subscriptions_inside_inline_fragment.graphql

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

crates/apollo-compiler/test_data/diagnostics/0123_conditional_subscriptions_inside_inline_fragment.txt

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

crates/apollo-compiler/test_data/diagnostics/0124_conditional_subscriptions_inside_named_fragment.graphql

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

crates/apollo-compiler/test_data/diagnostics/0124_conditional_subscriptions_inside_named_fragment.txt

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

crates/apollo-compiler/test_data/serializer/diagnostics/0123_conditional_subscriptions_inside_inline_fragment.graphql

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

crates/apollo-compiler/test_data/serializer/diagnostics/0124_conditional_subscriptions_inside_named_fragment.graphql

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