From 3f5a5e41b7fddc746ef90a8b273b97345ab8d29c Mon Sep 17 00:00:00 2001
From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com>
Date: Thu, 3 Apr 2025 18:49:02 -0500
Subject: [PATCH 1/4] fix: validate subscription with conditional selections

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

See: https://github.com/graphql/graphql-spec/pull/860
---
 crates/apollo-compiler/src/executable/mod.rs  | 10 ++
 crates/apollo-compiler/src/validation/mod.rs  | 21 ++++
 .../src/validation/operation.rs               | 21 ++++
 .../tests/validation/operation.rs             | 96 +++++++++++++++++++
 4 files changed, 148 insertions(+)

diff --git a/crates/apollo-compiler/src/executable/mod.rs b/crates/apollo-compiler/src/executable/mod.rs
index bc7ce5b6..7cac7781 100644
--- a/crates/apollo-compiler/src/executable/mod.rs
+++ b/crates/apollo-compiler/src/executable/mod.rs
@@ -266,6 +266,16 @@ 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>,
+        /// Field name that specify @skip or @include directives.
+        field: Name,
+    },
 
     #[error("{0}")]
     ConflictingFieldType(Box<ConflictingFieldType>),
diff --git a/crates/apollo-compiler/src/validation/mod.rs b/crates/apollo-compiler/src/validation/mod.rs
index 837054a1..63e35722 100644
--- a/crates/apollo-compiler/src/validation/mod.rs
+++ b/crates/apollo-compiler/src/validation/mod.rs
@@ -338,6 +338,9 @@ impl DiagnosticData {
                 ExecutableBuildError::SubscriptionUsesIntrospection { .. } => {
                     "SubscriptionUsesIntrospection"
                 }
+                ExecutableBuildError::SubscriptionUsesConditionalSelection { .. } => {
+                    "SubscriptionUsesConditionalSelection"
+                }
                 ExecutableBuildError::ConflictingFieldType(_) => "ConflictingFieldType",
                 ExecutableBuildError::ConflictingFieldName(_) => "ConflictingFieldName",
                 ExecutableBuildError::ConflictingFieldArgument(_) => "ConflictingFieldArgument",
@@ -586,6 +589,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 +854,12 @@ impl ToCliReport for DiagnosticData {
                         format_args!("{field} is an introspection field"),
                     );
                 }
+                ExecutableBuildError::SubscriptionUsesConditionalSelection { field, .. } => {
+                    report.with_label_opt(
+                        self.location,
+                        format_args!("{field} specifies @skip or @include condition"),
+                    );
+                }
                 ExecutableBuildError::ConflictingFieldType(inner) => {
                     let ConflictingFieldType {
                         alias,
diff --git a/crates/apollo-compiler/src/validation/operation.rs b/crates/apollo-compiler/src/validation/operation.rs
index ed86d558..52bce8a8 100644
--- a/crates/apollo-compiler/src/validation/operation.rs
+++ b/crates/apollo-compiler/src/validation/operation.rs
@@ -46,6 +46,27 @@ pub(crate) fn validate_subscription(
                 },
             );
         }
+
+        // first rule validates that we only have single selection
+        let has_conditional_field = fields
+            .iter()
+            .find(|field| {
+                field
+                    .field
+                    .directives
+                    .iter()
+                    .any(|d| matches!(d.name.as_str(), "skip" | "include"))
+            })
+            .map(|field| field.field);
+        if let Some(conditional_field) = has_conditional_field {
+            diagnostics.push(
+                conditional_field.location(),
+                executable::BuildError::SubscriptionUsesConditionalSelection {
+                    name: operation.name.clone(),
+                    field: conditional_field.name.clone(),
+                },
+            );
+        }
     }
 }
 
diff --git a/crates/apollo-compiler/tests/validation/operation.rs b/crates/apollo-compiler/tests/validation/operation.rs
index fb3ca59b..ea235af2 100644
--- a/crates/apollo-compiler/tests/validation/operation.rs
+++ b/crates/apollo-compiler/tests/validation/operation.rs
@@ -218,3 +218,99 @@ type Product {
         "{errors}"
     );
 }
+
+#[test]
+fn it_validates_subscription_cannot_specify_multiple_fields() {
+    let input = r#"
+subscription MultipleSubs {
+  ticker1
+  ticker2
+}
+
+type Query {
+  hello: String
+}
+
+type Subscription {
+  ticker1: String
+  ticker2: String
+}
+"#;
+
+    let errors = Parser::new()
+        .parse_mixed_validate(input, "schema.graphql")
+        .unwrap_err()
+        .to_string();
+    assert!(
+        errors.contains("subscription `MultipleSubs` can only have one root field"),
+        "{errors}"
+    );
+    assert!(
+        errors.contains("There are 2 root fields: ticker1, ticker2. This is not allowed."),
+        "{errors}"
+    );
+}
+
+#[test]
+fn it_validates_subscription_cannot_select_introspection_fields() {
+    let input = r#"
+subscription IntrospectionSub {
+  __typename
+}
+
+type Query {
+  hello: String
+}
+
+type Subscription {
+  ticker: String
+}
+"#;
+
+    let errors = Parser::new()
+        .parse_mixed_validate(input, "schema.graphql")
+        .unwrap_err()
+        .to_string();
+    assert!(
+        errors.contains(
+            "subscription `IntrospectionSub` can not have an introspection field as a root field"
+        ),
+        "{errors}"
+    );
+    assert!(
+        errors.contains("__typename is an introspection field"),
+        "{errors}"
+    );
+}
+
+#[test]
+fn it_validates_subscription_cannot_select_conditional_fields() {
+    let input = r#"
+subscription ConditionalSub($condition: Boolean = true) {
+  ticker @include(if: $condition)
+}
+
+type Query {
+  hello: String
+}
+
+type Subscription {
+  ticker: String
+}
+"#;
+
+    let errors = Parser::new()
+        .parse_mixed_validate(input, "schema.graphql")
+        .unwrap_err()
+        .to_string();
+    assert!(
+        errors.contains(
+            "subscription `ConditionalSub` can not specify @skip or @include on root fields"
+        ),
+        "{errors}"
+    );
+    assert!(
+        errors.contains("ticker specifies @skip or @include condition"),
+        "{errors}"
+    );
+}

From 8194481b5098cf8524d57da427bd702ad1e067f6 Mon Sep 17 00:00:00 2001
From: dariuszkuc <9501705+dariuszkuc@users.noreply.github.com>
Date: Fri, 4 Apr 2025 10:32:52 -0500
Subject: [PATCH 2/4] check for all selections

---
 crates/apollo-compiler/src/executable/mod.rs  |  4 +-
 crates/apollo-compiler/src/validation/mod.rs  |  6 +-
 .../src/validation/operation.rs               | 19 ++--
 .../0120_conditional_subscriptions.graphql    | 11 +++
 .../0120_conditional_subscriptions.txt        | 10 ++
 ...subscriptions_with_inline_fragment.graphql | 13 +++
 ...nal_subscriptions_with_inline_fragment.txt | 12 +++
 ..._subscriptions_with_named_fragment.graphql | 15 +++
 ...onal_subscriptions_with_named_fragment.txt | 10 ++
 .../0120_conditional_subscriptions.graphql    | 11 +++
 ...subscriptions_with_inline_fragment.graphql | 13 +++
 ...ubscriptions_with_inline_fragments.graphql | 13 +++
 ..._subscriptions_with_named_fragment.graphql | 15 +++
 .../tests/validation/operation.rs             | 96 -------------------
 14 files changed, 137 insertions(+), 111 deletions(-)
 create mode 100644 crates/apollo-compiler/test_data/diagnostics/0120_conditional_subscriptions.graphql
 create mode 100644 crates/apollo-compiler/test_data/diagnostics/0120_conditional_subscriptions.txt
 create mode 100644 crates/apollo-compiler/test_data/diagnostics/0121_conditional_subscriptions_with_inline_fragment.graphql
 create mode 100644 crates/apollo-compiler/test_data/diagnostics/0121_conditional_subscriptions_with_inline_fragment.txt
 create mode 100644 crates/apollo-compiler/test_data/diagnostics/0122_conditional_subscriptions_with_named_fragment.graphql
 create mode 100644 crates/apollo-compiler/test_data/diagnostics/0122_conditional_subscriptions_with_named_fragment.txt
 create mode 100644 crates/apollo-compiler/test_data/serializer/diagnostics/0120_conditional_subscriptions.graphql
 create mode 100644 crates/apollo-compiler/test_data/serializer/diagnostics/0121_conditional_subscriptions_with_inline_fragment.graphql
 create mode 100644 crates/apollo-compiler/test_data/serializer/diagnostics/0121_conditional_subscriptions_with_inline_fragments.graphql
 create mode 100644 crates/apollo-compiler/test_data/serializer/diagnostics/0122_conditional_subscriptions_with_named_fragment.graphql

diff --git a/crates/apollo-compiler/src/executable/mod.rs b/crates/apollo-compiler/src/executable/mod.rs
index 7cac7781..c5154e39 100644
--- a/crates/apollo-compiler/src/executable/mod.rs
+++ b/crates/apollo-compiler/src/executable/mod.rs
@@ -273,8 +273,8 @@ pub(crate) enum BuildError {
     SubscriptionUsesConditionalSelection {
         /// Name of the operation
         name: Option<Name>,
-        /// Field name that specify @skip or @include directives.
-        field: Name,
+        /// Selection that specify @skip or @include directives.
+        selection: Selection,
     },
 
     #[error("{0}")]
diff --git a/crates/apollo-compiler/src/validation/mod.rs b/crates/apollo-compiler/src/validation/mod.rs
index 63e35722..2eaf6e23 100644
--- a/crates/apollo-compiler/src/validation/mod.rs
+++ b/crates/apollo-compiler/src/validation/mod.rs
@@ -854,10 +854,12 @@ impl ToCliReport for DiagnosticData {
                         format_args!("{field} is an introspection field"),
                     );
                 }
-                ExecutableBuildError::SubscriptionUsesConditionalSelection { field, .. } => {
+                ExecutableBuildError::SubscriptionUsesConditionalSelection {
+                    selection, ..
+                } => {
                     report.with_label_opt(
                         self.location,
-                        format_args!("{field} specifies @skip or @include condition"),
+                        format_args!("{selection} specifies @skip or @include condition"),
                     );
                 }
                 ExecutableBuildError::ConflictingFieldType(inner) => {
diff --git a/crates/apollo-compiler/src/validation/operation.rs b/crates/apollo-compiler/src/validation/operation.rs
index 52bce8a8..aecdc2aa 100644
--- a/crates/apollo-compiler/src/validation/operation.rs
+++ b/crates/apollo-compiler/src/validation/operation.rs
@@ -48,22 +48,19 @@ pub(crate) fn validate_subscription(
         }
 
         // first rule validates that we only have single selection
-        let has_conditional_field = fields
-            .iter()
-            .find(|field| {
-                field
-                    .field
-                    .directives
+        let has_conditional_selection =
+            operation.selection_set.selections.iter().find(|selection| {
+                selection
+                    .directives()
                     .iter()
                     .any(|d| matches!(d.name.as_str(), "skip" | "include"))
-            })
-            .map(|field| field.field);
-        if let Some(conditional_field) = has_conditional_field {
+            });
+        if let Some(conditional_selection) = has_conditional_selection {
             diagnostics.push(
-                conditional_field.location(),
+                operation.location(),
                 executable::BuildError::SubscriptionUsesConditionalSelection {
                     name: operation.name.clone(),
-                    field: conditional_field.name.clone(),
+                    selection: conditional_selection.clone(),
                 },
             );
         }
diff --git a/crates/apollo-compiler/test_data/diagnostics/0120_conditional_subscriptions.graphql b/crates/apollo-compiler/test_data/diagnostics/0120_conditional_subscriptions.graphql
new file mode 100644
index 00000000..e43c3d8a
--- /dev/null
+++ b/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
+}
\ No newline at end of file
diff --git a/crates/apollo-compiler/test_data/diagnostics/0120_conditional_subscriptions.txt b/crates/apollo-compiler/test_data/diagnostics/0120_conditional_subscriptions.txt
new file mode 100644
index 00000000..539c9bd1
--- /dev/null
+++ b/crates/apollo-compiler/test_data/diagnostics/0120_conditional_subscriptions.txt
@@ -0,0 +1,10 @@
+Error: subscription `ConditionalSub` can not specify @skip or @include on root fields
+   ╭─[ 0120_conditional_subscriptions.graphql:1:1 ]
+   │
+ 1 │ ╭─▶ subscription ConditionalSub($condition: Boolean = true) {
+   ┆ ┆   
+ 3 │ ├─▶ }
+   │ │       
+   │ ╰─────── ticker @include(if: $condition) specifies @skip or @include condition
+───╯
+
diff --git a/crates/apollo-compiler/test_data/diagnostics/0121_conditional_subscriptions_with_inline_fragment.graphql b/crates/apollo-compiler/test_data/diagnostics/0121_conditional_subscriptions_with_inline_fragment.graphql
new file mode 100644
index 00000000..91d883a0
--- /dev/null
+++ b/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
+}
\ No newline at end of file
diff --git a/crates/apollo-compiler/test_data/diagnostics/0121_conditional_subscriptions_with_inline_fragment.txt b/crates/apollo-compiler/test_data/diagnostics/0121_conditional_subscriptions_with_inline_fragment.txt
new file mode 100644
index 00000000..5f7f82dc
--- /dev/null
+++ b/crates/apollo-compiler/test_data/diagnostics/0121_conditional_subscriptions_with_inline_fragment.txt
@@ -0,0 +1,12 @@
+Error: subscription `ConditionalInlineSub` can not specify @skip or @include on root fields
+   ╭─[ 0121_conditional_subscriptions_with_inline_fragment.graphql:1:1 ]
+   │
+ 1 │ ╭─▶ subscription ConditionalInlineSub($condition: Boolean = true) {
+   ┆ ┆   
+ 5 │ ├─▶ }
+   │ │       
+   │ ╰─────── ... @include(if: $condition) {
+  ticker
+} specifies @skip or @include condition
+───╯
+
diff --git a/crates/apollo-compiler/test_data/diagnostics/0122_conditional_subscriptions_with_named_fragment.graphql b/crates/apollo-compiler/test_data/diagnostics/0122_conditional_subscriptions_with_named_fragment.graphql
new file mode 100644
index 00000000..f9961b28
--- /dev/null
+++ b/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
+}
\ No newline at end of file
diff --git a/crates/apollo-compiler/test_data/diagnostics/0122_conditional_subscriptions_with_named_fragment.txt b/crates/apollo-compiler/test_data/diagnostics/0122_conditional_subscriptions_with_named_fragment.txt
new file mode 100644
index 00000000..c57c1702
--- /dev/null
+++ b/crates/apollo-compiler/test_data/diagnostics/0122_conditional_subscriptions_with_named_fragment.txt
@@ -0,0 +1,10 @@
+Error: subscription `ConditionalInlineSub` can not specify @skip or @include on root fields
+   ╭─[ 0122_conditional_subscriptions_with_named_fragment.graphql:1:1 ]
+   │
+ 1 │ ╭─▶ subscription ConditionalInlineSub($condition: Boolean = true) {
+   ┆ ┆   
+ 3 │ ├─▶ }
+   │ │       
+   │ ╰─────── ...mySubscription @include(if: $condition) specifies @skip or @include condition
+───╯
+
diff --git a/crates/apollo-compiler/test_data/serializer/diagnostics/0120_conditional_subscriptions.graphql b/crates/apollo-compiler/test_data/serializer/diagnostics/0120_conditional_subscriptions.graphql
new file mode 100644
index 00000000..182e45da
--- /dev/null
+++ b/crates/apollo-compiler/test_data/serializer/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
+}
diff --git a/crates/apollo-compiler/test_data/serializer/diagnostics/0121_conditional_subscriptions_with_inline_fragment.graphql b/crates/apollo-compiler/test_data/serializer/diagnostics/0121_conditional_subscriptions_with_inline_fragment.graphql
new file mode 100644
index 00000000..c0c9b802
--- /dev/null
+++ b/crates/apollo-compiler/test_data/serializer/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
+}
diff --git a/crates/apollo-compiler/test_data/serializer/diagnostics/0121_conditional_subscriptions_with_inline_fragments.graphql b/crates/apollo-compiler/test_data/serializer/diagnostics/0121_conditional_subscriptions_with_inline_fragments.graphql
new file mode 100644
index 00000000..c0c9b802
--- /dev/null
+++ b/crates/apollo-compiler/test_data/serializer/diagnostics/0121_conditional_subscriptions_with_inline_fragments.graphql
@@ -0,0 +1,13 @@
+subscription ConditionalInlineSub($condition: Boolean = true) {
+  ... @include(if: $condition) {
+    ticker
+  }
+}
+
+type Query {
+  hello: String
+}
+
+type Subscription {
+  ticker: String
+}
diff --git a/crates/apollo-compiler/test_data/serializer/diagnostics/0122_conditional_subscriptions_with_named_fragment.graphql b/crates/apollo-compiler/test_data/serializer/diagnostics/0122_conditional_subscriptions_with_named_fragment.graphql
new file mode 100644
index 00000000..a23b43ce
--- /dev/null
+++ b/crates/apollo-compiler/test_data/serializer/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
+}
diff --git a/crates/apollo-compiler/tests/validation/operation.rs b/crates/apollo-compiler/tests/validation/operation.rs
index ea235af2..fb3ca59b 100644
--- a/crates/apollo-compiler/tests/validation/operation.rs
+++ b/crates/apollo-compiler/tests/validation/operation.rs
@@ -218,99 +218,3 @@ type Product {
         "{errors}"
     );
 }
-
-#[test]
-fn it_validates_subscription_cannot_specify_multiple_fields() {
-    let input = r#"
-subscription MultipleSubs {
-  ticker1
-  ticker2
-}
-
-type Query {
-  hello: String
-}
-
-type Subscription {
-  ticker1: String
-  ticker2: String
-}
-"#;
-
-    let errors = Parser::new()
-        .parse_mixed_validate(input, "schema.graphql")
-        .unwrap_err()
-        .to_string();
-    assert!(
-        errors.contains("subscription `MultipleSubs` can only have one root field"),
-        "{errors}"
-    );
-    assert!(
-        errors.contains("There are 2 root fields: ticker1, ticker2. This is not allowed."),
-        "{errors}"
-    );
-}
-
-#[test]
-fn it_validates_subscription_cannot_select_introspection_fields() {
-    let input = r#"
-subscription IntrospectionSub {
-  __typename
-}
-
-type Query {
-  hello: String
-}
-
-type Subscription {
-  ticker: String
-}
-"#;
-
-    let errors = Parser::new()
-        .parse_mixed_validate(input, "schema.graphql")
-        .unwrap_err()
-        .to_string();
-    assert!(
-        errors.contains(
-            "subscription `IntrospectionSub` can not have an introspection field as a root field"
-        ),
-        "{errors}"
-    );
-    assert!(
-        errors.contains("__typename is an introspection field"),
-        "{errors}"
-    );
-}
-
-#[test]
-fn it_validates_subscription_cannot_select_conditional_fields() {
-    let input = r#"
-subscription ConditionalSub($condition: Boolean = true) {
-  ticker @include(if: $condition)
-}
-
-type Query {
-  hello: String
-}
-
-type Subscription {
-  ticker: String
-}
-"#;
-
-    let errors = Parser::new()
-        .parse_mixed_validate(input, "schema.graphql")
-        .unwrap_err()
-        .to_string();
-    assert!(
-        errors.contains(
-            "subscription `ConditionalSub` can not specify @skip or @include on root fields"
-        ),
-        "{errors}"
-    );
-    assert!(
-        errors.contains("ticker specifies @skip or @include condition"),
-        "{errors}"
-    );
-}

From de3695d2c6f74cae63a1a389e626060c1538411f Mon Sep 17 00:00:00 2001
From: Simon Sapin <simon@apollographql.com>
Date: Fri, 4 Apr 2025 20:14:46 +0200
Subject: [PATCH 3/4] Subscription root validation:

* Recur into inline fragments and fragment spread
* Point to the faulty directive
* Skip printing an entire selection set
---
 crates/apollo-compiler/src/executable/mod.rs  |  2 --
 crates/apollo-compiler/src/validation/mod.rs  |  9 ++------
 .../src/validation/operation.rs               | 19 +---------------
 .../src/validation/selection.rs               | 22 +++++++++++++++++--
 .../0120_conditional_subscriptions.txt        | 10 ++++-----
 ...nal_subscriptions_with_inline_fragment.txt | 12 ++++------
 ...onal_subscriptions_with_named_fragment.txt | 10 ++++-----
 ...bscriptions_inside_inline_fragment.graphql | 13 +++++++++++
 ...l_subscriptions_inside_inline_fragment.txt |  8 +++++++
 ...ubscriptions_inside_named_fragment.graphql | 15 +++++++++++++
 ...al_subscriptions_inside_named_fragment.txt |  8 +++++++
 ...bscriptions_inside_inline_fragment.graphql | 13 +++++++++++
 ...ubscriptions_inside_named_fragment.graphql | 15 +++++++++++++
 13 files changed, 107 insertions(+), 49 deletions(-)
 create mode 100644 crates/apollo-compiler/test_data/diagnostics/0123_conditional_subscriptions_inside_inline_fragment.graphql
 create mode 100644 crates/apollo-compiler/test_data/diagnostics/0123_conditional_subscriptions_inside_inline_fragment.txt
 create mode 100644 crates/apollo-compiler/test_data/diagnostics/0124_conditional_subscriptions_inside_named_fragment.graphql
 create mode 100644 crates/apollo-compiler/test_data/diagnostics/0124_conditional_subscriptions_inside_named_fragment.txt
 create mode 100644 crates/apollo-compiler/test_data/serializer/diagnostics/0123_conditional_subscriptions_inside_inline_fragment.graphql
 create mode 100644 crates/apollo-compiler/test_data/serializer/diagnostics/0124_conditional_subscriptions_inside_named_fragment.graphql

diff --git a/crates/apollo-compiler/src/executable/mod.rs b/crates/apollo-compiler/src/executable/mod.rs
index c5154e39..36ff1b16 100644
--- a/crates/apollo-compiler/src/executable/mod.rs
+++ b/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}")]
diff --git a/crates/apollo-compiler/src/validation/mod.rs b/crates/apollo-compiler/src/validation/mod.rs
index 2eaf6e23..39dc67ad 100644
--- a/crates/apollo-compiler/src/validation/mod.rs
+++ b/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 {
diff --git a/crates/apollo-compiler/src/validation/operation.rs b/crates/apollo-compiler/src/validation/operation.rs
index aecdc2aa..3e50c420 100644
--- a/crates/apollo-compiler/src/validation/operation.rs
+++ b/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(),
-                },
-            );
-        }
     }
 }
 
diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs
index da07bf78..1f246f5e 100644
--- a/crates/apollo-compiler/src/validation/selection.rs
+++ b/crates/apollo-compiler/src/validation/selection.rs
@@ -67,6 +67,7 @@ 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();
@@ -74,6 +75,20 @@ pub(crate) fn expand_selections<'doc>(
 
     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(
diff --git a/crates/apollo-compiler/test_data/diagnostics/0120_conditional_subscriptions.txt b/crates/apollo-compiler/test_data/diagnostics/0120_conditional_subscriptions.txt
index 539c9bd1..1c9bcf3b 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0120_conditional_subscriptions.txt
+++ b/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
 ───╯
 
diff --git a/crates/apollo-compiler/test_data/diagnostics/0121_conditional_subscriptions_with_inline_fragment.txt b/crates/apollo-compiler/test_data/diagnostics/0121_conditional_subscriptions_with_inline_fragment.txt
index 5f7f82dc..4be10ccf 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0121_conditional_subscriptions_with_inline_fragment.txt
+++ b/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
 ───╯
 
diff --git a/crates/apollo-compiler/test_data/diagnostics/0122_conditional_subscriptions_with_named_fragment.txt b/crates/apollo-compiler/test_data/diagnostics/0122_conditional_subscriptions_with_named_fragment.txt
index c57c1702..db28ea3f 100644
--- a/crates/apollo-compiler/test_data/diagnostics/0122_conditional_subscriptions_with_named_fragment.txt
+++ b/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
 ───╯
 
diff --git a/crates/apollo-compiler/test_data/diagnostics/0123_conditional_subscriptions_inside_inline_fragment.graphql b/crates/apollo-compiler/test_data/diagnostics/0123_conditional_subscriptions_inside_inline_fragment.graphql
new file mode 100644
index 00000000..011599e7
--- /dev/null
+++ b/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
+}
\ No newline at end of file
diff --git a/crates/apollo-compiler/test_data/diagnostics/0123_conditional_subscriptions_inside_inline_fragment.txt b/crates/apollo-compiler/test_data/diagnostics/0123_conditional_subscriptions_inside_inline_fragment.txt
new file mode 100644
index 00000000..b0c56481
--- /dev/null
+++ b/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
+───╯
+
diff --git a/crates/apollo-compiler/test_data/diagnostics/0124_conditional_subscriptions_inside_named_fragment.graphql b/crates/apollo-compiler/test_data/diagnostics/0124_conditional_subscriptions_inside_named_fragment.graphql
new file mode 100644
index 00000000..e6553a09
--- /dev/null
+++ b/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
+}
\ No newline at end of file
diff --git a/crates/apollo-compiler/test_data/diagnostics/0124_conditional_subscriptions_inside_named_fragment.txt b/crates/apollo-compiler/test_data/diagnostics/0124_conditional_subscriptions_inside_named_fragment.txt
new file mode 100644
index 00000000..48e295de
--- /dev/null
+++ b/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
+───╯
+
diff --git a/crates/apollo-compiler/test_data/serializer/diagnostics/0123_conditional_subscriptions_inside_inline_fragment.graphql b/crates/apollo-compiler/test_data/serializer/diagnostics/0123_conditional_subscriptions_inside_inline_fragment.graphql
new file mode 100644
index 00000000..5a3f1c1f
--- /dev/null
+++ b/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
+}
diff --git a/crates/apollo-compiler/test_data/serializer/diagnostics/0124_conditional_subscriptions_inside_named_fragment.graphql b/crates/apollo-compiler/test_data/serializer/diagnostics/0124_conditional_subscriptions_inside_named_fragment.graphql
new file mode 100644
index 00000000..55fe965a
--- /dev/null
+++ b/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
+}

From a11a5308deed01bd01aba403f1a8fa443d228de7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ren=C3=A9e=20Kooi?= <renee.kooi@apollographql.com>
Date: Tue, 8 Apr 2025 13:22:37 +0200
Subject: [PATCH 4/4] Use a single special-purpose walk in subscription
 validation

---
 crates/apollo-compiler/src/validation/mod.rs  |   7 +-
 .../src/validation/operation.rs               | 123 +++++--
 .../src/validation/selection.rs               |  24 +-
 .../src/validation/variable.rs                |   9 +-
 ...ubscription_conditions_not_at_root.graphql |  21 ++
 ...17_subscription_conditions_not_at_root.txt | 302 ++++++++++++++++++
 ...ubscription_conditions_not_at_root.graphql |  21 ++
 7 files changed, 451 insertions(+), 56 deletions(-)
 create mode 100644 crates/apollo-compiler/test_data/ok/0117_subscription_conditions_not_at_root.graphql
 create mode 100644 crates/apollo-compiler/test_data/ok/0117_subscription_conditions_not_at_root.txt
 create mode 100644 crates/apollo-compiler/test_data/serializer/ok/0117_subscription_conditions_not_at_root.graphql

diff --git a/crates/apollo-compiler/src/validation/mod.rs b/crates/apollo-compiler/src/validation/mod.rs
index 39dc67ad..78bb8789 100644
--- a/crates/apollo-compiler/src/validation/mod.rs
+++ b/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;
diff --git a/crates/apollo-compiler/src/validation/operation.rs b/crates/apollo-compiler/src/validation/operation.rs
index 3e50c420..a598e47c 100644
--- a/crates/apollo-compiler/src/validation/operation.rs
+++ b/crates/apollo-compiler/src/validation/operation.rs
@@ -1,52 +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),
-            Some((operation, diagnostics)),
-        );
+    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,
+            },
+        );
     }
 }
 
diff --git a/crates/apollo-compiler/src/validation/selection.rs b/crates/apollo-compiler/src/validation/selection.rs
index 1f246f5e..ba3de7b7 100644
--- a/crates/apollo-compiler/src/validation/selection.rs
+++ b/crates/apollo-compiler/src/validation/selection.rs
@@ -64,10 +64,9 @@ 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>,
-    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();
@@ -75,20 +74,6 @@ pub(crate) fn expand_selections<'doc>(
 
     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))
@@ -535,11 +520,8 @@ 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,
-            None,
-        ))
+        self.alloc
+            .alloc(expand_selections(&self.document.fragments, selection_sets))
     }
 
     pub(crate) fn validate_operation(
diff --git a/crates/apollo-compiler/src/validation/variable.rs b/crates/apollo-compiler/src/validation/variable.rs
index 0976dd4d..53212c3d 100644
--- a/crates/apollo-compiler/src/validation/variable.rs
+++ b/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,
diff --git a/crates/apollo-compiler/test_data/ok/0117_subscription_conditions_not_at_root.graphql b/crates/apollo-compiler/test_data/ok/0117_subscription_conditions_not_at_root.graphql
new file mode 100644
index 00000000..3a1a1ca1
--- /dev/null
+++ b/crates/apollo-compiler/test_data/ok/0117_subscription_conditions_not_at_root.graphql
@@ -0,0 +1,21 @@
+subscription ConditionalSub($includeContent: Boolean = true, $small: Boolean = true) {
+    messages {
+        username
+        text @include(if: $includeContent)
+        avatar @skip(if: $small)
+    }
+}
+
+type Query {
+    hello: String
+}
+
+type Message {
+    username: String
+    text: String
+    avatar: String
+}
+
+type Subscription {
+    messages: Message
+}
diff --git a/crates/apollo-compiler/test_data/ok/0117_subscription_conditions_not_at_root.txt b/crates/apollo-compiler/test_data/ok/0117_subscription_conditions_not_at_root.txt
new file mode 100644
index 00000000..5824191a
--- /dev/null
+++ b/crates/apollo-compiler/test_data/ok/0117_subscription_conditions_not_at_root.txt
@@ -0,0 +1,302 @@
+Schema {
+    sources: {
+        1: SourceFile {
+            path: "built_in.graphql",
+            source_text: include_str!("built_in.graphql"),
+        },
+        46: SourceFile {
+            path: "0117_subscription_conditions_not_at_root.graphql",
+            source_text: "subscription ConditionalSub($includeContent: Boolean = true, $small: Boolean = true) {\n    messages {\n        username\n        text @include(if: $includeContent)\n        avatar @skip(if: $small)\n    }\n}\n\ntype Query {\n    hello: String\n}\n\ntype Message {\n    username: String\n    text: String\n    avatar: String\n}\n\ntype Subscription {\n    messages: Message\n}\n",
+        },
+    },
+    schema_definition: SchemaDefinition {
+        description: None,
+        directives: [],
+        query: Some(
+            ComponentName {
+                origin: Definition,
+                name: "Query",
+            },
+        ),
+        mutation: None,
+        subscription: Some(
+            ComponentName {
+                origin: Definition,
+                name: "Subscription",
+            },
+        ),
+    },
+    directive_definitions: {
+        "skip": built_in_directive!("skip"),
+        "include": built_in_directive!("include"),
+        "deprecated": built_in_directive!("deprecated"),
+        "specifiedBy": built_in_directive!("specifiedBy"),
+    },
+    types: {
+        "__Schema": built_in_type!("__Schema"),
+        "__Type": built_in_type!("__Type"),
+        "__TypeKind": built_in_type!("__TypeKind"),
+        "__Field": built_in_type!("__Field"),
+        "__InputValue": built_in_type!("__InputValue"),
+        "__EnumValue": built_in_type!("__EnumValue"),
+        "__Directive": built_in_type!("__Directive"),
+        "__DirectiveLocation": built_in_type!("__DirectiveLocation"),
+        "String": built_in_type!("String"),
+        "Boolean": built_in_type!("Boolean"),
+        "Query": Object(
+            204..236 @46 ObjectType {
+                description: None,
+                name: "Query",
+                implements_interfaces: {},
+                directives: [],
+                fields: {
+                    "hello": Component {
+                        origin: Definition,
+                        node: 221..234 @46 FieldDefinition {
+                            description: None,
+                            name: "hello",
+                            arguments: [],
+                            ty: Named(
+                                "String",
+                            ),
+                            directives: [],
+                        },
+                    },
+                },
+            },
+        ),
+        "Message": Object(
+            238..311 @46 ObjectType {
+                description: None,
+                name: "Message",
+                implements_interfaces: {},
+                directives: [],
+                fields: {
+                    "username": Component {
+                        origin: Definition,
+                        node: 257..273 @46 FieldDefinition {
+                            description: None,
+                            name: "username",
+                            arguments: [],
+                            ty: Named(
+                                "String",
+                            ),
+                            directives: [],
+                        },
+                    },
+                    "text": Component {
+                        origin: Definition,
+                        node: 278..290 @46 FieldDefinition {
+                            description: None,
+                            name: "text",
+                            arguments: [],
+                            ty: Named(
+                                "String",
+                            ),
+                            directives: [],
+                        },
+                    },
+                    "avatar": Component {
+                        origin: Definition,
+                        node: 295..309 @46 FieldDefinition {
+                            description: None,
+                            name: "avatar",
+                            arguments: [],
+                            ty: Named(
+                                "String",
+                            ),
+                            directives: [],
+                        },
+                    },
+                },
+            },
+        ),
+        "Subscription": Object(
+            313..356 @46 ObjectType {
+                description: None,
+                name: "Subscription",
+                implements_interfaces: {},
+                directives: [],
+                fields: {
+                    "messages": Component {
+                        origin: Definition,
+                        node: 337..354 @46 FieldDefinition {
+                            description: None,
+                            name: "messages",
+                            arguments: [],
+                            ty: Named(
+                                "Message",
+                            ),
+                            directives: [],
+                        },
+                    },
+                },
+            },
+        ),
+    },
+}
+ExecutableDocument {
+    sources: {
+        1: SourceFile {
+            path: "built_in.graphql",
+            source_text: include_str!("built_in.graphql"),
+        },
+        46: SourceFile {
+            path: "0117_subscription_conditions_not_at_root.graphql",
+            source_text: "subscription ConditionalSub($includeContent: Boolean = true, $small: Boolean = true) {\n    messages {\n        username\n        text @include(if: $includeContent)\n        avatar @skip(if: $small)\n    }\n}\n\ntype Query {\n    hello: String\n}\n\ntype Message {\n    username: String\n    text: String\n    avatar: String\n}\n\ntype Subscription {\n    messages: Message\n}\n",
+        },
+    },
+    operations: OperationMap {
+        anonymous: None,
+        named: {
+            "ConditionalSub": 0..202 @46 Operation {
+                operation_type: Subscription,
+                name: Some(
+                    "ConditionalSub",
+                ),
+                variables: [
+                    28..59 @46 VariableDefinition {
+                        name: "includeContent",
+                        ty: 45..52 @46 Named(
+                            "Boolean",
+                        ),
+                        default_value: Some(
+                            55..59 @46 Boolean(
+                                true,
+                            ),
+                        ),
+                        directives: [],
+                    },
+                    61..83 @46 VariableDefinition {
+                        name: "small",
+                        ty: 69..76 @46 Named(
+                            "Boolean",
+                        ),
+                        default_value: Some(
+                            79..83 @46 Boolean(
+                                true,
+                            ),
+                        ),
+                        directives: [],
+                    },
+                ],
+                directives: [],
+                selection_set: SelectionSet {
+                    ty: "Subscription",
+                    selections: [
+                        Field(
+                            91..200 @46 Field {
+                                definition: 337..354 @46 FieldDefinition {
+                                    description: None,
+                                    name: "messages",
+                                    arguments: [],
+                                    ty: Named(
+                                        "Message",
+                                    ),
+                                    directives: [],
+                                },
+                                alias: None,
+                                name: "messages",
+                                arguments: [],
+                                directives: [],
+                                selection_set: SelectionSet {
+                                    ty: "Message",
+                                    selections: [
+                                        Field(
+                                            110..118 @46 Field {
+                                                definition: 257..273 @46 FieldDefinition {
+                                                    description: None,
+                                                    name: "username",
+                                                    arguments: [],
+                                                    ty: Named(
+                                                        "String",
+                                                    ),
+                                                    directives: [],
+                                                },
+                                                alias: None,
+                                                name: "username",
+                                                arguments: [],
+                                                directives: [],
+                                                selection_set: SelectionSet {
+                                                    ty: "String",
+                                                    selections: [],
+                                                },
+                                            },
+                                        ),
+                                        Field(
+                                            127..161 @46 Field {
+                                                definition: 278..290 @46 FieldDefinition {
+                                                    description: None,
+                                                    name: "text",
+                                                    arguments: [],
+                                                    ty: Named(
+                                                        "String",
+                                                    ),
+                                                    directives: [],
+                                                },
+                                                alias: None,
+                                                name: "text",
+                                                arguments: [],
+                                                directives: [
+                                                    132..161 @46 Directive {
+                                                        name: "include",
+                                                        arguments: [
+                                                            141..160 @46 Argument {
+                                                                name: "if",
+                                                                value: 145..160 @46 Variable(
+                                                                    "includeContent",
+                                                                ),
+                                                            },
+                                                        ],
+                                                    },
+                                                ],
+                                                selection_set: SelectionSet {
+                                                    ty: "String",
+                                                    selections: [],
+                                                },
+                                            },
+                                        ),
+                                        Field(
+                                            170..194 @46 Field {
+                                                definition: 295..309 @46 FieldDefinition {
+                                                    description: None,
+                                                    name: "avatar",
+                                                    arguments: [],
+                                                    ty: Named(
+                                                        "String",
+                                                    ),
+                                                    directives: [],
+                                                },
+                                                alias: None,
+                                                name: "avatar",
+                                                arguments: [],
+                                                directives: [
+                                                    177..194 @46 Directive {
+                                                        name: "skip",
+                                                        arguments: [
+                                                            183..193 @46 Argument {
+                                                                name: "if",
+                                                                value: 187..193 @46 Variable(
+                                                                    "small",
+                                                                ),
+                                                            },
+                                                        ],
+                                                    },
+                                                ],
+                                                selection_set: SelectionSet {
+                                                    ty: "String",
+                                                    selections: [],
+                                                },
+                                            },
+                                        ),
+                                    ],
+                                },
+                            },
+                        ),
+                    ],
+                },
+            },
+        },
+    },
+    fragments: {},
+}
diff --git a/crates/apollo-compiler/test_data/serializer/ok/0117_subscription_conditions_not_at_root.graphql b/crates/apollo-compiler/test_data/serializer/ok/0117_subscription_conditions_not_at_root.graphql
new file mode 100644
index 00000000..a5dc9ad4
--- /dev/null
+++ b/crates/apollo-compiler/test_data/serializer/ok/0117_subscription_conditions_not_at_root.graphql
@@ -0,0 +1,21 @@
+subscription ConditionalSub($includeContent: Boolean = true, $small: Boolean = true) {
+  messages {
+    username
+    text @include(if: $includeContent)
+    avatar @skip(if: $small)
+  }
+}
+
+type Query {
+  hello: String
+}
+
+type Message {
+  username: String
+  text: String
+  avatar: String
+}
+
+type Subscription {
+  messages: Message
+}