relax variables selection for subgraph queries (#755)

* relax variables selection for subgraph queries

for federated subgraph queries, we encountered multiple issues:
- if we got a null entity from a previous call, selecting data from it
  would return an error and fail the subgraph request. This is invalid,
because for a request like `query { topProducts { reviews { body } } }`
and a products subgraph returning `[ {"upc": 0}, null, { "upc": 2} ]`,
we should do a query to the reviews subgraph for the products 0 and 2
- if the list of representations to query a subgraph is empty, we were
  still doing a HTTP request expecting an empty result

this commit fixes those issues by ignoring selection errors when
creating variables, then canceling the subgraph request if the
representations array is empty

* changelog and lint

* Update CHANGELOG.md

Co-authored-by: Bryn Cooke <BrynCooke@gmail.com>

Co-authored-by: Bryn Cooke <BrynCooke@gmail.com>

CHANGELOG.md

@@ -53,10 +53,14 @@ server:
   ```
   In addition, other existing uplink env variables are now also configurable via arg. 
 
-- Make deduplication and caching more robust against cancellation [PR #752](https://github.com/apollographql/router/pull/752)
+- **Make deduplication and caching more robust against cancellation** [PR #752](https://github.com/apollographql/router/pull/752)
 
   Cancelling a request could put the router in an unresponsive state where the deduplication layer or cache would make subgraph requests hang.
 
+- **Relax variables selection for subgraph queries** ([PR #755](https://github.com/apollographql/router/pull/755))
+
+  federated subgraph queries relying on partial or invalid data from previous subgraph queries could result in response failures or empty subgraph queries. The router is now more flexible when selecting data from previous queries, while still keeping a correct form for the final response
+
 ## 🛠 Maintenance
 ## 📚 Documentation
 

apollo-router-core/src/json_ext.rs

@@ -72,10 +72,11 @@ pub trait ValueExt {
     /// Select all values matching a `Path`.
     ///
     /// the function passed as argument will be called with the values found and their Path
+    /// if it encounters an invalid value, it will ignore it and continue
     #[track_caller]
-    fn select_values_and_paths<'a, F>(&'a self, path: &'a Path, f: F) -> Result<(), FetchError>
+    fn select_values_and_paths<'a, F>(&'a self, path: &'a Path, f: F)
     where
-        F: FnMut(Path, &'a Value) -> Result<(), FetchError>;
+        F: FnMut(Path, &'a Value);
 }
 
 impl ValueExt for Value {
@@ -314,41 +315,32 @@ impl ValueExt for Value {
     }
 
     #[track_caller]
-    fn select_values_and_paths<'a, F>(&'a self, path: &'a Path, mut f: F) -> Result<(), FetchError>
+    fn select_values_and_paths<'a, F>(&'a self, path: &'a Path, mut f: F)
     where
-        F: FnMut(Path, &'a Value) -> Result<(), FetchError>,
+        F: FnMut(Path, &'a Value),
     {
         iterate_path(&Path::default(), &path.0, self, &mut f)
     }
 }
 
-fn iterate_path<'a, F>(
-    parent: &Path,
-    path: &'a [PathElement],
-    data: &'a Value,
-    f: &mut F,
-) -> Result<(), FetchError>
+fn iterate_path<'a, F>(parent: &Path, path: &'a [PathElement], data: &'a Value, f: &mut F)
 where
-    F: FnMut(Path, &'a Value) -> Result<(), FetchError>,
+    F: FnMut(Path, &'a Value),
 {
     match path.get(0) {
         None => f(parent.clone(), data),
-        Some(PathElement::Flatten) => match data.as_array() {
-            None => Err(FetchError::ExecutionInvalidContent {
-                reason: "not an array".to_string(),
-            }),
-            Some(array) => {
+        Some(PathElement::Flatten) => {
+            if let Some(array) = data.as_array() {
                 for (i, value) in array.iter().enumerate() {
                     iterate_path(
                         &parent.join(Path::from(i.to_string())),
                         &path[1..],
                         value,
                         f,
-                    )?;
+                    );
                 }
-                Ok(())
             }
-        },
+        }
         Some(PathElement::Index(i)) => {
             if let Value::Array(a) = data {
                 if let Some(value) = a.get(*i) {
@@ -358,30 +350,14 @@ where
                         value,
                         f,
                     )
-                } else {
-                    Err(FetchError::ExecutionPathNotFound {
-                        reason: format!("index {} not found", i),
-                    })
                 }
-            } else {
-                Err(FetchError::ExecutionInvalidContent {
-                    reason: "not an array".to_string(),
-                })
             }
         }
         Some(PathElement::Key(k)) => {
             if let Value::Object(o) = data {
                 if let Some(value) = o.get(k.as_str()) {
                     iterate_path(&parent.join(Path::from(k)), &path[1..], value, f)
-                } else {
-                    Err(FetchError::ExecutionPathNotFound {
-                        reason: format!("key {} not found", k),
-                    })
                 }
-            } else {
-                Err(FetchError::ExecutionInvalidContent {
-                    reason: "not an object".to_string(),
-                })
             }
         }
     }
@@ -567,8 +543,7 @@ mod tests {
         let mut v = Vec::new();
         data.select_values_and_paths(path, |_path, value| {
             v.push(value);
-            Ok(())
-        })?;
+        });
         Ok(v)
     }
 

apollo-router-core/src/query_planner/mod.rs

@@ -292,7 +292,7 @@ pub(crate) mod fetch {
             current_dir: &Path,
             context: &Context,
             schema: &Schema,
-        ) -> Result<Variables, FetchError> {
+        ) -> Option<Variables> {
             let body = context.request.body();
             if !requires.is_empty() {
                 let mut variables = Object::with_capacity(1 + variable_usages.len());
@@ -306,29 +306,23 @@ pub(crate) mod fetch {
                 let mut paths = Vec::new();
                 let mut values = Vec::new();
                 data.select_values_and_paths(current_dir, |path, value| {
-                    match value {
-                        Value::Object(content) => {
-                            let object = select_object(content, requires, schema)?;
-                            if let Some(value) = object {
-                                paths.push(path);
-                                values.push(value)
-                            }
-                        }
-                        _ => {
-                            return Err(FetchError::ExecutionInvalidContent {
-                                reason: "not an object".to_string(),
-                            })
+                    if let Value::Object(content) = value {
+                        if let Ok(Some(value)) = select_object(content, requires, schema) {
+                            paths.push(path);
+                            values.push(value);
                         }
                     }
-                    Ok(())
-                })?;
-                let representations = Value::Array(values);
+                });
 
-                variables.insert("representations", representations);
+                if values.is_empty() {
+                    return None;
+                }
+
+                variables.insert("representations", Value::Array(values));
 
-                Ok(Variables { variables, paths })
+                Some(Variables { variables, paths })
             } else {
-                Ok(Variables {
+                Some(Variables {
                     variables: variable_usages
                         .iter()
                         .filter_map(|key| {
@@ -359,15 +353,21 @@ pub(crate) mod fetch {
                 ..
             } = self;
 
-            let Variables { variables, paths } = Variables::new(
+            let Variables { variables, paths } = match Variables::new(
                 &self.requires,
                 self.variable_usages.as_ref(),
                 data,
                 current_dir,
                 context,
                 schema,
             )
-            .await?;
+            .await
+            {
+                Some(variables) => variables,
+                None => {
+                    return Ok((Value::from_path(current_dir, Value::Null), Vec::new()));
+                }
+            };
 
             let subgraph_request = SubgraphRequest {
                 http_request: http_compat::RequestBuilder::new(

apollo-router-core/src/query_planner/selection.rs

@@ -125,12 +125,9 @@ mod tests {
         schema: &Schema,
     ) -> Result<Value, FetchError> {
         let mut values = Vec::new();
-        response
-            .data
-            .select_values_and_paths(path, |_path, value| {
-                values.push(value);
-                Ok(())
-            })?;
+        response.data.select_values_and_paths(path, |_path, value| {
+            values.push(value);
+        });
 
         Ok(Value::Array(
             values