Repository: apollographql/apollo-rs (rust)

Issue:
Operations with empty queries should not parse
## Description
According to the GraphQL grammar spec variable definitions should always contain at least one variable meaning that `query Op() { field }` should be invalid.

See [spec](https://github.com/graphql/graphql-spec/blob/afc0a35d271ba9502c3c68aeda6e6c6fbc223774/spec/Appendix%20B%20--%20Grammar%20Summary.md?plain=1#L212).

### Steps to reproduce
Parse this example query https://github.com/apollographql/apollo-rs/compare/main...allancalix:apollo-rs:invalid-query

### Expected result
The query does not return a valid parse tree and should return errors.

### Actual result
The query is treated as having an empty set of variables.

### Environment
- `apollo-rs` crate: apollo-parser
- Crate version: `HEAD`

Pull request:
fix(parser): variable definitions cannot be empty
fixes #546 

Patch:
diff --git a/crates/apollo-compiler/src/database/hir.rs b/crates/apollo-compiler/src/database/hir.rs
index 08b840c..528c31b 100644
--- a/crates/apollo-compiler/src/database/hir.rs
+++ b/crates/apollo-compiler/src/database/hir.rs
@@ -1286,11 +1286,31 @@ impl SelectionSet {
 
     /// Returns true if all the [`Selection`]s in this selection set are themselves introspections.
     pub fn is_introspection(&self, db: &dyn HirDatabase) -> bool {
-        self.selection().iter().all(|selection| match selection {
-            Selection::Field(field) => field.is_introspection(),
-            Selection::FragmentSpread(spread) => spread.is_introspection(db),
-            Selection::InlineFragment(inline) => inline.is_introspection(db),
-        })
+        fn is_introspection_impl(
+            db: &dyn HirDatabase,
+            set: &SelectionSet,
+            seen_fragments: &mut HashSet<Arc<FragmentDefinition>>,
+        ) -> bool {
+            set.selection().iter().all(|selection| match selection {
+                Selection::Field(field) => field.is_introspection(),
+                Selection::FragmentSpread(spread) => {
+                    let maybe_fragment = spread.fragment(db);
+                    maybe_fragment.map_or(false, |fragment| {
+                        if seen_fragments.contains(&fragment) {
+                            false
+                        } else {
+                            seen_fragments.insert(Arc::clone(&fragment));
+                            is_introspection_impl(db, &fragment.selection_set, seen_fragments)
+                        }
+                    })
+                }
+                Selection::InlineFragment(inline) => {
+                    is_introspection_impl(db, &inline.selection_set, seen_fragments)
+                }
+            })
+        }
+
+        is_introspection_impl(db, self, &mut HashSet::new())
     }
 
     /// Create a selection set for the concatenation of two selection sets' fields.
diff --git a/crates/apollo-compiler/src/validation/operation.rs b/crates/apollo-compiler/src/validation/operation.rs
index 89f859e..da663f0 100644
--- a/crates/apollo-compiler/src/validation/operation.rs
+++ b/crates/apollo-compiler/src/validation/operation.rs
@@ -174,12 +174,7 @@ pub fn validate_query_operations(
             queries
                 .iter()
                 .filter_map(|op| {
-                    let has_cycles = op
-                        .fragment_references(db.upcast())
-                        .iter()
-                        .any(|fragment| !db.validate_fragment_cycles(fragment.clone()).is_empty());
-
-                    if !has_cycles && !op.is_introspection(db.upcast()) {
+                    if !op.is_introspection(db.upcast()) {
                         let diagnostic = ApolloDiagnostic::new(
                             db,
                             op.loc().into(),
diff --git a/crates/apollo-parser/src/parser/grammar/variable.rs b/crates/apollo-parser/src/parser/grammar/variable.rs
index 0ec9681..319de0f 100644
--- a/crates/apollo-parser/src/parser/grammar/variable.rs
+++ b/crates/apollo-parser/src/parser/grammar/variable.rs
@@ -11,9 +11,10 @@ pub(crate) fn variable_definitions(p: &mut Parser) {
     let _g = p.start_node(SyntaxKind::VARIABLE_DEFINITIONS);
     p.bump(S!['(']);
 
-    // TODO @lrlna error: expected a variable definition to follow an opening brace
     if let Some(T![$]) = p.peek() {
         variable_definition(p, false);
+    } else {
+        p.err("expected a Variable Definition")
     }
     p.expect(T![')'], S![')']);
 }


Test patch:
diff --git a/crates/apollo-parser/test_data/parser/err/0047_empty_variable_definition.graphql b/crates/apollo-parser/test_data/parser/err/0047_empty_variable_definition.graphql
new file mode 100644
index 0000000..a936113
--- /dev/null
+++ b/crates/apollo-parser/test_data/parser/err/0047_empty_variable_definition.graphql
@@ -0,0 +1,3 @@
+query Op() {
+  field
+}
\ No newline at end of file
diff --git a/crates/apollo-parser/test_data/parser/err/0047_empty_variable_definition.txt b/crates/apollo-parser/test_data/parser/err/0047_empty_variable_definition.txt
new file mode 100644
index 0000000..fa1a26b
--- /dev/null
+++ b/crates/apollo-parser/test_data/parser/err/0047_empty_variable_definition.txt
@@ -0,0 +1,21 @@
+- DOCUMENT@0..22
+    - OPERATION_DEFINITION@0..22
+        - OPERATION_TYPE@0..5
+            - query_KW@0..5 "query"
+        - WHITESPACE@5..6 " "
+        - NAME@6..8
+            - IDENT@6..8 "Op"
+        - VARIABLE_DEFINITIONS@8..10
+            - L_PAREN@8..9 "("
+            - R_PAREN@9..10 ")"
+        - WHITESPACE@10..11 " "
+        - SELECTION_SET@11..22
+            - L_CURLY@11..12 "{"
+            - WHITESPACE@12..15 "\n  "
+            - FIELD@15..20
+                - NAME@15..20
+                    - IDENT@15..20 "field"
+            - WHITESPACE@20..21 "\n"
+            - R_CURLY@21..22 "}"
+- ERROR@9:10 "expected a Variable Definition" )
+recursion limit: 4096, high: 1
\ No newline at end of file