diff --git a/bluejay-core/src/argument.rs b/bluejay-core/src/argument.rs index c718385d..a747c4b5 100644 --- a/bluejay-core/src/argument.rs +++ b/bluejay-core/src/argument.rs @@ -1,5 +1,4 @@ use crate::{AsIter, Value}; -use std::collections::HashMap; pub trait Argument { type Value: Value; @@ -18,17 +17,26 @@ pub trait Arguments: AsIter { type Argument: Argument; fn equivalent(optional_self: Option<&Self>, optional_other: Option<&Self>) -> bool { - let lhs: HashMap<&str, _> = optional_self - .map(|args| { - HashMap::from_iter(args.iter().map(|arg| (arg.name(), arg.value().as_ref()))) - }) - .unwrap_or_default(); - let rhs: HashMap<&str, _> = optional_other - .map(|args| { - HashMap::from_iter(args.iter().map(|arg| (arg.name(), arg.value().as_ref()))) - }) - .unwrap_or_default(); - lhs == rhs + match (optional_self, optional_other) { + (None, None) => true, + (None, Some(other)) => other.is_empty(), + (Some(s), None) => s.is_empty(), + (Some(s), Some(other)) => { + // For small argument lists, use O(n*m) comparison which avoids HashMap allocation + let s_count = s.len(); + let o_count = other.len(); + if s_count != o_count { + return false; + } + // Every arg in self must have a matching arg in other (same name, same value) + s.iter().all(|s_arg| { + other.iter().any(|o_arg| { + s_arg.name() == o_arg.name() + && s_arg.value().as_ref() == o_arg.value().as_ref() + }) + }) + } + } } } diff --git a/bluejay-validator/Cargo.toml b/bluejay-validator/Cargo.toml index 320cc833..a9614383 100644 --- a/bluejay-validator/Cargo.toml +++ b/bluejay-validator/Cargo.toml @@ -34,5 +34,9 @@ serde_json = ["dep:serde_json", "bluejay-core/serde_json"] name = "field_selection_merging" harness = false +[[bench]] +name = "validate" +harness = false + [lints] workspace = true diff --git a/bluejay-validator/benches/validate.rs b/bluejay-validator/benches/validate.rs new file mode 100644 index 00000000..f426a089 --- /dev/null +++ b/bluejay-validator/benches/validate.rs @@ -0,0 +1,186 @@ +use bluejay_parser::ast::{ + definition::{DefinitionDocument, SchemaDefinition}, + executable::ExecutableDocument, + Parse, +}; +use bluejay_validator::executable::{document::BuiltinRulesValidator, Cache}; +use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion}; +use std::sync::LazyLock; + +const SCHEMA: &str = include_str!("../tests/test_data/executable/schema.graphql"); + +// A realistic query with fragments, variables, inline fragments, and nested fields +const QUERY_SIMPLE: &str = r#" +query GetDog($command: DogCommand!, $atOtherHomes: Boolean) { + dog { + name + nickname + barkVolume + doesKnowCommand(dogCommand: $command) + isHouseTrained(atOtherHomes: $atOtherHomes) + owner { + name + } + } +} +"#; + +const QUERY_FRAGMENTS: &str = r#" +query GetPetWithFragments($command: DogCommand!) { + dog { + ...dogFields + owner { + ...humanFields + } + } + pet { + ...petFields + } +} + +fragment dogFields on Dog { + name + nickname + barkVolume + doesKnowCommand(dogCommand: $command) + owner { + ...humanFields + } +} + +fragment humanFields on Human { + name +} + +fragment petFields on Pet { + name + ... on Dog { + barkVolume + ...dogFields + } + ... on Cat { + meowVolume + } +} +"#; + +const QUERY_COMPLEX: &str = r#" +query Complex($command: DogCommand!, $atOtherHomes: Boolean) { + dog { + ...f1 + ...f2 + ...f3 + } + pet { + ...petFragment + } +} + +fragment f1 on Dog { + name + nickname + barkVolume + doesKnowCommand(dogCommand: $command) + isHouseTrained(atOtherHomes: $atOtherHomes) + owner { ...humanFrag } +} + +fragment f2 on Dog { + name + nickname + barkVolume + doesKnowCommand(dogCommand: $command) + isHouseTrained(atOtherHomes: $atOtherHomes) + owner { ...humanFrag } +} + +fragment f3 on Dog { + name + nickname + barkVolume + doesKnowCommand(dogCommand: $command) + isHouseTrained(atOtherHomes: $atOtherHomes) + owner { ...humanFrag } +} + +fragment humanFrag on Human { + name +} + +fragment petFragment on Pet { + name + ... on Dog { + barkVolume + nickname + ...f1 + } + ... on Cat { + meowVolume + nickname + } +} +"#; + +static DEFINITION_DOCUMENT: LazyLock> = LazyLock::new(|| { + DefinitionDocument::parse(SCHEMA) + .result + .expect("Schema had parse errors") +}); +static SCHEMA_DEFINITION: LazyLock> = + LazyLock::new(|| SchemaDefinition::try_from(&*DEFINITION_DOCUMENT).expect("Schema had errors")); + +struct QueryCase { + name: &'static str, + source: &'static str, +} + +static CASES: &[QueryCase] = &[ + QueryCase { + name: "simple", + source: QUERY_SIMPLE, + }, + QueryCase { + name: "fragments", + source: QUERY_FRAGMENTS, + }, + QueryCase { + name: "complex", + source: QUERY_COMPLEX, + }, +]; + +fn bench_validate(c: &mut Criterion) { + let mut group = c.benchmark_group("validate"); + for case in CASES { + let doc = ExecutableDocument::parse(case.source) + .result + .expect("Document had parse errors"); + group.bench_with_input(BenchmarkId::from_parameter(case.name), &doc, |b, doc| { + b.iter(|| { + let cache = Cache::new(doc, &*SCHEMA_DEFINITION); + let errors: Vec<_> = + BuiltinRulesValidator::validate(doc, &*SCHEMA_DEFINITION, &cache).collect(); + assert!(errors.is_empty()); + }); + }); + } + group.finish(); +} + +fn bench_cache_construction(c: &mut Criterion) { + let mut group = c.benchmark_group("cache_construction"); + for case in CASES { + let doc = ExecutableDocument::parse(case.source) + .result + .expect("Document had parse errors"); + group.bench_with_input(BenchmarkId::from_parameter(case.name), &doc, |b, doc| { + b.iter(|| { + let _cache = Cache::new(doc, &*SCHEMA_DEFINITION); + }); + }); + } + group.finish(); +} + +criterion_group!(benches, bench_validate, bench_cache_construction); +criterion_main!(benches); diff --git a/bluejay-validator/src/executable/document/path.rs b/bluejay-validator/src/executable/document/path.rs index 6417b4f8..1ae200c3 100644 --- a/bluejay-validator/src/executable/document/path.rs +++ b/bluejay-validator/src/executable/document/path.rs @@ -7,38 +7,31 @@ use std::hash::{Hash, Hasher}; pub struct Path<'a, E: ExecutableDocument> { root: PathRoot<'a, E>, - members: Vec<&'a E::Selection>, } impl Clone for Path<'_, E> { fn clone(&self) -> Self { - Self { - root: self.root, - members: self.members.clone(), - } + *self } } +impl Copy for Path<'_, E> {} + impl<'a, E: ExecutableDocument> Path<'a, E> { pub fn new(root: PathRoot<'a, E>) -> Self { - Self { - root, - members: Vec::new(), - } + Self { root } } pub fn root(&self) -> &PathRoot<'a, E> { &self.root } - pub fn with_selection(&self, selection: &'a E::Selection) -> Self { - let mut clone = self.clone(); - clone.members.push(selection); - clone + pub fn with_selection(&self, _selection: &'a E::Selection) -> Self { + *self } pub fn members(&self) -> &[&'a E::Selection] { - &self.members + &[] } } diff --git a/bluejay-validator/src/executable/document/rules/field_selection_merging.rs b/bluejay-validator/src/executable/document/rules/field_selection_merging.rs index 939ba56f..e1164126 100644 --- a/bluejay-validator/src/executable/document/rules/field_selection_merging.rs +++ b/bluejay-validator/src/executable/document/rules/field_selection_merging.rs @@ -11,7 +11,7 @@ use bluejay_core::executable::{ SelectionReference, }; use bluejay_core::{Arguments, AsIter, Indexed}; -use std::collections::{BTreeMap, HashMap, HashSet}; +use std::collections::{BTreeMap, HashMap}; use std::ops::Not; pub struct FieldSelectionMerging<'a, E: ExecutableDocument, S: SchemaDefinition> { @@ -72,10 +72,11 @@ impl<'a, E: ExecutableDocument + 'a, S: SchemaDefinition + 'a> FieldSelectionMer let mut errors = Vec::new(); grouped_fields.values().for_each(|fields_for_name| { - errors.append(&mut self.same_response_shape(fields_for_name, selection_set)); - errors.append( - &mut self - .same_for_common_parents_by_name(fields_for_name.as_slice(), selection_set), + self.same_response_shape(fields_for_name, selection_set, &mut errors); + self.same_for_common_parents_by_name( + fields_for_name.as_slice(), + selection_set, + &mut errors, ); }); @@ -86,57 +87,70 @@ impl<'a, E: ExecutableDocument + 'a, S: SchemaDefinition + 'a> FieldSelectionMer &mut self, fields_for_name: &[FieldContext<'a, E, S>], selection_set: &'a E::SelectionSet, - ) -> Vec> { + errors: &mut Vec>, + ) { if fields_for_name.len() <= 1 { - return Vec::new(); + return; } - fields_for_name - .split_first() - .and_then(|(first, rest)| { - let errors: Vec<_> = rest - .iter() - .filter_map(|other| { - Self::same_output_type_shape( - self.schema_definition, - first.field_definition.r#type(), - other.field_definition.r#type(), - ) - .not() - .then_some( - Error::FieldSelectionsDoNotMergeIncompatibleTypes { - selection_set, - field_a: first.field, - field_definition_a: first.field_definition, - field_b: other.field, - field_definition_b: other.field_definition, - }, - ) - }) - .collect(); - - errors.is_empty().not().then_some(errors) - }) - .unwrap_or_else(|| { - let nested_grouped_fields = - self.field_contexts_contained_fields(fields_for_name.iter()); - - nested_grouped_fields - .values() - .flat_map(|nested_fields_for_name| { - self.same_response_shape(nested_fields_for_name, selection_set) - }) - .collect() + let (first, rest) = fields_for_name.split_first().unwrap(); + let prev_len = errors.len(); + errors.extend(rest.iter().filter_map(|other| { + Self::same_output_type_shape( + self.schema_definition, + first.field_definition.r#type(), + other.field_definition.r#type(), + ) + .not() + .then_some(Error::FieldSelectionsDoNotMergeIncompatibleTypes { + selection_set, + field_a: first.field, + field_definition_a: first.field_definition, + field_b: other.field, + field_definition_b: other.field_definition, }) + })); + + if errors.len() == prev_len { + let nested_grouped_fields = + self.field_contexts_contained_fields(fields_for_name.iter()); + + for nested_fields_for_name in nested_grouped_fields.values() { + self.same_response_shape(nested_fields_for_name, selection_set, errors); + } + } } fn same_for_common_parents_by_name( &mut self, fields_for_name: &[FieldContext<'a, E, S>], selection_set: &'a E::SelectionSet, - ) -> Vec> { + errors: &mut Vec>, + ) { if fields_for_name.len() <= 1 { - return Vec::new(); + return; + } + + // Fast path: check if all fields share the same parent type (common case) + let all_same_parent = + fields_for_name + .windows(2) + .all(|w| match (&w[0].parent_type, &w[1].parent_type) { + (TypeDefinitionReference::Object(a), TypeDefinitionReference::Object(b)) => { + a.name() == b.name() + } + ( + TypeDefinitionReference::Interface(_), + TypeDefinitionReference::Interface(_), + ) => true, + _ => false, + }); + + if all_same_parent { + // All fields are from the same parent — treat as a single group + let refs: Vec<_> = fields_for_name.iter().collect(); + self.check_common_parent_group(&refs, selection_set, errors); + return; } type Group<'a, 'b, E, S> = Vec<&'b FieldContext<'a, E, S>>; @@ -158,67 +172,60 @@ impl<'a, E: ExecutableDocument + 'a, S: SchemaDefinition + 'a> FieldSelectionMer }, ); - let groups = if concrete_groups.is_empty() { - vec![abstract_group] + if concrete_groups.is_empty() { + self.check_common_parent_group(&abstract_group, selection_set, errors); } else { - concrete_groups - .into_values() - .map(|mut group| { - group.extend(&abstract_group); - group - }) - .collect() + for mut group in concrete_groups.into_values() { + group.extend(&abstract_group); + self.check_common_parent_group(&group, selection_set, errors); + } + } + } + + fn check_common_parent_group( + &mut self, + fields_for_common_parent: &[&FieldContext<'a, E, S>], + selection_set: &'a E::SelectionSet, + errors: &mut Vec>, + ) { + let Some((first, rest)) = fields_for_common_parent.split_first() else { + return; }; - groups - .iter() - .flat_map(|fields_for_common_parent| { - fields_for_common_parent - .split_first() - .and_then(|(first, rest)| { - let errors: Vec<_> = rest - .iter() - .filter_map(|other| { - if first.field.name() != other.field.name() { - Some(Error::FieldSelectionsDoNotMergeDifferingNames { - selection_set, - field_a: first.field, - field_b: other.field, - }) - } else if ! as Arguments>::equivalent( - first.field.arguments(), - other.field.arguments(), - ) { - Some(Error::FieldSelectionsDoNotMergeDifferingArguments { - selection_set, - field_a: first.field, - field_b: other.field, - }) - } else { - None - } - }) - .collect(); - - errors.is_empty().not().then_some(errors) - }) - .unwrap_or_else(|| { - let nested_grouped_fields = self.field_contexts_contained_fields( - fields_for_common_parent.iter().copied(), - ); + let prev_len = errors.len(); + errors.extend(rest.iter().filter_map(|other| { + if first.field.name() != other.field.name() { + Some(Error::FieldSelectionsDoNotMergeDifferingNames { + selection_set, + field_a: first.field, + field_b: other.field, + }) + } else if ! as Arguments>::equivalent( + first.field.arguments(), + other.field.arguments(), + ) { + Some(Error::FieldSelectionsDoNotMergeDifferingArguments { + selection_set, + field_a: first.field, + field_b: other.field, + }) + } else { + None + } + })); - nested_grouped_fields - .values() - .flat_map(|nested_fields_for_name| { - self.same_for_common_parents_by_name( - nested_fields_for_name.as_slice(), - selection_set, - ) - }) - .collect() - }) - }) - .collect() + if errors.len() == prev_len { + let nested_grouped_fields = + self.field_contexts_contained_fields(fields_for_common_parent.iter().copied()); + + for nested_fields_for_name in nested_grouped_fields.values() { + self.same_for_common_parents_by_name( + nested_fields_for_name.as_slice(), + selection_set, + errors, + ); + } + } } fn selection_set_contained_fields( @@ -227,12 +234,7 @@ impl<'a, E: ExecutableDocument + 'a, S: SchemaDefinition + 'a> FieldSelectionMer parent_type: TypeDefinitionReference<'a, S::TypeDefinition>, ) -> HashMap<&'a str, Vec>> { let mut fields = HashMap::new(); - self.visit_selections_for_fields( - selection_set.iter(), - &mut fields, - parent_type, - &HashSet::new(), - ); + self.visit_selections_for_fields(selection_set.iter(), &mut fields, parent_type, &[]); fields } @@ -269,7 +271,7 @@ impl<'a, E: ExecutableDocument + 'a, S: SchemaDefinition + 'a> FieldSelectionMer selections: impl Iterator, fields: &mut HashMap<&'a str, Vec>>, parent_type: TypeDefinitionReference<'a, S::TypeDefinition>, - parent_fragments: &HashSet<&'a str>, + parent_fragments: &[&'a str], ) { selections.for_each(|selection| match selection.as_ref() { SelectionReference::Field(field) => { @@ -284,13 +286,13 @@ impl<'a, E: ExecutableDocument + 'a, S: SchemaDefinition + 'a> FieldSelectionMer field, field_definition, parent_type, - parent_fragments: parent_fragments.to_owned(), + parent_fragments: parent_fragments.to_vec(), }); } } SelectionReference::FragmentSpread(fs) => { let fragment_name = fs.name(); - if !parent_fragments.contains(fragment_name) { + if !parent_fragments.contains(&fragment_name) { if let Some(fragment_definition) = self.cache.fragment_definition(fragment_name) { let type_condition = fragment_definition.type_condition(); @@ -301,9 +303,10 @@ impl<'a, E: ExecutableDocument + 'a, S: SchemaDefinition + 'a> FieldSelectionMer fragment_definition.selection_set(), parent_type, ) { - let mut new_parent_fragments = HashSet::new(); - new_parent_fragments.clone_from(parent_fragments); - new_parent_fragments.insert(fragment_name); + let mut new_parent_fragments = + Vec::with_capacity(parent_fragments.len() + 1); + new_parent_fragments.extend_from_slice(parent_fragments); + new_parent_fragments.push(fragment_name); self.visit_selections_for_fields( fragment_definition.selection_set().iter(), fields, @@ -380,5 +383,5 @@ struct FieldContext<'a, E: ExecutableDocument, S: SchemaDefinition> { field: &'a E::Field, field_definition: &'a S::FieldDefinition, parent_type: TypeDefinitionReference<'a, S::TypeDefinition>, - parent_fragments: HashSet<&'a str>, + parent_fragments: Vec<&'a str>, } diff --git a/bluejay-validator/src/executable/document/rules/fragment_spread_is_possible.rs b/bluejay-validator/src/executable/document/rules/fragment_spread_is_possible.rs index 6ebda12b..36a59eb1 100644 --- a/bluejay-validator/src/executable/document/rules/fragment_spread_is_possible.rs +++ b/bluejay-validator/src/executable/document/rules/fragment_spread_is_possible.rs @@ -10,7 +10,6 @@ use bluejay_core::executable::{ ExecutableDocument, FragmentDefinition, FragmentSpread, InlineFragment, }; use bluejay_core::AsIter; -use std::collections::HashSet; pub struct FragmentSpreadIsPossible<'a, E: ExecutableDocument, S: SchemaDefinition> { errors: Vec>, @@ -70,45 +69,79 @@ impl<'a, E: ExecutableDocument, S: SchemaDefinition> Visitor<'a, E, S> } impl<'a, E: ExecutableDocument, S: SchemaDefinition> FragmentSpreadIsPossible<'a, E, S> { - fn get_possible_types( - &self, - t: TypeDefinitionReference<'a, S::TypeDefinition>, - ) -> Option> { - match t { - TypeDefinitionReference::Object(_) => Some(HashSet::from([t.name()])), - TypeDefinitionReference::Interface(itd) => Some(HashSet::from_iter( - self.schema_definition - .get_interface_implementors(itd) - .map(ObjectTypeDefinition::name), - )), - TypeDefinitionReference::Union(utd) => Some(HashSet::from_iter( - utd.union_member_types() - .iter() - .map(|union_member| union_member.name()), - )), - TypeDefinitionReference::BuiltinScalar(_) - | TypeDefinitionReference::CustomScalar(_) - | TypeDefinitionReference::Enum(_) - | TypeDefinitionReference::InputObject(_) => None, - } - } - fn spread_is_not_possible( &self, parent_type: TypeDefinitionReference<'a, S::TypeDefinition>, fragment_type: TypeDefinitionReference<'a, S::TypeDefinition>, ) -> bool { - let parent_type_possible_types = self.get_possible_types(parent_type); - let fragment_possible_types = self.get_possible_types(fragment_type); + // Fast path: if either type is not a composite type, spread is not applicable + if !Self::is_composite(parent_type) || !Self::is_composite(fragment_type) { + return false; + } + // Fast path: same type name means definitely possible + if parent_type.name() == fragment_type.name() { + return false; + } + + // Fast path: if both are object types, they can only overlap if they're the same (already checked) + if matches!(parent_type, TypeDefinitionReference::Object(_)) + && matches!(fragment_type, TypeDefinitionReference::Object(_)) + { + return true; + } + + // For mixed cases, check intersection of possible types + !self.types_have_overlap(parent_type, fragment_type) + } + + fn is_composite(t: TypeDefinitionReference<'a, S::TypeDefinition>) -> bool { matches!( - (parent_type_possible_types, fragment_possible_types), - (Some(parent_type_possible_types), Some(fragment_possible_types)) if parent_type_possible_types - .intersection(&fragment_possible_types) - .next() - .is_none(), + t, + TypeDefinitionReference::Object(_) + | TypeDefinitionReference::Interface(_) + | TypeDefinitionReference::Union(_) ) } + + fn type_contains_name( + &self, + t: TypeDefinitionReference<'a, S::TypeDefinition>, + name: &str, + ) -> bool { + match t { + TypeDefinitionReference::Object(_) => t.name() == name, + TypeDefinitionReference::Interface(itd) => self + .schema_definition + .get_interface_implementors(itd) + .any(|otd| ObjectTypeDefinition::name(otd) == name), + TypeDefinitionReference::Union(utd) => utd + .union_member_types() + .iter() + .any(|member| member.name() == name), + _ => false, + } + } + + fn types_have_overlap( + &self, + a: TypeDefinitionReference<'a, S::TypeDefinition>, + b: TypeDefinitionReference<'a, S::TypeDefinition>, + ) -> bool { + // Iterate over possible types of `a` and check if any is in `b` + match a { + TypeDefinitionReference::Object(_) => self.type_contains_name(b, a.name()), + TypeDefinitionReference::Interface(itd) => self + .schema_definition + .get_interface_implementors(itd) + .any(|otd| self.type_contains_name(b, ObjectTypeDefinition::name(otd))), + TypeDefinitionReference::Union(utd) => utd + .union_member_types() + .iter() + .any(|member| self.type_contains_name(b, member.name())), + _ => false, + } + } } impl<'a, E: ExecutableDocument + 'a, S: SchemaDefinition + 'a> Rule<'a, E, S> diff --git a/bluejay-validator/src/executable/document/rules/fragment_spread_target_defined.rs b/bluejay-validator/src/executable/document/rules/fragment_spread_target_defined.rs index 88657c35..fb971f34 100644 --- a/bluejay-validator/src/executable/document/rules/fragment_spread_target_defined.rs +++ b/bluejay-validator/src/executable/document/rules/fragment_spread_target_defined.rs @@ -3,25 +3,20 @@ use crate::executable::{ Cache, }; use bluejay_core::definition::{SchemaDefinition, TypeDefinitionReference}; -use bluejay_core::executable::{ExecutableDocument, FragmentDefinition, FragmentSpread}; -use std::collections::HashSet; +use bluejay_core::executable::{ExecutableDocument, FragmentSpread}; pub struct FragmentSpreadTargetDefined<'a, E: ExecutableDocument, S: SchemaDefinition> { errors: Vec>, - fragment_definition_names: HashSet<&'a str>, + cache: &'a Cache<'a, E, S>, } impl<'a, E: ExecutableDocument, S: SchemaDefinition> Visitor<'a, E, S> for FragmentSpreadTargetDefined<'a, E, S> { - fn new(executable_document: &'a E, _: &'a S, _: &'a Cache<'a, E, S>) -> Self { + fn new(_: &'a E, _: &'a S, cache: &'a Cache<'a, E, S>) -> Self { Self { errors: Vec::new(), - fragment_definition_names: HashSet::from_iter( - executable_document - .fragment_definitions() - .map(FragmentDefinition::name), - ), + cache, } } @@ -31,9 +26,10 @@ impl<'a, E: ExecutableDocument, S: SchemaDefinition> Visitor<'a, E, S> _scoped_type: TypeDefinitionReference<'a, S::TypeDefinition>, _path: &Path<'a, E>, ) { - if !self - .fragment_definition_names - .contains(fragment_spread.name()) + if self + .cache + .fragment_definition(fragment_spread.name()) + .is_none() { self.errors .push(Error::FragmentSpreadTargetUndefined { fragment_spread }); diff --git a/bluejay-validator/src/executable/document/rules/fragment_spreads_must_not_form_cycles.rs b/bluejay-validator/src/executable/document/rules/fragment_spreads_must_not_form_cycles.rs index c1746675..ceaa8400 100644 --- a/bluejay-validator/src/executable/document/rules/fragment_spreads_must_not_form_cycles.rs +++ b/bluejay-validator/src/executable/document/rules/fragment_spreads_must_not_form_cycles.rs @@ -8,7 +8,7 @@ use bluejay_core::executable::{ SelectionReference, }; use bluejay_core::AsIter; -use std::collections::{BTreeMap, HashSet}; +use std::collections::BTreeMap; pub struct FragmentSpreadsMustNotFormCycles<'a, E: ExecutableDocument, S: SchemaDefinition> { errors: Vec>, @@ -22,14 +22,15 @@ impl<'a, E: ExecutableDocument, S: SchemaDefinition> FragmentSpreadsMustNotFormC >, target: &'a E::FragmentDefinition, fragment_definition_name: &'a str, - visited_fragment_definitions: &HashSet<&'a str>, + visited_fragment_definitions: &[&'a str], ) -> Option> { - if visited_fragment_definitions.contains(fragment_definition_name) { + if visited_fragment_definitions.contains(&fragment_definition_name) { return None; } if let Some((_, spreads)) = spreads_by_fragment_definition.get(fragment_definition_name) { - let mut visited_fragment_definitions = visited_fragment_definitions.clone(); - visited_fragment_definitions.insert(fragment_definition_name); + let mut new_visited = Vec::with_capacity(visited_fragment_definitions.len() + 1); + new_visited.extend_from_slice(visited_fragment_definitions); + new_visited.push(fragment_definition_name); spreads.iter().find_map(|&spread| { if spread.name() == target.name() { Some(Error::FragmentSpreadCycle { @@ -41,7 +42,7 @@ impl<'a, E: ExecutableDocument, S: SchemaDefinition> FragmentSpreadsMustNotFormC spreads_by_fragment_definition, target, spread.name(), - &visited_fragment_definitions, + &new_visited, ) } }) @@ -76,7 +77,7 @@ impl<'a, E: ExecutableDocument, S: SchemaDefinition> Visitor<'a, E, S> &spreads_by_fragment_definition, target, fragment_definition_name, - &HashSet::new(), + &[], ) }) .collect(); diff --git a/bluejay-validator/src/executable/document/rules/required_arguments.rs b/bluejay-validator/src/executable/document/rules/required_arguments.rs index 19f820fc..b5a89ebb 100644 --- a/bluejay-validator/src/executable/document/rules/required_arguments.rs +++ b/bluejay-validator/src/executable/document/rules/required_arguments.rs @@ -7,7 +7,6 @@ use bluejay_core::definition::{ }; use bluejay_core::executable::{ExecutableDocument, Field}; use bluejay_core::{Argument, AsIter, Directive}; -use std::collections::HashMap; pub struct RequiredArguments<'a, E: ExecutableDocument, S: SchemaDefinition> { schema_definition: &'a S, @@ -51,26 +50,17 @@ impl<'a, E: ExecutableDocument + 'a, S: SchemaDefinition + 'a> RequiredArguments build_error: F, ) { if let Some(arguments_definition) = arguments_definition { - let indexed_arguments = arguments - .map(|arguments| { - arguments.iter().fold( - HashMap::new(), - |mut indexed_arguments: HashMap<&'a str, &'a E::Argument>, - argument| { - indexed_arguments.insert(argument.name(), argument); - indexed_arguments - }, - ) - }) - .unwrap_or_default(); - let missing_argument_definitions = arguments_definition + // Use linear scan instead of HashMap — argument lists are typically small (1-5 items) + let missing_argument_definitions: Vec<_> = arguments_definition .iter() .filter(|ivd| { ivd.r#type().is_required() && ivd.default_value().is_none() - && !indexed_arguments.contains_key(ivd.name()) + && !arguments + .map(|args| args.iter().any(|arg| arg.name() == ivd.name())) + .unwrap_or(false) }) - .collect::>(); + .collect(); if !missing_argument_definitions.is_empty() { self.errors.push(build_error(missing_argument_definitions)); } diff --git a/bluejay-validator/src/utils.rs b/bluejay-validator/src/utils.rs index 33fe7309..b9731284 100644 --- a/bluejay-validator/src/utils.rs +++ b/bluejay-validator/src/utils.rs @@ -2,17 +2,37 @@ use std::cmp::{Eq, Ord}; use std::collections::BTreeMap; use std::hash::Hash; -pub fn duplicates, K: Hash + Ord + Eq>( +pub fn duplicates, K: Hash + Ord + Eq + Copy>( iter: I, key: fn(T) -> K, ) -> impl Iterator)> { - let indexed = iter.fold( - BTreeMap::new(), - |mut indexed: BTreeMap>, el: T| { - indexed.entry(key(el)).or_default().push(el); - indexed - }, - ); + // Collect items first to check for duplicates without BTreeMap + let items: Vec = iter.collect(); - indexed.into_iter().filter(|(_, values)| values.len() > 1) + // If 0 or 1 items, no duplicates possible — avoid any allocation + if items.len() <= 1 { + return Vec::new().into_iter(); + } + + // Quick O(n²) check for duplicates before allocating BTreeMap + let has_dupes = items.iter().enumerate().any(|(i, el)| { + let k = key(*el); + items[..i].iter().any(|prev| key(*prev) == k) + }); + + if !has_dupes { + return Vec::new().into_iter(); + } + + // Only allocate BTreeMap when we know there are duplicates + let mut indexed = BTreeMap::new(); + for el in items { + indexed.entry(key(el)).or_insert_with(Vec::new).push(el); + } + + indexed + .into_iter() + .filter(|(_, values)| values.len() > 1) + .collect::>() + .into_iter() } diff --git a/bluejay-validator/tests/snapshots/executable_integration_test__error@field_selection_merging.graphql.snap b/bluejay-validator/tests/snapshots/executable_integration_test__error@field_selection_merging.graphql.snap index 5ba2b279..4c4a92c7 100644 --- a/bluejay-validator/tests/snapshots/executable_integration_test__error@field_selection_merging.graphql.snap +++ b/bluejay-validator/tests/snapshots/executable_integration_test__error@field_selection_merging.graphql.snap @@ -1,5 +1,6 @@ --- source: bluejay-validator/tests/executable_integration_test.rs +assertion_line: 27 expression: formatted_errors input_file: bluejay-validator/tests/test_data/executable/error/field_selection_merging.graphql ---