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.
dgraph-io · Sep 3, 2020 · 243a336 · 243a336
1 parent 41c2052
commit 243a336
Show file tree

Hide file tree

Showing 4 changed files with 161 additions and 119 deletions.
diff --git a/graphql/e2e/schema/schema_test.go b/graphql/e2e/schema/schema_test.go
@@ -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("../../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
@@ -220,122 +220,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: `
@@ -348,7 +232,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)
@@ -358,7 +245,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
@@ -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
+}
diff --git a/graphql/schema/wrappers.go b/graphql/schema/wrappers.go
@@ -673,7 +673,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()