fix(dev): intermediate composition states have noisy errors

[aaronArinder]

notes

• the way we're composing subgraphs with dev --supergraph-config means that there are intermediate states of composition, and sometimes those intermediate states fail composition because they require other subgraphs to be present
• lots of errors show up, but they're only ephemeral
• fixes those errors by waiting until we've at least as many subgraphs loaded in as are represented by the supergraph.yaml, running composition only when we've at least those many
• Don't show intermediate composition errors for rover dev --supergraph-config #1919

before:

after

testing

I added three new subgraphs to the supergraph example; there's a branch with the same code as this one (aaron/fix-intermediate-composition-errors-testing), but with those new subgraphs added. Comment out the subgraph length check to see the failure; from within the examples directory, run cargo r -- dev --supergraph-config ./supergraph.yaml. You might have to run it a couple times to see the failure because of the thread pool

[aaronArinder]

renamed this private method because I found it super confusing (it's not the supergraph config in terms of the yaml, but rather the unfolding supergraph config as it's being built out of subgraphs added to it--ie, it can represent intermediate states of the yaml version of the config); updated the comment, too, because there's no longer a to_string()

[jonathanrainer]

LGTM

[loshz]

How expensive could the clone be here? I think we're doing this to get around the ownership problem of taking from the Option, but I'm wondering if there's a len() method on self.supergraph_config so we don't need to transform it into an iterator?

Something like this:

if let Some(supergraph_config) = &self.supergraph_config {
    if self.subgraphs.len() < supergraph_config.len() {
        return LeaderMessageKind::MessageReceived;
    }
}

[aaronArinder]

that's a good question, and kind of a tricky one. len() only works on iterables; so, there's no len() off of SupergraphConfig, and there's also no way to get the subgraphs out of that config without having to do error handling (the return type of get_subgraph_definitions() is ConfigResult<Vec<SubgraphDefinition>>); otherwise, we might have been able to do something like supergraph_config.subgraphs.len().

So, to get an iterable to do len() for our comparison, and when limited to just into_iter() (there's no iter() off of SupergraphConfig), we need an owned value. That's why we clone() here.

Cost is a good thing to call out, though! I think for very large supergraph configs, this might be slow; the reason I'm not too worried about it yet (which is to say, why I'm leaving it as some later iteration) is because for large supergraph configs, the noise generated by intermediate composition errors is more annoying and misleading than a little bit of added time to add subgraphs to the internal representation of the config

[dotdat]

This seems like a failing of SupergraphConfig in apollo-federation-types. Adding a getter for this data would probably be a good idea.

[aaronArinder]

there is, get_subgraph_definitions(), but internally it composes the supergraph before giving back the subgraphs; so, it puts the cart a bit before the horse from our perspective, in that we're also composing

a more permissive getter that sends back whatever it has without composition would be nice