Skip to content

Commit 8389e13

Browse files
oliriceolirice
authored
Merge pull request #479 from supabase/fix_fragment_spread_cycles
Fix fragment spread cycles
2 parents eecd43e + 011b495 commit 8389e13

File tree

5 files changed

+556
-1
lines changed

5 files changed

+556
-1
lines changed

‎docs/changelog.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,3 +58,4 @@
5858
- bugfix: make non-default args non-null in UDFs
5959
- bugfix: default value of a string type argument in a UDF was wrapped in single quotes
6060
- feature: add support for array types in UDFs
61+
- bugfix: fix crash when there are cycles in fragments

‎src/parser_util.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ where
4949
return Ok(());
5050
};
5151

52-
can_fields_merge(&matching_field, &field, type_name, field_map)?;
52+
can_fields_merge(matching_field, &field, type_name, field_map)?;
5353

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

‎src/resolve.rs

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,12 @@
1+
use std::collections::HashSet;
2+
13
use crate::builder::*;
24
use crate::graphql::*;
35
use crate::omit::*;
46
use crate::parser_util::*;
57
use crate::sql_types::get_one_readonly;
68
use crate::transpile::{MutationEntrypoint, QueryEntrypoint};
9+
use graphql_parser::query::Selection;
710
use graphql_parser::query::{
811
Definition, Document, FragmentDefinition, Mutation, OperationDefinition, Query, SelectionSet,
912
Text, VariableDefinition,
@@ -81,6 +84,18 @@ where
8184
|| (operation_names.len() == 1 && operation_name.is_none() ))
8285
.map(|x| x.0);
8386

87+
for fd in &fragment_defs {
88+
match detect_fragment_cycles(fd, &mut HashSet::new(), &fragment_defs, 1) {
89+
Ok(()) => {}
90+
Err(message) => {
91+
return GraphQLResponse {
92+
data: Omit::Omitted,
93+
errors: Omit::Present(vec![ErrorMessage { message }]),
94+
}
95+
}
96+
}
97+
}
98+
8499
match maybe_op {
85100
None => GraphQLResponse {
86101
data: Omit::Omitted,
@@ -500,3 +515,80 @@ where
500515
}
501516
}
502517
}
518+
519+
const STACK_DEPTH_LIMIT: u32 = 50;
520+
521+
fn detect_fragment_cycles<'a, 'b, T>(
522+
fragment_definition: &'b FragmentDefinition<'a, T>,
523+
visited: &mut HashSet<&'b str>,
524+
fragment_definitions: &'b [FragmentDefinition<'a, T>],
525+
stack_depth: u32,
526+
) -> Result<(), String>
527+
where
528+
T: Text<'a>,
529+
{
530+
if stack_depth > STACK_DEPTH_LIMIT {
531+
return Err(format!(
532+
"Fragment cycle depth is greater than {STACK_DEPTH_LIMIT}"
533+
));
534+
}
535+
if visited.contains(fragment_definition.name.as_ref()) {
536+
return Err("Found a cycle between fragments".to_string());
537+
} else {
538+
visited.insert(fragment_definition.name.as_ref());
539+
}
540+
detect_fragment_cycles_in_selection_set(
541+
&fragment_definition.selection_set,
542+
visited,
543+
fragment_definitions,
544+
stack_depth + 1,
545+
)?;
546+
547+
visited.remove(fragment_definition.name.as_ref());
548+
Ok(())
549+
}
550+
551+
fn detect_fragment_cycles_in_selection_set<'a, 'b, T>(
552+
selection_set: &'b SelectionSet<'a, T>,
553+
visited: &mut HashSet<&'b str>,
554+
fragment_definitions: &'b [FragmentDefinition<'a, T>],
555+
stack_depth: u32,
556+
) -> Result<(), String>
557+
where
558+
T: Text<'a>,
559+
{
560+
if stack_depth > STACK_DEPTH_LIMIT {
561+
return Err(format!(
562+
"Fragment cycle depth is greater than {STACK_DEPTH_LIMIT}"
563+
));
564+
}
565+
for selection in &selection_set.items {
566+
match selection {
567+
Selection::Field(field) => {
568+
detect_fragment_cycles_in_selection_set(
569+
&field.selection_set,
570+
visited,
571+
fragment_definitions,
572+
stack_depth + 1,
573+
)?;
574+
}
575+
Selection::FragmentSpread(fragment_spread) => {
576+
for fd in fragment_definitions {
577+
if fd.name == fragment_spread.fragment_name {
578+
detect_fragment_cycles(fd, visited, fragment_definitions, stack_depth + 1)?;
579+
break;
580+
}
581+
}
582+
}
583+
Selection::InlineFragment(inline_fragment) => {
584+
detect_fragment_cycles_in_selection_set(
585+
&inline_fragment.selection_set,
586+
visited,
587+
fragment_definitions,
588+
stack_depth + 1,
589+
)?;
590+
}
591+
}
592+
}
593+
Ok(())
594+
}

‎test/expected/issue_fragment_spread_cycles.out

Lines changed: 239 additions & 0 deletions
Large diffs are not rendered by default.

‎test/sql/issue_fragment_spread_cycles.sql

Lines changed: 223 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,223 @@
1+
begin;
2+
3+
-- example from the reported issue
4+
select graphql.resolve($$
5+
query {
6+
...A
7+
}
8+
9+
fragment A on Query {
10+
__typename
11+
...B
12+
}
13+
14+
fragment B on Query {
15+
__typename
16+
...A
17+
}
18+
$$);
19+
20+
-- example from graphql spec
21+
select graphql.resolve($$
22+
{
23+
dog {
24+
...nameFragment
25+
}
26+
}
27+
28+
fragment nameFragment on Dog {
29+
name
30+
...barkVolumeFragment
31+
}
32+
33+
fragment barkVolumeFragment on Dog {
34+
barkVolume
35+
...nameFragment
36+
}
37+
$$);
38+
39+
-- example from dockerfile
40+
create table account(
41+
id serial primary key,
42+
email varchar(255) not null,
43+
created_at timestamp not null
44+
);
45+
46+
create table blog(
47+
id serial primary key,
48+
owner_id integer not null references account(id) on delete cascade,
49+
name varchar(255) not null,
50+
description varchar(255),
51+
created_at timestamp not null
52+
);
53+
54+
create type blog_post_status as enum ('PENDING', 'RELEASED');
55+
56+
create table blog_post(
57+
id uuid not null default gen_random_uuid() primary key,
58+
blog_id integer not null references blog(id) on delete cascade,
59+
title varchar(255) not null,
60+
body varchar(10000),
61+
status blog_post_status not null,
62+
created_at timestamp not null
63+
);
64+
65+
insert into public.account(email, created_at)
66+
values
67+
('aardvark@x.com', now()),
68+
('bat@x.com', now()),
69+
('cat@x.com', now()),
70+
('dog@x.com', now()),
71+
('elephant@x.com', now());
72+
73+
insert into blog(owner_id, name, description, created_at)
74+
values
75+
((select id from account where email ilike 'a%'), 'A: Blog 1', 'a desc1', now()),
76+
((select id from account where email ilike 'a%'), 'A: Blog 2', 'a desc2', now()),
77+
((select id from account where email ilike 'a%'), 'A: Blog 3', 'a desc3', now()),
78+
((select id from account where email ilike 'b%'), 'B: Blog 3', 'b desc1', now());
79+
80+
comment on schema public is '@graphql({"inflect_names": true})';
81+
82+
select graphql.resolve($$
83+
{
84+
blogCollection {
85+
edges {
86+
node {
87+
... blogFragment
88+
}
89+
}
90+
}
91+
}
92+
93+
fragment blogFragment on Blog {
94+
owner {
95+
... accountFragment
96+
}
97+
}
98+
99+
fragment accountFragment on Account {
100+
blogCollection {
101+
edges {
102+
node {
103+
... blogFragment
104+
}
105+
}
106+
}
107+
}
108+
$$);
109+
110+
select graphql.resolve($$
111+
{
112+
blogCollection {
113+
edges {
114+
node {
115+
... blogFragment
116+
}
117+
}
118+
}
119+
}
120+
121+
fragment blogFragment on Blog {
122+
owner {
123+
blogCollection {
124+
edges {
125+
node {
126+
... blogFragment
127+
}
128+
}
129+
}
130+
}
131+
}
132+
$$);
133+
134+
select graphql.resolve($$
135+
{
136+
blogCollection {
137+
edges {
138+
node {
139+
id
140+
}
141+
}
142+
}
143+
}
144+
145+
fragment blogFragment on Blog {
146+
owner {
147+
blogCollection {
148+
edges {
149+
node {
150+
... blogFragment
151+
}
152+
}
153+
}
154+
}
155+
}
156+
$$);
157+
158+
-- test that a recursion limit of 50 is good enough for most queries
159+
select graphql.resolve($$
160+
{
161+
blogCollection {
162+
edges {
163+
node {
164+
... blogFragment
165+
}
166+
}
167+
}
168+
}
169+
170+
fragment blogFragment on Blog {
171+
owner {
172+
blogCollection {
173+
edges {
174+
node {
175+
owner {
176+
blogCollection {
177+
edges {
178+
node {
179+
owner {
180+
blogCollection {
181+
edges {
182+
node {
183+
owner {
184+
blogCollection {
185+
edges {
186+
node {
187+
owner {
188+
blogCollection {
189+
edges {
190+
node {
191+
owner {
192+
blogCollection {
193+
edges {
194+
node {
195+
id
196+
}
197+
}
198+
}
199+
}
200+
}
201+
}
202+
}
203+
}
204+
}
205+
}
206+
}
207+
}
208+
}
209+
}
210+
}
211+
}
212+
}
213+
}
214+
}
215+
}
216+
}
217+
}
218+
}
219+
}
220+
}
221+
$$);
222+
223+
rollback;

0 commit comments

Comments
 (0)