Parse supergraph schema early to avoid re-parsing#5736
Conversation
|
@SimonSapin, please consider creating a changeset entry in |
|
CI performance tests
|
| ), | ||
| )) | ||
| .supergraph_sdl(schema.raw_sdl.clone()) | ||
| .supergraph_schema(Arc::new(schema.supergraph_schema().clone())) |
There was a problem hiding this comment.
why are we creating a new Arc here? the supergraph_schema method returns a &Valid<FederationSchema> but the struct we get it from already has an Arc:
pub struct ValidFederationSchema {
schema: Arc<Valid<FederationSchema>>,
}
There was a problem hiding this comment.
Arc<Valid<FederationSchema>> contains a apollo_compiler::Schema together with other data, so get an Arc<Valid<apollo_compiler::Schema>> we need a new Arc.
I previously tried to change FederationSchema to contain an Arc, but that clashes with code that mutates schemas a lot (namely extract subgraph schemas from supergraph, and eventually composition). Maybe we apollo-federation should have separate types for mutable v.s. immutable schemas, but that’s larger change.
| pub(crate) fn build( | ||
| configuration: &Configuration, | ||
| schema: &apollo_compiler::ast::Document, | ||
| schema: &Schema, |
| for subgraph_url in schema | ||
| .definitions | ||
| .iter() | ||
| .filter_map(|def| def.as_enum_type_definition()) | ||
| .filter(|def| def.name == "join__Graph") | ||
| .flat_map(|def| def.values.iter()) | ||
| .flat_map(|val| val.directives.iter()) | ||
| .filter(|d| d.name == "join__graph") | ||
| .filter_map(|dir| (dir.arguments.iter().find(|arg| arg.name == "url"))) | ||
| .filter_map(|arg| arg.value.as_str()) |
There was a problem hiding this comment.
that code can finally be deleted :)
(BTW could youmerge after the connectors PR? This part will conflict with it, because there's some connectors specific code in license enforcement)
There was a problem hiding this comment.
Auto-merge happened before I saw this, sorry! Is connectors merging very soon? If this PR is an obstacle it could be reverted long enough to merge connectors and I’ll manage the conflict later.
There was a problem hiding this comment.
This PR was merged to dev but not forward ported to next, right?
| } | ||
|
|
||
|
|
||
| directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE |
There was a problem hiding this comment.
is that (and other schema changes) to convert to fed 2?
There was a problem hiding this comment.
No, previously some of these files had just enough Federation boilerplate to be able to parse as AST and go through license_enforcement.rs. Now they have enough to validate as well, but fed 1 vs fed 2 is unchanged.
| .directives | ||
| .get_all(LINK_DIRECTIVE_NAME) | ||
| .filter_map(|link| { | ||
| ParsedLinkSpec::from_link_directive(link).map(|maybe_spec| { |
There was a problem hiding this comment.
Should this be replaced with something from apollo-federation? (Through schema.federation_supergraph().schema.metadata())
3 participants
As discussed in #5623 (comment)
This may may startup and reload slightly faster for large schemas, but more importantly it makes
spec::Schemaavailable in more places which will help upcoming changes for Rust replatforming.Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩