Use Result instead of IResult for JSONSelection::parse.

#6089 (comment)
apollographql · Nov 1, 2024 · 76949bd · 76949bd
1 parent cd42e99
commit 76949bd
Show file tree

Hide file tree

Showing 19 changed files with 591 additions and 644 deletions.
diff --git a/apollo-federation/src/sources/connect/expand/mod.rs b/apollo-federation/src/sources/connect/expand/mod.rs
@@ -522,7 +522,7 @@ mod helpers {
                             to_schema,
                             &self.directive_deny_list,
                         );
-                        let (_, parsed) = JSONSelection::parse(&selection).map_err(|e| {
+                        let parsed = JSONSelection::parse(&selection).map_err(|e| {
                             FederationError::internal(format!(
                                 "could not parse fake selection for sibling field: {e}"
                             ))

diff --git a/apollo-federation/src/sources/connect/expand/visitors/mod.rs b/apollo-federation/src/sources/connect/expand/visitors/mod.rs
@@ -275,8 +275,7 @@ mod tests {
     fn it_iterates_over_empty_path() {
         let mut visited = Vec::new();
         let visitor = TestVisitor::new(&mut visited);
-        let (unmatched, selection) = JSONSelection::parse("").unwrap();
-        assert!(unmatched.is_empty());
+        let selection = JSONSelection::parse("").unwrap();
 
         visitor
             .walk(selection.next_subselection().cloned().unwrap())
@@ -288,8 +287,7 @@ mod tests {
     fn it_iterates_over_simple_selection() {
         let mut visited = Vec::new();
         let visitor = TestVisitor::new(&mut visited);
-        let (unmatched, selection) = JSONSelection::parse("a b c d").unwrap();
-        assert!(unmatched.is_empty());
+        let selection = JSONSelection::parse("a b c d").unwrap();
 
         visitor
             .walk(selection.next_subselection().cloned().unwrap())
@@ -306,9 +304,7 @@ mod tests {
     fn it_iterates_over_aliased_selection() {
         let mut visited = Vec::new();
         let visitor = TestVisitor::new(&mut visited);
-        let (unmatched, selection) =
-            JSONSelection::parse("a: one b: two c: three d: four").unwrap();
-        assert!(unmatched.is_empty());
+        let selection = JSONSelection::parse("a: one b: two c: three d: four").unwrap();
 
         visitor
             .walk(selection.next_subselection().cloned().unwrap())
@@ -325,8 +321,7 @@ mod tests {
     fn it_iterates_over_nested_selection() {
         let mut visited = Vec::new();
         let visitor = TestVisitor::new(&mut visited);
-        let (unmatched, selection) = JSONSelection::parse("a { b { c { d { e } } } } f").unwrap();
-        assert!(unmatched.is_empty());
+        let selection = JSONSelection::parse("a { b { c { d { e } } } } f").unwrap();
 
         visitor
             .walk(selection.next_subselection().cloned().unwrap())
@@ -345,7 +340,7 @@ mod tests {
     fn it_iterates_over_paths() {
         let mut visited = Vec::new();
         let visitor = TestVisitor::new(&mut visited);
-        let (unmatched, selection) = JSONSelection::parse(
+        let selection = JSONSelection::parse(
             "a
             $.b {
                 c
@@ -357,7 +352,6 @@ mod tests {
             j",
         )
         .unwrap();
-        assert!(unmatched.is_empty());
 
         visitor
             .walk(selection.next_subselection().cloned().unwrap())
@@ -376,7 +370,7 @@ mod tests {
     fn it_iterates_over_complex_selection() {
         let mut visited = Vec::new();
         let visitor = TestVisitor::new(&mut visited);
-        let (unmatched, selection) = JSONSelection::parse(
+        let selection = JSONSelection::parse(
             "id
             name
             username
@@ -400,7 +394,6 @@ mod tests {
             }",
         )
         .unwrap();
-        assert!(unmatched.is_empty());
 
         visitor
             .walk(selection.next_subselection().cloned().unwrap())

diff --git a/apollo-federation/src/sources/connect/json_selection/helpers.rs b/apollo-federation/src/sources/connect/json_selection/helpers.rs
@@ -13,10 +13,7 @@ use super::ParseResult;
 #[macro_export]
 macro_rules! selection {
     ($input:expr) => {
-        if let Ok((remainder, parsed)) =
-            $crate::sources::connect::json_selection::JSONSelection::parse($input)
-        {
-            assert_eq!(remainder, "");
+        if let Ok(parsed) = $crate::sources::connect::json_selection::JSONSelection::parse($input) {
             parsed
         } else {
             panic!("invalid selection: {:?}", $input);

diff --git a/apollo-federation/src/sources/connect/json_selection/location.rs b/apollo-federation/src/sources/connect/json_selection/location.rs
@@ -315,15 +315,13 @@ mod tests {
 
     #[test]
     fn test_arrow_path_ranges() {
-        let (remainder, parsed) =
-            JSONSelection::parse("  __typename: @ -> echo ( \"Frog\" , )  ").unwrap();
-        assert_eq!(remainder, "");
+        let parsed = JSONSelection::parse("  __typename: @ -> echo ( \"Frog\" , )  ").unwrap();
         assert_debug_snapshot!(parsed);
     }
 
     #[test]
     fn test_parse_with_range_snapshots() {
-        let (remainder, parsed) = JSONSelection::parse(
+        let parsed = JSONSelection::parse(
             r#"
         path: some.nested.path { isbn author { name }}
         alias: "not an identifier" {
@@ -337,7 +335,6 @@ mod tests {
         "#,
         )
         .unwrap();
-        assert_eq!(remainder, "");
         assert_snapshot!(format!("{:#?}", parsed));
     }
 }
diff --git a/apollo-federation/src/sources/connect/json_selection/parser.rs b/apollo-federation/src/sources/connect/json_selection/parser.rs
@@ -139,19 +139,24 @@ impl JSONSelection {
     // if we drastically change implementation details. That's why we use &str
     // as the input type and a custom JSONSelectionParseError type as the error
     // type, rather than using Span or nom::error::Error directly.
-    pub fn parse(input: &str) -> IResult<&str, Self, JSONSelectionParseError> {
+    pub fn parse(input: &str) -> Result<Self, JSONSelectionParseError> {
         match JSONSelection::parse_span(new_span(input)) {
             Ok((remainder, selection)) => {
-                // To avoid exposing the implementation details of nom_locate's
-                // LocatedSpan, we report the remainder as a &str instead.
-                Ok((remainder.fragment(), selection))
+                let fragment = remainder.fragment();
+                if fragment.is_empty() {
+                    Ok(selection)
+                } else {
+                    Err(JSONSelectionParseError {
+                        message: "Unexpected trailing characters".to_string(),
+                        fragment: fragment.to_string(),
+                        offset: remainder.location_offset(),
+                    })
+                }
             }
 
             Err(e) => match e {
                 nom::Err::Error(e) | nom::Err::Failure(e) => {
-                    // If a non-fatal nom::Err::Error bubbles all the way up
-                    // here, then it becomes a fatal nom::Err::Failure.
-                    Err(nom::Err::Failure(JSONSelectionParseError {
+                    Err(JSONSelectionParseError {
                         message: if let Some(message_str) = e.input.extra {
                             message_str.to_string()
                         } else {
@@ -162,7 +167,7 @@ impl JSONSelection {
                         },
                         fragment: e.input.fragment().to_string(),
                         offset: e.input.location_offset(),
-                    }))
+                    })
                 }
 
                 nom::Err::Incomplete(_) => unreachable!("nom::Err::Incomplete not expected here"),
@@ -2808,12 +2813,7 @@ mod tests {
     #[test]
     fn test_ranged_locations() {
         fn check(input: &str, expected: JSONSelection) {
-            let (remainder, parsed) = JSONSelection::parse(input).unwrap();
-            // We can't (and don't want to) use span_is_all_spaces_or_comments
-            // here because remainder is a &str not a Span, and
-            // JSONSelection::parse is responsible for consuming the entire
-            // string when possible (see all_consuming).
-            assert_eq!(remainder, "");
+            let parsed = JSONSelection::parse(input).unwrap();
             assert_eq!(parsed, expected);
         }
 

diff --git a/apollo-federation/src/sources/connect/json_selection/pretty.rs b/apollo-federation/src/sources/connect/json_selection/pretty.rs
@@ -448,9 +448,7 @@ mod tests {
 
     #[test]
     fn it_prints_root_selection() {
-        let (unmatched, root_selection) = JSONSelection::parse("id name").unwrap();
-        assert!(unmatched.is_empty());
-
+        let root_selection = JSONSelection::parse("id name").unwrap();
         test_permutations(root_selection, "id\nname");
     }
 }
diff --git a/apollo-federation/src/sources/connect/json_selection/selection_set.rs b/apollo-federation/src/sources/connect/json_selection/selection_set.rs
@@ -299,8 +299,7 @@ mod tests {
         }
         "###,
         )
-        .unwrap()
-        .1;
+        .unwrap();
 
         let schema = Schema::parse_and_validate(
             r###"
@@ -368,8 +367,7 @@ mod tests {
         }
         "###,
         )
-        .unwrap()
-        .1;
+        .unwrap();
 
         let schema = Schema::parse_and_validate(
             r###"
@@ -474,8 +472,7 @@ mod tests {
         }
         "###,
         )
-        .unwrap()
-        .1;
+        .unwrap();
 
         let schema = Schema::parse_and_validate(
             r###"
@@ -545,8 +542,7 @@ mod tests {
             }
             "###,
         )
-        .unwrap()
-        .1;
+        .unwrap();
 
         let schema = Schema::parse_and_validate(
             r###"
@@ -595,8 +591,7 @@ mod tests {
             }
             "###,
         )
-        .unwrap()
-        .1;
+        .unwrap();
 
         let schema = Schema::parse_and_validate(
             r###"

diff --git a/...pollo_federation__sources__connect__json_selection__parser__tests__error_snapshots-2.snap b/...pollo_federation__sources__connect__json_selection__parser__tests__error_snapshots-2.snap
@@ -3,11 +3,9 @@ source: apollo-federation/src/sources/connect/json_selection/parser.rs
 expression: "JSONSelection::parse(\"$bogus\")"
 ---
 Err(
-    Failure(
-        JSONSelectionParseError {
-            message: "Unknown variable",
-            fragment: "$bogus",
-            offset: 0,
-        },
-    ),
+    JSONSelectionParseError {
+        message: "Unknown variable",
+        fragment: "$bogus",
+        offset: 0,
+    },
 )
diff --git a/...pollo_federation__sources__connect__json_selection__parser__tests__error_snapshots-3.snap b/...pollo_federation__sources__connect__json_selection__parser__tests__error_snapshots-3.snap
@@ -3,11 +3,9 @@ source: apollo-federation/src/sources/connect/json_selection/parser.rs
 expression: "JSONSelection::parse(\"id $.object\")"
 ---
 Err(
-    Failure(
-        JSONSelectionParseError {
-            message: "Named path selection must either begin with alias or end with subselection",
-            fragment: "$.object",
-            offset: 3,
-        },
-    ),
+    JSONSelectionParseError {
+        message: "Named path selection must either begin with alias or end with subselection",
+        fragment: "$.object",
+        offset: 3,
+    },
 )
diff --git a/.../apollo_federation__sources__connect__json_selection__parser__tests__error_snapshots.snap b/.../apollo_federation__sources__connect__json_selection__parser__tests__error_snapshots.snap
@@ -3,11 +3,9 @@ source: apollo-federation/src/sources/connect/json_selection/parser.rs
 expression: "JSONSelection::parse(\".data\")"
 ---
 Err(
-    Failure(
-        JSONSelectionParseError {
-            message: "Key paths cannot start with just .key (use $.key instead)",
-            fragment: ".data",
-            offset: 0,
-        },
-    ),
+    JSONSelectionParseError {
+        message: "Key paths cannot start with just .key (use $.key instead)",
+        fragment: ".data",
+        offset: 0,
+    },
 )
diff --git a/...ederation__sources__connect__json_selection__parser__tests__path_with_subselection-4.snap b/...ederation__sources__connect__json_selection__parser__tests__path_with_subselection-4.snap
@@ -3,11 +3,9 @@ source: apollo-federation/src/sources/connect/json_selection/parser.rs
 expression: "JSONSelection::parse(r#\"\n            id\n            created\n            choices->first.message\n            model\n        \"#)"
 ---
 Err(
-    Failure(
-        JSONSelectionParseError {
-            message: "Named path selection must either begin with alias or end with subselection",
-            fragment: "choices->first.message\n            model\n        ",
-            offset: 48,
-        },
-    ),
+    JSONSelectionParseError {
+        message: "Named path selection must either begin with alias or end with subselection",
+        fragment: "choices->first.message\n            model\n        ",
+        offset: 48,
+    },
 )