cherry-pick v20.07: fix(GraphQL): fix introspection completion bug (#6385)

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!) {

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},
 	}

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
+}

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()