Remove variable namespace checks from JSON selection (#6640)

apollographql · Jan 24, 2025 · bf30386 · bf30386
1 parent 6848f8c
commit bf30386
Show file tree

Hide file tree

Showing 14 changed files with 146 additions and 146 deletions.
diff --git a/apollo-federation/src/sources/connect/json_selection/apply_to.rs b/apollo-federation/src/sources/connect/json_selection/apply_to.rs
@@ -43,18 +43,10 @@ impl JSONSelection {
 
         let mut vars_with_paths: VarsWithPathsMap = IndexMap::default();
         for (var_name, var_data) in vars {
-            if let Some(known_var) = KnownVariable::from_str(var_name.as_str()) {
-                vars_with_paths.insert(
-                    known_var,
-                    (var_data, InputPath::empty().append(json!(var_name))),
-                );
-            } else {
-                errors.insert(ApplyToError::new(
-                    format!("Unknown variable {}", var_name),
-                    vec![json!(var_name)],
-                    None,
-                ));
-            }
+            vars_with_paths.insert(
+                KnownVariable::from_str(var_name.as_str()),
+                (var_data, InputPath::empty().append(json!(var_name))),
+            );
         }
         // The $ variable initially refers to the root data value, but is
         // rebound by nested selection sets to refer to the root value the

diff --git a/apollo-federation/src/sources/connect/json_selection/fixtures.rs b/apollo-federation/src/sources/connect/json_selection/fixtures.rs
@@ -0,0 +1,30 @@
+use std::fmt::Display;
+use std::str::FromStr;
+
+/// A namespace used in tests to avoid dependencies on specific external namespaces
+#[derive(Debug, PartialEq)]
+pub(super) enum Namespace {
+    Args,
+    This,
+}
+
+impl FromStr for Namespace {
+    type Err = String;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        match s {
+            "$args" => Ok(Self::Args),
+            "$this" => Ok(Self::This),
+            _ => Err(format!("Unknown variable namespace: {}", s)),
+        }
+    }
+}
+
+impl Display for Namespace {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            Self::Args => write!(f, "$args"),
+            Self::This => write!(f, "$this"),
+        }
+    }
+}
diff --git a/apollo-federation/src/sources/connect/json_selection/known_var.rs b/apollo-federation/src/sources/connect/json_selection/known_var.rs
@@ -1,26 +1,23 @@
-use std::str::FromStr;
-
 #[cfg(test)]
 use super::location::WithRange;
-use crate::sources::connect::variable::Namespace;
 
 #[derive(PartialEq, Eq, Clone, Hash)]
 pub(crate) enum KnownVariable {
-    External(Namespace),
+    External(String),
     Dollar,
     AtSign,
 }
 
 impl KnownVariable {
-    pub(crate) fn from_str(var_name: &str) -> Option<Self> {
+    pub(crate) fn from_str(var_name: &str) -> Self {
         match var_name {
-            "$" => Some(Self::Dollar),
-            "@" => Some(Self::AtSign),
-            s => Namespace::from_str(s).ok().map(Self::External),
+            "$" => Self::Dollar,
+            "@" => Self::AtSign,
+            s => Self::External(s.to_string()),
         }
     }
 
-    pub(crate) fn as_str(&self) -> &'static str {
+    pub(crate) fn as_str(&self) -> &str {
         match self {
             Self::External(namespace) => namespace.as_str(),
             Self::Dollar => "$",
@@ -45,9 +42,3 @@ impl std::fmt::Display for KnownVariable {
         write!(f, "{}", self.as_str())
     }
 }
-
-impl From<Namespace> for KnownVariable {
-    fn from(namespace: Namespace) -> Self {
-        Self::External(namespace)
-    }
-}
diff --git a/apollo-federation/src/sources/connect/json_selection/lit_expr.rs b/apollo-federation/src/sources/connect/json_selection/lit_expr.rs
@@ -272,10 +272,10 @@ mod tests {
     use super::super::known_var::KnownVariable;
     use super::super::location::strip_ranges::StripRanges;
     use super::*;
+    use crate::sources::connect::json_selection::fixtures::Namespace;
     use crate::sources::connect::json_selection::helpers::span_is_all_spaces_or_comments;
     use crate::sources::connect::json_selection::location::new_span;
     use crate::sources::connect::json_selection::PathList;
-    use crate::sources::connect::variable::Namespace;
 
     fn check_parse(input: &str, expected: LitExpr) {
         match LitExpr::parse(new_span(input)) {
@@ -524,7 +524,7 @@ mod tests {
                     Key::field("a").into_with_range(),
                     LitExpr::Path(PathSelection {
                         path: PathList::Var(
-                            KnownVariable::from(Namespace::Args).into_with_range(),
+                            KnownVariable::External(Namespace::Args.to_string()).into_with_range(),
                             PathList::Key(
                                 Key::field("a").into_with_range(),
                                 PathList::Empty.into_with_range(),
@@ -539,7 +539,7 @@ mod tests {
                     Key::field("b").into_with_range(),
                     LitExpr::Path(PathSelection {
                         path: PathList::Var(
-                            KnownVariable::from(Namespace::This).into_with_range(),
+                            KnownVariable::External(Namespace::This.to_string()).into_with_range(),
                             PathList::Key(
                                 Key::field("b").into_with_range(),
                                 PathList::Empty.into_with_range(),

diff --git a/apollo-federation/src/sources/connect/json_selection/mod.rs b/apollo-federation/src/sources/connect/json_selection/mod.rs
@@ -17,3 +17,5 @@ pub(crate) use location::Ranged;
 pub use parser::*;
 #[cfg(test)]
 pub(crate) use pretty::*;
+#[cfg(test)]
+mod fixtures;
diff --git a/apollo-federation/src/sources/connect/json_selection/parser.rs b/apollo-federation/src/sources/connect/json_selection/parser.rs
@@ -1,4 +1,5 @@
 use std::fmt::Display;
+use std::str::FromStr;
 
 use apollo_compiler::collections::IndexSet;
 use nom::branch::alt;
@@ -28,7 +29,6 @@ use super::location::OffsetRange;
 use super::location::Ranged;
 use super::location::Span;
 use super::location::WithRange;
-use crate::sources::connect::variable::Namespace;
 use crate::sources::connect::variable::VariableNamespace;
 use crate::sources::connect::variable::VariablePathPart;
 use crate::sources::connect::variable::VariableReference;
@@ -231,7 +231,9 @@ impl JSONSelection {
         }
     }
 
-    pub fn external_variables(&self) -> impl Iterator<Item = Namespace> + '_ {
+    pub fn external_variables<'a, N: FromStr + ToString + 'a>(
+        &'a self,
+    ) -> impl Iterator<Item = N> + 'a {
         self.external_var_paths()
             .into_iter()
             .flat_map(|var_path| var_path.variable_reference())
@@ -479,7 +481,7 @@ impl PathSelection {
         PathList::parse(input).map(|(input, path)| (input, Self { path }))
     }
 
-    pub(crate) fn variable_reference(&self) -> Option<VariableReference<Namespace>> {
+    pub(crate) fn variable_reference<N: FromStr + ToString>(&self) -> Option<VariableReference<N>> {
         match self.path.as_ref() {
             PathList::Var(var, tail) => match var.as_ref() {
                 KnownVariable::External(namespace) => {
@@ -493,7 +495,7 @@ impl PathSelection {
                         .unwrap_or_default();
                     Some(VariableReference {
                         namespace: VariableNamespace {
-                            namespace: *namespace,
+                            namespace: N::from_str(namespace).ok()?,
                             location: var.range().unwrap_or_default(),
                         },
                         path: parts,
@@ -662,29 +664,13 @@ impl PathList {
                 let full_range = merge_ranges(dollar_range.clone(), rest.range());
                 return if let Some(var) = opt_var {
                     let full_name = format!("{}{}", dollar.as_ref(), var.as_str());
-                    if let Some(known_var) = KnownVariable::from_str(full_name.as_str()) {
-                        let var_range = merge_ranges(dollar_range.clone(), var.range());
-                        let ranged_known_var = WithRange::new(known_var, var_range);
-                        Ok((
-                            remainder,
-                            WithRange::new(Self::Var(ranged_known_var, rest), full_range),
-                        ))
-                    } else {
-                        Err(nom_fail_message(
-                            input,
-                            // Here's an error where we might like to use
-                            // format! to include the full_name of the unknown
-                            // variable in the error message, but that means
-                            // we'd have to store the message as an owned
-                            // String, which would make Span no longer Copy,
-                            // which leads to more cloning of Spans in the
-                            // parser code. For now, the input Span reported
-                            // with the error will begin with the unknown
-                            // variable name, which should be enough to
-                            // interpret this static message.
-                            "Unknown variable",
-                        ))
-                    }
+                    let known_var = KnownVariable::from_str(full_name.as_str());
+                    let var_range = merge_ranges(dollar_range.clone(), var.range());
+                    let ranged_known_var = WithRange::new(known_var, var_range);
+                    Ok((
+                        remainder,
+                        WithRange::new(Self::Var(ranged_known_var, rest), full_range),
+                    ))
                 } else {
                     let ranged_dollar_var =
                         WithRange::new(KnownVariable::Dollar, dollar_range.clone());
@@ -1268,6 +1254,7 @@ mod tests {
     use super::super::location::strip_ranges::StripRanges;
     use super::*;
     use crate::selection;
+    use crate::sources::connect::json_selection::fixtures::Namespace;
     use crate::sources::connect::json_selection::helpers::span_is_all_spaces_or_comments;
     use crate::sources::connect::json_selection::location::new_span;
 
@@ -1941,7 +1928,7 @@ mod tests {
             "$this",
             PathSelection {
                 path: PathList::Var(
-                    KnownVariable::from(Namespace::This).into_with_range(),
+                    KnownVariable::External(Namespace::This.to_string()).into_with_range(),
                     PathList::Empty.into_with_range(),
                 )
                 .into_with_range(),
@@ -1963,7 +1950,7 @@ mod tests {
             "$this { hello }",
             PathSelection {
                 path: PathList::Var(
-                    KnownVariable::from(Namespace::This).into_with_range(),
+                    KnownVariable::External(Namespace::This.to_string()).into_with_range(),
                     PathList::Selection(SubSelection {
                         selections: vec![NamedSelection::Field(
                             None,
@@ -2000,7 +1987,7 @@ mod tests {
         check_path_selection(
             "$this { before alias: $args.arg after }",
             PathList::Var(
-                KnownVariable::from(Namespace::This).into_with_range(),
+                KnownVariable::External(Namespace::This.to_string()).into_with_range(),
                 PathList::Selection(SubSelection {
                     selections: vec![
                         NamedSelection::Field(None, Key::field("before").into_with_range(), None),
@@ -2009,7 +1996,8 @@ mod tests {
                             inline: false,
                             path: PathSelection {
                                 path: PathList::Var(
-                                    KnownVariable::from(Namespace::Args).into_with_range(),
+                                    KnownVariable::External(Namespace::Args.to_string())
+                                        .into_with_range(),
                                     PathList::Key(
                                         Key::field("arg").into_with_range(),
                                         PathList::Empty.into_with_range(),
@@ -2047,7 +2035,8 @@ mod tests {
                                     inline: false,
                                     path: PathSelection {
                                         path: PathList::Var(
-                                            KnownVariable::from(Namespace::Args).into_with_range(),
+                                            KnownVariable::External(Namespace::Args.to_string())
+                                                .into_with_range(),
                                             PathList::Key(
                                                 Key::field("arg").into_with_range(),
                                                 PathList::Empty.into_with_range(),
@@ -2072,7 +2061,7 @@ mod tests {
             "$args.a.b.c",
             PathSelection {
                 path: PathList::Var(
-                    KnownVariable::from(Namespace::Args).into_with_range(),
+                    KnownVariable::External(Namespace::Args.to_string()).into_with_range(),
                     PathList::from_slice(
                         &[
                             Key::Field("a".to_string()),
@@ -2214,7 +2203,7 @@ mod tests {
             selection!("$this").strip_ranges(),
             JSONSelection::Path(PathSelection {
                 path: PathList::Var(
-                    KnownVariable::from(Namespace::This).into_with_range(),
+                    KnownVariable::External(Namespace::This.to_string()).into_with_range(),
                     PathList::Empty.into_with_range()
                 )
                 .into_with_range(),
@@ -2267,7 +2256,7 @@ mod tests {
                     inline: false,
                     path: PathSelection {
                         path: PathList::Var(
-                            KnownVariable::from(Namespace::This).into_with_range(),
+                            KnownVariable::External(Namespace::This.to_string()).into_with_range(),
                             PathList::Selection(SubSelection {
                                 selections: vec![
                                     NamedSelection::Field(
@@ -2300,10 +2289,6 @@ mod tests {
         // to achieve the same effect without ambiguity.
         assert_debug_snapshot!(JSONSelection::parse(".data"));
 
-        // We statically verify that all variables are KnownVariables, and
-        // $bogus is not one of them.
-        assert_debug_snapshot!(JSONSelection::parse("$bogus"));
-
         // If you want to mix a path selection with other named selections, the
         // path selection must have a trailing subselection, to enforce that it
         // returns an object with statically known keys.
@@ -2943,7 +2928,10 @@ mod tests {
             JSONSelection::Path(PathSelection {
                 path: WithRange::new(
                     PathList::Var(
-                        WithRange::new(KnownVariable::from(Namespace::Args), Some(0..5)),
+                        WithRange::new(
+                            KnownVariable::External(Namespace::Args.to_string()),
+                            Some(0..5),
+                        ),
                         WithRange::new(
                             PathList::Key(
                                 WithRange::new(Key::field("product"), Some(6..13)),
@@ -2968,7 +2956,10 @@ mod tests {
             JSONSelection::Path(PathSelection {
                 path: WithRange::new(
                     PathList::Var(
-                        WithRange::new(KnownVariable::from(Namespace::Args), Some(1..6)),
+                        WithRange::new(
+                            KnownVariable::External(Namespace::Args.to_string()),
+                            Some(1..6),
+                        ),
                         WithRange::new(
                             PathList::Key(
                                 WithRange::new(Key::field("product"), Some(9..16)),
@@ -3007,7 +2998,7 @@ mod tests {
                             path: WithRange::new(
                                 PathList::Var(
                                     WithRange::new(
-                                        KnownVariable::from(Namespace::Args),
+                                        KnownVariable::External(Namespace::Args.to_string()),
                                         Some(15..20),
                                     ),
                                     WithRange::new(
@@ -3142,16 +3133,4 @@ mod tests {
         let var_paths = selection.external_var_paths();
         assert_eq!(var_paths.len(), 0);
     }
-
-    #[test]
-    fn test_parse_unknown_variable() {
-        assert_eq!(
-            JSONSelection::parse("a b { c: $foobar.x.y.z { d } }"),
-            Err(JSONSelectionParseError {
-                message: "Unknown variable".to_string(),
-                fragment: "$foobar.x.y.z { d } }".to_string(),
-                offset: 9,
-            })
-        );
-    }
 }
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
@@ -1,11 +1,11 @@
 ---
 source: apollo-federation/src/sources/connect/json_selection/parser.rs
-expression: "JSONSelection::parse(\"$bogus\")"
+expression: "JSONSelection::parse(\"id $.object\")"
 ---
 Err(
     JSONSelectionParseError {
-        message: "Unknown variable",
-        fragment: "$bogus",
-        offset: 0,
+        message: "Named path selection must either begin with alias or ..., or end with subselection",
+        fragment: "$.object",
+        offset: 3,
     },
 )
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