ROVER-69 Respond to PR comments

crates/rover-client/Cargo.toml

@@ -57,3 +57,4 @@ indoc = { workspace = true}
 httpmock = { workspace = true }
 pretty_assertions = { workspace = true }
 strip-ansi-escapes = { workspace = true }
+rstest = { workspace = true }

crates/rover-client/src/operations/subgraph/fetch_all/runner.rs

@@ -1,7 +1,6 @@
 use graphql_client::*;
 
 use crate::blocking::StudioClient;
-use crate::operations::config::is_federated::{self, IsFederatedInput};
 use crate::RoverClientError;
 
 use super::types::*;
@@ -34,50 +33,48 @@ fn get_subgraphs_from_response_data(
     input: SubgraphFetchAllInput,
     response_data: SubgraphFetchAllResponseData,
 ) -> Result<Vec<Subgraph>, RoverClientError> {
-    if let Some(maybe_variant) = response_data.variant {
-        match maybe_variant {
-            SubgraphFetchAllGraphVariant::GraphVariant(variant) => {
-                if let Some(subgraphs) = variant.subgraphs {
-                    Ok(subgraphs
-                        .into_iter()
-                        .map(|subgraph| {
-                            Subgraph::builder()
-                                .name(subgraph.name.clone())
-                                .and_url(subgraph.url)
-                                .sdl(subgraph.active_partial_schema.sdl)
-                                .build()
-                        })
-                        .collect())
-                } else {
-                    Err(RoverClientError::ExpectedFederatedGraph {
-                        graph_ref: input.graph_ref,
-                        can_operation_convert: true,
-                    })
-                }
-            }
-            _ => Err(RoverClientError::InvalidGraphRef),
-        }
-    } else {
-        Err(RoverClientError::GraphNotFound {
+    match response_data.variant {
+        None => Err(RoverClientError::GraphNotFound {
             graph_ref: input.graph_ref,
-        })
+        }),
+        Some(SubgraphFetchAllGraphVariant::GraphVariant(variant)) => variant.subgraphs.map_or_else(
+            || {
+                Err(RoverClientError::ExpectedFederatedGraph {
+                    graph_ref: input.graph_ref,
+                    can_operation_convert: true,
+                })
+            },
+            |subgraphs| {
+                Ok(subgraphs
+                    .into_iter()
+                    .map(|subgraph| {
+                        Subgraph::builder()
+                            .name(subgraph.name.clone())
+                            .and_url(subgraph.url)
+                            .sdl(subgraph.active_partial_schema.sdl)
+                            .build()
+                    })
+                    .collect())
+            },
+        ),
+        _ => Err(RoverClientError::InvalidGraphRef),
     }
 }
 
 #[cfg(test)]
 mod tests {
+    use rstest::{fixture, rstest};
     use serde_json::json;
 
     use crate::shared::GraphRef;
 
     use super::*;
 
-    #[test]
-    fn get_services_from_response_data_works() {
+    #[rstest]
+    fn get_services_from_response_data_works(#[from(mock_input)] input: SubgraphFetchAllInput) {
         let sdl = "extend type User @key(fields: \"id\") {\n  id: ID! @external\n  age: Int\n}\n"
             .to_string();
         let url = "http://my.subgraph.com".to_string();
-        let input = mock_input();
         let json_response = json!({
             "variant": {
                 "__typename": "GraphVariant",
@@ -104,14 +101,15 @@ mod tests {
         assert_eq!(output.unwrap(), vec![expected_subgraph]);
     }
 
-    #[test]
-    fn get_services_from_response_data_errs_with_no_variant() {
+    #[rstest]
+    fn get_services_from_response_data_errs_with_no_variant(mock_input: SubgraphFetchAllInput) {
         let json_response = json!({ "variant": null });
         let data: SubgraphFetchAllResponseData = serde_json::from_value(json_response).unwrap();
-        let output = get_subgraphs_from_response_data(mock_input(), data);
+        let output = get_subgraphs_from_response_data(mock_input, data);
         assert!(output.is_err());
     }
 
+    #[fixture]
     fn mock_input() -> SubgraphFetchAllInput {
         let graph_ref = GraphRef {
             name: "mygraph".to_string(),

crates/rover-client/src/operations/subgraph/fetch_all/types.rs

@@ -1,3 +1,4 @@
+use apollo_federation_types::config::{SchemaSource, SubgraphConfig};
 use buildstructor::Builder;
 use derive_getters::Getters;
 
@@ -29,3 +30,12 @@ pub struct Subgraph {
     url: Option<String>,
     sdl: String,
 }
+
+impl From<Subgraph> for SubgraphConfig {
+    fn from(value: Subgraph) -> Self {
+        Self {
+            routing_url: value.url,
+            schema: SchemaSource::Sdl { sdl: value.sdl },
+        }
+    }
+}

src/utils/supergraph_config.rs

@@ -43,18 +43,8 @@ impl RemoteSubgraphs {
             client,
         )?;
         let subgraphs = subgraphs
-            .iter()
-            .map(|subgraph| {
-                (
-                    subgraph.name().clone(),
-                    SubgraphConfig {
-                        routing_url: subgraph.url().clone(),
-                        schema: SchemaSource::Sdl {
-                            sdl: subgraph.sdl().clone(),
-                        },
-                    },
-                )
-            })
+            .into_iter()
+            .map(|subgraph| (subgraph.name().clone(), subgraph.into()))
             .collect();
         let supergraph_config = SupergraphConfig::new(subgraphs, Some(federation_version.clone()));
         let remote_subgraphs = RemoteSubgraphs(supergraph_config);
@@ -109,17 +99,16 @@ pub fn get_supergraph_config(
     };
 
     // Merge Remote and Local Supergraph Configs
-    let supergraph_config = match remote_subgraphs {
-        Some(remote_subgraphs) => match supergraph_config {
-            Some(supergraph_config) => {
-                let mut merged_supergraph_config = remote_subgraphs.inner().clone();
-                merged_supergraph_config.merge_subgraphs(&supergraph_config);
-                eprintln!("{}merging supergraph schema files", Emoji::Merge,);
-                Some(merged_supergraph_config)
-            }
-            None => Some(remote_subgraphs.inner().clone()),
-        },
-        None => supergraph_config,
+    let supergraph_config = match (remote_subgraphs, supergraph_config) {
+        (Some(remote_subgraphs), Some(supergraph_config)) => {
+            let mut merged_supergraph_config = remote_subgraphs.inner().clone();
+            merged_supergraph_config.merge_subgraphs(&supergraph_config);
+            eprintln!("{}merging supergraph schema files", Emoji::Merge,);
+            Some(merged_supergraph_config)
+        }
+        (Some(remote_subgraphs), None) => Some(remote_subgraphs.inner().clone()),
+        (None, Some(supergraph_config)) => Some(supergraph_config),
+        (None, None) => None,
     };
     eprintln!("{}supergraph config loaded successfully", Emoji::Success,);
     Ok(supergraph_config)