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

fix(GraphQL): fix introspection completion bug #6385

Merged
merged 2 commits into from
Sep 3, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 113 additions & 0 deletions graphql/e2e/schema/introspection.graphql
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
query {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have this file at one place and reference it from everywhere else? I thought we already had it somewhere else too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made it reusable

__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
}
30 changes: 30 additions & 0 deletions graphql/e2e/schema/schema_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,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("introspection.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!) {
Expand Down
10 changes: 9 additions & 1 deletion graphql/schema/wrappers.go
Original file line number Diff line number Diff line change
Expand Up @@ -673,7 +673,15 @@ 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()
dgraphAlias := f.DgraphPredicate()
Copy link
Contributor

Choose a reason for hiding this comment

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

This could have been simplified by appending the if below to the if above in this block with an and condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment about why didn't do it, because it is a performance-critical function.

// 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.
if dgraphAlias != "" {
return dgraphAlias
}
}
// if not repeated, alias it using its name
return f.Name()
Expand Down