Resolve Node IDs in dependent steps & run response middleware on partial success (#216)

* Add failing test for partial success with unresolved Node IDs
* Resolve Node IDs in dependent steps, even on partial successes
* Add failing test for partial success not scrubbing unrequested fields from response
* Fix builtin middleware not running on partial success

Fixes #214

Author: JohnStarich

execute.go

@@ -256,16 +256,12 @@ func executeOneStep(
 	}
 
 	// fire the query
-	err := queryer.Query(ctx.RequestContext, &graphql.QueryInput{
+	queryErr := queryer.Query(ctx.RequestContext, &graphql.QueryInput{
 		Query:         step.QueryString,
 		QueryDocument: step.QueryDocument,
 		Variables:     variables,
 		OperationName: operationName,
 	}, &queryResult)
-	if err != nil {
-		ctx.logger.Warn("Network Error: ", err)
-		return queryResult, nil, err
-	}
 
 	// NOTE: this insertion point could point to a list of values. If it did, we have to have
 	//       passed it to the this invocation of this function. It is safe to trust this
@@ -313,7 +309,7 @@ func executeOneStep(
 			}
 		}
 	}
-	return queryResult, dependentSteps, nil
+	return queryResult, dependentSteps, queryErr
 }
 
 func max(a, b int) int {

gateway.go

@@ -90,12 +90,9 @@ func (g *Gateway) Execute(ctx *RequestContext, plans QueryPlanList) (map[string]
 
 	// TODO: handle plans of more than one query
 	// execute the plan and return the results
-	result, err := g.executor.Execute(executionContext)
-	if err != nil {
-		if len(result) == 0 {
-			return nil, err
-		}
-		return result, err
+	result, executeErr := g.executor.Execute(executionContext)
+	if executeErr != nil && len(result) == 0 {
+		result = nil
 	}
 
 	// now that we have our response, throw it through the list of middlewarse
@@ -106,7 +103,7 @@ func (g *Gateway) Execute(ctx *RequestContext, plans QueryPlanList) (map[string]
 	}
 
 	// we're done here
-	return result, nil
+	return result, executeErr
 }
 
 func (g *Gateway) internalSchema() (*ast.Schema, error) {
@@ -189,7 +186,8 @@ func New(sources []*graphql.RemoteSchema, configs ...Option) (*Gateway, error) {
 			{
 				URL:    internalSchemaLocation,
 				Schema: internal,
-			}},
+			},
+		},
 			false,
 		),
 	)
@@ -337,23 +335,20 @@ func fieldURLs(schemas []*graphql.RemoteSchema, stripInternal bool) FieldURLMap
 	for _, remoteSchema := range schemas {
 		// each type defined by the schema can be found at remoteSchema.URL
 		for name, typeDef := range remoteSchema.Schema.Types {
-
 			// if the type is part of the introspection (and can't be left up to the backing services)
 			if !strings.HasPrefix(typeDef.Name, "__") || !stripInternal {
 				// you can ask for __typename at any service that defines the type
 				locations.RegisterURL(name, "__typename", remoteSchema.URL)
 
 				// each field of each type can be found here
 				for _, fieldDef := range typeDef.Fields {
-
 					// if the field is not an introspection field
 					if !(name == typeNameQuery && strings.HasPrefix(fieldDef.Name, "__")) {
 						locations.RegisterURL(name, fieldDef.Name, remoteSchema.URL)
 					} else if !stripInternal { // its an introspection name
 						// register the location for the field
 						locations.RegisterURL(name, fieldDef.Name, remoteSchema.URL)
 					}
-
 				}
 			}
 		}

gateway_test.go

@@ -754,6 +754,7 @@ type User {
 	`, resp.Body.String())
 }
 
+// TestDataAndErrorsBothReturnFromOneServicePartialSuccess verifies fix for https://github.com/nautilus/gateway/issues/212
 func TestDataAndErrorsBothReturnFromOneServicePartialSuccess(t *testing.T) {
 	t.Parallel()
 	schema, err := graphql.LoadSchema(`
@@ -801,3 +802,157 @@ type Query {
 		}
 	`, resp.Body.String())
 }
+
+// TestGatewayRunsResponseMiddlewaresOnError verifies part of fix for https://github.com/nautilus/gateway/issues/212
+// The issue included the 'id' field not getting scrubbed when an error was returned, and scrubbing is a builtin response middleware.
+func TestGatewayRunsResponseMiddlewaresOnError(t *testing.T) {
+	t.Parallel()
+	schema, err := graphql.LoadSchema(`
+type Query {
+	foo: String
+}
+`)
+	require.NoError(t, err)
+	queryerFactory := QueryerFactory(func(ctx *PlanningContext, url string) graphql.Queryer {
+		return graphql.QueryerFunc(func(input *graphql.QueryInput) (interface{}, error) {
+			return map[string]interface{}{
+					"foo": nil,
+				}, graphql.ErrorList{
+					&graphql.Error{
+						Message: "foo is broken",
+						Path:    []interface{}{"foo"},
+					},
+				}
+		})
+	})
+	executedResponseMiddleware := false
+	gateway, err := New([]*graphql.RemoteSchema{
+		{Schema: schema, URL: "boo"},
+	}, WithQueryerFactory(&queryerFactory), WithMiddlewares(ResponseMiddleware(func(*ExecutionContext, map[string]interface{}) error {
+		executedResponseMiddleware = true
+		return nil
+	})))
+	require.NoError(t, err)
+
+	req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(`{"query": "query { foo }"}`))
+	resp := httptest.NewRecorder()
+	gateway.GraphQLHandler(resp, req)
+	assert.Equal(t, http.StatusOK, resp.Code)
+	assert.JSONEq(t, `
+		{
+			"data": {
+				"foo": null
+			},
+			"errors": [
+				{
+					"message": "foo is broken",
+					"path": ["foo"],
+					"extensions": null
+				}
+			]
+		}
+	`, resp.Body.String())
+	assert.True(t, executedResponseMiddleware, "All response middleware should run, even on responses with errors")
+}
+
+// TestPartialSuccessAlsoResolvesValidNodeIDs verifies fix for https://github.com/nautilus/gateway/issues/214
+func TestPartialSuccessAlsoResolvesValidNodeIDs(t *testing.T) {
+	t.Parallel()
+	schemaFoo, err := graphql.LoadSchema(`
+type Query {
+	foo: Foo
+}
+
+type Foo {
+	bar: Bar
+	boo: String
+}
+
+interface Node {
+	id: ID!
+}
+
+type Bar implements Node {
+	id: ID!
+}
+`)
+	require.NoError(t, err)
+	schemaBar, err := graphql.LoadSchema(`
+type Query {
+	node(id: ID!): Node
+}
+
+interface Node {
+	id: ID!
+}
+
+type Bar implements Node {
+	id: ID!
+	baz: String
+}
+`)
+	require.NoError(t, err)
+	const query = `
+		query {
+			foo {
+				bar {
+					baz
+				}
+			}
+		}
+	`
+	queryerFactory := QueryerFactory(func(ctx *PlanningContext, url string) graphql.Queryer {
+		return graphql.QueryerFunc(func(input *graphql.QueryInput) (interface{}, error) {
+			t.Log("Received request:", input.Query)
+			if strings.Contains(input.Query, "node(") {
+				return map[string]interface{}{
+					"node": map[string]interface{}{
+						"baz": "biff",
+					},
+				}, nil
+			}
+			return map[string]interface{}{
+					"foo": map[string]interface{}{
+						"bar": map[string]interface{}{
+							"id": "bar-id",
+						},
+						"boo": nil,
+					},
+				}, graphql.ErrorList{
+					&graphql.Error{
+						Message: "boo is broken",
+						Path:    []interface{}{"foo", "boo"},
+					},
+				}
+		})
+	})
+	gateway, err := New([]*graphql.RemoteSchema{
+		{Schema: schemaFoo, URL: "foo"},
+		{Schema: schemaBar, URL: "bar"},
+	}, WithQueryerFactory(&queryerFactory))
+	require.NoError(t, err)
+
+	req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(fmt.Sprintf(`{"query": %q}`, query)))
+	resp := httptest.NewRecorder()
+	gateway.GraphQLHandler(resp, req)
+	assert.Equal(t, http.StatusOK, resp.Code)
+	assert.JSONEq(t, `
+		{
+			"data": {
+				"foo": {
+					"bar": {
+						"baz": "biff"
+					},
+					"boo": null
+				}
+			},
+			"errors": [
+				{
+					"message": "boo is broken",
+					"path": ["foo", "boo"],
+					"extensions": null
+				}
+			]
+		}
+	`, resp.Body.String())
+}