Repository: apollographql/apollo-rs (rust)

Issue:
infinite loop when giving a bad input string value
## Description
When the subgraph schema is malformed or contains a typo rover crash with a timeout without any understandable errors.

### Steps to reproduce

use this schema:

```graphql
extend schema
  @link(url: "https://specs.apollo.dev/federation/v2.0",
        import: ["@key", "@external])

```

Look at the unclosed `"@external` 


Pull request:
fix(lexer,parser): do not run in infinite loop when we're in an unclosed string value
I don't know if it's the right way to fix it, but we already have this kind of issues with value if we don't pop error so I added a parameter to pop the token if needed to not have an infinite loop again there.
close #266 

Patch:
diff --git a/crates/apollo-compiler/src/diagnostics.rs b/crates/apollo-compiler/src/diagnostics.rs
index e741d54..f1fca33 100644
--- a/crates/apollo-compiler/src/diagnostics.rs
+++ b/crates/apollo-compiler/src/diagnostics.rs
@@ -59,9 +59,9 @@ impl ApolloDiagnostic {
     }
 }
 
-impl fmt::Display for ApolloDiagnostic {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        let report = match self {
+impl ApolloDiagnostic {
+    pub fn report(&self) -> Report {
+        match self {
             ApolloDiagnostic::MissingIdent(diagnostic) => Report::new(diagnostic.clone()),
             ApolloDiagnostic::UniqueDefinition(diagnostic) => Report::new(diagnostic.clone()),
             ApolloDiagnostic::SingleRootField(diagnostic) => Report::new(diagnostic.clone()),
@@ -84,9 +84,13 @@ impl fmt::Display for ApolloDiagnostic {
             ApolloDiagnostic::OutputType(diagnostic) => Report::new(diagnostic.clone()),
             ApolloDiagnostic::ObjectType(diagnostic) => Report::new(diagnostic.clone()),
             ApolloDiagnostic::UndefinedField(diagnostic) => Report::new(diagnostic.clone()),
-        };
+        }
+    }
+}
 
-        writeln!(f, "{:?}", report)
+impl fmt::Display for ApolloDiagnostic {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        writeln!(f, "{:?}", self.report())
     }
 }
 
diff --git a/crates/apollo-compiler/src/lib.rs b/crates/apollo-compiler/src/lib.rs
index 53cf45c..e90bf4a 100644
--- a/crates/apollo-compiler/src/lib.rs
+++ b/crates/apollo-compiler/src/lib.rs
@@ -14,7 +14,7 @@ pub use queries::{
     values,
 };
 
-use diagnostics::ApolloDiagnostic;
+pub use diagnostics::ApolloDiagnostic;
 use validation::Validator;
 
 pub struct ApolloCompiler {
diff --git a/crates/apollo-compiler/src/queries/database.rs b/crates/apollo-compiler/src/queries/database.rs
index 5b77a1b..9a3de9a 100644
--- a/crates/apollo-compiler/src/queries/database.rs
+++ b/crates/apollo-compiler/src/queries/database.rs
@@ -79,6 +79,8 @@ pub trait SourceDatabase {
 
     fn find_object_type_by_name(&self, name: String) -> Option<Arc<ObjectTypeDefinition>>;
 
+    fn find_union_by_name(&self, name: String) -> Option<Arc<UnionTypeDefinition>>;
+
     fn find_interface(&self, id: Uuid) -> Option<Arc<InterfaceTypeDefinition>>;
 
     fn find_interface_by_name(&self, name: String) -> Option<Arc<InterfaceTypeDefinition>>;
@@ -344,7 +346,10 @@ fn find_operation(db: &dyn SourceDatabase, id: Uuid) -> Option<Arc<OperationDefi
     })
 }
 
-fn find_operation_by_name(db: &dyn SourceDatabase, name: String) -> Option<Arc<OperationDefinition>> {
+fn find_operation_by_name(
+    db: &dyn SourceDatabase,
+    name: String,
+) -> Option<Arc<OperationDefinition>> {
     db.operations().iter().find_map(|op| {
         if let Some(n) = op.name() {
             if n == name {
@@ -596,6 +601,15 @@ fn unions(db: &dyn SourceDatabase) -> Arc<Vec<UnionTypeDefinition>> {
     Arc::new(unions)
 }
 
+fn find_union_by_name(db: &dyn SourceDatabase, name: String) -> Option<Arc<UnionTypeDefinition>> {
+    db.unions().iter().find_map(|union| {
+        if name == union.name() {
+            return Some(Arc::new(union.clone()));
+        }
+        None
+    })
+}
+
 fn interfaces(db: &dyn SourceDatabase) -> Arc<Vec<InterfaceTypeDefinition>> {
     let interfaces = db
         .definitions()
diff --git a/crates/apollo-compiler/src/queries/values.rs b/crates/apollo-compiler/src/queries/values.rs
index ee28fdc..0ed8993 100644
--- a/crates/apollo-compiler/src/queries/values.rs
+++ b/crates/apollo-compiler/src/queries/values.rs
@@ -871,7 +871,9 @@ impl SelectionSet {
             .selection()
             .iter()
             .filter_map(|sel| match sel {
-                Selection::FragmentSpread(fragment_spread) => return Some(fragment_spread.as_ref().clone()),
+                Selection::FragmentSpread(fragment_spread) => {
+                    return Some(fragment_spread.as_ref().clone())
+                }
                 _ => None,
             })
             .collect();
@@ -1407,31 +1409,36 @@ pub struct InputValueDefinition {
 }
 
 impl InputValueDefinition {
-    // Get a reference to input value definition's name.
+    /// Get a reference to input value definition's name.
     pub fn name(&self) -> &str {
         self.name.as_ref()
     }
 
-    // Get a reference to input value definition's directives.
+    /// Get a reference to input value definition's directives.
     pub fn directives(&self) -> &[Directive] {
         self.directives.as_ref()
     }
 
-    // Get a reference to SyntaxNodePtr of the current HIR node.
+    /// Get a reference to SyntaxNodePtr of the current HIR node.
     pub fn ast_ptr(&self) -> Option<&SyntaxNodePtr> {
         self.ast_ptr.as_ref()
     }
 
-    // Get current HIR node's AST node.
+    /// Get current HIR node's AST node.
     pub fn ast_node(&self, db: &dyn SourceDatabase) -> Option<SyntaxNode> {
         self.ast_ptr()
             .map(|ptr| ptr.to_node(db.document().deref().syntax()))
     }
 
-    // Get a reference to input value definition's type.
+    /// Get a reference to input value definition's type.
     pub fn ty(&self) -> &Type {
         &self.ty
     }
+
+    /// Get a refernce to inpul value definition's default_value.
+    pub fn default_value(&self) -> Option<&Value> {
+        self.default_value.as_ref()
+    }
 }
 
 #[derive(Clone, Debug, Hash, PartialEq, Eq)]
diff --git a/crates/apollo-parser/src/lexer/mod.rs b/crates/apollo-parser/src/lexer/mod.rs
index d357633..1334260 100644
--- a/crates/apollo-parser/src/lexer/mod.rs
+++ b/crates/apollo-parser/src/lexer/mod.rs
@@ -160,6 +160,17 @@ impl Cursor<'_> {
                     was_backslash = c == '\\';
                 }
 
+                if !buf.ends_with('"') {
+                    // If it's an unclosed string then take all remaining tokens into this string value
+                    while !self.is_eof() {
+                        buf.push(self.bump().unwrap());
+                    }
+                    self.add_err(Error::new(
+                        "expected a closing \" for the string value",
+                        buf.clone(),
+                    ));
+                }
+
                 if let Some(mut err) = self.err() {
                     err.data = buf;
                     return Err(err);
diff --git a/crates/apollo-parser/src/parser/grammar/argument.rs b/crates/apollo-parser/src/parser/grammar/argument.rs
index 559bdf5..0a6ae9f 100644
--- a/crates/apollo-parser/src/parser/grammar/argument.rs
+++ b/crates/apollo-parser/src/parser/grammar/argument.rs
@@ -13,7 +13,7 @@ pub(crate) fn argument(p: &mut Parser, mut is_argument: bool) {
         name::name(p);
         if let Some(T![:]) = p.peek() {
             p.bump(S![:]);
-            value::value(p);
+            value::value(p, false);
             is_argument = true;
             if p.peek().is_some() {
                 guard.finish_node();
diff --git a/crates/apollo-parser/src/parser/grammar/value.rs b/crates/apollo-parser/src/parser/grammar/value.rs
index 8e57fec..bd97271 100644
--- a/crates/apollo-parser/src/parser/grammar/value.rs
+++ b/crates/apollo-parser/src/parser/grammar/value.rs
@@ -15,7 +15,7 @@ use crate::{
 ///     EnumValue
 ///     ListValue
 ///     ObjectValue
-pub(crate) fn value(p: &mut Parser) {
+pub(crate) fn value(p: &mut Parser, pop_on_error: bool) {
     match p.peek() {
         Some(T![$]) => variable::variable(p),
         Some(TokenKind::Int) => {
@@ -50,7 +50,22 @@ pub(crate) fn value(p: &mut Parser) {
         }
         Some(T!['[']) => list_value(p),
         Some(T!['{']) => object_value(p),
-        _ => p.err("expected a valid Value"),
+        Some(token) => {
+            let error_message = format!("expected a valid Value got {token:?}");
+            if pop_on_error {
+                p.err_and_pop(&error_message);
+            } else {
+                p.err(&error_message);
+            }
+        }
+        _ => {
+            let error_message = "expected a valid Value";
+            if pop_on_error {
+                p.err_and_pop(error_message);
+            } else {
+                p.err(error_message);
+            }
+        }
     }
 }
 /// See: https://spec.graphql.org/October2021/#EnumValue
@@ -81,8 +96,10 @@ pub(crate) fn list_value(p: &mut Parser) {
         if node == T![']'] {
             p.bump(S![']']);
             break;
+        } else if node == TokenKind::Eof {
+            break;
         } else {
-            value(p);
+            value(p, true);
         }
     }
 }
@@ -123,7 +140,7 @@ pub(crate) fn object_field(p: &mut Parser) {
 
         if let Some(T![:]) = p.peek() {
             p.bump(S![:]);
-            value(p);
+            value(p, true);
             if p.peek().is_some() {
                 guard.finish_node();
                 object_field(p)
@@ -139,7 +156,7 @@ pub(crate) fn object_field(p: &mut Parser) {
 pub(crate) fn default_value(p: &mut Parser) {
     let _g = p.start_node(SyntaxKind::DEFAULT_VALUE);
     p.bump(S![=]);
-    value(p);
+    value(p, false);
 }
 
 #[cfg(test)]
@@ -354,4 +371,24 @@ query GraphQuery($graph_id: ID!, $variant: String) {
 
         assert!(ast.errors().next().is_some());
     }
+
+    #[test]
+    fn it_returns_error_for_unfinished_string_value_in_list() {
+        let schema = r#"extend schema
+  @link(url: "https://specs.apollo.dev/federation/v2.0",
+        import: ["@key", "@external])
+        
+type Vehicle @key(fields: "id") {
+  id: ID!,
+  type: String,
+  modelCode: String,
+  brandName: String,
+  launchDate: String
+}
+"#;
+
+        let parser = Parser::new(schema);
+        let ast = parser.parse();
+        assert!(!ast.errors.is_empty());
+    }
 }


Test patch:
diff --git a/crates/apollo-parser/test_data/parser/err/0031_argument_with_erronous_string_value.txt b/crates/apollo-parser/test_data/parser/err/0031_argument_with_erronous_string_value.txt
index d41a60e..16d01d5 100644
--- a/crates/apollo-parser/test_data/parser/err/0031_argument_with_erronous_string_value.txt
+++ b/crates/apollo-parser/test_data/parser/err/0031_argument_with_erronous_string_value.txt
@@ -17,5 +17,5 @@
                     - WHITESPACE@14..15 "\n"
             - R_CURLY@15..16 "}"
 - ERROR@13:25 "unexpected escaped character" "\string ID"
-- ERROR@25:26 "expected a valid Value" )
+- ERROR@25:26 "expected a valid Value got RParen" )
 recursion limit: 4096, high: 2
\ No newline at end of file
diff --git a/crates/apollo-parser/test_data/parser/err/0033_directive_with_erronous_string_value.txt b/crates/apollo-parser/test_data/parser/err/0033_directive_with_erronous_string_value.txt
index adb61ad..269e335 100644
--- a/crates/apollo-parser/test_data/parser/err/0033_directive_with_erronous_string_value.txt
+++ b/crates/apollo-parser/test_data/parser/err/0033_directive_with_erronous_string_value.txt
@@ -67,5 +67,5 @@
                     - WHITESPACE@117..118 "\n"
             - R_CURLY@118..119 "}"
 - ERROR@105:128 "unexpected escaped character" "\ errronous string \""
-- ERROR@128:129 "expected a valid Value" )
+- ERROR@128:129 "expected a valid Value got RParen" )
 recursion limit: 4096, high: 0
\ No newline at end of file
diff --git a/crates/apollo-parser/test_data/parser/err/0034_directive_with_unfinished_string_value.graphql b/crates/apollo-parser/test_data/parser/err/0034_directive_with_unfinished_string_value.graphql
new file mode 100644
index 0000000..3abb1c4
--- /dev/null
+++ b/crates/apollo-parser/test_data/parser/err/0034_directive_with_unfinished_string_value.graphql
@@ -0,0 +1,12 @@
+extend schema
+  @link(url: "https://specs.apollo.dev/federation/v2.0",
+        import: ["@key", "@external])
+
+
+type Vehicle @key(fields: "id") {
+  id: ID!,
+  type: String,
+  modelCode: String,
+  brandName: String,
+  launchDate: String
+}
diff --git a/crates/apollo-parser/test_data/parser/err/0034_directive_with_unfinished_string_value.txt b/crates/apollo-parser/test_data/parser/err/0034_directive_with_unfinished_string_value.txt
new file mode 100644
index 0000000..94dde27
--- /dev/null
+++ b/crates/apollo-parser/test_data/parser/err/0034_directive_with_unfinished_string_value.txt
@@ -0,0 +1,46 @@
+- DOCUMENT@0..97
+    - SCHEMA_EXTENSION@0..97
+        - extend_KW@0..6 "extend"
+        - WHITESPACE@6..7 " "
+        - schema_KW@7..13 "schema"
+        - WHITESPACE@13..16 "\n  "
+        - DIRECTIVES@16..97
+            - DIRECTIVE@16..97
+                - AT@16..17 "@"
+                - NAME@17..21
+                    - IDENT@17..21 "link"
+                - ARGUMENTS@21..97
+                    - L_PAREN@21..22 "("
+                    - ARGUMENT@22..79
+                        - NAME@22..25
+                            - IDENT@22..25 "url"
+                        - COLON@25..26 ":"
+                        - WHITESPACE@26..27 " "
+                        - STRING_VALUE@27..79
+                            - STRING@27..69 "\"https://specs.apollo.dev/federation/v2.0\""
+                            - COMMA@69..70 ","
+                            - WHITESPACE@70..79 "\n        "
+                    - ARGUMENT@79..97
+                        - NAME@79..85
+                            - IDENT@79..85 "import"
+                        - COLON@85..86 ":"
+                        - WHITESPACE@86..87 " "
+                        - LIST_VALUE@87..97
+                            - L_BRACK@87..88 "["
+                            - STRING_VALUE@88..97
+                                - STRING@88..94 "\"@key\""
+                                - COMMA@94..95 ","
+                                - WHITESPACE@95..96 " "
+                                - WHITESPACE@96..97 "\n"
+- ERROR@96:236 "expected a closing \" for the string value" "@external])
+
+type Vehicle @key(fields: "id") {
+  id: ID!,
+  type: String,
+  modelCode: String,
+  brandName: String,
+  launchDate: String
+}
+
+- ERROR@237:237 "expected R_PAREN, got EOF" EOF
+recursion limit: 4096, high: 0
\ No newline at end of file