From 0bb2092fd325c71228cd3074a2f1476a108d11f9 Mon Sep 17 00:00:00 2001 From: jonathanrainer Date: Tue, 10 Sep 2024 16:17:33 +0100 Subject: [PATCH 1/7] ROVER-135 Working version of resolution behaviour Still needs tests and will need a federation-rs PR merging to get it all aligned. --- Cargo.lock | 5 +- Cargo.toml | 2 +- src/utils/supergraph_config.rs | 527 ++++++++++++++++++--------------- 3 files changed, 284 insertions(+), 250 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e14b0e6e6..e4090f544 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -152,9 +152,8 @@ dependencies = [ [[package]] name = "apollo-federation-types" -version = "0.13.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bd88512dea257880f1e47d5541d14ba8a573b267fdc54af78f1b0bfdd3b73861" +version = "0.13.1" +source = "git+https://github.com/apollographql/federation-rs?branch=jr/ROVER-135#a198f474da4126707520f86c2fe4b7645d9b103c" dependencies = [ "camino", "log", diff --git a/Cargo.toml b/Cargo.toml index 97e6eebe1..4a606deda 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -69,7 +69,7 @@ apollo-parser = "0.8" apollo-encoder = "0.8" # https://github.com/apollographql/federation-rs -apollo-federation-types = "0.13.2" +apollo-federation-types = { git = "https://github.com/apollographql/federation-rs", branch = "jr/ROVER-135" } # crates.io dependencies ariadne = "0.4" diff --git a/src/utils/supergraph_config.rs b/src/utils/supergraph_config.rs index c24c23e1e..ca784fe5a 100644 --- a/src/utils/supergraph_config.rs +++ b/src/utils/supergraph_config.rs @@ -1,4 +1,5 @@ use std::collections::BTreeMap; +use std::env::current_dir; use std::str::FromStr; use anyhow::anyhow; @@ -7,8 +8,8 @@ use apollo_federation_types::config::{ FederationVersion, SchemaSource, SubgraphConfig, SupergraphConfig, }; use apollo_parser::{cst, Parser}; +use camino::Utf8PathBuf; use futures::future::join_all; - use rover_client::blocking::{GraphQLClient, StudioClient}; use rover_client::operations::subgraph; use rover_client::operations::subgraph::fetch::SubgraphFetchInput; @@ -105,9 +106,20 @@ pub async fn get_supergraph_config( // set up watchers on the subgraph sources. This branch is what `rover dev` uses. // So we run the `expand` function only to hydrate the YAML into a series of objects, // but we don't need to completely resolve all of those objects. - let config = file_descriptor + let mut config = file_descriptor .read_file_descriptor("supergraph config", &mut std::io::stdin()) .and_then(|contents| expand_supergraph_yaml(&contents))?; + // Once we have expanded the supergraph.yaml we need to make some changes to the paths + // to ensure we maintain correct semantics + config = match file_descriptor { + FileDescriptorType::Stdin => { + let current_dir = Utf8PathBuf::try_from(current_dir()?)?; + correctly_resolve_paths(config, ¤t_dir)? + } + FileDescriptorType::File(file_path) => { + correctly_resolve_paths(config, &file_path.parent().unwrap().to_path_buf())? + } + }; Some(config) } } else { @@ -124,241 +136,30 @@ pub async fn get_supergraph_config( Ok(supergraph_config) } -/// Merge local and remote supergraphs, making sure that the federation version is correct: eg, when -/// `--graph-ref` is passed, it should be the remote version; otherwise, it should be the local -/// version -fn merge_supergraph_configs( - remote_config: Option, - local_config: Option, - target_federation_version: Option<&FederationVersion>, -) -> Option { - // We use the federation version explicitly given at the command line as top - // priority; otherwise the version explicitly given in the local config - // file; otherwise the version fetched from Studio; otherwise LatestFedTwo. - let resolved_federation_version = target_federation_version - .cloned() - .or_else(|| { - local_config - .as_ref() - .and_then(|it| it.get_federation_version()) - }) - .or_else(|| { - remote_config - .as_ref() - .and_then(|it| it.get_federation_version()) - }) - .unwrap_or(FederationVersion::LatestFedTwo); - - match (remote_config, local_config) { - (Some(remote_config), Some(local_config)) => { - eprintln!("merging supergraph schema files"); - let mut merged_config = remote_config; - merged_config.merge_subgraphs(&local_config); - merged_config.set_federation_version(resolved_federation_version); - Some(merged_config) - } - (Some(remote_config), None) => { - let mut merged_config = remote_config; - merged_config.set_federation_version(resolved_federation_version); - Some(merged_config) - } - (None, Some(local_config)) => { - let mut merged_config = local_config; - merged_config.set_federation_version(resolved_federation_version); - Some(merged_config) - } - (None, None) => None, - } -} - -#[cfg(test)] -mod test_merge_supergraph_configs { - use super::*; - use rstest::{fixture, rstest}; - - #[fixture] - #[once] - fn local_supergraph_config_with_latest_fed_one_version() -> SupergraphConfig { - let federation_version_string = - format!("federation_version: {}\n", FederationVersion::LatestFedOne); - let subgraphs = "subgraphs: {}".to_string(); - let supergraph_yaml = format!("{}{}", federation_version_string, subgraphs); - let supergraph_config: SupergraphConfig = serde_yaml::from_str(&supergraph_yaml).unwrap(); - supergraph_config - } - - #[fixture] - #[once] - fn supergraph_config_without_fed_version() -> SupergraphConfig { - let supergraph_yaml = "subgraphs: {}".to_string(); - let supergraph_config: SupergraphConfig = serde_yaml::from_str(&supergraph_yaml).unwrap(); - supergraph_config - } - - #[fixture] - #[once] - fn remote_supergraph_config_with_latest_fed_two_version() -> SupergraphConfig { - let federation_version_string = - format!("federation_version: {}\n", FederationVersion::LatestFedTwo); - let subgraphs = "subgraphs: {}".to_string(); - let supergraph_yaml = format!("{}{}", federation_version_string, subgraphs); - let supergraph_config: SupergraphConfig = serde_yaml::from_str(&supergraph_yaml).unwrap(); - supergraph_config - } - - enum TestCase { - /* - * This block represents remote/local supergraph configs _with_ a fed version - * */ - // When both and target, target takes precedence - RemoteAndLocalWithTarget, - // When both and no target, local takes precendence unless it isn't set, in which case the - // latest fedceration version is used - RemoteAndLocalWithoutTarget, - // No remote, but local; target takes precendence - NoRemoteLocalWithTarget, - // No remote, but local; no target, local takes precedence - NoRemoteLocalWithoutTarget, - // Remote, no local, but with target; target takes precendence - RemoteNoLocalWithTarget, - // Remote, no local; no target, local takes precedence and if not present defaults to - // latest federation version - RemoteNoLocalWithoutTarget, - /* - * This block represents remote/local supergraph configs _without_ a fed version - * */ - // Precendence goes to latest fed version - RemoteNoFedVersionLocalNoFedVersion, - // Precedence goes to local - RemoteNoFedVersionLocalHasVersionNoTarget, - // Precedence goes to remote - RemoteFedVersionLocalNoFedVersionNoTarget, - } - - #[rstest] - #[case::remote_and_local_with_target( - TestCase::RemoteAndLocalWithTarget, - Some(FederationVersion::LatestFedOne), - // Expected because target - FederationVersion::LatestFedOne - )] - #[case::remote_and_local_without_target( - TestCase::RemoteAndLocalWithoutTarget, - None, - // Expected because local has fed one - FederationVersion::LatestFedOne - )] - #[case::no_remote_and_local_with_target( - TestCase::NoRemoteLocalWithTarget, - // Target is fed two because local has fed one - Some(FederationVersion::LatestFedTwo), - // Expected because target - FederationVersion::LatestFedTwo - )] - #[case::no_remote_and_local_without_target( - TestCase::NoRemoteLocalWithoutTarget, - None, - // Expected because local has fed one - FederationVersion::LatestFedOne - )] - #[case::remote_no_local_with_target( - TestCase::RemoteNoLocalWithTarget, - // Tasrget is fed one because remote has fed two - Some(FederationVersion::LatestFedOne), - // Expected because target - FederationVersion::LatestFedOne - )] - #[case::remote_no_local_without_target( - TestCase::RemoteNoLocalWithoutTarget, - None, - // Expected because remote is fed two - FederationVersion::LatestFedTwo - )] - #[case::remote_no_fed_local_no_fed( - TestCase::RemoteNoFedVersionLocalNoFedVersion, - None, - // Expected because latest - FederationVersion::LatestFedTwo - )] - #[case::remote_no_fed_local_has_version_no_target( - TestCase::RemoteNoFedVersionLocalHasVersionNoTarget, - None, - // Expected because local - FederationVersion::LatestFedOne - )] - #[case::remote_no_fed_local_has_version_no_target( - TestCase::RemoteFedVersionLocalNoFedVersionNoTarget, - None, - // Expected because remote - FederationVersion::LatestFedTwo - )] - fn it_merges_local_and_remote_supergraphs( - #[case] test_case: TestCase, - #[case] target_federation_version: Option, - #[case] expected_federation_version: FederationVersion, - local_supergraph_config_with_latest_fed_one_version: &SupergraphConfig, - remote_supergraph_config_with_latest_fed_two_version: &SupergraphConfig, - supergraph_config_without_fed_version: &SupergraphConfig, - ) { - let federation_version = match test_case { - TestCase::RemoteAndLocalWithTarget | TestCase::RemoteAndLocalWithoutTarget => { - merge_supergraph_configs( - Some(remote_supergraph_config_with_latest_fed_two_version.clone()), - Some(local_supergraph_config_with_latest_fed_one_version.clone()), - target_federation_version.as_ref(), - ) - .unwrap() - .get_federation_version() - .expect("no federation version, but there should always be a federation version") - } - TestCase::NoRemoteLocalWithTarget | TestCase::NoRemoteLocalWithoutTarget => { - merge_supergraph_configs( - None, - Some(local_supergraph_config_with_latest_fed_one_version.clone()), - target_federation_version.as_ref(), - ) - .unwrap() - .get_federation_version() - .expect("no federation version, but there should always be a federation version") - } - TestCase::RemoteNoLocalWithTarget | TestCase::RemoteNoLocalWithoutTarget => { - merge_supergraph_configs( - Some(remote_supergraph_config_with_latest_fed_two_version.clone()), - None, - target_federation_version.as_ref(), - ) - .unwrap() - .get_federation_version() - .expect("no federation version, but there should always be a federation version") +fn correctly_resolve_paths( + supergraph_config: SupergraphConfig, + root_to_resolve_from: &Utf8PathBuf, +) -> Result { + supergraph_config + .into_iter() + .map(|(a, b)| match b.schema { + SchemaSource::File { file } => { + match root_to_resolve_from.join(file).canonicalize_utf8() { + Ok(canonical_file_name) => Ok(( + a, + SubgraphConfig { + routing_url: b.routing_url, + schema: SchemaSource::File { + file: canonical_file_name, + }, + }, + )), + Err(err) => Err(RoverError::new(err)), + } } - TestCase::RemoteNoFedVersionLocalNoFedVersion => merge_supergraph_configs( - Some(supergraph_config_without_fed_version.clone()), - Some(supergraph_config_without_fed_version.clone()), - target_federation_version.as_ref(), - ) - .unwrap() - .get_federation_version() - .expect("no federation version, but there should always be a federation version"), - TestCase::RemoteNoFedVersionLocalHasVersionNoTarget => merge_supergraph_configs( - Some(supergraph_config_without_fed_version.clone()), - Some(local_supergraph_config_with_latest_fed_one_version.clone()), - target_federation_version.as_ref(), - ) - .unwrap() - .get_federation_version() - .expect("no federation version, but there should always be a federation version"), - TestCase::RemoteFedVersionLocalNoFedVersionNoTarget => merge_supergraph_configs( - Some(remote_supergraph_config_with_latest_fed_two_version.clone()), - Some(supergraph_config_without_fed_version.clone()), - target_federation_version.as_ref(), - ) - .unwrap() - .get_federation_version() - .expect("no federation version, but there should always be a federation version"), - }; - - assert_eq!(federation_version, expected_federation_version); - } + _ => Ok((a, b)), + }) + .collect() } #[cfg(test)] @@ -651,6 +452,243 @@ mod test_get_supergraph_config { } } +/// Merge local and remote supergraphs, making sure that the federation version is correct: eg, when +/// `--graph-ref` is passed, it should be the remote version; otherwise, it should be the local +/// version +fn merge_supergraph_configs( + remote_config: Option, + local_config: Option, + target_federation_version: Option<&FederationVersion>, +) -> Option { + match (remote_config, local_config) { + (Some(remote_config), Some(local_config)) => { + eprintln!("merging supergraph schema files"); + let mut merged_config = remote_config; + merged_config.merge_subgraphs(&local_config); + let federation_version = + resolve_federation_version(target_federation_version.cloned(), &local_config); + merged_config.set_federation_version(federation_version); + Some(merged_config) + } + (Some(remote_config), None) => { + let federation_version = + resolve_federation_version(target_federation_version.cloned(), &remote_config); + let mut merged_config = remote_config; + merged_config.set_federation_version(federation_version); + Some(merged_config) + } + (None, Some(local_config)) => { + let federation_version = + resolve_federation_version(target_federation_version.cloned(), &local_config); + let mut merged_config = local_config; + merged_config.set_federation_version(federation_version); + Some(merged_config) + } + (None, None) => None, + } +} + +#[cfg(test)] +mod test_merge_supergraph_configs { + use super::*; + use rstest::{fixture, rstest}; + + #[fixture] + #[once] + fn local_supergraph_config_with_latest_fed_one_version() -> SupergraphConfig { + let federation_version_string = + format!("federation_version: {}\n", FederationVersion::LatestFedOne); + let subgraphs = "subgraphs: {}".to_string(); + let supergraph_yaml = format!("{}{}", federation_version_string, subgraphs); + let supergraph_config: SupergraphConfig = serde_yaml::from_str(&supergraph_yaml).unwrap(); + supergraph_config + } + + #[fixture] + #[once] + fn supergraph_config_without_fed_version() -> SupergraphConfig { + let supergraph_yaml = "subgraphs: {}".to_string(); + let supergraph_config: SupergraphConfig = serde_yaml::from_str(&supergraph_yaml).unwrap(); + supergraph_config + } + + #[fixture] + #[once] + fn remote_supergraph_config_with_latest_fed_two_version() -> SupergraphConfig { + let federation_version_string = + format!("federation_version: {}\n", FederationVersion::LatestFedTwo); + let subgraphs = "subgraphs: {}".to_string(); + let supergraph_yaml = format!("{}{}", federation_version_string, subgraphs); + let supergraph_config: SupergraphConfig = serde_yaml::from_str(&supergraph_yaml).unwrap(); + supergraph_config + } + + enum TestCase { + /* + * This block represents remote/local supergraph configs _with_ a fed version + * */ + // When both and target, target takes precedence + RemoteAndLocalWithTarget, + // When both and no target, local takes precendence unless it isn't set, in which case the + // latest fedceration version is used + RemoteAndLocalWithoutTarget, + // No remote, but local; target takes precendence + NoRemoteLocalWithTarget, + // No remote, but local; no target, local takes precedence + NoRemoteLocalWithoutTarget, + // Remote, no local, but with target; target takes precendence + RemoteNoLocalWithTarget, + // Remote, no local; no target, local takes precedence and if not present defaults to + // latest federation version + RemoteNoLocalWithoutTarget, + /* + * This block represents remote/local supergraph configs _without_ a fed version + * */ + // Precendence goes to latest fed version + RemoteNoFedVersionLocalNoFedVersion, + // Precedence goes to local + RemoteNoFedVersionLocalHasVersionNoTarget, + // Precedence goes to remote + RemoteFedVersionLocalNoFedVersionNoTarget, + } + + #[rstest] + #[case::remote_and_local_with_target( + TestCase::RemoteAndLocalWithTarget, + Some(FederationVersion::LatestFedOne), + // Expected because target + FederationVersion::LatestFedOne + )] + #[case::remote_and_local_without_target( + TestCase::RemoteAndLocalWithoutTarget, + None, + // Expected because local has fed one + FederationVersion::LatestFedOne + )] + #[case::no_remote_and_local_with_target( + TestCase::NoRemoteLocalWithTarget, + // Target is fed two because local has fed one + Some(FederationVersion::LatestFedTwo), + // Expected because target + FederationVersion::LatestFedTwo + )] + #[case::no_remote_and_local_without_target( + TestCase::NoRemoteLocalWithoutTarget, + None, + // Expected because local has fed one + FederationVersion::LatestFedOne + )] + #[case::remote_no_local_with_target( + TestCase::RemoteNoLocalWithTarget, + // Tasrget is fed one because remote has fed two + Some(FederationVersion::LatestFedOne), + // Expected because target + FederationVersion::LatestFedOne + )] + #[case::remote_no_local_without_target( + TestCase::RemoteNoLocalWithoutTarget, + None, + // Expected because remote is fed two + FederationVersion::LatestFedTwo + )] + #[case::remote_no_fed_local_no_fed( + TestCase::RemoteNoFedVersionLocalNoFedVersion, + None, + // Expected because latest + FederationVersion::LatestFedTwo + )] + #[case::remote_no_fed_local_has_version_no_target( + TestCase::RemoteNoFedVersionLocalHasVersionNoTarget, + None, + // Expected because local + FederationVersion::LatestFedOne + )] + #[case::remote_no_fed_local_has_version_no_target( + TestCase::RemoteFedVersionLocalNoFedVersionNoTarget, + None, + // Expected because remote + FederationVersion::LatestFedTwo + )] + fn it_merges_local_and_remote_supergraphs( + #[case] test_case: TestCase, + #[case] target_federation_version: Option, + #[case] expected_federation_version: FederationVersion, + local_supergraph_config_with_latest_fed_one_version: &SupergraphConfig, + remote_supergraph_config_with_latest_fed_two_version: &SupergraphConfig, + supergraph_config_without_fed_version: &SupergraphConfig, + ) { + let federation_version = match test_case { + TestCase::RemoteAndLocalWithTarget | TestCase::RemoteAndLocalWithoutTarget => { + merge_supergraph_configs( + Some(remote_supergraph_config_with_latest_fed_two_version.clone()), + Some(local_supergraph_config_with_latest_fed_one_version.clone()), + target_federation_version.as_ref(), + ) + .unwrap() + .get_federation_version() + .expect("no federation version, but there should always be a federation version") + } + TestCase::NoRemoteLocalWithTarget | TestCase::NoRemoteLocalWithoutTarget => { + merge_supergraph_configs( + None, + Some(local_supergraph_config_with_latest_fed_one_version.clone()), + target_federation_version.as_ref(), + ) + .unwrap() + .get_federation_version() + .expect("no federation version, but there should always be a federation version") + } + TestCase::RemoteNoLocalWithTarget | TestCase::RemoteNoLocalWithoutTarget => { + merge_supergraph_configs( + Some(remote_supergraph_config_with_latest_fed_two_version.clone()), + None, + target_federation_version.as_ref(), + ) + .unwrap() + .get_federation_version() + .expect("no federation version, but there should always be a federation version") + } + TestCase::RemoteNoFedVersionLocalNoFedVersion => merge_supergraph_configs( + Some(supergraph_config_without_fed_version.clone()), + Some(supergraph_config_without_fed_version.clone()), + target_federation_version.as_ref(), + ) + .unwrap() + .get_federation_version() + .expect("no federation version, but there should always be a federation version"), + TestCase::RemoteNoFedVersionLocalHasVersionNoTarget => merge_supergraph_configs( + Some(supergraph_config_without_fed_version.clone()), + Some(local_supergraph_config_with_latest_fed_one_version.clone()), + target_federation_version.as_ref(), + ) + .unwrap() + .get_federation_version() + .expect("no federation version, but there should always be a federation version"), + TestCase::RemoteFedVersionLocalNoFedVersionNoTarget => merge_supergraph_configs( + Some(remote_supergraph_config_with_latest_fed_two_version.clone()), + Some(supergraph_config_without_fed_version.clone()), + target_federation_version.as_ref(), + ) + .unwrap() + .get_federation_version() + .expect("no federation version, but there should always be a federation version"), + }; + + assert_eq!(federation_version, expected_federation_version); + } +} + +fn resolve_federation_version( + requested_federation_version: Option, + supergraph_config: &SupergraphConfig, +) -> FederationVersion { + requested_federation_version.unwrap_or_else(|| { + supergraph_config + .get_federation_version() + .unwrap_or_else(|| FederationVersion::LatestFedTwo) + }) +} + pub(crate) async fn resolve_supergraph_yaml( unresolved_supergraph_yaml: &FileDescriptorType, client_config: StudioClientConfig, @@ -671,9 +709,6 @@ pub(crate) async fn resolve_supergraph_yaml( .into_iter() .collect::>(); - // WARNING: this is a departure from how both main and geal's branch work; by collecting the - // futs we're able to run them all at once rather than in parallel (even when async); takes - // resolution down from ~1min for 100 subgraphs to ~10s let futs = supergraph_config .iter() .map(|(subgraph_name, subgraph_data)| async { @@ -916,13 +951,6 @@ pub(crate) async fn resolve_supergraph_yaml( Ok(resolved_supergraph_config) } -fn expand_supergraph_yaml(content: &str) -> RoverResult { - serde_yaml::from_str(content) - .map_err(RoverError::from) - .and_then(expand) - .and_then(|v| serde_yaml::from_value(v).map_err(RoverError::from)) -} - #[cfg(test)] mod test_resolve_supergraph_yaml { use std::fs; @@ -1548,3 +1576,10 @@ type _Service {\n sdl: String\n}"#; Ok(()) } } + +fn expand_supergraph_yaml(content: &str) -> RoverResult { + serde_yaml::from_str(content) + .map_err(RoverError::from) + .and_then(expand) + .and_then(|v| serde_yaml::from_value(v).map_err(RoverError::from)) +} From 6fec97965bffcb48bd7cd2af97021fb4fa5fa753 Mon Sep 17 00:00:00 2001 From: jonathanrainer Date: Wed, 4 Sep 2024 14:55:56 +0100 Subject: [PATCH 2/7] ROVER-135 Fix behaviour w.r.t erroring --- src/error/metadata/suggestion.rs | 20 ++++++++----- src/utils/supergraph_config.rs | 49 ++++++++++++++++++++++---------- 2 files changed, 47 insertions(+), 22 deletions(-) diff --git a/src/error/metadata/suggestion.rs b/src/error/metadata/suggestion.rs index 513086b04..84c46cdc3 100644 --- a/src/error/metadata/suggestion.rs +++ b/src/error/metadata/suggestion.rs @@ -1,10 +1,9 @@ -use std::cmp::Ordering; -use std::fmt::{self, Display, Write as _}; - +use crate::utils::env::RoverEnvKey; +use camino::Utf8PathBuf; use rover_client::shared::GraphRef; use rover_std::Style; - -use crate::utils::env::RoverEnvKey; +use std::cmp::Ordering; +use std::fmt::{self, Display, Write as _}; use serde::Serialize; @@ -79,6 +78,10 @@ pub enum RoverErrorSuggestion { frontend_url_root: String, }, AddRoutingUrlToSupergraphYaml, + InvalidSupergraphYamlSubgraphSchemaPath { + subgraph_name: String, + supergraph_yaml_path: Utf8PathBuf, + }, PublishSubgraphWithRoutingUrl { subgraph_name: String, graph_ref: String, @@ -256,8 +259,11 @@ UpgradePlan => "Rover has likely reached rate limits while running graph or subg format!("Try publishing the subgraph with a routing URL like so `rover subgraph publish {graph_ref} --name {subgraph_name} --routing-url `") }, AllowInvalidRoutingUrlOrSpecifyValidUrl => format!("Try publishing the subgraph with a valid routing URL. If you are sure you want to publish an invalid routing URL, re-run this command with the {} option.", Style::Command.paint("`--allow-invalid-routing-url`")), - ContactApolloAccountManager => {"Discuss your requirements with your Apollo point of contact.".to_string()} - TryAgainLater => {"Please try again later.".to_string()} + ContactApolloAccountManager => {"Discuss your requirements with your Apollo point of contact.".to_string()}, + TryAgainLater => {"Please try again later.".to_string()}, + InvalidSupergraphYamlSubgraphSchemaPath { + subgraph_name, supergraph_yaml_path + } => format!("Make sure the specified path for subgraph '{}' is relative to the location of the supergraph schema file ({})", subgraph_name, Style::Path.paint(supergraph_yaml_path)) }; write!(formatter, "{}", &suggestion) } diff --git a/src/utils/supergraph_config.rs b/src/utils/supergraph_config.rs index ca784fe5a..139be7a94 100644 --- a/src/utils/supergraph_config.rs +++ b/src/utils/supergraph_config.rs @@ -1,5 +1,6 @@ use std::collections::BTreeMap; use std::env::current_dir; +use std::path; use std::str::FromStr; use anyhow::anyhow; @@ -26,6 +27,7 @@ use crate::options::ProfileOpt; use crate::utils::client::StudioClientConfig; use crate::utils::expansion::expand; use crate::utils::parsers::FileDescriptorType; +use crate::RoverErrorSuggestion::InvalidSupergraphYamlSubgraphSchemaPath; use crate::{RoverError, RoverErrorSuggestion, RoverResult}; /// Nominal type that captures the behavior of collecting remote subgraphs into a @@ -142,23 +144,40 @@ fn correctly_resolve_paths( ) -> Result { supergraph_config .into_iter() - .map(|(a, b)| match b.schema { - SchemaSource::File { file } => { - match root_to_resolve_from.join(file).canonicalize_utf8() { - Ok(canonical_file_name) => Ok(( - a, - SubgraphConfig { - routing_url: b.routing_url, - schema: SchemaSource::File { - file: canonical_file_name, + .map( + |(subgraph_name, subgraph_config)| match subgraph_config.schema { + SchemaSource::File { file } => { + let potential_canonical_file = root_to_resolve_from.join(&file); + match potential_canonical_file.canonicalize_utf8() { + Ok(canonical_file_name) => Ok(( + subgraph_name, + SubgraphConfig { + routing_url: subgraph_config.routing_url, + schema: SchemaSource::File { + file: canonical_file_name, + }, }, - }, - )), - Err(err) => Err(RoverError::new(err)), + )), + Err(err) => { + let mut rover_err = RoverError::new(anyhow!(err).context(format!( + "Could not find schema file ({}) for subgraph '{}'", + path::absolute(potential_canonical_file) + .unwrap() + .as_path() + .display(), + subgraph_name + ))); + rover_err.set_suggestion(InvalidSupergraphYamlSubgraphSchemaPath { + subgraph_name, + supergraph_yaml_path: root_to_resolve_from.clone(), + }); + Err(rover_err) + } + } } - } - _ => Ok((a, b)), - }) + _ => Ok((subgraph_name, subgraph_config)), + }, + ) .collect() } From 080638a5499321f89cc961b16f3e25b780bdc6bb Mon Sep 17 00:00:00 2001 From: jonathanrainer Date: Thu, 5 Sep 2024 10:47:35 +0100 Subject: [PATCH 3/7] ROVER-135 Actually fix the test itself --- src/utils/supergraph_config.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/utils/supergraph_config.rs b/src/utils/supergraph_config.rs index 139be7a94..d94580b33 100644 --- a/src/utils/supergraph_config.rs +++ b/src/utils/supergraph_config.rs @@ -277,11 +277,6 @@ mod test_get_supergraph_config { Some(String::from("pandas")), Some(vec![(String::from("pandas"), String::from("local"))]), )] - #[case::local_takes_precedence( - Some(String::from("pandas")), - Some(String::from("pandas")), - Some(vec![(String::from("pandas"), String::from("local"))]), - )] #[tokio::test] async fn test_get_supergraph_config( config: Config, @@ -331,6 +326,15 @@ mod test_get_supergraph_config { sdl } } + sourceVariant { + subgraphs { + name + url + activePartialSchema { + sdl + } + } + } } } } From 71155d7af206e56cceca01ab22533ac0e0d947f8 Mon Sep 17 00:00:00 2001 From: jonathanrainer Date: Tue, 10 Sep 2024 16:18:55 +0100 Subject: [PATCH 4/7] ROVER-135 Add new tests for paths --- src/utils/supergraph_config.rs | 177 +++++++++++++++++++++++++++++++-- 1 file changed, 171 insertions(+), 6 deletions(-) diff --git a/src/utils/supergraph_config.rs b/src/utils/supergraph_config.rs index d94580b33..965951c67 100644 --- a/src/utils/supergraph_config.rs +++ b/src/utils/supergraph_config.rs @@ -183,24 +183,28 @@ fn correctly_resolve_paths( #[cfg(test)] mod test_get_supergraph_config { + use std::fs; use std::fs::File; use std::io::Write; use std::path::PathBuf; use std::str::FromStr; use std::time::Duration; - use apollo_federation_types::config::FederationVersion; + use anyhow::Result; + use apollo_federation_types::config::{FederationVersion, SchemaSource, SupergraphConfig}; use camino::Utf8PathBuf; + use houston::Config; use httpmock::MockServer; use indoc::indoc; + use rover_client::shared::GraphRef; use rstest::{fixture, rstest}; use semver::Version; use serde_json::{json, Value}; use speculoos::assert_that; use speculoos::prelude::OptionAssertions; - - use houston::Config; - use rover_client::shared::GraphRef; + use tempfile::{NamedTempFile, TempDir}; + use tracing::debug; + use tracing_test::traced_test; use crate::options::ProfileOpt; use crate::utils::client::{ClientBuilder, StudioClientConfig}; @@ -278,6 +282,7 @@ mod test_get_supergraph_config { Some(vec![(String::from("pandas"), String::from("local"))]), )] #[tokio::test] + #[traced_test] async fn test_get_supergraph_config( config: Config, profile_opt: ProfileOpt, @@ -286,6 +291,7 @@ mod test_get_supergraph_config { #[case] local_subgraph: Option, #[case] expected: Option>, ) { + debug!("Starting test"); let server = MockServer::start(); let sdl = "extend type User @key(fields: \"id\") {\n id: ID! @external\n age: Int\n}\n" .to_string(); @@ -421,7 +427,7 @@ mod test_get_supergraph_config { sdl.escape_default() ); let mut supergraph_config_path = - tempfile::NamedTempFile::new().expect("Could not create temporary file"); + NamedTempFile::new().expect("Could not create temporary file"); supergraph_config_path .as_file_mut() .write_all(&supergraph_config.into_bytes()) @@ -473,6 +479,112 @@ mod test_get_supergraph_config { } } } + + #[rstest] + #[case::no_supplied_fed_version(None, None, FederationVersion::LatestFedTwo)] + #[case::using_supergraph_yaml_version( + None, + Some(FederationVersion::LatestFedOne), + FederationVersion::LatestFedOne + )] + #[case::using_requested_fed_version( + Some(FederationVersion::LatestFedOne), + None, + FederationVersion::LatestFedOne + )] + #[case::using_requested_fed_version_with_supergraph_yaml_version( + Some(FederationVersion::LatestFedOne), + Some(FederationVersion::LatestFedTwo), + FederationVersion::LatestFedOne + )] + fn test_resolve_federation_version( + #[case] requested_federation_version: Option, + #[case] supergraph_yaml_federation_version: Option, + #[case] expected_federation_version: FederationVersion, + ) -> Result<()> { + let federation_version_string = supergraph_yaml_federation_version + .map(|version| format!("federation_version: {}\n", version)) + .unwrap_or_default(); + let subgraphs = "subgraphs: {}".to_string(); + let supergraph_yaml = format!("{}{}", federation_version_string, subgraphs); + let supergraph_config: SupergraphConfig = serde_yaml::from_str(&supergraph_yaml)?; + let federation_version = + resolve_federation_version(requested_federation_version, &supergraph_config); + assert_that!(federation_version).is_equal_to(expected_federation_version); + Ok(()) + } + + #[rstest] + #[tokio::test] + async fn test_file_paths_become_canonicalised_on_read( + config: Config, + latest_fed2_version: &FederationVersion, + profile_opt: ProfileOpt, + ) { + let supergraph_config = format!( + indoc! { + r#" + federation_version: {} + subgraphs: + my_subgraph: + routing_url: https://subgraphs-for-all.com/subgraph1 + schema: + file: ../../../schema.graphql + "# + }, + latest_fed2_version, + ); + let root_test_folder = TempDir::new().expect("Can't create top-level test folder"); + let schema_path = root_test_folder.path().join("schema.graphql"); + fs::write(schema_path.clone(), "there is something here").unwrap(); + + let first_level_folder = + TempDir::new_in(&root_test_folder).expect("Can't create first-level test folder"); + let second_level_folder = + TempDir::new_in(&first_level_folder).expect("Can't create second-level test folder"); + let third_level_folder = + TempDir::new_in(&second_level_folder).expect("Can't create third-level test folder"); + let supergraph_config_path = third_level_folder.path().join("supergraph.yaml"); + fs::write( + supergraph_config_path.clone(), + &supergraph_config.into_bytes(), + ) + .expect("Could not write supergraph.yaml"); + + let studio_client_config = StudioClientConfig::new( + None, + config, + false, + ClientBuilder::default(), + Some(Duration::from_secs(3)), + ); + + let sc_config = get_supergraph_config( + &None, + &Some(FileDescriptorType::File( + Utf8PathBuf::from_path_buf(supergraph_config_path).unwrap(), + )), + None, + studio_client_config, + &profile_opt, + false, + ) + .await + .expect("Could not create Supergraph Config") + .expect("SuperGraph Config was None which was unexpected"); + + for ((_, subgraph_config), b) in sc_config + .into_iter() + .zip([schema_path.canonicalize().unwrap()]) + { + match subgraph_config.schema { + SchemaSource::File { file } => { + assert_that!(file.as_std_path()).is_equal_to(b.as_path()) + } + _ => panic!("Incorrect schema source found"), + } + } + } } /// Merge local and remote supergraphs, making sure that the federation version is correct: eg, when @@ -1069,7 +1181,7 @@ mod test_resolve_supergraph_yaml { subgraphs: "# }; - let config = super::expand_supergraph_yaml(yaml).unwrap(); + let config = expand_supergraph_yaml(yaml).unwrap(); assert_eq!( config.get_federation_version(), Some(FederationVersion::LatestFedOne) @@ -1199,6 +1311,59 @@ subgraphs: assert_eq!(people_subgraph.sdl, "there is also something here"); } + #[rstest] + #[tokio::test] + async fn check_relative_schema_paths_resolve_correctly( + client_config: StudioClientConfig, + profile_opt: ProfileOpt, + latest_fed2_version: &FederationVersion, + ) { + let raw_good_yaml = format!( + r#" +federation_version: {} +subgraphs: + films: + routing_url: https://films.example.com + schema: + file: ../../films.graphql + people: + routing_url: https://people.example.com + schema: + file: ../../people.graphql"#, + latest_fed2_version.to_string() + ); + let tmp_home = TempDir::new().unwrap(); + let tmp_dir = Utf8PathBuf::try_from(tmp_home.path().to_path_buf()).unwrap(); + let mut config_path = tmp_dir.clone(); + config_path.push("layer"); + config_path.push("layer"); + fs::create_dir_all(&config_path).unwrap(); + config_path.push("config.yaml"); + fs::write(&config_path, raw_good_yaml).unwrap(); + let films_path = tmp_dir.join("films.graphql"); + let people_path = tmp_dir.join("people.graphql"); + fs::write(films_path, "there is something here").unwrap(); + fs::write(people_path, "there is also something here").unwrap(); + let subgraph_definitions = resolve_supergraph_yaml( + &FileDescriptorType::File(config_path), + client_config, + &profile_opt, + ) + .await + .unwrap() + .get_subgraph_definitions() + .unwrap(); + let film_subgraph = subgraph_definitions.first().unwrap(); + let people_subgraph = subgraph_definitions.get(1).unwrap(); + + assert_eq!(film_subgraph.name, "films"); + assert_eq!(film_subgraph.url, "https://films.example.com"); + assert_eq!(film_subgraph.sdl, "there is something here"); + assert_eq!(people_subgraph.name, "people"); + assert_eq!(people_subgraph.url, "https://people.example.com"); + assert_eq!(people_subgraph.sdl, "there is also something here"); + } + const INTROSPECTION_SDL: &str = r#"directive @key(fields: _FieldSet!, resolvable: Boolean = true) repeatable on OBJECT | INTERFACE directive @requires(fields: _FieldSet!) on FIELD_DEFINITION From a7f1e8d69aef9ce6ef0757a9933ff15ec55f45ab Mon Sep 17 00:00:00 2001 From: jonathanrainer Date: Thu, 5 Sep 2024 14:05:15 +0100 Subject: [PATCH 5/7] ROVER-135 Fix test that was broken for a long time --- src/utils/telemetry.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/utils/telemetry.rs b/src/utils/telemetry.rs index 61dddefea..b340a4ba4 100644 --- a/src/utils/telemetry.rs +++ b/src/utils/telemetry.rs @@ -151,15 +151,15 @@ mod tests { #[test] fn it_can_serialize_commands_with_arguments() { - let args = vec![PKG_NAME, "config", "list", "--help"]; + let args = vec![PKG_NAME, "explain", "E002"]; let rover = Rover::parse_from(args); let actual_serialized_command = rover .serialize_command() .expect("could not serialize command"); let mut expected_arguments = HashMap::new(); - expected_arguments.insert("help".to_string(), json!(true)); + expected_arguments.insert("code".to_string(), json!("E002")); let expected_serialized_command = Command { - name: "config whoami".to_string(), + name: "explain".to_string(), arguments: expected_arguments, }; assert_eq!(actual_serialized_command, expected_serialized_command); From 0eb9e4184059447b12e546a7eb11142a9df6d9f9 Mon Sep 17 00:00:00 2001 From: jonathanrainer Date: Fri, 6 Sep 2024 08:40:34 +0100 Subject: [PATCH 6/7] ROVER-135 Fix tests running on Windows It appears these particular tests were relying on specific Windows line breaks etc. This is very error-prone, and in reality unnecessary as we can match on a smaller subset of the query itself and still get sensible answers. --- src/utils/supergraph_config.rs | 122 +++++---------------------------- 1 file changed, 16 insertions(+), 106 deletions(-) diff --git a/src/utils/supergraph_config.rs b/src/utils/supergraph_config.rs index 965951c67..650025e52 100644 --- a/src/utils/supergraph_config.rs +++ b/src/utils/supergraph_config.rs @@ -203,8 +203,6 @@ mod test_get_supergraph_config { use speculoos::assert_that; use speculoos::prelude::OptionAssertions; use tempfile::{NamedTempFile, TempDir}; - use tracing::debug; - use tracing_test::traced_test; use crate::options::ProfileOpt; use crate::utils::client::{ClientBuilder, StudioClientConfig}; @@ -282,7 +280,6 @@ mod test_get_supergraph_config { Some(vec![(String::from("pandas"), String::from("local"))]), )] #[tokio::test] - #[traced_test] async fn test_get_supergraph_config( config: Config, profile_opt: ProfileOpt, @@ -291,7 +288,6 @@ mod test_get_supergraph_config { #[case] local_subgraph: Option, #[case] expected: Option>, ) { - debug!("Starting test"); let server = MockServer::start(); let sdl = "extend type User @key(fields: \"id\") {\n id: ID! @external\n age: Int\n}\n" .to_string(); @@ -300,6 +296,12 @@ mod test_get_supergraph_config { let graphref_raw = format!("{name}@{variant}"); let url = format!("http://{}.remote.com", name); server.mock(|when, then| { + let request_body_partial = json!({ + "variables": { + "graph_ref": graphref_raw, + }, + "operationName": "SubgraphFetchAllQuery" + }); let body = json!({ "data": { "variant": { @@ -318,80 +320,7 @@ mod test_get_supergraph_config { }); when.method(httpmock::Method::POST) .path("/") - .json_body_obj(&json!({ - "query": indoc!{ - r#" - query SubgraphFetchAllQuery($graph_ref: ID!) { - variant(ref: $graph_ref) { - __typename - ... on GraphVariant { - subgraphs { - name - url - activePartialSchema { - sdl - } - } - sourceVariant { - subgraphs { - name - url - activePartialSchema { - sdl - } - } - } - } - } - } - "# - }, - "variables": { - "graph_ref": graphref_raw, - }, - "operationName": "SubgraphFetchAllQuery" - })); - then.status(200) - .header("content-type", "application/json") - .json_body(body); - }); - - server.mock(|when, then| { - let body = json!({ - "data": { - "graph": { - "variant": { - "subgraphs": [ - { - "name": name - } - ] - } - } - } - }); - when.method(httpmock::Method::POST) - .path("/") - .json_body_obj(&json!({ - "query": indoc!{ - r#" - query IsFederatedGraph($graph_id: ID!, $variant: String!) { - graph(id: $graph_id) { - variant(name: $variant) { - subgraphs { - name - } - } - } - } - "# - }, - "variables": { - "graph_id": name, - "variant": "current" - }, - "operationName": "IsFederatedGraph" - })); + .json_body_partial(request_body_partial.to_string()); then.status(200) .header("content-type", "application/json") .json_body(body); @@ -1540,9 +1469,16 @@ type _Service {\n sdl: String\n}"#; let graph_id = "testgraph"; let variant = "current"; let graphref = format!("{}@{}", graph_id, variant); - let server = MockServer::start(); + let server = MockServer::start_async().await; let subgraph_fetch_mock = server.mock(|when, then| { + let request_partial = json!({ + "variables": { + "graph_ref": graphref, + "subgraph_name": "products" + }, + "operationName": "SubgraphFetchQuery" + }); let body = json!({ "data": { "variant": { @@ -1563,33 +1499,7 @@ type _Service {\n sdl: String\n}"#; }); when.method(httpmock::Method::POST) .path("/") - .json_body_obj(&json!({ - "query": indoc!{ - r#" - query SubgraphFetchQuery($graph_ref: ID!, $subgraph_name: ID!) { - variant(ref: $graph_ref) { - __typename - ... on GraphVariant { - subgraph(name: $subgraph_name) { - url, - activePartialSchema { - sdl - } - } - subgraphs { - name - } - } - } - } - "# - }, - "variables": { - "graph_ref": graphref, - "subgraph_name": "products" - }, - "operationName": "SubgraphFetchQuery" - })); + .json_body_partial(request_partial.to_string()); then.status(200) .header("content-type", "application/json") .json_body(body); From 4b67c928e39c3db0bc5fa6f2b1d66bed72a2249a Mon Sep 17 00:00:00 2001 From: jonathanrainer Date: Tue, 10 Sep 2024 16:23:19 +0100 Subject: [PATCH 7/7] ROVER-135 Resolving odd rebase --- src/utils/supergraph_config.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/utils/supergraph_config.rs b/src/utils/supergraph_config.rs index 650025e52..c30e1c77e 100644 --- a/src/utils/supergraph_config.rs +++ b/src/utils/supergraph_config.rs @@ -207,7 +207,7 @@ mod test_get_supergraph_config { use crate::options::ProfileOpt; use crate::utils::client::{ClientBuilder, StudioClientConfig}; use crate::utils::parsers::FileDescriptorType; - use crate::utils::supergraph_config::get_supergraph_config; + use crate::utils::supergraph_config::{get_supergraph_config, resolve_federation_version}; #[fixture] #[once] @@ -1277,6 +1277,7 @@ subgraphs: &FileDescriptorType::File(config_path), client_config, &profile_opt, + false, ) .await .unwrap()