Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix fragment spread cycles #479

Merged
merged 7 commits into from
Jan 2, 2024
Merged

Fix fragment spread cycles #479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cbe8a00
fix for fragment cycles crashing Postgres
imor Dec 26, 2023
c879167
Merge branch 'master' into fix_fragment_spread_cycles
imor Dec 27, 2023
274d7df
fix a clippy warning
imor Dec 27, 2023
9e8decc
limit stack recursion to 50
imor Dec 27, 2023
5048fe8
add a comment
imor Dec 27, 2023
a74e261
update changelog
imor Dec 27, 2023
011b495
add more tests
imor Dec 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,4 @@
- bugfix: make non-default args non-null in UDFs
- bugfix: default value of a string type argument in a UDF was wrapped in single quotes
- feature: add support for array types in UDFs
- bugfix: fix crash when there are cycles in fragments
2 changes: 1 addition & 1 deletion src/parser_util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ where
return Ok(());
};

can_fields_merge(&matching_field, &field, type_name, field_map)?;
can_fields_merge(matching_field, &field, type_name, field_map)?;

field.position = field.position.min(matching_field.position);

Expand Down
92 changes: 92 additions & 0 deletions src/resolve.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
use std::collections::HashSet;

use crate::builder::*;
use crate::graphql::*;
use crate::omit::*;
use crate::parser_util::*;
use crate::sql_types::get_one_readonly;
use crate::transpile::{MutationEntrypoint, QueryEntrypoint};
use graphql_parser::query::Selection;
use graphql_parser::query::{
Definition, Document, FragmentDefinition, Mutation, OperationDefinition, Query, SelectionSet,
Text, VariableDefinition,
Expand Down Expand Up @@ -81,6 +84,18 @@ where
|| (operation_names.len() == 1 && operation_name.is_none() ))
.map(|x| x.0);

for fd in &fragment_defs {
match detect_fragment_cycles(fd, &mut HashSet::new(), &fragment_defs, 1) {
Ok(()) => {}
Err(message) => {
return GraphQLResponse {
data: Omit::Omitted,
errors: Omit::Present(vec![ErrorMessage { message }]),
}
}
}
}

match maybe_op {
None => GraphQLResponse {
data: Omit::Omitted,
Expand Down Expand Up @@ -500,3 +515,80 @@ where
}
}
}

const STACK_DEPTH_LIMIT: u32 = 50;

fn detect_fragment_cycles<'a, 'b, T>(
fragment_definition: &'b FragmentDefinition<'a, T>,
visited: &mut HashSet<&'b str>,
fragment_definitions: &'b [FragmentDefinition<'a, T>],
stack_depth: u32,
) -> Result<(), String>
where
T: Text<'a>,
{
if stack_depth > STACK_DEPTH_LIMIT {
return Err(format!(
"Fragment cycle depth is greater than {STACK_DEPTH_LIMIT}"
));
}
if visited.contains(fragment_definition.name.as_ref()) {
return Err("Found a cycle between fragments".to_string());
} else {
visited.insert(fragment_definition.name.as_ref());
}
detect_fragment_cycles_in_selection_set(
&fragment_definition.selection_set,
visited,
fragment_definitions,
stack_depth + 1,
)?;

visited.remove(fragment_definition.name.as_ref());
Ok(())
}

fn detect_fragment_cycles_in_selection_set<'a, 'b, T>(
selection_set: &'b SelectionSet<'a, T>,
visited: &mut HashSet<&'b str>,
fragment_definitions: &'b [FragmentDefinition<'a, T>],
stack_depth: u32,
) -> Result<(), String>
where
T: Text<'a>,
{
if stack_depth > STACK_DEPTH_LIMIT {
return Err(format!(
"Fragment cycle depth is greater than {STACK_DEPTH_LIMIT}"
));
}
for selection in &selection_set.items {
match selection {
Selection::Field(field) => {
detect_fragment_cycles_in_selection_set(
&field.selection_set,
visited,
fragment_definitions,
stack_depth + 1,
)?;
}
Selection::FragmentSpread(fragment_spread) => {
for fd in fragment_definitions {
if fd.name == fragment_spread.fragment_name {
detect_fragment_cycles(fd, visited, fragment_definitions, stack_depth + 1)?;
break;
}
}
}
Selection::InlineFragment(inline_fragment) => {
detect_fragment_cycles_in_selection_set(
&inline_fragment.selection_set,
visited,
fragment_definitions,
stack_depth + 1,
)?;
}
}
}
Ok(())
}
239 changes: 239 additions & 0 deletions test/expected/issue_fragment_spread_cycles.out
View file
Open in desktop

Large diffs are not rendered by default.

223 changes: 223 additions & 0 deletions test/sql/issue_fragment_spread_cycles.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,223 @@
begin;

-- example from the reported issue
select graphql.resolve($$
query {
...A
}

fragment A on Query {
__typename
...B
}

fragment B on Query {
__typename
...A
}
$$);

-- example from graphql spec
select graphql.resolve($$
{
dog {
...nameFragment
}
}

fragment nameFragment on Dog {
name
...barkVolumeFragment
}

fragment barkVolumeFragment on Dog {
barkVolume
...nameFragment
}
$$);

-- example from dockerfile
create table account(
id serial primary key,
email varchar(255) not null,
created_at timestamp not null
);

create table blog(
id serial primary key,
owner_id integer not null references account(id) on delete cascade,
name varchar(255) not null,
description varchar(255),
created_at timestamp not null
);

create type blog_post_status as enum ('PENDING', 'RELEASED');

create table blog_post(
id uuid not null default gen_random_uuid() primary key,
blog_id integer not null references blog(id) on delete cascade,
title varchar(255) not null,
body varchar(10000),
status blog_post_status not null,
created_at timestamp not null
);

insert into public.account(email, created_at)
values
('[email protected]', now()),
('[email protected]', now()),
('[email protected]', now()),
('[email protected]', now()),
('[email protected]', now());

insert into blog(owner_id, name, description, created_at)
values
((select id from account where email ilike 'a%'), 'A: Blog 1', 'a desc1', now()),
((select id from account where email ilike 'a%'), 'A: Blog 2', 'a desc2', now()),
((select id from account where email ilike 'a%'), 'A: Blog 3', 'a desc3', now()),
((select id from account where email ilike 'b%'), 'B: Blog 3', 'b desc1', now());

comment on schema public is '@graphql({"inflect_names": true})';

select graphql.resolve($$
{
blogCollection {
edges {
node {
... blogFragment
}
}
}
}

fragment blogFragment on Blog {
owner {
... accountFragment
}
}

fragment accountFragment on Account {
blogCollection {
edges {
node {
... blogFragment
}
}
}
}
$$);

select graphql.resolve($$
{
blogCollection {
edges {
node {
... blogFragment
}
}
}
}

fragment blogFragment on Blog {
owner {
blogCollection {
edges {
node {
... blogFragment
}
}
}
}
}
$$);

select graphql.resolve($$
{
blogCollection {
edges {
node {
id
}
}
}
}

fragment blogFragment on Blog {
owner {
blogCollection {
edges {
node {
... blogFragment
}
}
}
}
}
$$);

-- test that a recursion limit of 50 is good enough for most queries
select graphql.resolve($$
{
blogCollection {
edges {
node {
... blogFragment
}
}
}
}

fragment blogFragment on Blog {
owner {
blogCollection {
edges {
node {
owner {
blogCollection {
edges {
node {
owner {
blogCollection {
edges {
node {
owner {
blogCollection {
edges {
node {
owner {
blogCollection {
edges {
node {
owner {
blogCollection {
edges {
node {
id
}
}
}
}
}
}
}
}
}
}
}
}
}
}
}
}
}
}
}
}
}
}
}
}
}
$$);

rollback;
Loading