ROVER-286 Removing routing_url gives correct error message
#2360
Conversation
There seems to have been a thought that a subgraph should be considered added and removed if the `routing_url` changes. That is not a good idea and is now reverted.
When we resolve our subgraphs we now use a default value rather than erroring if the Routing URL is missing in two specific cases. Otherwise we cannot boot the CompositionPipeline without all routing_urls which goes against our requirement we be able to start up even with a broken `supergraph.yaml`
|
| let routing_url = unresolved_subgraph | ||
| .routing_url() | ||
| .clone() | ||
| .unwrap_or(String::from("UNKNOWN")); |
There was a problem hiding this comment.
This change, and the similar one which removes ResolveSubgraphError::MissingRoutingUrl are made so that if the routing_url is missing in these two cases, when we come to resolve. We'll resolve with a junk value instead of erroring. Erroring at this point causes it to bubble up to the very top level of the composition pipeline, and that is fatal for the LSP early on. This junk value is not visible to users and as there is no value, and UNKNOWN is not a valid URL no traffic will be directed anywhere we don't want.
Furthermore once a user tries to correct this it will get overwritten with either a correct value, or an error will get thrown if they don't include a routing_url in the first place, so this strikes a balance between getting good behaviour even if the supergraph.yaml is broken and not having very complex error handling
| pub fn get_subgraph(&mut self, name: &str) -> Option<FullyResolvedSubgraph> { | ||
| self.subgraphs.get(name).cloned() | ||
| } |
There was a problem hiding this comment.
This is to enable us to get a copy of a subgraph from a live SupergraphConfig, then update its routing_url and put it back again, as update requires us to have the whole subgraph. I tried a version that used get_mut but this caused FileWatchers to be dropped amongst other things and debugging that seemed harder than just using this slightly more round-a-bout method
| None => { | ||
| debug!("Could not find subgraph '{}'", name); | ||
| continue | ||
| }, |
There was a problem hiding this comment.
This arm is really only included for completeness, as if this method is called there should be a subgraph otherwise it wouldn't be being called in the first place.
| /// A change to the watched subgraphs schema | ||
| SchemaChanged(SubgraphSchemaChanged), | ||
| /// A change to the watched subgraphs routing_url | ||
| RoutingUrlChanged(SubgraphRoutingUrlChanged), |
There was a problem hiding this comment.
Clippy complained about all of these having the same prefix so I made a slight change here
There was a problem hiding this comment.
It seems that at some point I thought that the pair of name and routing_url should uniquely identify a subgraph but that's nonsense and changing this doesn't break anything so I'm planning on reverting this and leaving it.
As per title, due to some particularly inconvenient handling of federation resolution subgraphs losing a routing_url while the LSP was running, or not having one to start with were causing us many problems. This is now resolved and I've run over the test suite we have and all tests are passing (with the exception of the
--profileones, which are a known issue).Some comments are added throughout to explain the decisions taken.