Add LSP functionality to Rover #2272

jonathanrainer · 2024-11-24T08:51:39Z

Now that the LSP has been finalised, and composition has been refactored such that it can consume events in the same way that rover dev does we can unify the two together.

This has been tested by running a custom build version of Rover against VSCode and it seems to function as intended. We will need to hash out a few more things in the review overall but this is a very positive step forwards

svc-apollo-docs · 2024-11-24T08:51:42Z

✅ Docs Preview Ready

No new or changed pages found.

jonathanrainer · 2024-11-25T09:07:38Z

crates/robot-panic/src/lib.rs

The work here is clearing up some stuff that should have been a while back after we upgraded to a new version of Rust. I think this hasn't been picked up because often we don't do release builds, which I had to do as part of this

jonathanrainer · 2024-11-25T09:07:56Z

crates/rover-std/Cargo.toml

This is just clearing up some unused dependencies, nothing to see here really

jonathanrainer · 2024-11-25T09:08:32Z

src/command/lsp/README.md

Not sure if we still need this file or if we just want to remove it now? I've left it here as it was part of the original PR but can easily remove it if not

jonathanrainer · 2024-11-25T09:08:47Z

src/command/lsp/input.txt

Same comment with this file, we can easily remove it if it's no longer necessary

jonathanrainer · 2024-11-25T09:09:10Z

src/command/lsp/mod.rs

This is where the real meat of the PR is, as this is where we wire together our work on Composition with the LSP itself

jonathanrainer · 2024-11-25T09:10:18Z

src/command/lsp/mod.rs

+    //TODO: Check this error handling is right i.e. if we don't get a supergraph.yaml passed
+    // this should fail rather than doing something else.


This one I wasn't particularly sure about, in my view, with this command, we should make passing a Supergraph YAML compulsory because I can't really understand what not passing one would even mean. That would mean we could make clap take it as a Path, rather than an Option and tidy this code up a bit.

also not sure; I can't tell from the crate docs whether the lsp is meant to be only for supergraphs (ie, federation) or if there's some other use for it

I would have assumed only supergraphs, but there's an odd option in the config for the lsp: force federation, which forces all graphs to be treated as subgraphs. Maybe that means you can use it on monographs? Do they have a supergraph.yml or are they complete as they are? If they still require a supergraph.yml, I think making it compulsory makes sense

The language server does standard GraphQL validations all on its own, outside of federation. So you can totally use our extension with rover lsp for a monograph if you don't pass in a supergraph.yaml file. Passing in that file is what triggers the composition mode—so most of this logic. If you don't pass in the file, rover lsp is mostly just a passthrough to the language server crate.

jonathanrainer · 2024-11-25T09:11:26Z

src/command/lsp/mod.rs

+        // TODO: Check defaulting behaviour here and see if we need to centralise
+        let federation_version = lazily_resolved_supergraph_config
+            .federation_version()
+            .clone()
+            .unwrap_or(FederationVersion::LatestFedTwo);


Have we centralised the behaviour on detecting the Federation Version? I feel like we definitely should if we haven't already

We've a rough outline of it here, but I think only the FullyResolvedSupergraphConfig captures that via its resolve_federation_version method. I could be wrong about that, though

Here's how I think the federation version is being resolved in this file, though:

default SupergraphConfigResolver starts the federation version at None

remote loading of subgraphs doesn't change the federation version, so it's still at None

loading the supergraph config yaml from file will take the federation version from that file if it exists, but otherwise will keep it the same (None); so, now we either have a config-defined fed version or just None

lazily resolving the subgraph config will take either the config-file defined fed version or None

finally, you're defaulting to fed 2

so, either it comes from the config file or it gets defaulted to fed 2, which seems alright, but we aren't really respecting the order in the confluence link above (or centralizing it in a way that is helpful rather than confusing/harmful)

oh, came across this in the source while looking at something else: panic when calling composition_did_update on a non-supergraph, so maybe requiring the supergraph config yaml is the strat!

this also raises a question about how panics are being handled; what happens in rover when the LSP server takes a dirt nap from some internal panic?

jonathanrainer · 2024-11-25T09:12:32Z

src/command/lsp/mod.rs

+        // TODO: Let the supergraph binary exist inside its own task that can respond to being re-installed etc.
+        let supergraph_binary =
+            InstallSupergraph::new(federation_version.clone(), client_config.clone())
+                .install(
+                    None,
+                    lsp_opts.plugin_opts.elv2_license_accepter,
+                    lsp_opts.plugin_opts.skip_update,
+                )
+                .await?;


One of the features (I think) of the old PR is that it allows people to update the version of Federation inside the supergraph.yaml while it's running. When doing the dev refactor we made an explicit decision that we didn't want to support that behaviour. Is that a dealbreaker for the LSP? Or could we ship this as is and see if there's sufficient demand after the fact to implement that?

It's a pretty bad UX to not watch supergraph.yaml because the way to manually restart the language server underneath VS Code is really clunky—and it's not at all obvious when you should do that. If someone modifies their supergraph.yaml file and still sees out of date validations, it's going to be frustrating.

jonathanrainer · 2024-11-25T09:15:21Z

src/command/lsp/mod.rs

+                lazily_resolved_supergraph_config
+                    .fully_resolve_subgraphs(&client_config, &studio_client)


This method is new because in the original version of the refactor you either had a FullyResolvedSupergraphConfig (so all the elements had been reduced to SDL sources), or a LazilyResolvedSupergraphConfig (which still had the capacity to spin up FileWatchers etc.).

The issue in this instance is that you need both (or specifically you need a way to obtain FullyResolvedSubgraphs). So I decided that it was best to implement a method to convert a LazilyResolvedSupergraphConfig to a FullyResolvedSubgraphs. Because this transformation is only possible one way as you lose information once they're fully resolved.

Happy to argue this one out, because it is a bit weird that the Runner requires a LazilyResolvedSupergraphConfig at the top and then FullyResolved in later methods but didn't want to touch that code too much due to the ongoing dev refactor

jonathanrainer · 2024-11-25T09:16:30Z

src/command/lsp/mod.rs

+                        )
+                        .await;
+                }
+                CompositionEvent::Error(CompositionError::Build { source: errors }) => {


This is slightly different to the original version because the dev refactor does not distinguish at the top level between "Completing with errors" and "Erroring". So we have to pattern match on the error itself to get what we want.

jonathanrainer · 2024-11-25T09:16:50Z

src/command/lsp/mod.rs

+                    // TODO: we could highlight the version of federation, since it failed.
+                    let message = format!("Failed run composition {federation_version}: {err}",);


Not sure what this comment means, it came over verbatim from the original PR

Basically instead of using Range::default() below, which results in no red squigglies in any files, if we start retaining location data when we parse supergraph.yaml, we could highlight part of that file when this error happens. The version is the most likely place, since it means there was something wrong with running the composition binary, not something wrong with actually composing.

jonathanrainer · 2024-11-25T09:17:26Z

src/command/lsp/mod.rs

+                CompositionEvent::SubgraphAdded(CompositionSubgraphAdded {
+                    name,
+                    schema_source,
+                }) => {
+                    debug!("Subgraph {} added", name);
+                    language_server.add_subgraph(name, schema_source).await;
+                }
+                CompositionEvent::SubgraphRemoved(CompositionSubgraphRemoved { name }) => {
+                    debug!("Subgraph {} removed", name);
+                    language_server.remove_subgraph(&name).await;
+                }


These are net new events because the language server needs to know when subgraphs are added and removed, so I expanded out the definition of CompositionEvents to include these.

trying to get an initial review out, but self-note: probs need to check that all the places we add/remove subgraphs are emitting the right event

jonathanrainer · 2024-11-25T09:18:00Z

src/composition/mod.rs

+    pub(crate) supergraph_sdl: String,
+    pub(crate) hints: Vec<BuildHint>,
+    pub(crate) federation_version: FederationVersion,


pub(crate) might not be right here, they could probably all be public since they're events

jonathanrainer · 2024-11-25T09:18:44Z

src/composition/supergraph/config/resolve/mod.rs

+    pub async fn fully_resolve_subgraphs(
+        self,
+        introspect_subgraph_impl: &impl IntrospectSubgraph,
+        fetch_remote_subgraph_impl: &impl FetchRemoteSubgraph,
+    ) -> Result<FullyResolvedSubgraphs, Vec<ResolveSubgraphError>> {
+        let subgraphs = stream::iter(self.subgraphs.into_iter().map(
+            |(name, lazily_resolved_subgraph)| async {
+                let result = FullyResolvedSubgraph::fully_resolve(
+                    introspect_subgraph_impl,
+                    fetch_remote_subgraph_impl,
+                    lazily_resolved_subgraph,
+                    name.clone(),
+                )
+                .await?;
+                Ok((name, result))
+            },
+        ))
+        .buffer_unordered(50)
+        .collect::<Vec<Result<(String, FullyResolvedSubgraph), ResolveSubgraphError>>>()
+        .await;
+        let (subgraphs, errors): (
+            Vec<(String, FullyResolvedSubgraph)>,
+            Vec<ResolveSubgraphError>,
+        ) = subgraphs.into_iter().partition_result();
+        if errors.is_empty() {
+            Ok(subgraphs.into())
+        } else {
+            Err(errors)
+        }
+    }


This is very similar to the method for doing this for the FullyResolvedSupergraphConfig but with enough difference to not want to refactor it into a single method

I wonder if we want to look at the two, largely because resolving the federation version consistently would be useful. The current implementation here doesn't really take into account subgraphs, unless I missed something.

jonathanrainer · 2024-11-25T09:19:08Z

src/composition/supergraph/config/resolve/mod.rs

+    pub fn upsert_subgraph(&mut self, name: String, schema: String) -> bool {
+        self.subgraphs.insert(name, schema).is_none()


Slightly changing the upsert mechanism allows us to see if we're adding a subgraph for the first time so we can emit the right events for the LSP

don't love this return type because I'm not sure what it means (outside of this comment); worth calling it out in its comment why it returns a bool? or, how do you feel about a more self-documenting enum whose variants capture what we'd want to know? (or, maybe better: should we just move the logic for telling whether we're adding subgraphs for the first time to the bit of code that actually cares about it? that'd better separate what we're up to)

jonathanrainer · 2024-11-25T09:19:43Z

src/composition/supergraph/config/resolve/mod.rs

+                    // This value is effectively unused but has to be set to something that
+                    // is not "null" when serialised otherwise the supergraph binary
+                    // complains
+                    routing_url: Some(String::from("localhost")),


This was a real surprise and not something I'd considered before, but without this composition did not work. We may want a different nonsense value (would the empty string work?)

huh, yeah, weird; maybe an empty string would be good? or, some other weird value like "NOOP VALUE" or something to capture that we're the ones who've added it and that we don't expect it to be useful (apart from getting composition to pass); I could see myself being pretty confused by the localhost default later on if I were debugging something with composition

Where is this conversion used? The routing URLs are important because that's how the router knows where to send subgraph traffic.

jonathanrainer · 2024-11-25T09:20:17Z

src/composition/supergraph/config/resolve/subgraph.rs

+        /// The result of joining the paths together, that caused the failure
+        joined_path: PathBuf,


Found the need to expand the definition of this error to aid my own debugging

jonathanrainer · 2024-11-25T09:21:03Z

src/composition/supergraph/config/resolve/subgraph.rs

+    }
+
+    /// Fully resolves a [`LazilyResolvedSubgraph`] to a [`FullyResolvedSubgraph`]
+    pub async fn fully_resolve(


Resolving and fully resolving are quite similar so I pulled out the common parts below and left us with a structure that is similar but allows the relevant differences to be drawn out

our language around resolution is super confusing; resolve and fully_resolve both return a FullyResolvedSubgraph, but the latter makes it sound like "this time I'm serious about fully resolving it" rather than the inputs being different

not sure there's anything to do about it, but it's confusing

the way github interleaves the diff makes it hard for me to really understand the differences between the two; probably worth having @dotdat take a stab at it

jonathanrainer · 2024-11-25T09:21:42Z

src/composition/watchers/composition.rs

This part is simply augmenting the composition system to emit the right events now that we care about subgraph additions and removals.

It also fixes a test that this broke

jonathanrainer · 2024-11-25T09:22:12Z

src/utils/supergraph_config.rs

These were some leftover clippies that I fixed to get CI passing

jonathanrainer · 2024-11-25T09:28:56Z

Also a general comment:

@dylan-apollo I couldn't see a way to inject a profile into the Rover invocation for the LSP, which meant I had to mess around to get my default profile set up. Is this intentional or did I miss something?

Required adding a lot of ceremony to getting subgraphs in a form ammenable to composition piece, however this may have been inevitable as this part of the refactor has not yet been used.

The LSP has to track subgraphs being added and removed so we need to expand the language of composition events to do this.

The first is that we weren't supplying the correct content root for the supergraph.yaml. The second is that we need to have a value of the routing URLm, even if it's not used, when it goes to the supergraph.yaml

aaronArinder · 2024-12-02T16:26:59Z

src/cli.rs

+
+    /// Start the language server
+    #[cfg(feature = "composition-js")]
+    #[clap(hide = true)]


to doublecheck, we want this hidden?

Yes, this is supposed to be an implementation detail of the VS Code extension right now, not something we broadly support (i.e., no public/stable API requirement). Maybe some day we'll lock it down, but probably not until we're done tweaking it for our own extensions.

aaronArinder · 2024-12-02T16:28:10Z

src/command/lsp/README.md

@@ -0,0 +1,14 @@
+# Build / configuration
+
+1. Clone mdg-private/language-server repo (adjacent to this repo, the language


this is sort of weird; we should hide the mdg-private callout, but also figure out what this readme is even supposed to convey (and who it's being conveyed to)

It was for us, from earlier versions before the language server crate was published. This can be removed now

aaronArinder · 2024-12-02T16:34:03Z

src/command/lsp/mod.rs

+    //TODO: Check this error handling is right i.e. if we don't get a supergraph.yaml passed
+    // this should fail rather than doing something else.


also not sure; I can't tell from the crate docs whether the lsp is meant to be only for supergraphs (ie, federation) or if there's some other use for it

I would have assumed only supergraphs, but there's an odd option in the config for the lsp: force federation, which forces all graphs to be treated as subgraphs. Maybe that means you can use it on monographs? Do they have a supergraph.yml or are they complete as they are? If they still require a supergraph.yml, I think making it compulsory makes sense

aaronArinder · 2024-12-02T16:34:29Z

src/command/lsp/mod.rs

+            }
+        })
+        .ok_or_else(|| anyhow!("Could not find supergraph.yaml file."))?;
+    let supergraph_content_root = supergraph_yaml_path.parent().unwrap().to_path_buf();


kill the unwrap?

aaronArinder · 2024-12-02T16:38:35Z

src/command/lsp/mod.rs

+        Config {
+            root_uri: supergraph_content_root.to_string(),
+            enable_auto_composition: false,
+            force_federation: false,
+            disable_telemetry: false,
+        },


I wonder if we should do something else with these options. Eg, the disable_telemetry should probably follow APOLLO_TELEMETRY_DISABLED, but I don't really understand force_federation's use or when you'd want to enable or diasble auto-composition (I guess if you're not relying on rover's composition pipeline?)

aaronArinder · 2024-12-02T18:38:03Z

src/composition/supergraph/config/resolve/mod.rs

+    pub fn upsert_subgraph(&mut self, name: String, schema: String) -> bool {
+        self.subgraphs.insert(name, schema).is_none()


don't love this return type because I'm not sure what it means (outside of this comment); worth calling it out in its comment why it returns a bool? or, how do you feel about a more self-documenting enum whose variants capture what we'd want to know? (or, maybe better: should we just move the logic for telling whether we're adding subgraphs for the first time to the bit of code that actually cares about it? that'd better separate what we're up to)

aaronArinder · 2024-12-02T18:39:17Z

src/composition/supergraph/config/resolve/mod.rs

+                    // This value is effectively unused but has to be set to something that
+                    // is not "null" when serialised otherwise the supergraph binary
+                    // complains
+                    routing_url: Some(String::from("localhost")),


huh, yeah, weird; maybe an empty string would be good? or, some other weird value like "NOOP VALUE" or something to capture that we're the ones who've added it and that we don't expect it to be useful (apart from getting composition to pass); I could see myself being pretty confused by the localhost default later on if I were debugging something with composition

aaronArinder · 2024-12-02T18:41:00Z

src/composition/supergraph/config/resolve/subgraph.rs

-    #[error("Could not find schema file ({path}) relative to ({supergraph_config_path}) for subgraph `{subgraph_name}`")]
+    #[error("Could not find schema file ({path}) relative to ({supergraph_config_path}) for subgraph `{subgraph_name}`"
+    )]


weird that this passes linting, but who am I to defy clippy

aaronArinder · 2024-12-02T18:50:33Z

src/composition/supergraph/config/resolve/subgraph.rs

+    }
+
+    /// Fully resolves a [`LazilyResolvedSubgraph`] to a [`FullyResolvedSubgraph`]
+    pub async fn fully_resolve(


our language around resolution is super confusing; resolve and fully_resolve both return a FullyResolvedSubgraph, but the latter makes it sound like "this time I'm serious about fully resolving it" rather than the inputs being different

not sure there's anything to do about it, but it's confusing

aaronArinder · 2024-12-02T18:51:22Z

src/composition/supergraph/config/resolve/subgraph.rs

+    }
+
+    /// Fully resolves a [`LazilyResolvedSubgraph`] to a [`FullyResolvedSubgraph`]
+    pub async fn fully_resolve(


the way github interleaves the diff makes it hard for me to really understand the differences between the two; probably worth having @dotdat take a stab at it

dylan-apollo · 2024-12-02T19:05:44Z

@dylan-apollo I couldn't see a way to inject a profile into the Rover invocation for the LSP, which meant I had to mess around to get my default profile set up. Is this intentional or did I miss something?

@jonathanrainer there is an extraArgs that can be put in the VS Code extension's config to pass a list of arbitrary extra things to Rover... like profile. That's really a catch-all workaround right now, though, we should probably make profile it's own, top-level option in the extension's config.

jonathanrainer requested a review from a team as a code owner November 24, 2024 08:51

jonathanrainer force-pushed the jr/task/ROVER-245 branch 2 times, most recently from 9d740fa to 91bbdc6 Compare November 24, 2024 08:54

jonathanrainer commented Nov 25, 2024

View reviewed changes

crates/rover-std/Cargo.toml Outdated

Copy link

Contributor Author

jonathanrainer Nov 25, 2024

Choose a reason for hiding this comment

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

This is just clearing up some unused dependencies, nothing to see here really

jonathanrainer commented Nov 25, 2024

View reviewed changes

src/command/lsp/input.txt Outdated

Copy link

Contributor Author

jonathanrainer Nov 25, 2024

Choose a reason for hiding this comment

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

Same comment with this file, we can easily remove it if it's no longer necessary

jonathanrainer commented Nov 25, 2024

View reviewed changes

src/utils/supergraph_config.rs Outdated

Copy link

Contributor Author

jonathanrainer Nov 25, 2024

Choose a reason for hiding this comment

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

These were some leftover clippies that I fixed to get CI passing

jonathanrainer added 5 commits November 26, 2024 13:21

ROVER-245 Fix up Cargo.tomls and deny.toml

0287cdc

ROVER-245 First attempt at partial integration

3957bab

Required adding a lot of ceremony to getting subgraphs in a form ammenable to composition piece, however this may have been inevitable as this part of the refactor has not yet been used.

ROVER-245 Plumbing in subgraph change events

dd046d8

The LSP has to track subgraphs being added and removed so we need to expand the language of composition events to do this.

ROVER-245 Removing deprecated PanicInfo

86d06cf

ROVER-245 Fix two small buglets

17f7d1e

The first is that we weren't supplying the correct content root for the supergraph.yaml. The second is that we need to have a value of the routing URLm, even if it's not used, when it goes to the supergraph.yaml

ROVER-245 Fix failing test and clippies

c02e230

jonathanrainer force-pushed the jr/task/ROVER-245 branch from 2a746aa to c02e230 Compare November 26, 2024 13:21

aaronArinder reviewed Dec 2, 2024

View reviewed changes

		//TODO: Check this error handling is right i.e. if we don't get a supergraph.yaml passed
		// this should fail rather than doing something else.

		lazily_resolved_supergraph_config
		.fully_resolve_subgraphs(&client_config, &studio_client)

		// TODO: we could highlight the version of federation, since it failed.
		let message = format!("Failed run composition {federation_version}: {err}",);

		pub fn upsert_subgraph(&mut self, name: String, schema: String) -> bool {
		self.subgraphs.insert(name, schema).is_none()

		/// The result of joining the paths together, that caused the failure
		joined_path: PathBuf,

		@@ -0,0 +1,14 @@
		# Build / configuration

		1. Clone mdg-private/language-server repo (adjacent to this repo, the language

Add LSP functionality to Rover #2272

Are you sure you want to change the base?

Add LSP functionality to Rover #2272

Conversation

jonathanrainer commented Nov 24, 2024 • edited Loading

svc-apollo-docs commented Nov 24, 2024 • edited Loading

✅ Docs Preview Ready

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

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

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

jonathanrainer commented Nov 25, 2024

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

dylan-apollo commented Dec 2, 2024

jonathanrainer commented Nov 24, 2024 •

edited

Loading

svc-apollo-docs commented Nov 24, 2024 •

edited

Loading