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.

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

Control memory explosion on large list of queries #102

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
/internal/tests/testdata/graphql-js

.idea/
Copy link
Member

@tonyghita tonyghita Jul 7, 2017

Choose a reason for hiding this comment

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

This looks like it's specific to your developer environment. It'd probably be best to add in your global .gitignore.

83 changes: 83 additions & 0 deletions graphql_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"github.com/neelance/graphql-go"
"github.com/neelance/graphql-go/example/starwars"
"github.com/neelance/graphql-go/gqltesting"
"fmt"
"bytes"
Copy link
Member

Choose a reason for hiding this comment

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

Imports are in the wrong group.

)

type helloWorldResolver1 struct{}
Expand Down Expand Up @@ -1744,3 +1746,84 @@ func TestComposedFragments(t *testing.T) {
},
})
}

func TestOverlappingAlias(t *testing.T) {
query := `
{
hero(episode: EMPIRE) {
a: name
a: id
}
}
`
result := starwarsSchema.Exec(context.Background(), query, "", nil)
if len(result.Errors) == 0 {
t.Fatal("Expected error from overlapping alias")
}
}

// go test -bench=FragmentQueries -benchmem
func BenchmarkFragmentQueries(b *testing.B) {
singleQuery := `
composed_%d: hero(episode: EMPIRE) {
name
...friendsNames
...friendsIds
}
`

queryTemplate := `
{
%s
}

fragment friendsNames on Character {
friends {
name
}
}

fragment friendsIds on Character {
friends {
id
}
}
`

testCases := []int {
1,
10,
100,
1000,
10000,
}

for _, c := range testCases {
// for each count, add a case for overlapping aliases vs non-overlapping aliases
for _, o := range []bool{ true } {

var buffer bytes.Buffer
for i := 0; i < c; i++ {
idx := 0
if o {
idx = i
}
buffer.WriteString(fmt.Sprintf(singleQuery, idx))
}

query := fmt.Sprintf(queryTemplate, buffer.String())
a := "overlapping"
if o {
a = "non-overlapping"
}
b.Run(fmt.Sprintf("%d queries %s aliases", c, a), func(b *testing.B) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we'd want to b.ResetTimer() after setup? What do you think?

for n := 0; n < b.N; n++ {
result := starwarsSchema.Exec(context.Background(), query, "", nil)
if len(result.Errors) != 0 {
b.Fatal(result.Errors[0])
}
}
})
}
}
}
43 changes: 26 additions & 17 deletions internal/validation/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,10 @@ func validateSelectionSet(c *opContext, sels []query.Selection, t schema.NamedTy
validateSelection(c, sel, t)
}

useCache := len(sels) <= 100
Copy link
Member

Choose a reason for hiding this comment

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

Should this threshold be configurable? How did we arrive at 100?

for i, a := range sels {
for _, b := range sels[i+1:] {
c.validateOverlap(a, b, nil, nil)
c.validateOverlap(a, b, nil, nil, useCache)
}
}
}
Expand Down Expand Up @@ -381,16 +382,21 @@ func detectFragmentCycleSel(c *context, sel query.Selection, fragVisited map[*qu
}
}

func (c *context) validateOverlap(a, b query.Selection, reasons *[]string, locs *[]errors.Location) {
func (c *context) validateOverlap(a, b query.Selection, reasons *[]string, locs *[]errors.Location, useCache bool) {
Copy link
Member

Choose a reason for hiding this comment

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

Just a side comment on the internal function signatures, does not apply to these changes: the internal functions may have a nicer APIs if we took structs instead of ever-expanding lists of arguments.

Copy link
Member

@pavelnikolov pavelnikolov Mar 25, 2020

Choose a reason for hiding this comment

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

I agree with @tonyghita and this is a refactoring that sooner or later has to be done would improve maintainability of the library. However, it will be a breaking change as well it will require a considerable amount of work.

if a == b {
return
}

if _, ok := c.overlapValidated[selectionPair{a, b}]; ok {
return
if useCache {
if _, ok := c.overlapValidated[selectionPair{a, b}]; ok {
return
}
key := selectionPair{b, a}
if _, ok := c.overlapValidated[key]; ok {
return
}
c.overlapValidated[key] = struct{}{}
}
c.overlapValidated[selectionPair{a, b}] = struct{}{}
c.overlapValidated[selectionPair{b, a}] = struct{}{}

switch a := a.(type) {
case *query.Field:
Expand All @@ -399,7 +405,7 @@ func (c *context) validateOverlap(a, b query.Selection, reasons *[]string, locs
if b.Alias.Loc.Before(a.Alias.Loc) {
a, b = b, a
}
if reasons2, locs2 := c.validateFieldOverlap(a, b); len(reasons2) != 0 {
if reasons2, locs2 := c.validateFieldOverlap(a, b, useCache); len(reasons2) != 0 {
locs2 = append(locs2, a.Alias.Loc, b.Alias.Loc)
if reasons == nil {
c.addErrMultiLoc(locs2, "OverlappingFieldsCanBeMerged", "Fields %q conflict because %s. Use different aliases on the fields to fetch both if this was intentional.", a.Alias.Name, strings.Join(reasons2, " and "))
Expand All @@ -413,13 +419,13 @@ func (c *context) validateOverlap(a, b query.Selection, reasons *[]string, locs

case *query.InlineFragment:
for _, sel := range b.Selections {
c.validateOverlap(a, sel, reasons, locs)
c.validateOverlap(a, sel, reasons, locs, useCache)
}

case *query.FragmentSpread:
if frag := c.doc.Fragments.Get(b.Name.Name); frag != nil {
for _, sel := range frag.Selections {
c.validateOverlap(a, sel, reasons, locs)
c.validateOverlap(a, sel, reasons, locs, useCache)
}
}

Expand All @@ -429,13 +435,13 @@ func (c *context) validateOverlap(a, b query.Selection, reasons *[]string, locs

case *query.InlineFragment:
for _, sel := range a.Selections {
c.validateOverlap(sel, b, reasons, locs)
c.validateOverlap(sel, b, reasons, locs, useCache)
}

case *query.FragmentSpread:
if frag := c.doc.Fragments.Get(a.Name.Name); frag != nil {
for _, sel := range frag.Selections {
c.validateOverlap(sel, b, reasons, locs)
c.validateOverlap(sel, b, reasons, locs, useCache)
}
}

Expand All @@ -444,21 +450,24 @@ func (c *context) validateOverlap(a, b query.Selection, reasons *[]string, locs
}
}

func (c *context) validateFieldOverlap(a, b *query.Field) ([]string, []errors.Location) {
func (c *context) validateFieldOverlap(a, b *query.Field, useCache bool) ([]string, []errors.Location) {
if a.Alias.Name != b.Alias.Name {
return nil, nil
}

if asf := c.fieldMap[a].sf; asf != nil {
if bsf := c.fieldMap[b].sf; bsf != nil {
afm := c.fieldMap[a]
bfm := c.fieldMap[b]

if asf := afm.sf; asf != nil {
if bsf := bfm.sf; bsf != nil {
if !typesCompatible(asf.Type, bsf.Type) {
return []string{fmt.Sprintf("they return conflicting types %s and %s", asf.Type, bsf.Type)}, nil
}
}
}

at := c.fieldMap[a].parent
bt := c.fieldMap[b].parent
at := afm.parent
bt := bfm.parent
if at == nil || bt == nil || at == bt {
if a.Name.Name != b.Name.Name {
return []string{fmt.Sprintf("%s and %s are different fields", a.Name.Name, b.Name.Name)}, nil
Expand All @@ -473,7 +482,7 @@ func (c *context) validateFieldOverlap(a, b *query.Field) ([]string, []errors.Lo
var locs []errors.Location
for _, a2 := range a.Selections {
for _, b2 := range b.Selections {
c.validateOverlap(a2, b2, &reasons, &locs)
c.validateOverlap(a2, b2, &reasons, &locs, useCache)
}
}
return reasons, locs
Expand Down