fix(compose): follow-up to #2101

The improvements in #2101 did not quite work in all cases, due to some
"enhancements" that the `resolve_supergraph_yaml` (which reads the
`--config` file) applies which are overzealous in the case where the
YAML file is an "override" to a graph ref.

First, `resolve_supergraph_yaml` sets a default `federation_version`
(based on whether or not any subgraph SDLs contain the `@link`
directive) if one is not provided. The YAML file's federation version
correctly takes precedence over a remote `federation_version`, which
means it's not appropriate for us to calculate this default if the file
lacks a `federation_version` but we have one from another source. This
PR adds a parameter to `resolve_supergraph_yaml` which suppresses this
defaulting in the case where you have also specified `--graph-ref` and
read a federation version from GraphOS (or if you've specified it
explicitly on the command line: previously this case "worked" in that
the CLI option took precedence, but it would print a warning first
telling you to specify the version).

Secondly, we want to no longer treat a lack of a `routing_url` for a
subgraph in the YAML file as an error, so that the improvement in #2101
which lets you merge local SubgraphConfigs with `routing_url: None` with
remote SubgraphConfigs that have routing URLs works. But
`resolve_supergraph_yaml` required every subgraph definition to have a
routing URL (and in fact used the `SubgraphDefinition` struct as an
intermediate data type, which requires the routing URL to exist). We
refactor to no longer use that as an intermediate data type so that it's
OK for the routing URL to still be None (like it is in the
`SupergraphConfig` type that the function returns).

(You may wonder if it's safe that the `SupergraphConfig` returned from
`resolve_supergraph_yaml` can now have None in its routing URLs when
that was not possible before. But it already was the case that a graph
read from GraphOS lacked routing URLs; this requirement was only
enforced here for local configuration. The lack of a routing URL gets
caught later and turned into an error when `Compose::exec` calls
`supergraph_config.get_subgraph_definitions()`.)

src/utils/supergraph_config.rs

@@ -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,
 };
@@ -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.
@@ -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);
@@ -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,
@@ -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)
                 }
@@ -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();
@@ -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()? {
-        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"))
@@ -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)
@@ -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())
@@ -1074,7 +1087,8 @@ subgraphs:
         assert!(resolve_supergraph_yaml(
             &FileDescriptorType::File(config_path),
             client_config,
-            &profile_opt
+            &profile_opt,
+            true
         )
         .await
         .is_ok())
@@ -1117,6 +1131,7 @@ subgraphs:
             &FileDescriptorType::File(config_path),
             client_config,
             &profile_opt,
+            true,
         )
         .await
         .unwrap()
@@ -1201,6 +1216,7 @@ type _Service {\n  sdl: String\n}"#;
             &unresolved_supergraph_config,
             client_config,
             &profile_opt,
+            true,
         )
         .await;
 
@@ -1273,6 +1289,7 @@ type _Service {\n  sdl: String\n}"#;
             &unresolved_supergraph_config,
             client_config,
             &profile_opt,
+            true,
         )
         .await;
 
@@ -1395,6 +1412,7 @@ type _Service {\n  sdl: String\n}"#;
             &unresolved_supergraph_config,
             studio_client_config,
             &profile_opt,
+            true,
         )
         .await;
 
@@ -1454,6 +1472,7 @@ type _Service {\n  sdl: String\n}"#;
             &unresolved_supergraph_config,
             client_config,
             &profile_opt,
+            true,
         )
         .await;
 
@@ -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(())
+    }
 }