Skip to content

Commit

Permalink
fix: provide more accurate error for invalid fragment spread (#538)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Aenimus
Aenimus committed Jun 6, 2023
1 parent c866e35 commit 2e219a6
Show file tree
Hide file tree
Showing 8 changed files with 133 additions and 20 deletions.
3 changes: 1 addition & 2 deletions examples/federation/gateway/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
)

// It's just a simple example of graphql federation gateway server, it's NOT a production ready code.
//
func logger() log.Logger {
logger, err := zap.NewDevelopmentConfig().Build()
if err != nil {
Expand Down Expand Up @@ -59,7 +58,7 @@ func startServer() {

datasourceWatcher := NewDatasourcePoller(httpClient, DatasourcePollerConfig{
Services: []ServiceConfig{
{Name: "accounts", URL: "http://localhost:4008/query", Fallback: fallback},
{Name: "accounts", URL: "http://localhost:4001/query", Fallback: fallback},
{Name: "products", URL: "http://localhost:4002/query", WS: "ws://localhost:4002/query"},
{Name: "reviews", URL: "http://localhost:4003/query"},
},
Expand Down
12 changes: 4 additions & 8 deletions examples/federation/go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ github.com/golang/protobuf v1.5.0 h1:LUVKkCeviFUMKqHa4tXIIij/lbhnMbP7Fn5wKdKkRh4
github.com/golang/protobuf v1.5.0/go.mod h1:FsONVRAS9T7sI+LIUmWTfcYkHO4aIWwzhcaSAoJOfIk=
github.com/google/go-cmp v0.4.0/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
github.com/google/go-cmp v0.5.6 h1:BKbKCqvP6I+rmFHt06ZmyQtvB8xAkWdhFyr0ZUNZcxQ=
github.com/google/go-cmp v0.5.8 h1:e6P7q2lk1O+qJJb4BtCQXlK8vWEO8V1ZeuEdJNOqZyg=
github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
github.com/google/renameio v0.1.0/go.mod h1:KWCgfxg9yswjAJkECMjeO8J8rahYeXnNhOm40UhjYkI=
github.com/google/uuid v1.1.1 h1:Gkbcsh/GbpXz7lPftLA3P6TYMwjCLYm83jiFQZF/3gY=
Expand Down Expand Up @@ -188,8 +188,6 @@ golang.org/x/crypto v0.0.0-20190510104115-cbcb75029529/go.mod h1:yigFU9vqHzYiE8U
golang.org/x/crypto v0.0.0-20191227163750-53104e6ec876/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto=
golang.org/x/crypto v0.0.0-20210314154223-e6e6c4f2bb5b/go.mod h1:T9bdIzuCu7OtxOm1hfPfRQxPLYneinmdGuTeoZ9dtd4=
golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc=
golang.org/x/crypto v0.0.0-20220315160706-3147a52a75dd h1:XcWmESyNjXJMLahc3mqVQJcgSTDxFxhETVlfk9uGc38=
golang.org/x/crypto v0.0.0-20220315160706-3147a52a75dd/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4=
golang.org/x/crypto v0.1.0 h1:MDRAIl0xIo9Io2xV565hzXHw3zVseKrJKodhohM5CjU=
golang.org/x/crypto v0.1.0/go.mod h1:RecgLatLF4+eUMCP1PoPZQb+cVrJcOPbHkTkbkB9sbw=
golang.org/x/exp v0.0.0-20230203172020-98cc5a0785f9 h1:frX3nT9RkKybPnjyI+yvZh6ZucTZatCCEm9D47sZ2zo=
Expand All @@ -198,16 +196,15 @@ golang.org/x/lint v0.0.0-20190930215403-16217165b5de/go.mod h1:6SW0HCj/g11FgYtHl
golang.org/x/lint v0.0.0-20191125180803-fdd1cda4f05f h1:J5lckAjkw6qYlOZNj90mLYNTEKDvWeuc1yieZ8qUzUE=
golang.org/x/mod v0.0.0-20190513183733-4bf6d317e70e/go.mod h1:mXi4GBBbnImb6dmsKGUJ2LatrhH/nqhxcFungHvyanc=
golang.org/x/mod v0.6.0-dev.0.20220106191415-9b9b3d81d5e3/go.mod h1:3p9vT2HGsQu2K1YbXdKPJLVgG5VJdoTa1poYQBtP1AY=
golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4 h1:6zppjxzCulZykYSLyVDYbneBfbaBIQPYMevg0bEwv2s=
golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4=
golang.org/x/mod v0.6.0 h1:b9gGHsz9/HhJ3HF5DHQytPpuwocVTChQJK3AvoLRD5I=
golang.org/x/mod v0.6.0/go.mod h1:4mET923SAdbXp2ki8ey+zGs1SLqsuM2Y0uvdZR/fUNI=
golang.org/x/net v0.0.0-20190311183353-d8887717615a/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg=
golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg=
golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s=
golang.org/x/net v0.0.0-20191116160921-f9c825593386/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s=
golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg=
golang.org/x/net v0.0.0-20211015210444-4f30a5c0130f/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y=
golang.org/x/net v0.0.0-20220722155237-a158d28d115b h1:PxfKdU9lEEDYjdIzOtC4qFWgkU2rGHdKlKowJSMN9h0=
golang.org/x/net v0.0.0-20220722155237-a158d28d115b/go.mod h1:XRhObCWvk6IyKnWLug+ECip1KBveYUHfp+8e9klMJ9c=
golang.org/x/net v0.1.0 h1:hZ/3BUoy5aId7sCpA/Tc5lt8DkFgdVS2onTpJsZ/fl0=
golang.org/x/net v0.1.0/go.mod h1:Cx3nUiGt4eDBEyega/BKRp+/AlGL8hYe7U9odMt2Cco=
Expand All @@ -226,7 +223,6 @@ golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBc
golang.org/x/sys v0.0.0-20211019181941-9d821ace8654/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab h1:2QkjZIsXupsJbJIdSjjUOgWK3aEtzyuh2mPt3l/CkeU=
golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
golang.org/x/sys v0.1.0 h1:kunALQeHf1/185U1i0GOB/fy1IPRDDpuoOOqRReG57U=
golang.org/x/sys v0.1.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
Expand All @@ -237,8 +233,8 @@ golang.org/x/text v0.3.2/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk=
golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
golang.org/x/text v0.3.6/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ=
golang.org/x/text v0.3.8 h1:nAL+RVCQ9uMn3vJZbV+MRnydTJFPf8qqY42YiA6MrqY=
golang.org/x/text v0.3.8/go.mod h1:E6s5w1FMmriuDzIBO73fBruAKo1PCIq6d2Q6DHfQ8WQ=
golang.org/x/text v0.4.0 h1:BrVqGRd7+k1DiOgtnFvAkoQEWQvBc25ouMJM6429SFg=
golang.org/x/text v0.4.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8=
golang.org/x/time v0.0.0-20191024005414-555d28b269f0/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
golang.org/x/time v0.0.0-20211116232009-f0f3c7e86c11 h1:GZokNIeuVkl3aZHJchRrr13WCsols02MLUcz1U9is6M=
Expand All @@ -251,8 +247,8 @@ golang.org/x/tools v0.0.0-20191029190741-b9c20aec41a5/go.mod h1:b+2E5dAYhXwXZwtn
golang.org/x/tools v0.0.0-20191108193012-7d206e10da11/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo=
golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo=
golang.org/x/tools v0.1.10/go.mod h1:Uh6Zz+xoGYZom868N8YTex3t7RhtHDBrE8Gzo9bV56E=
golang.org/x/tools v0.1.12 h1:VveCTK38A2rkS8ZqFY25HIDFscX5X9OoEhJd3quQmXU=
golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc=
golang.org/x/tools v0.2.0 h1:G6AHpWxTMGY1KyEYoAQ5WTtIekUUvDNjan3ugu60JvE=
golang.org/x/tools v0.2.0/go.mod h1:y4OqIKeOV/fWJetJ8bXPU1sEVniLMIyDAZWeHdV+NTA=
golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
Expand Down
18 changes: 14 additions & 4 deletions pkg/astvalidation/operation_rule_fragments.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package astvalidation

import (
"bytes"

"github.com/wundergraph/graphql-go-tools/pkg/ast"
"github.com/wundergraph/graphql-go-tools/pkg/astvisitor"
"github.com/wundergraph/graphql-go-tools/pkg/operationreport"
Expand Down Expand Up @@ -30,13 +29,24 @@ type fragmentsVisitor struct {
}

func (f *fragmentsVisitor) EnterFragmentSpread(ref int) {
fragmentName := f.operation.FragmentSpreadNameBytes(ref)
fragmentDefinitionRef, exists := f.operation.FragmentDefinitionRef(fragmentName)
if !exists {
f.StopWithExternalErr(operationreport.ErrFragmentUndefined(fragmentName))
return
}
fragmentTypeName := f.operation.FragmentDefinitionTypeName(fragmentDefinitionRef)
enclosingTypeName := f.EnclosingTypeDefinition.NameBytes(f.definition)
if !bytes.Equal(fragmentTypeName, enclosingTypeName) {
f.StopWithExternalErr(operationreport.ErrInvalidFragmentSpread(fragmentName, fragmentTypeName, enclosingTypeName))
return
}
if f.Ancestors[0].Kind == ast.NodeKindOperationDefinition {
spreadName := f.operation.FragmentSpreadNameBytes(ref)
f.StopWithExternalErr(operationreport.ErrFragmentSpreadFormsCycle(spreadName))
f.StopWithExternalErr(operationreport.ErrFragmentSpreadFormsCycle(fragmentName))
}
}

func (f *fragmentsVisitor) LeaveDocument(operation, definition *ast.Document) {
func (f *fragmentsVisitor) LeaveDocument(_, _ *ast.Document) {
for i := range f.fragmentDefinitionsVisited {
if !f.operation.FragmentDefinitionIsUsed(f.fragmentDefinitionsVisited[i]) {
fragmentName := f.fragmentDefinitionsVisited[i]
Expand Down
34 changes: 31 additions & 3 deletions pkg/astvalidation/operation_validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2540,14 +2540,30 @@ func TestExecutionValidation(t *testing.T) {
})
t.Run("5.5.2 Fragment Spreads", func(t *testing.T) {
t.Run("5.5.2.1 Fragment spread target defined", func(t *testing.T) {
t.Run("133", func(t *testing.T) {
t.Run("Undefined fragment returns ErrFragmentUndefined", func(t *testing.T) {
run(t, `
{
dog {
...undefinedFragment
}
}`,
Fragments(), Invalid, withExpectNormalizationError())
Fragments(), Invalid, withExpectNormalizationError(), withValidationErrors("undefinedFragment undefined"))
})
t.Run("Undefined fragment after valid fragment returns ErrFragmentUndefined", func(t *testing.T) {
run(t, `
{
cat {
...validCatFragment
}
dog {
...undefinedFragment
}
}
fragment validCatFragment on Cat {
name
meowVolume
}`,
Fragments(), Invalid, withExpectNormalizationError(), withValidationErrors("undefinedFragment undefined"))
})
})
t.Run("5.5.2.2 Fragment spreads must not form cycles", func(t *testing.T) {
Expand All @@ -2566,7 +2582,7 @@ func TestExecutionValidation(t *testing.T) {
barkVolume
...nameFragment
}`,
Fragments(), Invalid)
Fragments(), Invalid, withValidationErrors("external: fragment spread: barkVolumeFragment forms fragment cycle"))
})
t.Run("136", func(t *testing.T) {
run(t, `
Expand Down Expand Up @@ -2669,6 +2685,18 @@ func TestExecutionValidation(t *testing.T) {
}`,
Fragments(), Valid)
})
t.Run("Spreading a fragment on an invalid type returns ErrInvalidFragmentSpread", func(t *testing.T) {
run(t, `
{
dog {
...invalidCatFragment
}
}
fragment invalidCatFragment on Cat {
meowVolume
}`,
Fragments(), Invalid, withValidationErrors("external: fragment spread: fragment invalidCatFragment must be spread on type Cat and not type Dog"))
})
})
t.Run("5.5.2.3.2 Abstract Spreads in Object Scope", func(t *testing.T) {
t.Run("139", func(t *testing.T) {
Expand Down
66 changes: 63 additions & 3 deletions pkg/graphql/execution_engine_v2_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ type ExecutionEngineV2TestCase struct {
}

func TestExecutionEngineV2_Execute(t *testing.T) {
run := func(testCase ExecutionEngineV2TestCase, withError bool) func(t *testing.T) {
run := func(testCase ExecutionEngineV2TestCase, withError bool, expectedErrorMessage string) func(t *testing.T) {
t.Helper()

return func(t *testing.T) {
Expand Down Expand Up @@ -188,18 +188,25 @@ func TestExecutionEngineV2_Execute(t *testing.T) {

if withError {
assert.Error(t, err)
if expectedErrorMessage != "" {
assert.Contains(t, err.Error(), expectedErrorMessage)
}
} else {
assert.NoError(t, err)
}
}
}

runWithError := func(testCase ExecutionEngineV2TestCase) func(t *testing.T) {
return run(testCase, true)
return run(testCase, true, "")
}

runWithAndCompareError := func(testCase ExecutionEngineV2TestCase, expectedErrorMessage string) func(t *testing.T) {
return run(testCase, true, expectedErrorMessage)
}

runWithoutError := func(testCase ExecutionEngineV2TestCase) func(t *testing.T) {
return run(testCase, false)
return run(testCase, false, "")
}

t.Run("execute with empty request object should not panic", runWithError(
Expand Down Expand Up @@ -1613,6 +1620,59 @@ func TestExecutionEngineV2_Execute(t *testing.T) {
expectedResponse: `{"data":{"hero":"Human"}}`,
},
))

t.Run("Spreading a fragment on an invalid type returns ErrInvalidFragmentSpread", runWithAndCompareError(
ExecutionEngineV2TestCase{
schema: starwarsSchema(t),
operation: loadStarWarsQuery(starwars.FileInvalidFragmentsQuery, nil),
dataSources: []plan.DataSourceConfiguration{
{
RootNodes: []plan.TypeField{
{
TypeName: "Query",
FieldNames: []string{"droid"},
},
},
ChildNodes: []plan.TypeField{
{
TypeName: "Droid",
FieldNames: []string{"name"},
},
},
Factory: &graphql_datasource.Factory{
HTTPClient: testNetHttpClient(t, roundTripperTestCase{
expectedHost: "example.com",
expectedPath: "/",
expectedBody: "",
sendResponseBody: `{"data":{"droid":{"name":"R2D2"}}}`,
sendStatusCode: 200,
}),
},
Custom: graphql_datasource.ConfigJson(graphql_datasource.Configuration{
Fetch: graphql_datasource.FetchConfiguration{
URL: "https://example.com/",
Method: "GET",
},
}),
},
},
fields: []plan.FieldConfiguration{
{
TypeName: "Query",
FieldName: "droid",
Arguments: []plan.ArgumentConfiguration{
{
Name: "id",
SourceType: plan.FieldArgumentSource,
RenderConfig: plan.RenderArgumentAsGraphQLValue,
},
},
},
},
expectedResponse: ``,
},
"fragment spread: fragment reviewFields must be spread on type Review and not type Droid",
))
}

func testNetHttpClient(t *testing.T, testCase roundTripperTestCase) *http.Client {
Expand Down
8 changes: 8 additions & 0 deletions pkg/operationreport/externalerror.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,14 @@ func ErrFragmentSpreadFormsCycle(spreadName ast.ByteSlice) (err ExternalError) {
return err
}

func ErrInvalidFragmentSpread(fragmentName, fragmentTypeName, enclosingName ast.ByteSlice) (err ExternalError) {
err.Message = fmt.Sprintf(
"fragment spread: fragment %s must be spread on type %s and not type %s",
fragmentName, fragmentTypeName, enclosingName,
)
return err
}

func ErrFragmentDefinedButNotUsed(fragmentName ast.ByteSlice) (err ExternalError) {
err.Message = fmt.Sprintf("fragment: %s defined but not used", fragmentName)
return err
Expand Down
1 change: 1 addition & 0 deletions pkg/starwars/starwars.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const (
FileMultiQueries = "testdata/queries/multi_queries.query"
FileMultiQueriesWithArguments = "testdata/queries/multi_queries_with_arguments.query"
FileInvalidQuery = "testdata/queries/invalid.query"
FileInvalidFragmentsQuery = "testdata/queries/invalid_fragments.query"
)

var (
Expand Down
11 changes: 11 additions & 0 deletions pkg/starwars/testdata/queries/invalid_fragments.query
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
query Fragments($droidID: ID!){
droid(id: $droidID) {
...reviewFields
}
}

fragment reviewFields on Review {
id
stars
commentary
}

0 comments on commit 2e219a6

Please sign in to comment.