Add `--graph-ref` to `supergraph compose` #2001

jonathanrainer · 2024-07-24T14:30:54Z

NOTE: This should not be merged until #1874 has been merged, and then this rebased on top

In #1984 we added support to pass a --graph-ref flag to rover dev. Since supergraph compose is performing the same function as rover dev it makes sense to expose the same option there too. However, this requires some refactoring in order to make work.

In principle the idea is fairly simple, we should centralise the construction of the SupergraphConfig object so that both dev and supergraph compose can hook in to it from a central location. However to do this I've also had to add a fair amount of testing because there was very little coverage in this area, however, due to @dotdat's work on #1977 we have a lot more test coverage than before. That work has been integrated into this PR.

Further more @dotdat was working on another PR #1985 , which optimises the calls made by the --graph-ref option to fetch subgraphs in one request rather than multiple. Due to being in this area and working on the --graph-ref option it also made sense to integrate that PR here as well, to prevent a complicated rebase later on.

This PR has been tested by the addition of unit tests and running supergraph compose locally.

There is also a small change to deny.toml in this PR to add support for BSD-2-Clause licences. We already support BSD-3-Clause, and 2-Clause is strictly a subset so this should not be a problem

jonathanrainer · 2024-07-25T10:11:03Z

crates/rover-client/src/operations/subgraph/fetch_all/fetch_all_query.graphql

This is the heart of the change, adding this new query into the rover client

jonathanrainer · 2024-07-25T10:11:55Z

deny.toml

As said in the PR description, BSD-2-Clause is here: https://opensource.org/license/bsd-2-clause and BSD-3-Clause is here: https://opensource.org/license/bsd-3-clause

On inspection one is a subset of the other and we already accept BSD-3-Clause

jonathanrainer · 2024-07-25T10:12:36Z

src/command/dev/do_dev.rs

The machinery for merging remote and local supergraph configs was here and this has now been centralised in utils, rather than it living as part of the dev command.

jonathanrainer · 2024-07-25T10:14:26Z

src/command/dev/mod.rs

@@ -91,7 +90,7 @@ pub struct SupergraphOpts {
        long = "supergraph-config", 
        conflicts_with_all = ["subgraph_name", "subgraph_url", "subgraph_schema_path"]
    )]
-    supergraph_config_path: Option<Utf8PathBuf>,
+    supergraph_config_path: Option<FileDescriptorType>,


It seems that here we modelled the config path as a Utf8PathBuf, but for compose we modelled it as a FileDescriptorType. I've gone with FileDescriptorType here so that does give the potential to allow supergraph.yaml's to be pipe in from stdout should people want to do that.

jonathanrainer · 2024-07-25T10:15:30Z

src/command/supergraph/compose/do_compose.rs

+    /// A [`GraphRef`] that is accessible in Apollo Studio.
+    /// This is used to initialize your supergraph with the values contained in this variant.
+    ///
+    /// This is analogous to providing a supergraph.yaml file with references to your graph variant in studio.
+    ///
+    /// If used in conjunction with `--config`, the values presented in the supergraph.yaml will take precedence over these values.
+    #[arg(long = "graph-ref")]
+    graph_ref: Option<GraphRef>,
+
+    /// The version of Apollo Federation to use for composition
+    #[arg(long = "federation-version")]
+    federation_version: Option<FederationVersion>,


We had to add more options to allow graph-ref but we can also add the option for federation_version to keep consistency with how dev works.

jonathanrainer · 2024-07-25T10:15:57Z

src/command/supergraph/compose/do_compose.rs

@@ -186,131 +215,11 @@ impl Compose {

 #[cfg(test)]
 mod tests {
-    use std::convert::TryFrom;


These tests have now been moved, and centralised in utils/supergraph_config.rs

jonathanrainer · 2024-07-25T10:17:16Z

src/utils/supergraph_config.rs

This is the meat of the change here, we centralise all functions that related to constructing a SupergraphConfig here, and add tests for them. That includes parsing, expanding, and also getting the subgraph configs remotely

aaronArinder

little worried about pagination and how that query will perform for larger numbers of subgraphs; we probably need to test with a larger set of subgraphs (maybe the performance subgraphs?) before it gets released, but I think this looks good overall and should be good to merge so we can resolve any conflicts with the async branch

aaronArinder · 2024-07-30T20:26:36Z

crates/rover-client/src/operations/subgraph/fetch_all/fetch_all_query.graphql

+query SubgraphFetchAllQuery($graph_ref: ID!) {
+  variant(ref: $graph_ref) {
+    __typename
+    ... on GraphVariant {
+      subgraphs {
+        name
+        url
+        activePartialSchema {
+          sdl
+        }
+      }
+    }
+  }
+}


does this query support pagination? I'm a hair worried about getting the sdl for, say, hundreds of subgraphs in one go

Had a look in studio and I can't see that it does, I think the only way we're going to know though is to try it with a big number of graphs and see how it performs. Do we have the power to point it at Indeed's subgraphs, since all we're doing is composing and we have the schemas I don't see why not right?

crates/rover-client/src/operations/subgraph/fetch_all/runner.rs

aaronArinder · 2024-07-30T20:31:11Z

crates/rover-client/src/operations/subgraph/fetch_all/runner.rs

+    if let Some(maybe_variant) = response_data.variant {
+        match maybe_variant {
+            SubgraphFetchAllGraphVariant::GraphVariant(variant) => {
+                if let Some(subgraphs) = variant.subgraphs {
+                    Ok(subgraphs


not a blocker, but wondering if this could be more readable with a .and_then(|variant| variant.map(/*blah*/)); don't waste time on it, though, because we can always just make it more readable later (mostly worried about those chunky if let ... else statements, which tend to benefit from .and_then()

but yeah, not a priority

Had a crack at refactoring this, I've collapsed the if/let and the inner match statement together, because there's no point having both when you could just have a little more complexity in the pattern, and then gone with a map_or_else. Let me know if you think it's better or we need a second pass at it :)

crates/rover-client/src/operations/subgraph/fetch_all/runner.rs

aaronArinder · 2024-07-30T20:33:28Z

src/command/dev/mod.rs

@@ -91,7 +90,7 @@ pub struct SupergraphOpts {
        long = "supergraph-config", 
        conflicts_with_all = ["subgraph_name", "subgraph_url", "subgraph_schema_path"]
    )]
-    supergraph_config_path: Option<Utf8PathBuf>,
+    supergraph_config_path: Option<FileDescriptorType>,


aaronArinder · 2024-07-30T20:35:48Z

src/utils/supergraph_config.rs

+        let subgraphs = subgraph::fetch_all::run(
+            SubgraphFetchAllInput {
+                graph_ref: graph_ref.clone(),
+            },
+            client,
+        )?;


this looks like where we'd have our side of the pagination; calling out more as a pointer for myself if we end up talking about pagination more

aaronArinder · 2024-07-30T20:37:16Z

src/utils/supergraph_config.rs

+                    SubgraphConfig {
+                        routing_url: subgraph.url().clone(),
+                        schema: SchemaSource::Sdl {
+                            sdl: subgraph.sdl().clone(),


are sdls pretty chonky? wondering if we absolutely need to clone them or if there's another way to get them where we need them without having to go down that route (mostly worried about a crazy amount of strings being processed when the number of subgraphs is non-trivial); might be an empirical question of what the actual performance looks like when we run this against larger sets of subgraphs and may end up not mattering, but worth worrying about so we don't slow down the supergraph compose command

This is a great callout, and I think we can be a bit clever here. Because we're consuming the subgraphs vector we can use into_iter and then if we implement From<Subgraph> for SubgraphConfig we can properly consume the subgraphs rather than cloning, so we should only ever have 1 string around for the SDL if it is chonky. It means we have to clone the name otherwise we've got references that are a bit of a mess but even with hundreds of subgraphs the overhead would be bytes which doesn't bother me.

aaronArinder · 2024-07-30T20:39:33Z

src/utils/supergraph_config.rs

+    // 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);
+                Some(merged_supergraph_config)
+            }
+            None => Some(remote_subgraphs.inner().clone()),
+        },
+        None => supergraph_config,
+    };
+    Ok(supergraph_config)
+}


this was kind of hard for me to follow; not a blocker, but wondering if .and_then() or if let would make it clearer? probs not worth worrying about given how quickly we'd like to get this in, though

Yer I agree it's confusing, went down a slightly different path and did a match on a tuple of the remote and local subgraphs because that's really what this is saying.

aaronArinder · 2024-07-30T20:40:47Z

src/utils/supergraph_config.rs

+
+        Ok(())
+    }
+}


super nice tests!

We do this in both supergraph commands and dev and having it in two places is not helping.

Rather than fetching each subgraph one at a time, make a single GraphQL request to get them all at once. This combines the work from #1985

@aaronArinder

> Important: 1 potentially breaking changes below, indicated by **❗ BREAKING ❗** ## ❗ BREAKING ❗ - **The --client-timeout flag now represents the period over which we allow retries - @aaronArinder PR #2019** The documentation for this flag indicated that this was the period over which Rover would retry a command if there were retryable HTTP errors. However this was not the case due to complexities in how the client was instantiated. This has now been corrected, so the documented behaviour matches the actual behaviour. ## 🚀 Features - **Make `rover` operate asynchronously - @aaronArinder @Geal PR #2035** Removes the use of the `reqwest` blocking client allowing `rover` to operate using an asynchronous `tokio` runtime. This will bring performance improvements, particularly where working with large sets of subgraphs. - **Add `--graph-ref` to `supergraph compose` - @jonathanrainer PR #2001** Adds the same capabilities to `supergraph compose` as were added to `rover dev` in 0.25.0. You can now specify an existing Studio graphref and the command will run composition over the subgraphs specified in the graphref, as well as any overrides specified in a given supergraph config. - **Add new `rover cloud` command - @loshz PR #2008** Adds a new command to allow you to push or pull the Router config to a Cloud Router that is running in Studio - **Add new `rover cloud config validate` subcommand - @loshz PR #2055** Adds a new command enabling you to validate the Router config for a Cloud Router ## 🐛 Fixes - **Don't run IsFederatedGraph before running SubgraphFetchQuery - @glasser PR #2004** Previously we were checking IsFederatedGraph before running SubgraphFetch, but the same check is actually performed in SubgraphFetch anyway so the first call to IsFederatedSubgraph is unnecessary. - **Allow `--graph-ref` to support contract variants - @jonathanrainer PR #2036** There was a bug where using the graphref of a contract variant would cause an error about non-federated graphs. This has been resolved and now contract variant graphrefs can also be used. - **Remove last reference to blocking `reqwest` client - @loshz PR #2050** One reference to the blocking `reqwest` client had been leftover from the move to `async` operation in #2035, this was removed. - **Ensure NPM installer on Windows works correctly - @jonathanrainer PR #2059** The NPM installer on Windows had been broken because it was attempt to rename a binary from `rover` to its correct name, rather than from `rover.exe` to its correct name. This has been corrected and extra CI and unit tests added to prevent a recurrence. - **Make sure a message is returned to the user when cloud config is updated correctly - @loshz PR #2063** - **Fix a regression in `rover dev` where it would no longer watch subgraphs correctly - @jonathanrainer PR #2065** ## 🛠 Maintenance - **Integrate the Smoke Tests Into Integration Test Framework To Allow Easier Extension - @jonathanrainer PR #1999** - **Add nicer names to GitHub actions workflow - @jonathanrainer PR #2002** - **Add test for subgraph introspect - @jonathanrainer PR #2003** - **Update node.js packages - @jonathanrainer PR #2006** Includes `eslint` to v9.8.0 and `node` to v20.16.0 - **Update Rust to v1.80.0 - @jonathanrainer PR #2007** - **Fix up CODEOWNERS to bring us inline with standard - @jonathanrainer PR #2016** - **Add E2E test for `supergraph compose` - @aaronArinder PR #2005** - **Add E2E test for `subgraph fetch` - @jonathanrainer PR #2015** - **Update Rust crates - @aaronArinder PR #2011** Includes `apollo-parser` to v0.8 and `octocrab` to v0.39.0 - **Update apollographql/router to v1.52.0 - @aaronArinder PR #2010** - **Add E2E test for `supergraph compose` - @aaronArinder PR #2005** - **Rename a test and add a `#[once]` macro to a fixture - @aaronArinder PR #2017** - **Add E2E tests for `graph introspect` - @jonathanrainer PR #2020** - **Add missing inherit for secrets - @jonathanrainer PR #2021** - **Add E2E tests for `whoami` - @jonathanrainer PR #2022** - **Update rstest to v0.22.0 - @jonathanrainer PR #2030** - **Add E2E tests for `config clear` - @aaronArinder PR #2029** - **Add E2E tests for `subgraph lint` - @aaronArinder PR #2023** - **Add E2E tests for `subgraph publish` - @jonathanrainer PR #2031** - **Add E2E tests for `graph fetch` - @aaronArinder PR #2026** - **Add E2E tests for `supergraph fetch` - @aaronArinder PR #2024** - **Add E2E tests for `subgraph list` - @aaronArinder PR #2027** - **Add E2E tests for `graph check` and `subgraph check` - @aaronArinder PR #2025** - **Add E2E tests for `install plugin` - @aaronArinder PR #2028** - **Make E2E tests account for changes in #2019 - @jonathanrainer PR #2032** - **Deprecate the use of Emoji - @loshz PR #2034** - **Let E2E tests message Slack if there are nightly failures - @jonathanrainer PR #2033** - **Tighten up Slack Messaging for E2E tests - @jonathanrainer PR #2039** - **Update `axios-mock-adapter` to v2.0.0 - @jonathanrainer PR #2043** - **Update `derive-getters` to v0.5.0 - @jonathanrainer PR #2042** - **Update `eslient` to v9.9.0 - @jonathanrainer PR #2041** - **Update Rust to v1.80.1 - @jonathanrainer PR #2040** - **Update axios to v1.7.4 - @jonathanrainer PR #2048** - **Update CODEONWERS - @aaronArinder PR #2052** - **Update termimad to v0.30.0 - @jonathanrainer PR #2054** - **Add step to fail workflow if matrix branch fails - @jonathanrainer PR #2044** - **Increase test coverage for operations/cloud/config - @loshz PR #2057** - **Update `gh` CircleCI Orb to v2.4.0 - @jonathanrainer PR #2062** - **Update `mockito` to v1.5.0 - @jonathanrainer PR #2061** - **Update `dircpy` to v0.3.19 - @jonathanrainer PR #2060** ## 📚 Documentation - **Document E2E test gotchas - @aaronArinder PR #2018** - **Fix table to be compatible with new docs platform - @shorgi PR #2038** - **Remove unhelpful note - @Meschreiber PR #2053** - **Add Summit callout - @Meschreiber PR #2058** - **Adds `--graph-ref` to supergraph compose docs - @jackonawalk PR #2037**

jonathanrainer force-pushed the jr/story/ROVER-69 branch 5 times, most recently from 3bbc0cf to e10de24 Compare July 25, 2024 10:01

jonathanrainer added the feature 🎉 new commands, flags, functionality, and improved error messages label Jul 25, 2024

jonathanrainer added this to the vNext milestone Jul 25, 2024

jonathanrainer commented Jul 25, 2024

View reviewed changes

jonathanrainer force-pushed the jr/story/ROVER-69 branch 3 times, most recently from 072eba7 to b24d184 Compare July 30, 2024 15:45

jonathanrainer marked this pull request as ready for review July 30, 2024 15:46

jonathanrainer requested a review from a team as a code owner July 30, 2024 15:46

aaronArinder approved these changes Jul 30, 2024

View reviewed changes

jonathanrainer force-pushed the jr/story/ROVER-69 branch 2 times, most recently from 6a0a629 to 05276fd Compare July 31, 2024 10:10

jonathanrainer added 8 commits August 1, 2024 15:13

ROVER-69 Unify supergraph_config parsing

9d9b6d0

We do this in both supergraph commands and dev and having it in two places is not helping.

ROVER-69 Migrate tests from #1977

e8f58b2

ROVER-69 Fix Federation Versions to remove WARNs

59d6f44

ROVER-69 Fix docs

7d888f5

ROVER-69 Optimise --graph-ref uses

ae69a95

Rather than fetching each subgraph one at a time, make a single GraphQL request to get them all at once. This combines the work from #1985

ROVER-69 Update to account for #2004

2a277b5

ROVER-69 Fix argument parsing

3b2b205

ROVER-69 Respond to PR comments

e08de0d

aaronArinder force-pushed the jr/story/ROVER-69 branch from 05276fd to e08de0d Compare August 1, 2024 19:13

aaronArinder enabled auto-merge (squash) August 1, 2024 19:14

aaronArinder mentioned this pull request Aug 1, 2024

Optimize rover dev --graph-ref by fetching all subgraph data at once #1985

Closed

aaronArinder merged commit 041c8d9 into main Aug 1, 2024
19 checks passed

aaronArinder deleted the jr/story/ROVER-69 branch August 1, 2024 19:24

jonathanrainer mentioned this pull request Aug 1, 2024

Add tests for supergraph/resolve_config.rs #1977

Closed

aaronArinder mentioned this pull request Aug 5, 2024

supergraph compose should not make a separate graphql query for each subgraph #1946

Open

jonathanrainer mentioned this pull request Aug 20, 2024

release: v0.26.0 #2064

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--graph-ref` to `supergraph compose` #2001

Add `--graph-ref` to `supergraph compose` #2001

jonathanrainer commented Jul 24, 2024 •

edited

Loading

jonathanrainer Jul 25, 2024

jonathanrainer Jul 25, 2024

jonathanrainer Jul 25, 2024 •

edited

Loading

jonathanrainer Jul 25, 2024

aaronArinder Jul 30, 2024

jonathanrainer Jul 25, 2024

jonathanrainer Jul 25, 2024

jonathanrainer Jul 25, 2024

aaronArinder left a comment

aaronArinder Jul 30, 2024

jonathanrainer Jul 31, 2024

aaronArinder Jul 30, 2024

jonathanrainer Jul 31, 2024

aaronArinder Jul 30, 2024

aaronArinder Jul 30, 2024

aaronArinder Jul 30, 2024

jonathanrainer Jul 31, 2024

aaronArinder Jul 30, 2024

jonathanrainer Jul 31, 2024

aaronArinder Jul 30, 2024

Add --graph-ref to supergraph compose #2001

Add --graph-ref to supergraph compose #2001

Conversation

jonathanrainer commented Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanrainer Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronArinder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `--graph-ref` to `supergraph compose` #2001

Add `--graph-ref` to `supergraph compose` #2001

jonathanrainer commented Jul 24, 2024 •

edited

Loading

jonathanrainer Jul 25, 2024 •

edited

Loading