This repository has been archived by the owner on May 3, 2024. It is now read-only.
Query Planner constructor and some strictness improvements #175
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For QP work, the API schema result needs to be a `FederationSchema`. We also know that the API schema result is valid, so we make it a `ValidFederationSchema`. API schema must also only be called on valid schemas so the input is changed to a `ValidFederationSchema`. Supergraphs are also federation schemas so this changes the constructors to build a `ValidFederationSchema` early. `ValidFederationSchema` contains an `Arc` while `FederationSchema` doesn't, and that is relied upon in a few places. I add some clones of the inner schema to convert from an immutable `ValidFederationSchema` to a mutable `FederationSchema`, it maybe could be done more efficiently, but it's OK.
…ible` if the spec exists in
The actual implementation of subgraph extraction is in the `query_graph` module.
goto-bus-stop
requested review from
dariuszkuc and
sachindshinde
as code owners
|
Most of the existing tests on the JS side are checking behaviours in Fed 1 supergraphs. I don't think I can port those over yet as Fed 1 supergraphs are out of scope. I will add some notes/questions to this PR about the differences between JS and Rust data structures |
goto-bus-stop
commented
Feb 20, 2024
goto-bus-stop
commented
Feb 20, 2024
| paths_limit: None, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub struct QueryPlanner { | ||
| config: Arc<QueryPlannerConfig>, | ||
| // TODO(@goto-bus-stop) not convinced that these arcs are all necessary | ||
| config: QueryPlannerConfig, | ||
| federated_query_graph: Arc<QueryGraph>, | ||
| supergraph_schema: ValidFederationSchema, | ||
| api_schema: ValidFederationSchema, | ||
| subgraph_federation_spec_definitions: Arc<IndexMap<NodeStr, &'static FederationSpecDefinition>>, |
There was a problem hiding this comment.
I didn't find this in the JS implementation. I do understand what it needs to do because of the type. But how is it going to be used?
SimonSapin
reviewed
Feb 20, 2024
src/lib.rs
Outdated
|
|
||
| pub struct Supergraph { | ||
| pub schema: Valid<Schema>, | ||
| pub schema: ValidFederationSchema, | ||
| } | ||
|
|
||
| impl Supergraph { | ||
| pub fn new(schema_str: &str) -> Result<Self, FederationError> { | ||
| let schema = Schema::parse_and_validate(schema_str, "schema.graphql")?; | ||
| // TODO: federation-specific validation |
There was a problem hiding this comment.
Is this TODO still relevant?
SimonSapin
reviewed
Feb 20, 2024
src/lib.rs
Outdated
| @@ -141,7 +151,7 @@ mod tests { | |||
| "#; | |||
|
|
|||
| let supergraph = Supergraph::new(schema).unwrap(); | |||
| let _subgraphs = database::extract_subgraphs(&supergraph) | |||
| let _subgraphs = crate::query_graph::extract_subgraphs_from_supergraph::extract_subgraphs_from_supergraph(&supergraph.schema, None) | |||
| .expect("Should have been able to extract subgraphs"); | |||
| // TODO: actual assertions on the subgraph once it's actually implemented. | |||
There was a problem hiding this comment.
Is it time to add assertions there? Does JS have corresponding tests?
There was a problem hiding this comment.
AFAICT this test is not directly in JS, but we could add some that just assert the whole subgraph shape is as expected here.
goto-bus-stop
force-pushed
the
renee/qp-constructor
branch
from
3dc0cb0
to
313f0d2
Compare
I think this must only compare the fields that are selected, not the parent type and the schema it's a part of. That's because the selections are parsed in the context of a subgraph schema, so writing the same `@key()` in subgraph A and B results in selection sets that don't compare equal, but their actual selections do. And that is what key resolution cares about.
SimonSapin
approved these changes
Feb 28, 2024
goto-bus-stop
changed the title
SimonSapin
pushed a commit
to apollographql/router
that referenced
this pull request
May 3, 2024
…phql/federation-next#175) This implements the constructor `QueryPlanner::new()`, which builds query graphs and does some minor precomputation, and: - fixes a few issues in subgraph extraction, were easy to miss since that code was not used before. also removed an old module that is obsolete. - introduce some use of `ValidFederationSchema` vs. `FederationSchema` in the API schema implementation, so there is less potential for misuse (this requires more cloning than i'd like--i'm not convinced that ValidFederationSchema being an Arc internally is the best thing to do, but understand the usefulness of its `==` implementation!) - move `@inaccessible` related functions onto the spec definition struct: this means you have to look up the inaccessible spec before using it, manually or from the schema, so there's less potential for misuse Closes FED-22 Closes FED-82
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This implements the constructor
QueryPlanner::new(), which builds query graphs and does some minor precomputation, and:ValidFederationSchemavs.FederationSchemain the API schema implementation, so there is less potential for misuse (this requires more cloning than i'd like--i'm not convinced that ValidFederationSchema being an Arc internally is the best thing to do, but understand the usefulness of its==implementation!)@inaccessiblerelated functions onto the spec definition struct: this means you have to look up the inaccessible spec before using it, manually or from the schema, so there's less potential for misuseCloses FED-22
Closes FED-82