fix(compose, dev): improve `--graph-ref` option #2101

glasser · 2024-09-05T23:03:11Z

Make rover supergraph compose --graph-ref REF --config CONFIG work as documented (and don't remove routing_urls fetched from GraphOS if they're missing from the local file).

Make rover supergraph compose --graph-ref REF and rover dev --graph-ref REF use the graph ref's federation version.

Fix docs.

(Individual commits have more detailed messages.)

…e_subgraphs` This function was added a few months ago in #541 and as far as I know is only used in Rover for the `supergraph compose` and `dev` commands to combine configuration from `--graph-ref` and `--supergraph-config`/`--config` (and the feature does not actually work for `supergraph compose` due to the issue fixed in apollographql/rover#2101). The use case here is to let you run composition or dev against a GraphOS graph with some local changes, and so "I just want to change the SDL and nothing else" is a reasonable use case. This change means that if an override specifies SDL but no routing URL for a subgraph, Rover will continue to use the routing URL fetched from GraphOS for that subgraph, which seems reasonable. While this is a "backwards incompatible" change, this crate is only intended for use by the Rover CLI and in the one case where this was called (`rover dev --graph-ref REF --supergraph-config CONFIG`) this seems like a clear improvement.

The `rover dev` and `rover supergraph compose` commands take a `--graph-ref` option (since v0.25.0 and v0.26.0 respectively) which loads subgraph definitions from GraphOS. These commands also take optional `--federation-version` arguments to let you specify the federation version, and you can also specify the federation version inside a supergraph.yaml file named with the `rover dev --supergraph-config`/`rover supergraph compose --config` option. Previously, if you did not specify a federation version via either of these mechanisms, the command would default to using the latest Fed v2 version. However, Studio has a specified federation version for each graph ref, which would be a better default for the case that the user does not specify the version directly. This change reads the graph's federation version from GraphOS in the same query that reads subgraph information, and uses that as the default federation version if a version is not specified via flag (priority 1) or config file (priority 2).

The actual implementation of the command already understands how to combine these flags, but their declaration did not let you do it.

There were a few typos in #2037 (and the override feature didn't work until this PR anyway).

…e_subgraphs` (#574) This function was added a few months ago in #541 and as far as I know is only used in Rover for the `supergraph compose` and `dev` commands to combine configuration from `--graph-ref` and `--supergraph-config`/`--config` (and the feature does not actually work for `supergraph compose` due to the issue fixed in apollographql/rover#2101). The use case here is to let you run composition or dev against a GraphOS graph with some local changes, and so "I just want to change the SDL and nothing else" is a reasonable use case. This change means that if an override specifies SDL but no routing URL for a subgraph, Rover will continue to use the routing URL fetched from GraphOS for that subgraph, which seems reasonable. While this is a "backwards incompatible" change, this crate is only intended for use by the Rover CLI and in the one case where this was called (`rover dev --graph-ref REF --supergraph-config CONFIG`) this seems like a clear improvement.

aaronArinder · 2024-09-06T18:55:39Z

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

+    fn from(
+        value: subgraph_fetch_all_query::SubgraphFetchAllQueryVariantOnGraphVariantSourceVariantLatestLaunch,
+    ) -> Self {
+        if let subgraph_fetch_all_query::SubgraphFetchAllQueryVariantOnGraphVariantSourceVariantLatestLaunchBuildInput::CompositionBuildInput(composition_build_input) = value.build_input {


this is a bit ugly; can we slim down the reference to CompositionBuildInput by importing it and the others?

Unfortunately there are two different CompositionBuildInputs in the two functions, so I'm not sure how to do that (though maybe I'm misunderstanding your suggestion — I'm not too strong at Rust).

oh! try something like this use blah::SuperLongName::BuildInput as ShorterBuildInput

aaronArinder · 2024-09-06T18:56:01Z

docs/source/commands/supergraphs.mdx

-rover supergraph config --graph-ref platform@staging
+rover supergraph compose --graph-ref platform@staging


aaronArinder · 2024-09-06T18:56:51Z

src/utils/supergraph_config.rs

-        let supergraph_config = SupergraphConfig::new(subgraphs, federation_version.cloned());
+        let supergraph_config = SupergraphConfig::new(subgraphs, federation_version);


aaronArinder · 2024-09-06T19:11:45Z

src/utils/supergraph_config.rs

+    let resolved_federation_version = target_federation_version
+        .cloned()
+        .or(local_config
+            .as_ref()
+            .and_then(|it| it.get_federation_version()))
+        .or(remote_config
+            .as_ref()
+            .and_then(|it| it.get_federation_version()))
+        .unwrap_or(FederationVersion::LatestFedTwo);


probably want .or_else so that what's inside each .or() isn't evaluated (.or_else is lazily evaluated)

aaronArinder · 2024-09-06T19:13:00Z

src/utils/supergraph_config.rs

+    // 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.


I'd add something about defaulting to latest_fed_two just to make it explicit that if there's no target, no remote, and no local, we're going to use that one

aaronArinder

minor nits; only thing I really care about is adding some clarity to the comment about which federation version is in use

pre-approving, but definitely want the commented expanded with the final default

stale, incoming changes

This pulls in apollographql/federation-rs#574 which improves the logic used to merge remote and local config such that if your local config does not specify a `routing_url` for a subgraph that also exists in the remote config, the remote config's URL is used instead of deleting it. Update the docs.

aaronArinder

ty for the changes! approving, but before merging, make sure the smoke tests pass (running now but moving on to other things)

glasser · 2024-09-06T20:39:01Z

Oops, this doesn't fully work — specifically, rover supergraph compose --graph-ref REF --config CONFIG where you leave the federation_version out of the config file does not actually use the ref's fed version, because resolve_supergraph_yaml has some smarts to try to deduce one. (Will follow up.)

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()`.)

Make `rover supergraph compose --graph-ref REF --config CONFIG` work as documented (and don't remove `routing_url`s fetched from GraphOS if they're missing from the local file). Make `rover supergraph compose --graph-ref REF` and `rover dev --graph-ref REF` use the graph ref's federation version. Fix docs. (Individual commits have more detailed messages.)

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()`.)

@glasser

## 🐛 Fixes - **Avoid misleading warning when `--output` is not specified - @glasser #2100** In the release of v0.26.1 logic was added to disable the output flag if the Federation version was less than 2.9, however this was being printed even when the `--output` flag was not supplied. This has been corrected. - **Improve `--graph-ref` option - @glasser #2101** In the release of v0.26.0 the `--graph-ref` option was added to `supergraph compose` as well as `rover dev`. However, the behaviour when `--graph-ref` was used in conjunction with `--config` did not work as documented. This is now fixed. Furthermore, both `rover dev` and `supergraph compose`, when using only the `--graph-ref` option, respect the graph ref's Federation version. - **Further improve `--graph-ref` option - @glasser #2105** Improves on the above by fixing some corner cases that prevented #2101 from working as intended ## 🛠 Maintenance - **Update `eslint` to v9.10.0 - @jonathanrainer #2106** - **Update `concurrently` to v9.0.0 - @jonathanrainer #2108** - **Update `manylinux` CI Docker Image to v2024.09.09 - @jonathanrainer #2110** - **Update Rust to v1.81.0 - @jonathanrainer #2107** - **Pass GitHub Tag to GitHub Actions Workflow - @glasser #2109** - **Add `tower` for use with HTTP/GraphQL clients - @dotdat #2067** ## 📚 Documentation - **Fix Glossary links - @Meschreiber @pnodet #2114**

@glasser

- **Avoid misleading warning when `--output` is not specified - @glasser In the release of v0.26.1 logic was added to disable the output flag if the Federation version was less than 2.9, however this was being printed even when the `--output` flag was not supplied. This has been corrected. - **Improve `--graph-ref` option - @glasser #2101** In the release of v0.26.0 the `--graph-ref` option was added to `supergraph compose` as well as `rover dev`. However, the behaviour when `--graph-ref` was used in conjunction with `--config` did not work as documented. This is now fixed. Furthermore, both `rover dev` and `supergraph compose`, when using only the `--graph-ref` option, respect the graph ref's Federation version. - **Further improve `--graph-ref` option - @glasser #2105** Improves on the above by fixing some corner cases that prevented #2101 from working as intended - **Update `eslint` to v9.10.0 - @jonathanrainer #2106** - **Update `concurrently` to v9.0.0 - @jonathanrainer #2108** - **Update `manylinux` CI Docker Image to v2024.09.09 - @jonathanrainer - **Update Rust to v1.81.0 - @jonathanrainer #2107** - **Pass GitHub Tag to GitHub Actions Workflow - @glasser #2109** - **Add `tower` for use with HTTP/GraphQL clients - @dotdat #2067** - **Fix Glossary links - @Meschreiber @pnodet #2114**

glasser requested review from a team as code owners September 5, 2024 23:03

glasser mentioned this pull request Sep 5, 2024

apollo-federation-types: merge routing_url in SupergraphConfig::merge_subgraphs apollographql/federation-rs#574

Merged

glasser force-pushed the glasser/improve-graph-ref branch from 8e0ed05 to 5a94020 Compare September 5, 2024 23:34

glasser added 3 commits September 5, 2024 16:40

compose: allow specifying --graph-ref and --config at the same time

928b4c1

The actual implementation of the command already understands how to combine these flags, but their declaration did not let you do it.

docs: document rover supergraph compose correctly

0b232ac

There were a few typos in #2037 (and the override feature didn't work until this PR anyway).

glasser force-pushed the glasser/improve-graph-ref branch from 5a94020 to 0b232ac Compare September 5, 2024 23:40

jonathanrainer added this to the v0.27.0 milestone Sep 6, 2024

aaronArinder reviewed Sep 6, 2024

View reviewed changes

aaronArinder previously approved these changes Sep 6, 2024

View reviewed changes

aaronArinder self-requested a review September 6, 2024 19:26

glasser added 4 commits September 6, 2024 12:29

Suggestions from code review

c3a6d64

More suggestions from code review

a8116ac

Fix fmt

ece1fcc

aaronArinder approved these changes Sep 6, 2024

View reviewed changes

glasser merged commit c075580 into main Sep 6, 2024
22 checks passed

glasser deleted the glasser/improve-graph-ref branch September 6, 2024 20:23

glasser mentioned this pull request Sep 6, 2024

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

Merged

jonathanrainer modified the milestones: v0.27.0, v0.26.2 Sep 9, 2024

jonathanrainer added the fix 🩹 fixes a bug label Sep 10, 2024

jonathanrainer mentioned this pull request Sep 10, 2024

release: v0.26.2 #2116

Merged

WontonSam mentioned this pull request Oct 2, 2024

[Snyk] Upgrade @apollo/rover from 0.14.2 to 0.26.2 WontonSam/apollo-federation-subgraph-compatibility#571

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(compose, dev): improve `--graph-ref` option #2101

fix(compose, dev): improve `--graph-ref` option #2101

glasser commented Sep 5, 2024 •

edited

Loading

aaronArinder Sep 6, 2024

glasser Sep 6, 2024

aaronArinder Sep 6, 2024

aaronArinder Sep 6, 2024

aaronArinder Sep 6, 2024

aaronArinder Sep 6, 2024

aaronArinder Sep 6, 2024

aaronArinder left a comment

aaronArinder left a comment

glasser commented Sep 6, 2024 •

edited

Loading

		rover supergraph config --graph-ref platform@staging
		rover supergraph compose --graph-ref platform@staging

		let supergraph_config = SupergraphConfig::new(subgraphs, federation_version.cloned());
		let supergraph_config = SupergraphConfig::new(subgraphs, federation_version);

fix(compose, dev): improve --graph-ref option #2101

fix(compose, dev): improve --graph-ref option #2101

Conversation

glasser commented Sep 5, 2024 • edited Loading

aaronArinder Sep 6, 2024

Choose a reason for hiding this comment

glasser Sep 6, 2024

Choose a reason for hiding this comment

aaronArinder Sep 6, 2024

Choose a reason for hiding this comment

aaronArinder Sep 6, 2024

Choose a reason for hiding this comment

aaronArinder Sep 6, 2024

Choose a reason for hiding this comment

aaronArinder Sep 6, 2024

Choose a reason for hiding this comment

aaronArinder Sep 6, 2024

Choose a reason for hiding this comment

aaronArinder left a comment

Choose a reason for hiding this comment

aaronArinder left a comment

Choose a reason for hiding this comment

glasser commented Sep 6, 2024 • edited Loading

fix(compose, dev): improve `--graph-ref` option #2101

fix(compose, dev): improve `--graph-ref` option #2101

glasser commented Sep 5, 2024 •

edited

Loading

glasser commented Sep 6, 2024 •

edited

Loading