Skip to content

Commit

Permalink
handle mutations containing @defer (#2102)
Browse files Browse the repository at this point in the history
Fix #2099

to validate the primary part of a request with `@defer`, we reconstruct
a partial query by walking through the query plan. The code responsible
for that was not detecting mutations.

while the PR in the current state fixes the issue, I'd like to refactor
a bit so that we do not special case query and mutation (which will be
annoying if we add subscription at some point), and moving the query
reconstruction elsewhere, to work directly on the query instead of the
query plan
  • Loading branch information
Geoffroy Couprie authored and garypen committed Nov 30, 2022
1 parent 3a716f9 commit 39b482a
Show file tree
Hide file tree
Showing 4 changed files with 147 additions and 9 deletions.
7 changes: 7 additions & 0 deletions NEXT_CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,13 @@ The error response will now contain the status code and status name. Example: `H

By [@col](https://github.com/col) in https://github.com/apollographql/router/pull/2118

### handle mutations containing @defer ([Issue #2099](https://github.com/apollographql/router/issues/2099))

The Router generates partial query shapes corresponding to the primary and deferred responses,
to validate the data sent back to the client. Those query shapes were invalid for mutations.

By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/2102

## 🛠 Maintenance
## 📚 Documentation

Expand Down
36 changes: 27 additions & 9 deletions apollo-router/src/query_planner/plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,18 @@ impl PlanNode {
// re-create full query with the right path
// parse the subselection
let mut subselections = HashMap::new();
self.collect_subselections(schema, &Path::default(), &mut subselections)?;
let operation_kind = if self.contains_mutations() {
OperationKind::Mutation
} else {
OperationKind::Query
};

self.collect_subselections(
schema,
&Path::default(),
&operation_kind,
&mut subselections,
)?;

Ok(subselections)
}
Expand All @@ -203,27 +214,28 @@ impl PlanNode {
&self,
schema: &Schema,
initial_path: &Path,
kind: &OperationKind,
subselections: &mut HashMap<SubSelection, Query>,
) -> Result<(), QueryPlannerError> {
// re-create full query with the right path
// parse the subselection
match self {
Self::Sequence { nodes } | Self::Parallel { nodes } => {
nodes.iter().try_fold(subselections, |subs, current| {
current.collect_subselections(schema, initial_path, subs)?;
current.collect_subselections(schema, initial_path, kind, subs)?;

Ok::<_, QueryPlannerError>(subs)
})?;
Ok(())
}
Self::Flatten(node) => {
node.node
.collect_subselections(schema, initial_path, subselections)
.collect_subselections(schema, initial_path, kind, subselections)
}
Self::Defer { primary, deferred } => {
let primary_path = initial_path.join(&primary.path.clone().unwrap_or_default());
if let Some(primary_subselection) = &primary.subselection {
let query = reconstruct_full_query(&primary_path, primary_subselection);
let query = reconstruct_full_query(&primary_path, kind, primary_subselection);
// ----------------------- Parse ---------------------------------
let sub_selection = Query::parse(&query, schema, &Default::default())?;
// ----------------------- END Parse ---------------------------------
Expand All @@ -239,7 +251,7 @@ impl PlanNode {

deferred.iter().try_fold(subselections, |subs, current| {
if let Some(subselection) = &current.subselection {
let query = reconstruct_full_query(&current.path, subselection);
let query = reconstruct_full_query(&current.path, kind, subselection);
// ----------------------- Parse ---------------------------------
let sub_selection = Query::parse(&query, schema, &Default::default())?;
// ----------------------- END Parse ---------------------------------
Expand All @@ -256,6 +268,7 @@ impl PlanNode {
current_node.collect_subselections(
schema,
&initial_path.join(&current.path),
kind,
subs,
)?;
}
Expand All @@ -271,10 +284,10 @@ impl PlanNode {
..
} => {
if let Some(node) = if_clause {
node.collect_subselections(schema, initial_path, subselections)?;
node.collect_subselections(schema, initial_path, kind, subselections)?;
}
if let Some(node) = else_clause {
node.collect_subselections(schema, initial_path, subselections)?;
node.collect_subselections(schema, initial_path, kind, subselections)?;
}
Ok(())
}
Expand Down Expand Up @@ -324,9 +337,14 @@ impl PlanNode {
}
}

fn reconstruct_full_query(path: &Path, subselection: &str) -> String {
let mut query = String::new();
fn reconstruct_full_query(path: &Path, kind: &OperationKind, subselection: &str) -> String {
let mut len = 0;
let mut query = match kind {
OperationKind::Query => "query",
OperationKind::Mutation => "mutation",
OperationKind::Subscription => "subscription",
}
.to_string();
for path_elt in path.iter() {
match path_elt {
json_ext::PathElement::Flatten | json_ext::PathElement::Index(_) => {}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
---
source: apollo-router/src/services/supergraph_service.rs
expression: stream.next_response().await.unwrap()
---
{
"errors": [
{
"message": "invalid type for variable: 'userId'",
"extensions": {
"type": "ValidationInvalidTypeVariable",
"name": "userId"
}
}
]
}
98 changes: 98 additions & 0 deletions apollo-router/src/services/supergraph_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -710,4 +710,102 @@ mod tests {

insta::assert_json_snapshot!(stream.next_response().await.unwrap());
}

#[tokio::test]
async fn query_reconstruction() {
let schema = r#"schema
@link(url: "https://specs.apollo.dev/link/v1.0")
@link(url: "https://specs.apollo.dev/join/v0.2", for: EXECUTION)
@link(url: "https://specs.apollo.dev/tag/v0.2")
@link(url: "https://specs.apollo.dev/inaccessible/v0.2", for: SECURITY)
{
query: Query
mutation: Mutation
}
directive @inaccessible on FIELD_DEFINITION | OBJECT | INTERFACE | UNION | ARGUMENT_DEFINITION | SCALAR | ENUM | ENUM_VALUE | INPUT_OBJECT | INPUT_FIELD_DEFINITION
directive @join__field(graph: join__Graph!, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
directive @join__graph(name: String!, url: String!) on ENUM_VALUE
directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE
directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR
directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA
directive @tag(name: String!) repeatable on FIELD_DEFINITION | OBJECT | INTERFACE | UNION | ARGUMENT_DEFINITION | SCALAR | ENUM | ENUM_VALUE | INPUT_OBJECT | INPUT_FIELD_DEFINITION
scalar join__FieldSet
enum join__Graph {
PRODUCTS @join__graph(name: "products", url: "http://products:4000/graphql")
USERS @join__graph(name: "users", url: "http://users:4000/graphql")
}
scalar link__Import
enum link__Purpose {
SECURITY
EXECUTION
}
type MakePaymentResult
@join__type(graph: USERS)
{
id: ID!
paymentStatus: PaymentStatus
}
type Mutation
@join__type(graph: USERS)
{
makePayment(userId: ID!): MakePaymentResult!
}
type PaymentStatus
@join__type(graph: USERS)
{
id: ID!
}
type Query
@join__type(graph: PRODUCTS)
@join__type(graph: USERS)
{
name: String
}
"#;

let service = TestHarness::builder()
.configuration_json(serde_json::json!({"include_subgraph_errors": { "all": true } }))
.unwrap()
.schema(schema)
.build()
.await
.unwrap();

let request = supergraph::Request::fake_builder()
.header("Accept", "multipart/mixed; deferSpec=20220824")
.query(
r#"mutation ($userId: ID!) {
makePayment(userId: $userId) {
id
... @defer {
paymentStatus {
id
}
}
}
}"#,
)
.build()
.unwrap();

let mut stream = service.oneshot(request).await.unwrap();

insta::assert_json_snapshot!(stream.next_response().await.unwrap());
}
}

0 comments on commit 39b482a

Please sign in to comment.