always use variable deduplication

NEXT_CHANGELOG.md

@@ -129,6 +129,11 @@ Users can disable this mechanism by setting the environment variable `APOLLO_TEL
 
 By [@bryncooke](https://github.com/bryncooke) in https://github.com/apollographql/router/pull/2173, https://github.com/apollographql/router/issues/2398, https://github.com/apollographql/router/pull/2413
 
+### Always deduplicate variables ([Issue #2387](https://github.com/apollographql/router/issues/2387))
+
+Variable deduplication allows the router to reduce the number of entities that are requested from subgraphs if some of them are redundant, and as such reduce the size of subgraph responses. It has been available for a while but was not active by default. This is now always on.
+
+By [@Geal](https://github.com/geal) in https://github.com/apollographql/router/pull/2445
 
 ## 🐛 Fixes
 

apollo-router/src/configuration/mod.rs

@@ -293,13 +293,6 @@ impl Configuration {
         plugins
     }
 
-    pub(crate) fn plugin_configuration(&self, plugin_name: &str) -> Option<Value> {
-        self.plugins()
-            .iter()
-            .find(|(name, _)| name == plugin_name)
-            .map(|(_, value)| value.clone())
-    }
-
     // checks that we can reload configuration from the current one to the new one
     pub(crate) fn is_compatible(&self, new: &Configuration) -> Result<(), &'static str> {
         if self.apollo_plugins.plugins.get(TELEMETRY_KEY)

apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap

@@ -3223,7 +3223,7 @@ expression: "&schema"
           "nullable": true
         },
         "deduplicate_variables": {
-          "description": "Enable variable deduplication optimization when sending requests to subgraphs (https://github.com/apollographql/router/issues/87)",
+          "description": "DEPRECATED, now always enabled: Enable variable deduplication optimization when sending requests to subgraphs (https://github.com/apollographql/router/issues/87)",
           "type": "boolean",
           "nullable": true
         },

apollo-router/src/plugin/test/mock/canned.rs

@@ -21,10 +21,6 @@ pub(crate) fn accounts_subgraph() -> MockSubgraph {
                                 "__typename": "User",
                                 "id": "2"
                             },
-                            {
-                                "__typename": "User",
-                                "id": "1"
-                            }
                         ]
                     }
                 }},
@@ -37,9 +33,6 @@ pub(crate) fn accounts_subgraph() -> MockSubgraph {
                             {
                                 "name": "Alan Turing"
                             },
-                            {
-                                "name": "Ada Lovelace"
-                            }
                         ]
                     }
                 }}
@@ -158,10 +151,6 @@ pub(crate) fn products_subgraph() -> MockSubgraph {
                                 "__typename": "Product",
                                 "upc": "1"
                             },
-                            {
-                                "__typename": "Product",
-                                "upc": "1"
-                            },
                             {
                                 "__typename": "Product",
                                 "upc": "2"
@@ -172,9 +161,6 @@ pub(crate) fn products_subgraph() -> MockSubgraph {
             json!{{
                     "data": {
                         "_entities": [
-                            {
-                                "name": "Table"
-                            },
                             {
                                 "name": "Table"
                             },

apollo-router/src/plugins/expose_query_plan.rs

@@ -158,8 +158,8 @@ mod tests {
 
         let account_mocks = vec![
             (
-                r#"{"query":"query TopProducts__accounts__3($representations:[_Any!]!){_entities(representations:$representations){...on User{name}}}","operationName":"TopProducts__accounts__3","variables":{"representations":[{"__typename":"User","id":"1"},{"__typename":"User","id":"2"},{"__typename":"User","id":"1"}]}}"#,
-                r#"{"data":{"_entities":[{"name":"Ada Lovelace"},{"name":"Alan Turing"},{"name":"Ada Lovelace"}]}}"#
+                r#"{"query":"query TopProducts__accounts__3($representations:[_Any!]!){_entities(representations:$representations){...on User{name}}}","operationName":"TopProducts__accounts__3","variables":{"representations":[{"__typename":"User","id":"1"},{"__typename":"User","id":"2"}]}}"#,
+                r#"{"data":{"_entities":[{"name":"Ada Lovelace"},{"name":"Alan Turing"}]}}"#
             )
         ].into_iter().map(|(query, response)| (serde_json::from_str(query).unwrap(), serde_json::from_str(response).unwrap())).collect();
         let account_service = MockSubgraph::new(account_mocks);
@@ -178,8 +178,8 @@ mod tests {
                 r#"{"data":{"topProducts":[{"__typename":"Product","upc":"1","name":"Table"},{"__typename":"Product","upc":"2","name":"Couch"}]}}"#
             ),
             (
-                r#"{"query":"query TopProducts__products__2($representations:[_Any!]!){_entities(representations:$representations){...on Product{name}}}","operationName":"TopProducts__products__2","variables":{"representations":[{"__typename":"Product","upc":"1"},{"__typename":"Product","upc":"1"},{"__typename":"Product","upc":"2"}]}}"#,
-                r#"{"data":{"_entities":[{"name":"Table"},{"name":"Table"},{"name":"Couch"}]}}"#
+                r#"{"query":"query TopProducts__products__2($representations:[_Any!]!){_entities(representations:$representations){...on Product{name}}}","operationName":"TopProducts__products__2","variables":{"representations":[{"__typename":"Product","upc":"1"},{"__typename":"Product","upc":"2"}]}}"#,
+                r#"{"data":{"_entities":[{"name":"Table"},{"name":"Couch"}]}}"#
             )
             ].into_iter().map(|(query, response)| (serde_json::from_str(query).unwrap(), serde_json::from_str(response).unwrap())).collect();
 

apollo-router/src/plugins/include_subgraph_errors.rs

@@ -159,8 +159,8 @@ mod test {
 
         let account_mocks = vec![
             (
-                r#"{"query":"query TopProducts__accounts__3($representations:[_Any!]!){_entities(representations:$representations){...on User{name}}}","operationName":"TopProducts__accounts__3","variables":{"representations":[{"__typename":"User","id":"1"},{"__typename":"User","id":"2"},{"__typename":"User","id":"1"}]}}"#,
-                r#"{"data":{"_entities":[{"name":"Ada Lovelace"},{"name":"Alan Turing"},{"name":"Ada Lovelace"}]}}"#
+                r#"{"query":"query TopProducts__accounts__3($representations:[_Any!]!){_entities(representations:$representations){...on User{name}}}","operationName":"TopProducts__accounts__3","variables":{"representations":[{"__typename":"User","id":"1"},{"__typename":"User","id":"2"}]}}"#,
+                r#"{"data":{"_entities":[{"name":"Ada Lovelace"},{"name":"Alan Turing"}]}}"#
             )
         ].into_iter().map(|(query, response)| (serde_json::from_str(query).unwrap(), serde_json::from_str(response).unwrap())).collect();
         let account_service = MockSubgraph::new(account_mocks);
@@ -179,8 +179,8 @@ mod test {
                 r#"{"data":{"topProducts":[{"__typename":"Product","upc":"1","name":"Table"},{"__typename":"Product","upc":"2","name":"Couch"}]}}"#
             ),
             (
-                r#"{"query":"query TopProducts__products__2($representations:[_Any!]!){_entities(representations:$representations){...on Product{name}}}","operationName":"TopProducts__products__2","variables":{"representations":[{"__typename":"Product","upc":"1"},{"__typename":"Product","upc":"1"},{"__typename":"Product","upc":"2"}]}}"#,
-                r#"{"data":{"_entities":[{"name":"Table"},{"name":"Table"},{"name":"Couch"}]}}"#
+                r#"{"query":"query TopProducts__products__2($representations:[_Any!]!){_entities(representations:$representations){...on Product{name}}}","operationName":"TopProducts__products__2","variables":{"representations":[{"__typename":"Product","upc":"1"},{"__typename":"Product","upc":"2"}]}}"#,
+                r#"{"data":{"_entities":[{"name":"Table"},{"name":"Couch"}]}}"#
             )
             ].into_iter().map(|(query, response)| (serde_json::from_str(query).unwrap(), serde_json::from_str(response).unwrap())).collect();
 

apollo-router/src/plugins/traffic_shaping/mod.rs

@@ -45,7 +45,6 @@ use crate::services::subgraph;
 use crate::services::subgraph_service::Compression;
 use crate::services::supergraph;
 use crate::services::SubgraphRequest;
-use crate::Configuration;
 
 const DEFAULT_TIMEOUT: Duration = Duration::from_secs(30);
 pub(crate) const APOLLO_TRAFFIC_SHAPING: &str = "apollo.traffic_shaping";
@@ -161,7 +160,7 @@ pub(crate) struct Config {
     #[serde(default)]
     /// Applied on specific subgraphs
     subgraphs: HashMap<String, Shaping>,
-    /// Enable variable deduplication optimization when sending requests to subgraphs (https://github.com/apollographql/router/issues/87)
+    /// DEPRECATED, now always enabled: Enable variable deduplication optimization when sending requests to subgraphs (https://github.com/apollographql/router/issues/87)
     deduplicate_variables: Option<bool>,
 }
 
@@ -368,15 +367,6 @@ impl TrafficShaping {
     }
 }
 
-impl TrafficShaping {
-    pub(crate) fn get_configuration_deduplicate_variables(configuration: &Configuration) -> bool {
-        configuration
-            .plugin_configuration(APOLLO_TRAFFIC_SHAPING)
-            .map(|conf| conf.get("deduplicate_variables") == Some(&serde_json::Value::Bool(true)))
-            .unwrap_or_default()
-    }
-}
-
 register_plugin!("apollo", "traffic_shaping", TrafficShaping);
 
 #[cfg(test)]

apollo-router/src/query_planner/bridge_query_planner.rs

@@ -17,11 +17,9 @@ use tracing::Instrument;
 
 use super::PlanNode;
 use super::QueryKey;
-use super::QueryPlanOptions;
 use crate::error::QueryPlannerError;
 use crate::graphql;
 use crate::introspection::Introspection;
-use crate::plugins::traffic_shaping::TrafficShaping;
 use crate::services::QueryPlannerContent;
 use crate::services::QueryPlannerRequest;
 use crate::services::QueryPlannerResponse;
@@ -41,7 +39,6 @@ pub(crate) struct BridgeQueryPlanner {
     schema: Arc<Schema>,
     introspection: Option<Arc<Introspection>>,
     configuration: Arc<Configuration>,
-    deduplicate_variables: bool,
 }
 
 impl BridgeQueryPlanner {
@@ -50,9 +47,6 @@ impl BridgeQueryPlanner {
         introspection: Option<Arc<Introspection>>,
         configuration: Arc<Configuration>,
     ) -> Result<Self, QueryPlannerError> {
-        // FIXME: The variables deduplication parameter lives in the traffic_shaping section of the config
-        let deduplicate_variables =
-            TrafficShaping::get_configuration_deduplicate_variables(&configuration);
         Ok(Self {
             planner: Arc::new(
                 Planner::new(
@@ -68,7 +62,6 @@ impl BridgeQueryPlanner {
             schema,
             introspection,
             configuration,
-            deduplicate_variables,
         })
     }
 
@@ -134,9 +127,6 @@ impl BridgeQueryPlanner {
                         root: node,
                         formatted_query_plan,
                         query: Arc::new(selections),
-                        options: QueryPlanOptions {
-                            enable_deduplicate_variables: self.deduplicate_variables,
-                        },
                     }),
                 })
             }

apollo-router/src/query_planner/caching_query_planner.rs

@@ -288,7 +288,6 @@ mod tests {
     use super::*;
     use crate::error::PlanErrors;
     use crate::query_planner::QueryPlan;
-    use crate::query_planner::QueryPlanOptions;
     use crate::spec::Query;
 
     mock! {
@@ -384,7 +383,6 @@ mod tests {
                 let query_plan: QueryPlan = QueryPlan {
                     formatted_query_plan: Default::default(),
                     root: serde_json::from_str(test_query_plan!()).unwrap(),
-                    options: QueryPlanOptions::default(),
                     usage_reporting: UsageReporting {
                         stats_report_key: "this is a test report key".to_string(),
                         referenced_fields_by_type: Default::default(),

apollo-router/src/query_planner/execution.rs

@@ -11,7 +11,6 @@ use super::log;
 use super::DeferredNode;
 use super::PlanNode;
 use super::QueryPlan;
-use super::QueryPlanOptions;
 use crate::error::Error;
 use crate::graphql::Request;
 use crate::graphql::Response;
@@ -59,7 +58,6 @@ impl QueryPlan {
                     supergraph_request,
                     deferred_fetches: &deferred_fetches,
                     query: &self.query,
-                    options: &self.options,
                 },
                 &root,
                 &Value::default(),
@@ -87,7 +85,6 @@ pub(crate) struct ExecutionParameters<'a> {
     pub(crate) supergraph_request: &'a Arc<http::Request<Request>>,
     pub(crate) deferred_fetches: &'a HashMap<String, Sender<(Value, Vec<Error>)>>,
     pub(crate) query: &'a Arc<Query>,
-    pub(crate) options: &'a QueryPlanOptions,
 }
 
 impl PlanNode {
@@ -247,7 +244,6 @@ impl PlanNode {
                                         schema: parameters.schema,
                                         supergraph_request: parameters.supergraph_request,
                                         deferred_fetches: &deferred_fetches,
-                                        options: parameters.options,
                                         query: parameters.query,
                                     },
                                     current_dir,
@@ -391,7 +387,6 @@ impl DeferredNode {
         let orig = parameters.supergraph_request.clone();
         let sf = parameters.service_factory.clone();
         let ctx = parameters.context.clone();
-        let opt = parameters.options.clone();
         let query = parameters.query.clone();
         let mut primary_receiver = primary_sender.subscribe();
         let mut value = parent_value.clone();
@@ -429,7 +424,6 @@ impl DeferredNode {
                             supergraph_request: &orig,
                             deferred_fetches: &deferred_fetches,
                             query: &query,
-                            options: &opt,
                         },
                         &Path::default(),
                         &value,

apollo-router/src/query_planner/fetch.rs

@@ -99,7 +99,6 @@ impl Variables {
         current_dir: &Path,
         request: &Arc<http::Request<Request>>,
         schema: &Schema,
-        enable_deduplicate_variables: bool,
     ) -> Option<Variables> {
         let body = request.body();
         if !requires.is_empty() {
@@ -112,46 +111,30 @@ impl Variables {
             }));
 
             let mut paths: HashMap<Path, usize> = HashMap::new();
-            let (paths, representations) = if enable_deduplicate_variables {
-                let mut values: IndexSet<Value> = IndexSet::new();
-                data.select_values_and_paths(current_dir, |path, value| {
-                    if let Value::Object(content) = value {
-                        if let Ok(Some(value)) = select_object(content, requires, schema) {
-                            match values.get_index_of(&value) {
-                                Some(index) => {
-                                    paths.insert(path.clone(), index);
-                                }
-                                None => {
-                                    paths.insert(path.clone(), values.len());
-                                    values.insert(value);
-                                }
+            let mut values: IndexSet<Value> = IndexSet::new();
+
+            data.select_values_and_paths(current_dir, |path, value| {
+                if let Value::Object(content) = value {
+                    if let Ok(Some(value)) = select_object(content, requires, schema) {
+                        match values.get_index_of(&value) {
+                            Some(index) => {
+                                paths.insert(path.clone(), index);
+                            }
+                            None => {
+                                paths.insert(path.clone(), values.len());
+                                values.insert(value);
                             }
                         }
                     }
-                });
-
-                if values.is_empty() {
-                    return None;
                 }
+            });
 
-                (paths, Value::Array(Vec::from_iter(values)))
-            } else {
-                let mut values: Vec<Value> = Vec::new();
-                data.select_values_and_paths(current_dir, |path, value| {
-                    if let Value::Object(content) = value {
-                        if let Ok(Some(value)) = select_object(content, requires, schema) {
-                            paths.insert(path.clone(), values.len());
-                            values.push(value);
-                        }
-                    }
-                });
+            if values.is_empty() {
+                return None;
+            }
 
-                if values.is_empty() {
-                    return None;
-                }
+            let representations = Value::Array(Vec::from_iter(values));
 
-                (paths, Value::Array(Vec::from_iter(values)))
-            };
             variables.insert("representations", representations);
 
             Some(Variables { variables, paths })
@@ -209,7 +192,6 @@ impl FetchNode {
             // Needs the original request here
             parameters.supergraph_request,
             parameters.schema,
-            parameters.options.enable_deduplicate_variables,
         )
         .await
         {

apollo-router/src/query_planner/plan.rs

@@ -17,12 +17,6 @@ use crate::spec::query::SubSelection;
 use crate::spec::Query;
 use crate::spec::Schema;
 
-/// Query planning options.
-#[derive(Clone, Eq, Hash, PartialEq, Debug, Default, Serialize, Deserialize)]
-pub(crate) struct QueryPlanOptions {
-    /// Enable the variable deduplication optimization on the QueryPlan
-    pub(crate) enable_deduplicate_variables: bool,
-}
 /// A planner key.
 ///
 /// This type consists of a query string and an optional operation string
@@ -36,7 +30,6 @@ pub struct QueryPlan {
     /// String representation of the query plan (not a json representation)
     pub(crate) formatted_query_plan: Option<String>,
     pub(crate) query: Arc<Query>,
-    pub(crate) options: QueryPlanOptions,
 }
 
 /// This default impl is useful for test users
@@ -56,7 +49,6 @@ impl QueryPlan {
             root: root.unwrap_or_else(|| PlanNode::Sequence { nodes: Vec::new() }),
             formatted_query_plan: Default::default(),
             query: Arc::new(Query::default()),
-            options: QueryPlanOptions::default(),
         }
     }
 }