From e77cca26cca747258a616cebd6fd3477c57805ba Mon Sep 17 00:00:00 2001 From: Abhimanyu Singh Gaur <12651351+abhimanyusinghgaur@users.noreply.github.com> Date: Thu, 3 Sep 2020 22:04:36 +0530 Subject: [PATCH] fix(GraphQL): fix introspection completion bug (#6385) Fixes GRAPHQL-673. This PR fixes the issue where introspection queries would break if there were two types implementing same interface and having fields with same name in those two types and that repeating field is also a field in introspection query. This was introduced after #6228. (cherry picked from commit 243a336d17c8a27ddd6a59ffec894fdbc68f6480) --- graphql/e2e/schema/schema_test.go | 30 +++++ graphql/schema/introspection_test.go | 123 +----------------- .../introspection/input/full_query.graphql | 113 ++++++++++++++++ graphql/schema/wrappers.go | 14 +- 4 files changed, 161 insertions(+), 119 deletions(-) create mode 100644 graphql/schema/testdata/introspection/input/full_query.graphql diff --git a/graphql/e2e/schema/schema_test.go b/graphql/e2e/schema/schema_test.go index c2f5bc2f66b..1b7749bdf07 100644 --- a/graphql/e2e/schema/schema_test.go +++ b/graphql/e2e/schema/schema_test.go @@ -366,6 +366,36 @@ func TestUpdateGQLSchemaFields(t *testing.T) { require.Equal(t, string(generatedSchema), updateResp.UpdateGQLSchema.GQLSchema.GeneratedSchema) } +func TestIntrospection(t *testing.T) { + // note that both the types implement the same interface and have a field called `name`, which + // has exact same name as a field in full introspection query. + schema := ` + interface Node { + id: ID! + } + + type Human implements Node { + name: String + } + + type Dog implements Node { + name: String + }` + updateGQLSchemaRequireNoErrors(t, schema, groupOneAdminServer) + query, err := ioutil.ReadFile("../../schema/testdata/introspection/input/full_query.graphql") + require.NoError(t, err) + + introspectionParams := &common.GraphQLParams{Query: string(query)} + resp := introspectionParams.ExecuteAsPost(t, groupOneServer) + + // checking that there are no errors in the response, i.e., we always get some data in the + // introspection response. + require.Nilf(t, resp.Errors, "%s", resp.Errors) + require.NotEmpty(t, resp.Data) + // TODO: we should actually compare data here, but there seems to be some issue with either the + // introspection response or the JSON comparison. Needs deeper looking. +} + func updateGQLSchema(t *testing.T, schema, url string) *common.GraphQLResponse { req := &common.GraphQLParams{ Query: `mutation updateGQLSchema($sch: String!) { diff --git a/graphql/schema/introspection_test.go b/graphql/schema/introspection_test.go index ebad506fc55..0595e52e462 100644 --- a/graphql/schema/introspection_test.go +++ b/graphql/schema/introspection_test.go @@ -221,122 +221,6 @@ func TestIntrospectionQueryWithVars(t *testing.T) { testutil.CompareJSON(t, string(expectedBuf), string(resp)) } -const ( - testIntrospectionQuery = `query { - __schema { - __typename - queryType { - name - __typename - } - mutationType { - name - __typename - } - subscriptionType { - name - __typename - } - types { - ...FullType - } - directives { - __typename - name - locations - args { - ...InputValue - } - } - } - } - fragment FullType on __Type { - kind - name - fields(includeDeprecated: true) { - __typename - name - args { - ...InputValue - __typename - } - type { - ...TypeRef - __typename - } - isDeprecated - deprecationReason - } - inputFields { - ...InputValue - __typename - } - interfaces { - ...TypeRef - __typename - } - enumValues(includeDeprecated: true) { - name - isDeprecated - deprecationReason - __typename - } - possibleTypes { - ...TypeRef - __typename - } - __typename - } - fragment InputValue on __InputValue { - __typename - name - type { - ...TypeRef - } - defaultValue - } - fragment TypeRef on __Type { - kind - name - ofType { - kind - name - ofType { - kind - name - ofType { - kind - name - ofType { - kind - name - ofType { - kind - name - ofType { - kind - name - ofType { - kind - name - __typename - } - __typename - } - __typename - } - __typename - } - __typename - } - __typename - } - __typename - } - __typename - }` -) - func TestFullIntrospectionQuery(t *testing.T) { sch := gqlparser.MustLoadSchema( &ast.Source{Name: "schema.graphql", Input: ` @@ -349,7 +233,10 @@ func TestFullIntrospectionQuery(t *testing.T) { } `}) - doc, gqlErr := parser.ParseQuery(&ast.Source{Input: testIntrospectionQuery}) + fullIntrospectionQuery, err := ioutil.ReadFile("testdata/introspection/input/full_query.graphql") + require.NoError(t, err) + + doc, gqlErr := parser.ParseQuery(&ast.Source{Input: string(fullIntrospectionQuery)}) require.Nil(t, gqlErr) listErr := validator.Validate(sch, doc) @@ -359,7 +246,7 @@ func TestFullIntrospectionQuery(t *testing.T) { require.NotNil(t, op) oper := &operation{op: op, vars: map[string]interface{}{}, - query: string(testIntrospectionQuery), + query: string(fullIntrospectionQuery), doc: doc, inSchema: &schema{schema: sch}, } diff --git a/graphql/schema/testdata/introspection/input/full_query.graphql b/graphql/schema/testdata/introspection/input/full_query.graphql new file mode 100644 index 00000000000..f3b63fe0ef9 --- /dev/null +++ b/graphql/schema/testdata/introspection/input/full_query.graphql @@ -0,0 +1,113 @@ +query { + __schema { + __typename + queryType { + name + __typename + } + mutationType { + name + __typename + } + subscriptionType { + name + __typename + } + types { + ...FullType + } + directives { + __typename + name + locations + args { + ...InputValue + } + } + } +} +fragment FullType on __Type { + kind + name + fields(includeDeprecated: true) { + __typename + name + args { + ...InputValue + __typename + } + type { + ...TypeRef + __typename + } + isDeprecated + deprecationReason + } + inputFields { + ...InputValue + __typename + } + interfaces { + ...TypeRef + __typename + } + enumValues(includeDeprecated: true) { + name + isDeprecated + deprecationReason + __typename + } + possibleTypes { + ...TypeRef + __typename + } + __typename +} +fragment InputValue on __InputValue { + __typename + name + type { + ...TypeRef + } + defaultValue +} +fragment TypeRef on __Type { + kind + name + ofType { + kind + name + ofType { + kind + name + ofType { + kind + name + ofType { + kind + name + ofType { + kind + name + ofType { + kind + name + ofType { + kind + name + __typename + } + __typename + } + __typename + } + __typename + } + __typename + } + __typename + } + __typename + } + __typename +} \ No newline at end of file diff --git a/graphql/schema/wrappers.go b/graphql/schema/wrappers.go index 078e526145d..ffdd272acd5 100644 --- a/graphql/schema/wrappers.go +++ b/graphql/schema/wrappers.go @@ -672,7 +672,19 @@ func (f *field) DgraphAlias() string { // if this field is repeated, then it should be aliased using its dgraph predicate which will be // unique across repeated fields if f.op.inSchema.repeatedFieldNames[f.Name()] { - return f.DgraphPredicate() + dgraphPredicate := f.DgraphPredicate() + // There won't be any dgraph predicate for fields in introspection queries, as they are not + // stored in dgraph. So we identify those fields using this condition, and just let the + // field name get returned for introspection query fields, because the raw data response is + // prepared for them using only the field name, so that is what should be used to pick them + // back up from that raw data response before completion is performed. + // Now, the reason to not combine this if check with the outer one is because this + // function is performance critical. If there are a million fields in the output, + // it would be called a million times. So, better to perform this check and allocate memory + // for the variable only when necessary to do so. + if dgraphPredicate != "" { + return dgraphPredicate + } } // if not repeated, alias it using its name return f.Name()