From 6a0a6291032ea08a9de03753d6f50e91ac9f518f Mon Sep 17 00:00:00 2001 From: jonathanrainer Date: Wed, 31 Jul 2024 10:14:22 +0100 Subject: [PATCH] ROVER-69 Respond to PR comments --- Cargo.lock | 1 + crates/rover-client/Cargo.toml | 1 + .../operations/subgraph/fetch_all/runner.rs | 64 +++++++++---------- .../operations/subgraph/fetch_all/types.rs | 10 +++ src/utils/supergraph_config.rs | 35 ++++------ 5 files changed, 55 insertions(+), 56 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3235246fc..19250bb4c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4568,6 +4568,7 @@ dependencies = [ "regex", "reqwest 0.12.5", "rover-std", + "rstest", "semver", "serde", "serde_json", diff --git a/crates/rover-client/Cargo.toml b/crates/rover-client/Cargo.toml index a69a5ccd3..ca0f8b8ee 100644 --- a/crates/rover-client/Cargo.toml +++ b/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 } diff --git a/crates/rover-client/src/operations/subgraph/fetch_all/runner.rs b/crates/rover-client/src/operations/subgraph/fetch_all/runner.rs index 33af7da27..64cab5f8c 100644 --- a/crates/rover-client/src/operations/subgraph/fetch_all/runner.rs +++ b/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, 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(), diff --git a/crates/rover-client/src/operations/subgraph/fetch_all/types.rs b/crates/rover-client/src/operations/subgraph/fetch_all/types.rs index 7462dd6ee..67b511a58 100644 --- a/crates/rover-client/src/operations/subgraph/fetch_all/types.rs +++ b/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, sdl: String, } + +impl From for SubgraphConfig { + fn from(value: Subgraph) -> Self { + Self { + routing_url: value.url, + schema: SchemaSource::Sdl { sdl: value.sdl }, + } + } +} diff --git a/src/utils/supergraph_config.rs b/src/utils/supergraph_config.rs index ea929d3e2..c11b8c51c 100644 --- a/src/utils/supergraph_config.rs +++ b/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)