Skip to content

Commit

Permalink
chore: fix recursion in checking resolvers (#2145)
Browse files Browse the repository at this point in the history
Co-authored-by: Tushar Mathur <[email protected]>
  • Loading branch information
ssddOnTop and tusharmath authored Jun 9, 2024
1 parent d81eca7 commit 92bb085
Show file tree
Hide file tree
Showing 5 changed files with 60 additions and 10 deletions.
7 changes: 5 additions & 2 deletions src/core/blueprint/definitions.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::collections::HashSet;

use async_graphql_value::ConstValue;
use regex::Regex;

Expand Down Expand Up @@ -322,8 +324,10 @@ pub fn fix_dangling_resolvers<'a>(
{
TryFold::<(&ConfigModule, &Field, &config::Type, &str), FieldDefinition, String>::new(
move |(config, field, ty, name), mut b_field| {
let mut set = HashSet::new();
if !field.has_resolver()
&& validate_field_has_resolver(name, field, &config.types, ty).is_succeed()
&& validate_field_has_resolver(name, field, &config.types, ty, &mut set)
.is_succeed()
{
b_field = b_field.resolver(Some(IR::Dynamic(DynamicValue::Value(
ConstValue::Object(Default::default()),
Expand Down Expand Up @@ -375,7 +379,6 @@ fn to_fields(
} else {
GraphQLOperationType::Query
};

// Process fields that are not marked as `omit`
let fields = Valid::from_iter(
type_of
Expand Down
23 changes: 16 additions & 7 deletions src/core/blueprint/schema.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::collections::{BTreeMap, HashMap};
use std::collections::{BTreeMap, HashMap, HashSet};

use async_graphql::parser::types::ConstDirective;

Expand All @@ -16,8 +16,8 @@ fn validate_query(config: &Config) -> Valid<(), String> {
let Some(query) = config.find_type(query_type_name) else {
return Valid::fail("Query type is not defined".to_owned()).trace(query_type_name);
};

validate_type_has_resolvers(query_type_name, query, &config.types)
let mut set = HashSet::new();
validate_type_has_resolvers(query_type_name, query, &config.types, &mut set)
})
.unit()
}
Expand All @@ -28,19 +28,28 @@ fn validate_type_has_resolvers(
name: &str,
ty: &Type,
types: &BTreeMap<String, Type>,
visited: &mut HashSet<String>,
) -> Valid<(), String> {
if visited.contains(name) {
return Valid::succeed(());
}

visited.insert(name.to_string());

Valid::from_iter(ty.fields.iter(), |(name, field)| {
validate_field_has_resolver(name, field, types, ty)
validate_field_has_resolver(name, field, types, ty, visited)
})
.trace(name)
.unit()
}

#[allow(clippy::too_many_arguments)]
pub fn validate_field_has_resolver(
name: &str,
field: &Field,
types: &BTreeMap<String, Type>,
parent_ty: &Type,
visited: &mut HashSet<String>,
) -> Valid<(), String> {
Valid::<(), String>::fail("No resolver has been found in the schema".to_owned())
.when(|| {
Expand All @@ -53,7 +62,7 @@ pub fn validate_field_has_resolver(
if ty.scalar() {
return true;
}
let res = validate_type_has_resolvers(type_name, ty, types);
let res = validate_type_has_resolvers(type_name, ty, types, visited);
return !res.is_succeed();
} else {
// It's a Scalar
Expand Down Expand Up @@ -94,8 +103,8 @@ fn validate_mutation(config: &Config) -> Valid<(), String> {
return Valid::fail("Mutation type is not defined".to_owned())
.trace(mutation_type_name);
};

validate_type_has_resolvers(mutation_type_name, mutation, &config.types)
let mut set = HashSet::new();
validate_type_has_resolvers(mutation_type_name, mutation, &config.types, &mut set)
} else {
Valid::succeed(())
}
Expand Down
2 changes: 1 addition & 1 deletion src/core/valid/valid.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ impl<A, E> Valid<A, E> {

pub fn from_iter<B>(
iter: impl IntoIterator<Item = A>,
f: impl Fn(A) -> Valid<B, E>,
mut f: impl FnMut(A) -> Valid<B, E>,
) -> Valid<Vec<B>, E> {
let mut values: Vec<B> = Vec::new();
let mut errors: ValidationError<E> = ValidationError::empty();
Expand Down
14 changes: 14 additions & 0 deletions tests/core/snapshots/test-recursive-types.md_error.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
source: tests/core/spec.rs
expression: errors
---
[
{
"message": "No resolver has been found in the schema",
"trace": [
"Query",
"foo"
],
"description": null
}
]
24 changes: 24 additions & 0 deletions tests/execution/test-recursive-types.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
---
error: true
---

# test-recursive-types

```graphql @config
schema {
query: Query
}

type Query {
foo: Foo
}

type Foo {
bars: [Bar]
}

type Bar {
foo: Foo
relatedBars: [Bar]
}
```

1 comment on commit 92bb085

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running 30s test @ http://localhost:8000/graphql

4 threads and 100 connections

Thread Stats Avg Stdev Max +/- Stdev
Latency 6.64ms 3.01ms 130.67ms 73.56%
Req/Sec 3.81k 204.86 4.14k 90.25%

455407 requests in 30.01s, 2.28GB read

Requests/sec: 15175.67

Transfer/sec: 77.89MB

Please sign in to comment.