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.

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

Don't run IsFederatedGraph before running SubgraphFetchQuery #2004

Merged
merged 1 commit into from
Jul 29, 2024

Conversation

glasser
Copy link
Member

@glasser glasser commented Jul 26, 2024

All the checks done by IsFederatedGraph are also done by SubgraphFetchQuery (checking to see if graph/variant exists and to see if the graph is federated). This is just an additional round-trip adding latency and potential errors. Additionally, with rover supergraph compose which is commonly run against many subgraphs of a single graph, these will run fully redundantly on every subgraph that is fetched.

@glasser glasser requested a review from a team as a code owner July 26, 2024 17:56
@glasser
Copy link
Member Author

glasser commented Jul 26, 2024

This code was first added in #1056 and it's not clear to me why.

@glasser
Copy link
Member Author

glasser commented Jul 26, 2024

(want to check to make sure that in the non-federated case the right error comes out the other side still — looks like it from code examination though)

@glasser
Copy link
Member Author

glasser commented Jul 26, 2024

OK, here's some manual QA. The graph I'm poking at is non-federated on current but federated on p-1.

Rover 0.25.0:

☸ prod (router) in 󰯭/rover on  glasser/just-fetch-it-do-not-check-first [⇡1] is 📦 v0.25.0 via 🦀 v1.79.0  p= 󰦜 at 12:01:26-07 ❯ rover --version                                                
Rover 0.25.0

☸ prod (router) in 󰯭/rover on  glasser/just-fetch-it-do-not-check-first [⇡1] is 📦 v0.25.0 via 🦀 v1.79.0  p= 󰦜 at 12:01:31-07 ❯ rover subgraph fetch Apollo-Fullstack-Demo-o3tsz8 --name x
Fetching SDL from Apollo-Fullstack-Demo-o3tsz8@current (subgraph: x) using credentials from the default profile.
error[E007]: The graph `Apollo-Fullstack-Demo-o3tsz8@current` is a non-federated graph. This operation is only possible for federated graphs.
        Try running the command on a valid federated graph, or use the appropriate `rover graph` command instead of `rover subgraph`.

☸ prod (router) in 󰯭/rover on  glasser/just-fetch-it-do-not-check-first [⇡1] is 📦 v0.25.0 via 🦀 v1.79.0  p= 󰦜 at 12:01:40-07 ❌1 ❯ rover subgraph fetch nonexistent --name x                 
Fetching SDL from nonexistent@current (subgraph: x) using credentials from the default profile.
error[E010]: Could not find graph with name "nonexistent@current"
        Make sure your graph name is typed correctly, and that your API key is valid.
        You can run `rover config whoami` to check if you are authenticated.
        If you are trying to create a new graph, you must do so online at https://studio.apollographql.com, by clicking "New Graph".

☸ prod (router) in 󰯭/rover on  glasser/just-fetch-it-do-not-check-first [⇡1] is 📦 v0.25.0 via 🦀 v1.79.0  p= 󰦜 at 12:01:49-07 ❌1 ❯ rover subgraph fetch Apollo-Fullstack-Demo-o3tsz8@p-1 --name x
Fetching SDL from Apollo-Fullstack-Demo-o3tsz8@p-1 (subgraph: x) using credentials from the default profile.
error[E009]: Could not find subgraph "x".
        Try running this command with one of the following valid subgraphs: [subgraph]

☸ prod (router) in 󰯭/rover on  glasser/just-fetch-it-do-not-check-first [⇡1] is 📦 v0.25.0 via 🦀 v1.79.0  p= 󰦜 at 12:01:56-07 ❌1 ❯ rover subgraph fetch Apollo-Fullstack-Demo-o3tsz8@p-1 --name subgraph   
Fetching SDL from Apollo-Fullstack-Demo-o3tsz8@p-1 (subgraph: subgraph) using credentials from the default profile.
Schema: 

type Maya {
  name: String
}

type MayaIsSOCOOL {
  howCool: Int
}

extend type Query {
  me: User
}

type User @key(fields: "id") {
  id: ID!
  name: String
  username: String
}


☸ prod (router) in 󰯭/rover on  glasser/just-fetch-it-do-not-check-first [⇡1] is 📦 v0.25.0 via 🦀 v1.79.0  p= 󰦜 at 12:02:04-07 ❯ rover --log=debug subgraph fetch Apollo-Fullstack-Demo-o3tsz8 --name x |& grep IsFederated
  DEBUG rover_client::blocking::client: Request Body: {"variables":{"graph_id":"Apollo-Fullstack-Demo-o3tsz8","variant":"current"},"query":"query IsFederatedGraph($graph_id: ID!, $variant: String!) {\n  graph(id: $graph_id) {\n    variant(name: $variant) {\n      subgraphs {\n        name\n      }\n    }\n  }\n}\n","operationName":"IsFederatedGraph"}

Rover from this branch:

☸ prod (router) in 󰯭/rover on  glasser/just-fetch-it-do-not-check-first [⇡1] is 📦 v0.25.0 via 🦀 v1.79.0  p= 󰦜 at 12:02:30-07 ❯ target/debug/rover subgraph fetch Apollo-Fullstack-Demo-o3tsz8 --name x                   
Fetching SDL from Apollo-Fullstack-Demo-o3tsz8@current (subgraph: x) using credentials from the default profile.
error[E007]: The graph `Apollo-Fullstack-Demo-o3tsz8@current` is a non-federated graph. This operation is only possible for federated graphs.
        If you are sure you want to convert a non-federated graph to a subgraph, you can re-run the same command with a `--convert` flag.

☸ prod (router) in 󰯭/rover on  glasser/just-fetch-it-do-not-check-first [⇡1] is 📦 v0.25.0 via 🦀 v1.79.0  p= 󰦜 at 12:02:50-07 ❌1 ❯ target/debug/rover subgraph fetch nonexistent --name x                                    
Fetching SDL from nonexistent@current (subgraph: x) using credentials from the default profile.
error[E010]: Could not find graph with name "nonexistent@current"
        Make sure your graph name is typed correctly, and that your API key is valid.
        You can run `rover config whoami` to check if you are authenticated.
        If you are trying to create a new graph, you must do so online at https://studio.apollographql.com, by clicking "New Graph".

☸ prod (router) in 󰯭/rover on  glasser/just-fetch-it-do-not-check-first [⇡1] is 📦 v0.25.0 via 🦀 v1.79.0  p= 󰦜 at 12:02:55-07 ❌1 ❯ target/debug/rover subgraph fetch Apollo-Fullstack-Demo-o3tsz8@p-1 --name x               
Fetching SDL from Apollo-Fullstack-Demo-o3tsz8@p-1 (subgraph: x) using credentials from the default profile.
error[E009]: Could not find subgraph "x".
        Try running this command with one of the following valid subgraphs: [subgraph]

☸ prod (router) in 󰯭/rover on  glasser/just-fetch-it-do-not-check-first [⇡1] is 📦 v0.25.0 via 🦀 v1.79.0  p= 󰦜 at 12:03:02-07 ❌1 ❯ target/debug/rover subgraph fetch Apollo-Fullstack-Demo-o3tsz8@p-1 --name subgraph        
Fetching SDL from Apollo-Fullstack-Demo-o3tsz8@p-1 (subgraph: subgraph) using credentials from the default profile.
Schema: 

type Maya {
  name: String
}

type MayaIsSOCOOL {
  howCool: Int
}

extend type Query {
  me: User
}

type User @key(fields: "id") {
  id: ID!
  name: String
  username: String
}


☸ prod (router) in 󰯭/rover on  glasser/just-fetch-it-do-not-check-first [⇡1] is 📦 v0.25.0 via 🦀 v1.79.0  p= 󰦜 at 12:03:07-07 ❯ target/debug/rover --log=debug subgraph fetch Apollo-Fullstack-Demo-o3tsz8 --name x |& grep IsFederated 

The actual commands return the same thing. The log grep shows we are not doing the IsFederatedGraph operation with the new build.

Copy link
Contributor

@aaronArinder aaronArinder left a comment

Choose a reason for hiding this comment

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

this looks pretty sensible, but I don't understand why it was introduced originally; maybe we can get to the bottom of it on monday together when we talk with the other rover people? It definitely gives me pause because it looks pretty intentional

All the checks done by IsFederatedGraph are also done by
SubgraphFetchQuery (checking to see if graph/variant exists and to see
if the graph is federated). This is just an additional round-trip adding
latency and potential errors. Additionally, with `rover supergraph compose`
which is commonly run against many subgraphs of a single graph, these
will run fully redundantly on every subgraph that is fetched.
@aaronArinder aaronArinder force-pushed the glasser/just-fetch-it-do-not-check-first branch from de42ee3 to 25142b6 Compare July 29, 2024 16:10
@aaronArinder aaronArinder enabled auto-merge (squash) July 29, 2024 16:10
@aaronArinder aaronArinder merged commit d771cbe into main Jul 29, 2024
19 checks passed
@aaronArinder aaronArinder deleted the glasser/just-fetch-it-do-not-check-first branch July 29, 2024 16:21
jonathanrainer added a commit that referenced this pull request Jul 30, 2024
jonathanrainer added a commit that referenced this pull request Jul 31, 2024
aaronArinder pushed a commit that referenced this pull request Aug 1, 2024
@jonathanrainer jonathanrainer added the fix 🩹 fixes a bug label Aug 20, 2024
@jonathanrainer jonathanrainer added this to the v0.26.0 milestone Aug 20, 2024
@jonathanrainer jonathanrainer mentioned this pull request Aug 20, 2024
jonathanrainer added a commit that referenced this pull request Aug 21, 2024
> 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**
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix 🩹 fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants