Skip to content

Commit

Permalink
fix(compiler): cache the implementers map per operation
Browse files Browse the repository at this point in the history
Fixes #862.

I had proposed a different approach that would let us cache the
implementers map for as long as the schema is immutable, so it could be
reused for different query validations. That approach had an issue that
made `Valid::assume_valid_ref` very, very subtle to use, so we are not
doing that right now.

This is a less ideal version but it does solve the immediate problem.
We already pass around an `OperationValidationConfig` structure and
this seems like a nice, non-invasive place to put the implementers
cache.
  • Loading branch information
goto-bus-stop committed Jun 5, 2024
1 parent ae464bf commit a2b0ca9
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 27 deletions.
5 changes: 1 addition & 4 deletions crates/apollo-compiler/src/executable/validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,6 @@ pub(crate) fn validate_field_set(
document,
Some((schema, &field_set.selection_set.ty)),
&field_set.selection_set,
crate::validation::operation::OperationValidationConfig {
schema: Some(schema),
variables: &[],
},
&crate::validation::operation::OperationValidationConfig::new(Some(schema), &[]),
)
}
2 changes: 1 addition & 1 deletion crates/apollo-compiler/src/validation/field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ pub(crate) fn validate_field(
// May be None if a parent selection was invalid
against_type: Option<(&crate::Schema, &ast::NamedType)>,
field: &Node<executable::Field>,
context: OperationValidationConfig<'_>,
context: &OperationValidationConfig<'_>,
) {
// First do all the validation that we can without knowing the type of the field.

Expand Down
11 changes: 7 additions & 4 deletions crates/apollo-compiler/src/validation/fragment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ fn validate_fragment_spread_type(
against_type: &NamedType,
type_condition: &NamedType,
selection: &executable::Selection,
context: &OperationValidationConfig<'_>,
) {
// Another diagnostic will be raised if the type condition was wrong.
// We reduce noise by silencing other issues with the fragment.
Expand All @@ -64,7 +65,7 @@ fn validate_fragment_spread_type(
return;
};

let implementers_map = schema.implementers_map();
let implementers_map = context.implementers_map();
let concrete_parent_types = get_possible_types(against_type_definition, &implementers_map);
let concrete_condition_types = get_possible_types(type_condition_definition, &implementers_map);

Expand Down Expand Up @@ -107,7 +108,7 @@ pub(crate) fn validate_inline_fragment(
document: &ExecutableDocument,
against_type: Option<(&crate::Schema, &ast::NamedType)>,
inline: &Node<executable::InlineFragment>,
context: OperationValidationConfig<'_>,
context: &OperationValidationConfig<'_>,
) {
super::directive::validate_directives(
diagnostics,
Expand Down Expand Up @@ -138,6 +139,7 @@ pub(crate) fn validate_inline_fragment(
against_type,
type_condition,
&executable::Selection::InlineFragment(inline.clone()),
&context,
);
}
super::selection::validate_selection_set(
Expand All @@ -159,7 +161,7 @@ pub(crate) fn validate_fragment_spread(
document: &ExecutableDocument,
against_type: Option<(&crate::Schema, &NamedType)>,
spread: &Node<executable::FragmentSpread>,
context: OperationValidationConfig<'_>,
context: &OperationValidationConfig<'_>,
) {
super::directive::validate_directives(
diagnostics,
Expand All @@ -179,6 +181,7 @@ pub(crate) fn validate_fragment_spread(
against_type,
def.type_condition(),
&executable::Selection::FragmentSpread(spread.clone()),
context,
);
}
validate_fragment_definition(diagnostics, document, def, context);
Expand All @@ -198,7 +201,7 @@ pub(crate) fn validate_fragment_definition(
diagnostics: &mut DiagnosticList,
document: &ExecutableDocument,
fragment: &Node<executable::Fragment>,
context: OperationValidationConfig<'_>,
context: &OperationValidationConfig<'_>,
) {
super::directive::validate_directives(
diagnostics,
Expand Down
42 changes: 34 additions & 8 deletions crates/apollo-compiler/src/validation/operation.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,41 @@
use std::collections::HashMap;
use std::sync::OnceLock;

use crate::ast;
use crate::ast::Name;
use crate::executable;
use crate::schema::Implementers;
use crate::validation::DiagnosticList;
use crate::{ast, executable, ExecutableDocument, Node, Schema};
use crate::ExecutableDocument;
use crate::Node;
use crate::Schema;

#[derive(Debug, Clone)]
#[derive(Debug)]
pub(crate) struct OperationValidationConfig<'a> {
/// When None, rules that require a schema to validate are disabled.
pub schema: Option<&'a crate::Schema>,
pub schema: Option<&'a Schema>,
/// The variables defined for this operation.
pub variables: &'a [Node<ast::VariableDefinition>],
implementers_map: OnceLock<HashMap<Name, Implementers>>,
}

impl<'a> OperationValidationConfig<'a> {
pub fn new(schema: Option<&'a Schema>, variables: &'a [Node<ast::VariableDefinition>]) -> Self {
Self {
schema,
variables,
implementers_map: Default::default(),
}
}

/// Returns a cached reference to the implementers map.
pub fn implementers_map(&self) -> &HashMap<Name, Implementers> {
self.implementers_map.get_or_init(|| {
self.schema
.map(|schema| schema.implementers_map())
.unwrap_or_default()
})
}
}

pub(crate) fn validate_subscription(
Expand Down Expand Up @@ -60,10 +89,7 @@ pub(crate) fn validate_operation(
document: &ExecutableDocument,
operation: &executable::Operation,
) {
let config = OperationValidationConfig {
schema,
variables: &operation.variables,
};
let config = OperationValidationConfig::new(schema, &operation.variables);

let against_type = if let Some(schema) = schema {
schema
Expand Down Expand Up @@ -92,7 +118,7 @@ pub(crate) fn validate_operation(
document,
against_type,
&operation.selection_set,
config,
&config,
);
}

Expand Down
16 changes: 6 additions & 10 deletions crates/apollo-compiler/src/validation/selection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -545,24 +545,20 @@ pub(crate) fn validate_selection_set(
document: &ExecutableDocument,
against_type: Option<(&crate::Schema, &NamedType)>,
selection_set: &SelectionSet,
context: OperationValidationConfig<'_>,
context: &OperationValidationConfig<'_>,
) {
for selection in &selection_set.selections {
match selection {
executable::Selection::Field(field) => super::field::validate_field(
diagnostics,
document,
against_type,
field,
context.clone(),
),
executable::Selection::Field(field) => {
super::field::validate_field(diagnostics, document, against_type, field, context)
}
executable::Selection::FragmentSpread(fragment) => {
super::fragment::validate_fragment_spread(
diagnostics,
document,
against_type,
fragment,
context.clone(),
context,
)
}
executable::Selection::InlineFragment(inline) => {
Expand All @@ -571,7 +567,7 @@ pub(crate) fn validate_selection_set(
document,
against_type,
inline,
context.clone(),
context,
)
}
}
Expand Down

0 comments on commit a2b0ca9

Please sign in to comment.