fix(executor): ensure variables passed to subgraph requests (#464)

Co-authored-by: Dotan Simha <[email protected]>

Author: rawkode

.github/workflows/build.yaml

@@ -179,12 +179,12 @@ jobs:
           file: ./docker/router.Dockerfile
           context: .
           platforms: ${{ env.DOCKER_PLATFORMS }}
-          push: true
+          push: ${{ github.event.pull_request.head.repo.full_name == github.repository }}
           tags: ${{ steps.meta.outputs.tags }}
           labels: ${{ steps.meta.outputs.labels }}
       - name: docker pr comment
         uses: marocchino/sticky-pull-request-comment@773744901bac0e8cbb5a0dc842800d45e9b2b405 # v2
-        if: ${{ github.event_name == 'pull_request' }}
+        if: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository }}
         with:
           header: ${{ github.workflow }}
           message: |
@@ -207,7 +207,7 @@ jobs:
   test-docker-image:
     name: test docker image
     runs-on: ubuntu-latest
-    if: ${{ github.event_name == 'pull_request' }}
+    if: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository }}
     needs:
       - docker
     steps:

lib/executor/src/execution/plan.rs

@@ -1,4 +1,7 @@
-use std::{borrow::Cow, collections::HashMap};
+use std::{
+    borrow::Cow,
+    collections::{BTreeSet, HashMap},
+};
 
 use bytes::{BufMut, Bytes};
 use futures::{future::BoxFuture, stream::FuturesUnordered, StreamExt};
@@ -651,6 +654,8 @@ impl<'exec> Executor<'exec> {
             self.client_request,
             &mut headers_map,
         )?;
+        let variable_refs =
+            select_fetch_variables(self.variable_values, node.variable_usages.as_ref());
 
         Ok(ExecutionJob::Fetch(FetchJob {
             fetch_node_id: node.id,
@@ -663,7 +668,7 @@ impl<'exec> Executor<'exec> {
                         query: node.operation.document_str.as_str(),
                         dedupe: self.dedupe_subgraph_requests,
                         operation_name: node.operation_name.as_deref(),
-                        variables: None,
+                        variables: variable_refs,
                         representations,
                         headers: headers_map,
                     },
@@ -688,3 +693,79 @@ fn condition_node_by_variables<'a>(
         condition_node.else_clause.as_deref()
     }
 }
+
+fn select_fetch_variables<'a>(
+    variable_values: &'a Option<HashMap<String, sonic_rs::Value>>,
+    variable_usages: Option<&BTreeSet<String>>,
+) -> Option<HashMap<&'a str, &'a sonic_rs::Value>> {
+    let values = variable_values.as_ref()?;
+
+    variable_usages.map(|variable_usages| {
+        variable_usages
+            .iter()
+            .filter_map(|var_name| {
+                values
+                    .get_key_value(var_name.as_str())
+                    .map(|(key, value)| (key.as_str(), value))
+            })
+            .collect()
+    })
+}
+
+#[cfg(test)]
+mod tests {
+    use super::select_fetch_variables;
+    use sonic_rs::Value;
+    use std::collections::{BTreeSet, HashMap};
+
+    fn value_from_number(n: i32) -> Value {
+        sonic_rs::from_str(&n.to_string()).unwrap()
+    }
+
+    #[test]
+    fn select_fetch_variables_only_used_variables() {
+        let mut variable_values_map = HashMap::new();
+        variable_values_map.insert("used".to_string(), value_from_number(1));
+        variable_values_map.insert("unused".to_string(), value_from_number(2));
+        let variable_values = Some(variable_values_map);
+
+        let mut usages = BTreeSet::new();
+        usages.insert("used".to_string());
+
+        let selected = select_fetch_variables(&variable_values, Some(&usages)).unwrap();
+
+        assert_eq!(selected.len(), 1);
+        assert!(selected.contains_key("used"));
+        assert!(!selected.contains_key("unused"));
+    }
+
+    #[test]
+    fn select_fetch_variables_ignores_missing_usage_entries() {
+        let mut variable_values_map = HashMap::new();
+        variable_values_map.insert("present".to_string(), value_from_number(3));
+        let variable_values = Some(variable_values_map);
+
+        let mut usages = BTreeSet::new();
+        usages.insert("present".to_string());
+        usages.insert("missing".to_string());
+
+        let selected = select_fetch_variables(&variable_values, Some(&usages)).unwrap();
+
+        assert_eq!(selected.len(), 1);
+        assert!(selected.contains_key("present"));
+        assert!(!selected.contains_key("missing"));
+    }
+
+    #[test]
+    fn select_fetch_variables_for_no_usage_entries() {
+        let mut variable_values_map = HashMap::new();
+        variable_values_map.insert("unused_1".to_string(), value_from_number(1));
+        variable_values_map.insert("unused_2".to_string(), value_from_number(2));
+
+        let variable_values = Some(variable_values_map);
+
+        let selected = select_fetch_variables(&variable_values, None);
+
+        assert!(selected.is_none());
+    }
+}