From 0ac544a516da4a2d402d7c090aa15eb186a5056a Mon Sep 17 00:00:00 2001 From: John Starich Date: Wed, 21 Aug 2024 17:15:55 -0500 Subject: [PATCH] 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 https://github.com/nautilus/gateway/issues/214 --- execute.go | 8 +-- gateway.go | 17 ++---- gateway_test.go | 155 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 163 insertions(+), 17 deletions(-) diff --git a/execute.go b/execute.go index e1933e5..c1b84f4 100644 --- a/execute.go +++ b/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 { diff --git a/gateway.go b/gateway.go index 83e3ef1..2b5e8fe 100644 --- a/gateway.go +++ b/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,7 +335,6 @@ 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 @@ -345,7 +342,6 @@ func fieldURLs(schemas []*graphql.RemoteSchema, stripInternal bool) FieldURLMap // 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) @@ -353,7 +349,6 @@ func fieldURLs(schemas []*graphql.RemoteSchema, stripInternal bool) FieldURLMap // register the location for the field locations.RegisterURL(name, fieldDef.Name, remoteSchema.URL) } - } } } diff --git a/gateway_test.go b/gateway_test.go index d95c4c2..ca49787 100644 --- a/gateway_test.go +++ b/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()) +}