[validation] Fix infinite loop on invalid queries in OverlappingFields #780
Conversation
|
|
||
| it('does not infinite loop on immediately spread fragment', () => { | ||
| expectPassesRule(OverlappingFieldsCanBeMerged, ` | ||
| fragment fragA on Human {name ...fragA }, |
There was a problem hiding this comment.
add a test for immediate spreads but in a larger cycle
fragment fragA on Human {name ...fragB }
fragment fragB on Human {name ...fragA }
|
I can confirm this issue :) This PR was open for quite a while now. Is there something else that prevents this PR from being merged? |
|
Nah, I think this is mergeable; it's not perfect, but it's certainly an upgrade over what exists before. @leebyron, any objections? |
There was a problem hiding this comment.
I just implemented the fix in sangria. @leebyron is right, this implementation will not work with larger cycles, so I implemented it in a different way. For what it's worth, here is my implementation of this fix (the algorithm implementation in sangria closely follows reference implementation, so it looks very similar despite the scala syntax):
https://github.com/sangria-graphql/sangria/pull/266/files
In this approach, I track visited fragment names for all branching paths. This allows me to detect larger cycles. This new addition has also influence on cachedFieldsAndFragmentNames. It will not work with selection set as a cache key, so I also included the set of visited fragments as a part of a cache key. I'm not yet 100% how big the impact this will have on a cache usefulness (it is now definitely diminished now).
Would appreciate your opinion on this approach.
I feel that it this fix becomes quite critical, we need to push it forward. I already saw this issue in production on several occasions. I also just verified and can confirm that it cases broken execution result in graphql-js as well. it looks like this:
{
"errors": [
{}
]
}| if (fragmentNames2.indexOf(fragmentName) !== -1) { | ||
| // This means a fragment spread in itself. We're going to infinite loop | ||
| // if we try and collect all fields. Pretend we did not index that fragment | ||
| fragmentNames2.splice(fragmentNames2.indexOf(fragmentName)); |
There was a problem hiding this comment.
I noticed one issue with this part. According to the docs, if splice takes only one argument (the start index), it will delete all elements in the array starting from this index.
I tested it in this example:
{
...a
}
fragment a on Query {
...a
...b
...c
}
fragment b on Query {
root {
test(a: "foo")
}
}
fragment c on Query {
root {
test(a: "bar")
}
}and indeed, conflicts in b and c are ignored.
Changing the code to this one fixed this particular issue:
fragmentNames2.splice(fragmentNames2.indexOf(fragmentName), 1);
|
Thanks for moving this forward Oleg! Just want to add that we have servers being restarted on a daily basis because of this. Looking forward to this fix landing. |
|
I'd love to see the fix handle larger cycles, since those are also likely and this PR as is will only partially improve the situation. If anyone is willing to help with this, that would be welcomed! |
`OverlappingFieldsCanBeMerged` would infinite loop when passed something like
```graphql
fragment A on User {
name
...A
}
```
It's not `OverlappingFieldsCanBeMerged`'s responsibility to detect that validation error, but we still would ideally avoid infinite looping.
This detects that case, and pretends that the infinite spread wasn't there for the purposes of this validation step.
Also, by memoizing and checking for self-references this removes duplicate reports.
Closes #780
|
I took the tests from this PR and wrote a complete solution in #1111 |
`OverlappingFieldsCanBeMerged` would infinite loop when passed something like
```graphql
fragment A on User {
name
...A
}
```
It's not `OverlappingFieldsCanBeMerged`'s responsibility to detect that validation error, but we still would ideally avoid infinite looping.
This detects that case, and pretends that the infinite spread wasn't there for the purposes of this validation step.
Also, by memoizing and checking for self-references this removes duplicate reports.
Closes #780
OverlappingFieldsCanBeMergedwould infinite loop when passed something likeIt's not
OverlappingFieldsCanBeMerged's responsibility to detect that validation error, but we still would ideally avoid infinite looping.This detects that case, and pretends that the infinite spread wasn't there for the purposes of this validation step.