Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(compose): follow-up to #2101 #2105

Merged
merged 1 commit into from
Sep 9, 2024
Merged

fix(compose): follow-up to #2101 #2105

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
230 changes: 151 additions & 79 deletions src/utils/supergraph_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use std::collections::BTreeMap;
use std::str::FromStr;

use anyhow::anyhow;
use apollo_federation_types::build::{BuildError, BuildErrors, SubgraphDefinition};
use apollo_federation_types::build::{BuildError, BuildErrors};
use apollo_federation_types::config::{
FederationVersion, SchemaSource, SubgraphConfig, SupergraphConfig,
};
Expand Down Expand Up @@ -86,7 +87,19 @@ pub async fn get_supergraph_config(
// In this branch we get a completely resolved config, so all the references in it are
// resolved to a concrete SDL that could be printed out to a user. This is what
// `supergraph compose` uses.
Some(resolve_supergraph_yaml(file_descriptor, client_config, profile_opt).await?)
Some(
resolve_supergraph_yaml(
file_descriptor,
client_config,
profile_opt,
federation_version.is_none()
&& remote_subgraphs
.as_ref()
.and_then(|it| it.inner().get_federation_version())
.is_none(),
)
.await?,
)
} else {
// Alternatively, we might actually want a more dynamic object so that we can
// set up watchers on the subgraph sources. This branch is what `rover dev` uses.
Expand Down Expand Up @@ -642,13 +655,8 @@ pub(crate) async fn resolve_supergraph_yaml(
unresolved_supergraph_yaml: &FileDescriptorType,
client_config: StudioClientConfig,
profile_opt: &ProfileOpt,
must_determine_federation_version: bool,
) -> RoverResult<SupergraphConfig> {
let err_no_routing_url = || {
let err = anyhow!("No routing_url found for schema file.");
let mut err = RoverError::new(err);
err.set_suggestion(RoverErrorSuggestion::ValidComposeRoutingUrl);
err
};
let err_invalid_graph_ref = || {
let err = anyhow!("Invalid graph ref.");
let mut err = RoverError::new(err);
Expand Down Expand Up @@ -690,15 +698,7 @@ pub(crate) async fn resolve_supergraph_yaml(
err.set_suggestion(RoverErrorSuggestion::ValidComposeFile);
err
})
.and_then(|schema| {
subgraph_data
.routing_url
.clone()
.ok_or_else(err_no_routing_url)
.map(|url| {
SubgraphDefinition::new(subgraph_name.clone(), url, &schema)
})
})
.map(|schema| (subgraph_data.routing_url.clone(), schema))
}
SchemaSource::SubgraphIntrospection {
subgraph_url,
Expand Down Expand Up @@ -726,13 +726,21 @@ pub(crate) async fn resolve_supergraph_yaml(
.map(|introspection_response| {
let schema = introspection_response.result;

// We don't require a routing_url in config for this variant of a schema,
// if one isn't provided, just use the URL they passed for introspection.
let url = &subgraph_data
.routing_url
.clone()
.unwrap_or_else(|| subgraph_url.to_string());
SubgraphDefinition::new(subgraph_name.clone(), url, schema)
(
// We don't require a routing_url in config for
// this variant of a schema, if one isn't
// provided, just use the URL they passed for
// introspection. (This does mean there's no way
// when combining `--graph-ref` and a config
// file to say "fetch the schema from
// introspection but use the routing URL from
// the graph" at the moment.)
subgraph_data
.routing_url
.clone()
.or_else(|| Some(subgraph_url.to_string())),
schema,
)
})
.map_err(RoverError::from)
}
Expand Down Expand Up @@ -770,50 +778,75 @@ pub(crate) async fn resolve_supergraph_yaml(
routing_url: Some(graph_registry_routing_url),
} = result.sdl.r#type
{
let url = subgraph_data
.routing_url
.clone()
.unwrap_or(graph_registry_routing_url);
SubgraphDefinition::new(
subgraph_name.clone(),
url,
&result.sdl.contents,
(
subgraph_data
.routing_url
.clone()
.or(Some(graph_registry_routing_url)),
result.sdl.contents,
)
} else {
panic!("whoops: rebase me");
}
})
}
SchemaSource::Sdl { sdl } => subgraph_data
.routing_url
.clone()
.ok_or_else(err_no_routing_url)
.map(|url| SubgraphDefinition::new(subgraph_name.clone(), url, sdl)),
SchemaSource::Sdl { sdl } => Ok((subgraph_data.routing_url.clone(), sdl.clone())),
};
Ok((cloned_subgraph_name, result))
});

let subgraph_definition_results = join_all(futs).await.into_iter();
let num_subgraphs = subgraph_definition_results.len();

let mut subgraph_definitions = Vec::new();
let mut subgraph_definition_errors = Vec::new();
let mut subgraph_configs = BTreeMap::new();
let mut subgraph_config_errors = Vec::new();

let mut fed_two_subgraph_names = Vec::new();

for res in subgraph_definition_results {
match res {
Ok((subgraph_name, subgraph_definition_result)) => match subgraph_definition_result {
Ok(subgraph_definition) => subgraph_definitions.push(subgraph_definition),
Err(e) => subgraph_definition_errors.push((subgraph_name, e)),
Ok((subgraph_name, subgraph_config_result)) => match subgraph_config_result {
Ok((routing_url, sdl)) => {
subgraph_configs.insert(
subgraph_name.clone(),
SubgraphConfig {
routing_url,
schema: SchemaSource::Sdl { sdl: sdl.clone() },
},
);
let parser = Parser::new(&sdl);
let parsed_ast = parser.parse();
let doc = parsed_ast.document();
'definitions: for definition in doc.definitions() {
let maybe_directives = match definition {
cst::Definition::SchemaExtension(ext) => ext.directives(),
cst::Definition::SchemaDefinition(def) => def.directives(),
_ => None,
}
.map(|d| d.directives());
if let Some(directives) = maybe_directives {
for directive in directives {
if let Some(directive_name) = directive.name() {
if "link" == directive_name.text() {
fed_two_subgraph_names.push(subgraph_name);
break 'definitions;
}
}
}
}
}
}
Err(e) => subgraph_config_errors.push((subgraph_name, e)),
},
Err(err) => {
eprintln!("err: {err}");
}
}
}

if !subgraph_definition_errors.is_empty() {
if !subgraph_config_errors.is_empty() {
let source = BuildErrors::from(
subgraph_definition_errors
subgraph_config_errors
.iter()
.map(|(subgraph_name, error)| {
let mut message = error.message();
Expand All @@ -837,31 +870,8 @@ pub(crate) async fn resolve_supergraph_yaml(
}));
}

let mut resolved_supergraph_config: SupergraphConfig = subgraph_definitions.into();

let mut fed_two_subgraph_names = Vec::new();
for subgraph_definition in resolved_supergraph_config.get_subgraph_definitions()? {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This call to get_subgraph_definitions() returns error if anything in the config lacks a routing URL. I could have just changed this to iterate over the SubgraphConfigs but I would have to do an assertion that every schema source was of type Sdl. So I thought it would be simpler to just do the parsing on the SDL before we put it in the SupergraphConfig instead of afterwards. (Technically this does mean that some error cases we do some extra work before returning the error that we didn't do before.)

let parser = Parser::new(&subgraph_definition.sdl);
let parsed_ast = parser.parse();
let doc = parsed_ast.document();
for definition in doc.definitions() {
let maybe_directives = match definition {
cst::Definition::SchemaExtension(ext) => ext.directives(),
cst::Definition::SchemaDefinition(def) => def.directives(),
_ => None,
}
.map(|d| d.directives());
if let Some(directives) = maybe_directives {
for directive in directives {
if let Some(directive_name) = directive.name() {
if "link" == directive_name.text() {
fed_two_subgraph_names.push(subgraph_definition.name.clone());
}
}
}
}
}
}
let mut resolved_supergraph_config: SupergraphConfig =
SupergraphConfig::new(subgraph_configs, None);

let print_inexact_warning = || {
eprintln!("{} An exact {} was not specified in '{}'. Future versions of {} will fail without specifying an exact federation version. See {} for more information.", Style::WarningPrefix.paint("WARN:"), Style::Command.paint("federation_version"), &unresolved_supergraph_yaml, Style::Command.paint("`rover supergraph compose`"), Style::Link.paint("https://www.apollographql.com/docs/rover/commands/supergraphs#setting-a-composition-version"))
Expand Down Expand Up @@ -891,14 +901,16 @@ pub(crate) async fn resolve_supergraph_yaml(

// otherwise, set the version to what they set
resolved_supergraph_config.set_federation_version(specified_federation_version)
} else if fed_two_subgraph_names.is_empty() {
// if they did not specify a version and no subgraphs contain `@link` directives, use Federation 1
print_inexact_warning();
resolved_supergraph_config.set_federation_version(FederationVersion::LatestFedOne)
} else {
// if they did not specify a version and at least one subgraph contains an `@link` directive, use Federation 2
print_inexact_warning();
resolved_supergraph_config.set_federation_version(FederationVersion::LatestFedTwo)
} else if must_determine_federation_version {
if fed_two_subgraph_names.is_empty() {
// if they did not specify a version and no subgraphs contain `@link` directives, use Federation 1
print_inexact_warning();
resolved_supergraph_config.set_federation_version(FederationVersion::LatestFedOne)
} else {
// if they did not specify a version and at least one subgraph contains an `@link` directive, use Federation 2
print_inexact_warning();
resolved_supergraph_config.set_federation_version(FederationVersion::LatestFedTwo)
}
}

Ok(resolved_supergraph_config)
Expand Down Expand Up @@ -1035,7 +1047,8 @@ mod test_resolve_supergraph_yaml {
assert!(resolve_supergraph_yaml(
&FileDescriptorType::File(config_path),
client_config,
&profile_opt
&profile_opt,
true
)
.await
.is_err())
Expand Down Expand Up @@ -1074,7 +1087,8 @@ subgraphs:
assert!(resolve_supergraph_yaml(
&FileDescriptorType::File(config_path),
client_config,
&profile_opt
&profile_opt,
true
)
.await
.is_ok())
Expand Down Expand Up @@ -1117,6 +1131,7 @@ subgraphs:
&FileDescriptorType::File(config_path),
client_config,
&profile_opt,
true,
)
.await
.unwrap()
Expand Down Expand Up @@ -1201,6 +1216,7 @@ type _Service {\n sdl: String\n}"#;
&unresolved_supergraph_config,
client_config,
&profile_opt,
true,
)
.await;

Expand Down Expand Up @@ -1273,6 +1289,7 @@ type _Service {\n sdl: String\n}"#;
&unresolved_supergraph_config,
client_config,
&profile_opt,
true,
)
.await;

Expand Down Expand Up @@ -1395,6 +1412,7 @@ type _Service {\n sdl: String\n}"#;
&unresolved_supergraph_config,
studio_client_config,
&profile_opt,
true,
)
.await;

Expand Down Expand Up @@ -1454,6 +1472,7 @@ type _Service {\n sdl: String\n}"#;
&unresolved_supergraph_config,
client_config,
&profile_opt,
true,
)
.await;

Expand All @@ -1475,4 +1494,57 @@ type _Service {\n sdl: String\n}"#;

Ok(())
}

#[rstest]
#[case::must_determine_federation_version(true)]
#[case::no_need_to_determine_federation_version(false)]
#[tokio::test]
async fn test_subgraph_federation_version_default(
#[case] must_determine_federation_version: bool,
schema: String,
profile_opt: ProfileOpt,
client_config: StudioClientConfig,
) -> Result<()> {
let supergraph_config = format!(
indoc! {
r#"
subgraphs:
products:
routing_url: http://localhost:8000/
schema:
sdl: "{}"
"#
},
schema.escape_default()
);

let mut supergraph_config_path = tempfile::NamedTempFile::new()?;
supergraph_config_path
.as_file_mut()
.write_all(&supergraph_config.into_bytes())?;

let unresolved_supergraph_config =
FileDescriptorType::File(supergraph_config_path.path().to_path_buf().try_into()?);

let resolved_config = super::resolve_supergraph_yaml(
&unresolved_supergraph_config,
client_config,
&profile_opt,
must_determine_federation_version,
)
.await;

assert_that!(resolved_config).is_ok();
let resolved_config = resolved_config.unwrap();
assert_eq!(
resolved_config.get_federation_version(),
if must_determine_federation_version {
Some(FederationVersion::LatestFedOne)
} else {
None
}
);

Ok(())
}
}
Loading