From c80e62556cd9cf9e30b9eb48de72cadc41e1f4ff Mon Sep 17 00:00:00 2001 From: Nicolas Maquet Date: Sat, 13 Jun 2020 18:42:18 +1200 Subject: [PATCH 1/2] Fix SIGSEGV when client subs to multiple fields --- internal/exec/subscribe.go | 4 ++++ subscription_test.go | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/internal/exec/subscribe.go b/internal/exec/subscribe.go index 6c7ea1a0..16cae69c 100644 --- a/internal/exec/subscribe.go +++ b/internal/exec/subscribe.go @@ -55,6 +55,10 @@ func (r *Request) Subscribe(ctx context.Context, s *resolvable.Schema, op *query } }() + if f == nil { + return sendAndReturnClosed(&Response{Errors: []*errors.QueryError{err}}) + } + if err != nil { if _, nonNullChild := f.field.Type.(*common.NonNull); nonNullChild { return sendAndReturnClosed(&Response{Errors: []*errors.QueryError{err}}) diff --git a/subscription_test.go b/subscription_test.go index 5b8c56ba..96649d94 100644 --- a/subscription_test.go +++ b/subscription_test.go @@ -56,6 +56,10 @@ func (r *helloSaidResolver) HelloSaid(ctx context.Context) (chan *helloSaidEvent return c, nil } +func (r *rootResolver) OtherField(ctx context.Context) <-chan int32 { + return make(chan int32) +} + func (r *helloSaidEventResolver) Msg() (string, error) { return r.msg, r.err } @@ -416,6 +420,36 @@ func TestRootOperations_validSubscriptionSchema(t *testing.T) { }) } +func TestError_multiple_subscription_fields(t *testing.T) { + gqltesting.RunSubscribes(t, []*gqltesting.TestSubscription{ + { + Name: "Explicit schema without subscription field", + Schema: graphql.MustParseSchema(` + schema { + query: Query + subscription: Subscription + } + type Query { + hello: String! + } + type Subscription { + helloSaid: HelloSaidEvent! + otherField: Int! + } + type HelloSaidEvent { + msg: String! + } + `, &rootResolver{helloSaidResolver: &helloSaidResolver{upstream: closedUpstream(&helloSaidEventResolver{msg: "Hello world!"})}}), + Query: `subscription { helloSaid { msg } otherField }`, + ExpectedResults: []gqltesting.TestResponse{ + { + Errors: []*qerrors.QueryError{qerrors.Errorf("can subscribe to at most one subscription at a time")}, + }, + }, + }, + }) +} + const schema = ` schema { subscription: Subscription, From 96ea6f07d73714de3b9cd645dc3a41e192cbcd9f Mon Sep 17 00:00:00 2001 From: Sean Sorrell Date: Wed, 17 Jun 2020 17:41:16 -0700 Subject: [PATCH 2/2] bugfix: correctly determine fragment usage In previous versions of this code, this validation would exit when it encountered a fragment legitimately used twice. This bugfix skips the recursion but does not stop progress altogether allowing other fragments to be marked as used. --- internal/validation/testdata/tests.json | 9 ++++++++- internal/validation/validation.go | 3 ++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/internal/validation/testdata/tests.json b/internal/validation/testdata/tests.json index 46df80d3..4687e8d1 100644 --- a/internal/validation/testdata/tests.json +++ b/internal/validation/testdata/tests.json @@ -1464,6 +1464,13 @@ "query": "\n query Foo($a: String, $b: String, $c: String) {\n ... on Type {\n field(a: $a) {\n field(b: $b) {\n ... on Type {\n field(c: $c)\n }\n }\n }\n }\n }\n ", "errors": [] }, + { + "name": "Validate: fragments are used even when they are nested", + "rule": "NoUnusedFragments", + "schema": 1, + "query": "\n query Foo() {\n ...StringFragment\n stringBox {\n ...StringFragment\n ...StringFragmentPrime\n}\n}\n\n\n fragment StringFragment on StringBox {\n scalar\n}\n\n fragment StringFragmentPrime on StringBox {\n unrelatedField\n}\n", + "errors": [] + }, { "name": "Validate: No unused variables/uses all variables in fragments", "rule": "NoUnusedVariables", @@ -3853,4 +3860,4 @@ ] } ] -} \ No newline at end of file +} diff --git a/internal/validation/validation.go b/internal/validation/validation.go index 94a9faf8..c8be7354 100644 --- a/internal/validation/validation.go +++ b/internal/validation/validation.go @@ -418,8 +418,9 @@ func markUsedFragments(c *context, sels []query.Selection, fragUsed map[*query.F } if _, ok := fragUsed[frag]; ok { - return + continue } + fragUsed[frag] = struct{}{} markUsedFragments(c, frag.Selections, fragUsed)