feat(compose): when --output is used, use --output in supergraph …

…bin (#2045)

- noticed a regression in how we handle setting the federation version
when there's no local supergraph config (only the remote); sometimes
remote supergraph configs don't have a federation version, leading to
trouble with composition (we used to default to the latest federation
version; this restores that behavior and adds a bunch of tests to ensure
it)
- there's a funky case (previous art) for which federation version
should take precedence when merging supergraph configs; see the comments
for details (and the github comment for a pointer to where it's at)
- we know that `--output` works as expected because we have tests for
it; this change uses supergraph's `--output` flag, but there's no way
(that I can find) to test the presence/absence of its output (which
suggests the _real_ heart of this change: quieting down the supergraph
binary when doing composition by not dumping it to stdout)

src/cli.rs

@@ -191,7 +191,11 @@ impl Rover {
             Command::Fed2(command) => command.run(self.get_client_config()?),
             Command::Supergraph(command) => {
                 command
-                    .run(self.get_install_override_path()?, self.get_client_config()?)
+                    .run(
+                        self.get_install_override_path()?,
+                        self.get_client_config()?,
+                        self.output_opts.output_file.clone(),
+                    )
                     .await
             }
             Command::Docs(command) => command.run(),

src/command/dev/compose.rs

@@ -70,6 +70,7 @@ impl ComposeRunner {
                     self.override_install_path.clone(),
                     self.client_config.clone(),
                     supergraph_config,
+                    None,
                 )
                 .await,
         );

src/command/supergraph/compose/do_compose.rs

@@ -9,7 +9,9 @@ use apollo_federation_types::{
 };
 use camino::Utf8PathBuf;
 use clap::{Args, Parser};
+use semver::Version;
 use serde::Serialize;
+use std::io::Read;
 
 use rover_client::shared::GraphRef;
 use rover_client::RoverClientError;
@@ -112,6 +114,7 @@ impl Compose {
         &self,
         override_install_path: Option<Utf8PathBuf>,
         client_config: StudioClientConfig,
+        output_file: Option<Utf8PathBuf>,
     ) -> RoverResult<RoverOutput> {
         let mut supergraph_config = get_supergraph_config(
             &self.opts.supergraph_config_source.graph_ref,
@@ -125,18 +128,29 @@ impl Compose {
         // WARNING: remove this unwrap
         .unwrap();
 
-        self.compose(override_install_path, client_config, &mut supergraph_config)
-            .await
+        self.compose(
+            override_install_path,
+            client_config,
+            &mut supergraph_config,
+            output_file,
+        )
+        .await
     }
 
     pub async fn compose(
         &self,
         override_install_path: Option<Utf8PathBuf>,
         client_config: StudioClientConfig,
         supergraph_config: &mut SupergraphConfig,
+        output_file: Option<Utf8PathBuf>,
     ) -> RoverResult<RoverOutput> {
         let output = self
-            .exec(override_install_path, client_config, supergraph_config)
+            .exec(
+                override_install_path,
+                client_config,
+                supergraph_config,
+                output_file,
+            )
             .await?;
         Ok(RoverOutput::CompositionResult(output))
     }
@@ -146,10 +160,13 @@ impl Compose {
         override_install_path: Option<Utf8PathBuf>,
         client_config: StudioClientConfig,
         supergraph_config: &mut SupergraphConfig,
+        output_file: Option<Utf8PathBuf>,
     ) -> RoverResult<CompositionOutput> {
+        let mut output_file = output_file;
         // first, grab the _actual_ federation version from the config we just resolved
         // (this will always be `Some` as long as we have created with `resolve_supergraph_yaml` so it is safe to unwrap)
         let federation_version = supergraph_config.get_federation_version().unwrap();
+
         let exe = self
             .maybe_install_supergraph(
                 override_install_path,
@@ -180,52 +197,116 @@ impl Compose {
         f.sync_all()?;
         tracing::debug!("config file written to {}", &yaml_path);
 
-        let federation_version = Self::extract_federation_version(&exe);
+        let federation_version = Self::extract_federation_version(&exe)?;
+        let exact_version = federation_version
+            .get_exact()
+            // This should be impossible to get to because we convert to a FederationVersion a few
+            // lines above and so _should_ have an exact version
+            .ok_or(RoverError::new(anyhow!(
+                "failed to get exact Federation version"
+            )))?;
 
         eprintln!(
             "composing supergraph with Federation {}",
-            &federation_version
+            &federation_version.get_tarball_version()
         );
 
-        let output = Command::new(&exe)
-            .args(["compose", yaml_path.as_ref()])
-            .output()
-            .context("Failed to execute command")?;
-        let stdout = str::from_utf8(&output.stdout)
-            .with_context(|| format!("Could not parse output of `{} compose`", &exe))?;
-
-        match serde_json::from_str::<BuildResult>(stdout) {
-            Ok(build_result) => match build_result {
-                Ok(build_output) => Ok(CompositionOutput {
-                    hints: build_output.hints,
-                    supergraph_sdl: build_output.supergraph_sdl,
-                    federation_version: Some(federation_version.to_string()),
-                }),
-                Err(build_errors) => Err(RoverError::from(RoverClientError::BuildErrors {
-                    source: build_errors,
-                    num_subgraphs,
-                })),
-            },
-            Err(bad_json) => Err(anyhow!("{}", bad_json))
-                .with_context(|| anyhow!("{} compose output: {}", &exe, stdout))
-                .with_context(|| anyhow!("Output from `{} compose` was malformed.", &exe))
-                .map_err(|e| {
-                    let mut error = RoverError::new(e);
-                    error.set_suggestion(RoverErrorSuggestion::SubmitIssue);
-                    error
-                }),
+        // When the `--output` flag is used, we need a supergraph binary version that is at least
+        // v2.9.0. We ignore that flag for composition when we have anything less than that
+        if output_file.is_some() && exact_version.major < 2
+            || (exact_version.major == 2 && exact_version.minor < 9)
+        {
+            eprintln!("ignoring `--output` because it is not supported in this version of the dependent binary, `supergraph`: {}. Upgrade to Federation 2.9.0 or greater to install a version of the binary that supports it.", federation_version);
+            output_file = None;
+        }
+
+        // Whether we use stdout or a file dependson whether the the `--output` option was used
+        let content = match output_file {
+            // If it was, we use a file in the supergraph binary; this cuts down the overall time
+            // it takes to do composition when we're working on really large compositions, but it
+            // carries with it the assumption that stdout is superfluous
+            Some(filepath) => {
+                Command::new(&exe)
+                    .args(["compose", yaml_path.as_ref(), filepath.as_ref()])
+                    .output()
+                    .context("Failed to execute command")?;
+
+                let mut composition_file = std::fs::File::open(&filepath).unwrap();
+                let mut content: String = String::new();
+                composition_file.read_to_string(&mut content).unwrap();
+                content
+            }
+            // When we aren't using `--output`, we dump the composition directly to stdout
+            None => {
+                let output = Command::new(&exe)
+                    .args(["compose", yaml_path.as_ref()])
+                    .output()
+                    .context("Failed to execute command")?;
+
+                let content = str::from_utf8(&output.stdout)
+                    .with_context(|| format!("Could not parse output of `{} compose`", &exe))?;
+                content.to_string()
+            }
+        };
+
+        // Make sure the composition is well-formed
+        let composition = match serde_json::from_str::<BuildResult>(&content) {
+            Ok(res) => res,
+            Err(err) => {
+                return Err(anyhow!("{}", err))
+                    .with_context(|| anyhow!("{} compose output: {}", &exe, content))
+                    .with_context(|| anyhow!("Output from `{} compose` was malformed.", &exe))
+                    .map_err(|e| {
+                        let mut error = RoverError::new(e);
+                        error.set_suggestion(RoverErrorSuggestion::SubmitIssue);
+                        error
+                    })
+            }
+        };
+
+        match composition {
+            Ok(build_output) => Ok(CompositionOutput {
+                hints: build_output.hints,
+                supergraph_sdl: build_output.supergraph_sdl,
+                federation_version: Some(format_version(federation_version.to_string())),
+            }),
+            Err(build_errors) => Err(RoverError::from(RoverClientError::BuildErrors {
+                source: build_errors,
+                num_subgraphs,
+            })),
         }
     }
 
-    fn extract_federation_version(exe: &Utf8PathBuf) -> &str {
+    /// Extracts the Federation Version from the executable
+    fn extract_federation_version(exe: &Utf8PathBuf) -> Result<FederationVersion, RoverError> {
         let file_name = exe.file_name().unwrap();
         let without_exe = file_name.strip_suffix(".exe").unwrap_or(file_name);
-        without_exe
-            .strip_prefix("supergraph-")
-            .unwrap_or(without_exe)
+        let version = match Version::parse(
+            without_exe
+                .strip_prefix("supergraph-v")
+                .unwrap_or(without_exe),
+        ) {
+            Ok(version) => version,
+            Err(err) => return Err(RoverError::new(err)),
+        };
+
+        match version.major {
+            0 | 1 => Ok(FederationVersion::ExactFedOne(version)),
+            2 => Ok(FederationVersion::ExactFedTwo(version)),
+            _ => Err(RoverError::new(anyhow!("unsupported Federation version"))),
+        }
     }
 }
 
+/// Format the a Version string (coming from an exact version, which includes a `=` rather than a
+/// `v`) for readability
+fn format_version(version: String) -> String {
+    let unformatted = &version[1..];
+    let mut formatted = unformatted.to_string();
+    formatted.insert(0, 'v');
+    formatted
+}
+
 #[cfg(test)]
 mod tests {
     use rstest::rstest;
@@ -247,7 +328,7 @@ mod tests {
     fn it_can_extract_a_version_correctly(#[case] file_path: &str, #[case] expected_value: &str) {
         let mut fake_path = Utf8PathBuf::new();
         fake_path.push(file_path);
-        let result = Compose::extract_federation_version(&fake_path);
-        assert_that(&result).is_equal_to(expected_value);
+        let result = Compose::extract_federation_version(&fake_path).unwrap();
+        assert_that(&result).matches(|f| format_version(f.to_string()) == expected_value);
     }
 }

src/command/supergraph/compose/no_compose.rs

@@ -25,6 +25,7 @@ impl Compose {
         &self,
         _override_install_path: Option<Utf8PathBuf>,
         _client_config: StudioClientConfig,
+        _output_file: Option<Utf8PathBuf>,
     ) -> RoverResult<RoverOutput> {
         let mut err = RoverError::new(anyhow!(
             "This version of Rover does not support this command."

src/command/supergraph/mod.rs

@@ -28,10 +28,15 @@ impl Supergraph {
         &self,
         override_install_path: Option<Utf8PathBuf>,
         client_config: StudioClientConfig,
+        output_file: Option<Utf8PathBuf>,
     ) -> RoverResult<RoverOutput> {
         match &self.command {
             Command::Fetch(command) => command.run(client_config).await,
-            Command::Compose(command) => command.run(override_install_path, client_config).await,
+            Command::Compose(command) => {
+                command
+                    .run(override_install_path, client_config, output_file)
+                    .await
+            }
         }
     }
 }

src/options/output.rs

@@ -85,7 +85,7 @@ pub struct OutputOpts {
 
     /// Specify a file to write Rover's output to
     #[arg(long = "output", short = 'o', global = true, value_parser = Self::parse_absolute_path)]
-    output_file: Option<Utf8PathBuf>,
+    pub output_file: Option<Utf8PathBuf>,
 }
 
 impl OutputOpts {