Skip to content

Commit

Permalink
Move some GraphQL analysis earlier in the pipeline (#6128)
Browse files Browse the repository at this point in the history 
The following is now done in `QueryAnalysisLayer` instead of in the `BridgeQueryPlanner` service:

* Finding the relevant operation in a GraphQL document based on `operationName`. Now `ParsedDocument` in context contains a reference to the parsed `apollo_compiler::executable::Operation`.
* Scan that operation’s root fields to find which are present: root `__typename`, schema intropsection, and "normal" non-meta fields. This is a preliminary step to moving introspection out of `BridgeQueryPlanner`.

As a side-effect observable to clients, the exact response for some error cases like an empty document changes slightly as the error no longer comes from JavaScript code.

Also drive-by refactor: change `spec::Query` to contain a single `Operation` instead of a `Vec<Operation>`. This removes conditionals and the need to pass `operationName` around in a lot of places, notably in execution code.
  • Loading branch information
@SimonSapin
SimonSapin authored Oct 10, 2024
1 parent bf61558 commit 644d903
Show file tree
Hide file tree
Showing 18 changed files with 296 additions and 388 deletions.
4 changes: 2 additions & 2 deletions Cargo.lock
View file
Original file line number Diff line number Diff line change
Expand Up @@ -218,9 +218,9 @@ dependencies = [

[[package]]
name = "apollo-parser"
version = "0.8.2"
version = "0.8.3"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "9692c1bfa7e0628e5c46bd0538571dd469138a0f062290fd4bf90e20e46f05da"
checksum = "b64257011a999f2e22275cf7a118f651e58dc9170e11b775d435de768fad0387"
dependencies = [
"memchr",
"rowan",
Expand Down
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ debug = 1
# https://doc.rust-lang.org/cargo/reference/workspaces.html#the-dependencies-table
[workspace.dependencies]
apollo-compiler = "=1.0.0-beta.24"
apollo-parser = "0.8.0"
apollo-parser = "0.8.3"
apollo-smith = "0.14.0"
async-trait = "0.1.77"
hex = { version = "0.4.3", features = ["serde"] }
Expand Down
17 changes: 7 additions & 10 deletions apollo-router/src/apollo_studio_interop/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,20 +223,17 @@ pub(crate) fn generate_extended_references(

pub(crate) fn extract_enums_from_response(
query: Arc<Query>,
operation_name: Option<&str>,
schema: &Valid<Schema>,
response_body: &Object,
existing_refs: &mut ReferencedEnums,
) {
if let Some(operation) = query.operation(operation_name) {
extract_enums_from_selection_set(
&operation.selection_set,
&query.fragments,
schema,
response_body,
existing_refs,
);
}
extract_enums_from_selection_set(
&query.operation.selection_set,
&query.fragments,
schema,
response_body,
existing_refs,
);
}

fn add_enum_value_to_map(
Expand Down
1 change: 0 additions & 1 deletion apollo-router/src/apollo_studio_interop/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,6 @@ fn enums_from_response(
let mut result = ReferencedEnums::new();
extract_enums_from_response(
Arc::new(query),
operation_name,
schema.supergraph_schema(),
&response_body,
&mut result,
Expand Down
14 changes: 7 additions & 7 deletions apollo-router/src/plugins/demand_control/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,7 @@ mod test {
use apollo_compiler::ast;
use apollo_compiler::validation::Valid;
use apollo_compiler::ExecutableDocument;
use apollo_compiler::Schema;
use futures::StreamExt;
use schemars::JsonSchema;
use serde::Deserialize;
Expand All @@ -482,7 +483,6 @@ mod test {
use crate::plugins::demand_control::DemandControlContext;
use crate::plugins::demand_control::DemandControlError;
use crate::plugins::test::PluginTestHarness;
use crate::query_planner::fetch::QueryHash;
use crate::services::execution;
use crate::services::layers::query_analysis::ParsedDocument;
use crate::services::layers::query_analysis::ParsedDocumentInner;
Expand Down Expand Up @@ -660,14 +660,14 @@ mod test {
}

fn context() -> Context {
let parsed_document = ParsedDocumentInner {
executable: Arc::new(Valid::assume_valid(ExecutableDocument::new())),
hash: Arc::new(QueryHash::default()),
ast: ast::Document::new(),
};
let schema = Schema::parse_and_validate("type Query { f: Int }", "").unwrap();
let ast = ast::Document::parse("{__typename}", "").unwrap();
let doc = ast.to_executable_validate(&schema).unwrap();
let parsed_document =
ParsedDocumentInner::new(ast, doc.into(), None, Default::default()).unwrap();
let ctx = Context::new();
ctx.extensions()
.with_lock(|mut lock| lock.insert(ParsedDocument::new(parsed_document)));
.with_lock(|mut lock| lock.insert::<ParsedDocument>(parsed_document));
ctx
}

Expand Down
35 changes: 13 additions & 22 deletions apollo-router/src/plugins/telemetry/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -698,30 +698,21 @@ impl Plugin for Telemetry {
fn execution_service(&self, service: execution::BoxService) -> execution::BoxService {
ServiceBuilder::new()
.instrument(move |req: &ExecutionRequest| {
let operation_kind = req
.query_plan
.query
.operation(req.supergraph_request.body().operation_name.as_deref())
.map(|op| *op.kind());
let operation_kind = req.query_plan.query.operation.kind();

match operation_kind {
Some(operation_kind) => match operation_kind {
OperationKind::Subscription => info_span!(
EXECUTION_SPAN_NAME,
"otel.kind" = "INTERNAL",
"graphql.operation.type" = operation_kind.as_apollo_operation_type(),
"apollo_private.operation.subtype" =
OperationSubType::SubscriptionRequest.as_str(),
),
_ => info_span!(
EXECUTION_SPAN_NAME,
"otel.kind" = "INTERNAL",
"graphql.operation.type" = operation_kind.as_apollo_operation_type(),
),
},
None => {
info_span!(EXECUTION_SPAN_NAME, "otel.kind" = "INTERNAL",)
}
OperationKind::Subscription => info_span!(
EXECUTION_SPAN_NAME,
"otel.kind" = "INTERNAL",
"graphql.operation.type" = operation_kind.as_apollo_operation_type(),
"apollo_private.operation.subtype" =
OperationSubType::SubscriptionRequest.as_str(),
),
_ => info_span!(
EXECUTION_SPAN_NAME,
"otel.kind" = "INTERNAL",
"graphql.operation.type" = operation_kind.as_apollo_operation_type(),
),
}
})
.service(service)
Expand Down
144 changes: 58 additions & 86 deletions apollo-router/src/query_planner/bridge_query_planner.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -508,19 +508,19 @@ impl BridgeQueryPlanner {
operation_name,
)?;

let (fragments, operations, defer_stats, schema_aware_hash) =
let (fragments, operation, defer_stats, schema_aware_hash) =
Query::extract_query_information(&self.schema, executable, operation_name)?;

let subselections = crate::spec::query::subselections::collect_subselections(
&self.configuration,
&operations,
&operation,
&fragments.map,
&defer_stats,
)?;
Ok(Query {
string: query,
fragments,
operations,
operation,
filtered_query: None,
unauthorized: UnauthorizedPaths {
paths: vec![],
Expand All @@ -543,11 +543,9 @@ impl BridgeQueryPlanner {
IntrospectionMode::Rust => {
let schema = self.schema.clone();
let response = Box::new(
tokio::task::spawn_blocking(move || {
Self::rust_introspection(&schema, &key, &doc)
})
.await
.expect("Introspection panicked")?,
tokio::task::spawn_blocking(move || Self::rust_introspection(&schema, &doc))
.await
.expect("Introspection panicked")?,
);
return Ok(QueryPlannerContent::Response { response });
}
Expand Down Expand Up @@ -582,7 +580,7 @@ impl BridgeQueryPlanner {
let schema = self.schema.clone();
let js_result_clone = js_result.clone();
tokio::task::spawn_blocking(move || {
let rust_result = match Self::rust_introspection(&schema, &key, &doc) {
let rust_result = match Self::rust_introspection(&schema, &doc) {
Ok(response) => {
if response.errors.is_empty() {
Ok(response)
Expand Down Expand Up @@ -619,11 +617,10 @@ impl BridgeQueryPlanner {

fn rust_introspection(
schema: &Schema,
key: &QueryKey,
doc: &ParsedDocument,
) -> Result<graphql::Response, QueryPlannerError> {
let schema = schema.api_schema();
let operation = doc.get_operation(key.operation_name.as_deref())?;
let operation = &doc.operation;
let variable_values = Default::default();
let variable_values =
apollo_compiler::execution::coerce_variable_values(schema, operation, &variable_values)
Expand Down Expand Up @@ -798,11 +795,12 @@ impl Service<QueryPlannerRequest> for BridgeQueryPlanner {
operation_name.as_deref(),
)
.map_err(|e| SpecError::QueryHashing(e.to_string()))?;
doc = Arc::new(ParsedDocumentInner {
executable: Arc::new(executable_document),
ast: modified_query,
hash: Arc::new(QueryHash(hash)),
});
doc = ParsedDocumentInner::new(
modified_query,
Arc::new(executable_document),
operation_name.as_deref(),
Arc::new(QueryHash(hash)),
)?;
context
.extensions()
.with_lock(|mut lock| lock.insert::<ParsedDocument>(doc.clone()));
Expand Down Expand Up @@ -880,10 +878,7 @@ impl BridgeQueryPlanner {
)
.await?;

if selections
.operation(key.operation_name.as_deref())
.is_some_and(|op| op.selection_set.is_empty())
{
if selections.operation.selection_set.is_empty() {
// All selections have @skip(true) or @include(false)
// Return an empty response now to avoid dealing with an empty query plan later
return Ok(QueryPlannerContent::Response {
Expand All @@ -895,70 +890,46 @@ impl BridgeQueryPlanner {
});
}

{
let operation = doc
.executable
.operations
.get(key.operation_name.as_deref())
.ok();
let mut has_root_typename = false;
let mut has_schema_introspection = false;
let mut has_other_root_fields = false;
if let Some(operation) = operation {
for field in operation.root_fields(&doc.executable) {
match field.name.as_str() {
"__typename" => has_root_typename = true,
"__schema" | "__type" if operation.is_query() => {
has_schema_introspection = true
}
_ => has_other_root_fields = true,
}
}
if has_root_typename && !has_schema_introspection && !has_other_root_fields {
// Fast path for __typename alone
if operation
.selection_set
.selections
.iter()
.all(|sel| sel.as_field().is_some_and(|f| f.name == "__typename"))
{
let root_type_name: serde_json_bytes::ByteString =
operation.object_type().as_str().into();
let data = Value::Object(
operation
.root_fields(&doc.executable)
.filter(|field| field.name == "__typename")
.map(|field| {
(
field.response_key().as_str().into(),
Value::String(root_type_name.clone()),
)
})
.collect(),
);
return Ok(QueryPlannerContent::Response {
response: Box::new(graphql::Response::builder().data(data).build()),
});
} else {
// fragments might use @include or @skip
}
}
if doc.has_root_typename && !doc.has_schema_introspection && !doc.has_explicit_root_fields {
// Fast path for __typename alone
if doc.operation.selection_set.selections.iter().all(|sel| {
sel.as_field()
.is_some_and(|f| f.name == "__typename" && f.directives.is_empty())
}) {
let root_type_name: serde_json_bytes::ByteString =
doc.operation.object_type().as_str().into();
let data = Value::Object(
doc.operation
.root_fields(&doc.executable)
.filter(|field| field.name == "__typename")
.map(|field| {
(
field.response_key().as_str().into(),
Value::String(root_type_name.clone()),
)
})
.collect(),
);
return Ok(QueryPlannerContent::Response {
response: Box::new(graphql::Response::builder().data(data).build()),
});
} else {
// Should be unreachable as QueryAnalysisLayer would have returned an error
// We have fragments which might use @skip or @include,
// or field directives which might be @skip or @include.
}
}

if has_schema_introspection {
if has_other_root_fields {
let error = graphql::Error::builder()
if doc.has_schema_introspection {
if doc.has_explicit_root_fields {
let error = graphql::Error::builder()
.message("Mixed queries with both schema introspection and concrete fields are not supported")
.extension_code("MIXED_INTROSPECTION")
.build();
return Ok(QueryPlannerContent::Response {
response: Box::new(graphql::Response::builder().error(error).build()),
});
}
return self.introspection(key, doc).await;
return Ok(QueryPlannerContent::Response {
response: Box::new(graphql::Response::builder().error(error).build()),
});
}
return self.introspection(key, doc).await;
}

let filter_res = if self.enable_authorization_directives {
Expand Down Expand Up @@ -1001,11 +972,12 @@ impl BridgeQueryPlanner {
key.operation_name.as_deref(),
)
.map_err(|e| SpecError::QueryHashing(e.to_string()))?;
doc = Arc::new(ParsedDocumentInner {
executable: Arc::new(executable_document),
ast: new_doc,
hash: Arc::new(QueryHash(hash)),
});
doc = ParsedDocumentInner::new(
new_doc,
Arc::new(executable_document),
key.operation_name.as_deref(),
Arc::new(QueryHash(hash)),
)?;
selections.unauthorized.paths = unauthorized_paths;
}

Expand Down Expand Up @@ -1268,10 +1240,11 @@ mod tests {

#[test(tokio::test)]
async fn test_plan_error() {
let result = plan(EXAMPLE_SCHEMA, "", "", None, PlanOptions::default()).await;
let query = "";
let result = plan(EXAMPLE_SCHEMA, query, query, None, PlanOptions::default()).await;

assert_eq!(
"couldn't plan query: query validation errors: Syntax Error: Unexpected <EOF>.",
"spec error: parsing error: syntax error: Unexpected <EOF>.",
result.unwrap_err().to_string()
);
}
Expand Down Expand Up @@ -1654,8 +1627,7 @@ mod tests {
operation_name.as_deref(),
&planner.schema(),
&configuration,
)
.unwrap();
)?;

planner
.get(
Expand Down
4 changes: 2 additions & 2 deletions apollo-router/src/query_planner/caching_query_planner.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ where
} in all_cache_keys
{
let context = Context::new();
let (doc, _operation_def) = match query_analysis
let doc = match query_analysis
.parse_document(&query, operation_name.as_deref())
.await
{
Expand Down Expand Up @@ -324,7 +324,7 @@ where
})
.await;
if entry.is_first() {
let (doc, _operation_def) = match query_analysis
let doc = match query_analysis
.parse_document(&query, operation_name.as_deref())
.await
{
Expand Down
Loading

0 comments on commit 644d903

Please sign in to comment.