Skip to content

Commit

Permalink
fix: level of null data propagation (#810)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Aenimus
Aenimus authored May 22, 2024
1 parent 022941c commit 537f4d6
Show file tree
Hide file tree
Showing 6 changed files with 203 additions and 36 deletions.
2 changes: 1 addition & 1 deletion execution/engine/execution_engine_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -749,7 +749,7 @@ func TestExecutionEngine_Execute(t *testing.T) {
},
},
},
expectedResponse: `{"errors":[{"message":"Failed to fetch from Subgraph at Path 'query'."}],"data":null}`,
expectedResponse: `{"errors":[{"message":"Failed to fetch from Subgraph at Path 'query'."},{"message":"Cannot return null for non-nullable field 'Query.hero'.","path":["hero"]}],"data":null}`,
}))

t.Run("execute operation and apply input coercion for lists without variables", runWithoutError(ExecutionEngineTestCase{
Expand Down
4 changes: 2 additions & 2 deletions v2/pkg/engine/resolve/authorization_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ func TestAuthorization(t *testing.T) {
resolveCtx := &Context{ctx: context.Background(), Variables: nil, authorizer: authorizer}

return res, resolveCtx,
`{"errors":[{"message":"Unauthorized request to Subgraph 'users' at Path 'query', Reason: Not allowed to fetch from users Subgraph."},{"message":"Failed to fetch from Subgraph 'reviews' at Path 'query.me'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me"]}]}},{"message":"Failed to fetch from Subgraph 'products' at Path 'query.me.reviews.@.product'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me","reviews","@","product"]}]}}],"data":null}`,
`{"errors":[{"message":"Unauthorized request to Subgraph 'users' at Path 'query', Reason: Not allowed to fetch from users Subgraph."},{"message":"Failed to fetch from Subgraph 'reviews' at Path 'query.me'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me"]}]}},{"message":"Failed to fetch from Subgraph 'products' at Path 'query.me.reviews.@.product'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me","reviews","@","product"]}]}}],"data":{"me":null}}`,
func(t *testing.T) {
assert.Equal(t, int64(1), authorizer.(*testAuthorizer).preFetchCalls.Load())
assert.Equal(t, int64(0), authorizer.(*testAuthorizer).objectFieldCalls.Load())
Expand Down Expand Up @@ -236,7 +236,7 @@ func TestAuthorization(t *testing.T) {
resolveCtx := &Context{ctx: context.Background(), Variables: nil, authorizer: authorizer}

return res, resolveCtx,
`{"errors":[{"message":"Unauthorized request to Subgraph 'users' at Path 'query'."},{"message":"Failed to fetch from Subgraph 'reviews' at Path 'query.me'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me"]}]}},{"message":"Failed to fetch from Subgraph 'products' at Path 'query.me.reviews.@.product'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me","reviews","@","product"]}]}}],"data":null}`,
`{"errors":[{"message":"Unauthorized request to Subgraph 'users' at Path 'query'."},{"message":"Failed to fetch from Subgraph 'reviews' at Path 'query.me'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me"]}]}},{"message":"Failed to fetch from Subgraph 'products' at Path 'query.me.reviews.@.product'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me","reviews","@","product"]}]}}],"data":{"me":null}}`,
func(t *testing.T) {
assert.Equal(t, int64(1), authorizer.(*testAuthorizer).preFetchCalls.Load())
assert.Equal(t, int64(0), authorizer.(*testAuthorizer).objectFieldCalls.Load())
Expand Down
10 changes: 5 additions & 5 deletions v2/pkg/engine/resolve/extensions_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ func TestExtensions(t *testing.T) {
res := generateTestFederationGraphQLResponse(t, ctrl)

return res, &Context{ctx: context.Background(), Variables: nil, authorizer: authorizer},
`{"errors":[{"message":"Unauthorized request to Subgraph 'users' at Path 'query', Reason: test."},{"message":"Failed to fetch from Subgraph 'reviews' at Path 'query.me'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me"]}]}},{"message":"Failed to fetch from Subgraph 'products' at Path 'query.me.reviews.@.product'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me","reviews","@","product"]}]}}],"data":null}`,
`{"errors":[{"message":"Unauthorized request to Subgraph 'users' at Path 'query', Reason: test."},{"message":"Failed to fetch from Subgraph 'reviews' at Path 'query.me'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me"]}]}},{"message":"Failed to fetch from Subgraph 'products' at Path 'query.me.reviews.@.product'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me","reviews","@","product"]}]}}],"data":{"me":null}}`,
func(t *testing.T) {}
}))
t.Run("authorization deny & rate limit deny", testFnWithPostEvaluation(func(t *testing.T, ctrl *gomock.Controller) (node *GraphQLResponse, ctx *Context, expectedOutput string, postEvaluation func(t *testing.T)) {
Expand All @@ -45,7 +45,7 @@ func TestExtensions(t *testing.T) {
res := generateTestFederationGraphQLResponse(t, ctrl)

return res, &Context{ctx: context.Background(), Variables: nil, authorizer: authorizer, rateLimiter: limiter, RateLimitOptions: RateLimitOptions{Enable: true, IncludeStatsInResponseExtension: true}},
`{"errors":[{"message":"Unauthorized request to Subgraph 'users' at Path 'query', Reason: test."},{"message":"Failed to fetch from Subgraph 'reviews' at Path 'query.me'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me"]}]}},{"message":"Failed to fetch from Subgraph 'products' at Path 'query.me.reviews.@.product'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me","reviews","@","product"]}]}}],"data":null,"extensions":{"authorization":{"missingScopes":[["read:users"]]},"rateLimit":{"Policy":"policy","Allowed":0,"Used":0}}}`,
`{"errors":[{"message":"Unauthorized request to Subgraph 'users' at Path 'query', Reason: test."},{"message":"Failed to fetch from Subgraph 'reviews' at Path 'query.me'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me"]}]}},{"message":"Failed to fetch from Subgraph 'products' at Path 'query.me.reviews.@.product'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me","reviews","@","product"]}]}}],"data":{"me":null},"extensions":{"authorization":{"missingScopes":[["read:users"]]},"rateLimit":{"Policy":"policy","Allowed":0,"Used":0}}}`,
func(t *testing.T) {}
}))
t.Run("authorization deny & rate limit", testFnWithPostEvaluation(func(t *testing.T, ctrl *gomock.Controller) (node *GraphQLResponse, ctx *Context, expectedOutput string, postEvaluation func(t *testing.T)) {
Expand All @@ -70,7 +70,7 @@ func TestExtensions(t *testing.T) {
res := generateTestFederationGraphQLResponse(t, ctrl)

return res, &Context{ctx: context.Background(), Variables: nil, authorizer: authorizer, rateLimiter: limiter, RateLimitOptions: RateLimitOptions{Enable: true, IncludeStatsInResponseExtension: true}},
`{"errors":[{"message":"Unauthorized request to Subgraph 'users' at Path 'query', Reason: test."},{"message":"Failed to fetch from Subgraph 'reviews' at Path 'query.me'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me"]}]}},{"message":"Failed to fetch from Subgraph 'products' at Path 'query.me.reviews.@.product'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me","reviews","@","product"]}]}}],"data":null,"extensions":{"authorization":{"missingScopes":[["read:users"]]},"rateLimit":{"Policy":"policy","Allowed":0,"Used":0}}}`,
`{"errors":[{"message":"Unauthorized request to Subgraph 'users' at Path 'query', Reason: test."},{"message":"Failed to fetch from Subgraph 'reviews' at Path 'query.me'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me"]}]}},{"message":"Failed to fetch from Subgraph 'products' at Path 'query.me.reviews.@.product'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me","reviews","@","product"]}]}}],"data":{"me":null},"extensions":{"authorization":{"missingScopes":[["read:users"]]},"rateLimit":{"Policy":"policy","Allowed":0,"Used":0}}}`,
func(t *testing.T) {}
}))
t.Run("authorization & rate limit deny", testFnWithPostEvaluation(func(t *testing.T, ctrl *gomock.Controller) (node *GraphQLResponse, ctx *Context, expectedOutput string, postEvaluation func(t *testing.T)) {
Expand All @@ -92,7 +92,7 @@ func TestExtensions(t *testing.T) {
res := generateTestFederationGraphQLResponse(t, ctrl)

return res, &Context{ctx: context.Background(), Variables: nil, authorizer: authorizer, rateLimiter: limiter, RateLimitOptions: RateLimitOptions{Enable: true, IncludeStatsInResponseExtension: true}},
`{"errors":[{"message":"Rate limit exceeded for Subgraph 'users' at Path 'query', Reason: rate limit exceeded."},{"message":"Failed to fetch from Subgraph 'reviews' at Path 'query.me'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me"]}]}},{"message":"Failed to fetch from Subgraph 'products' at Path 'query.me.reviews.@.product'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me","reviews","@","product"]}]}}],"data":null,"extensions":{"rateLimit":{"Policy":"policy","Allowed":0,"Used":1}}}`,
`{"errors":[{"message":"Rate limit exceeded for Subgraph 'users' at Path 'query', Reason: rate limit exceeded."},{"message":"Failed to fetch from Subgraph 'reviews' at Path 'query.me'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me"]}]}},{"message":"Failed to fetch from Subgraph 'products' at Path 'query.me.reviews.@.product'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me","reviews","@","product"]}]}}],"data":{"me":null},"extensions":{"rateLimit":{"Policy":"policy","Allowed":0,"Used":1}}}`,
func(t *testing.T) {}
}))
t.Run("authorization & rate limit & trace", testFnWithPostEvaluation(func(t *testing.T, ctrl *gomock.Controller) (node *GraphQLResponse, ctx *Context, expectedOutput string, postEvaluation func(t *testing.T)) {
Expand Down Expand Up @@ -120,7 +120,7 @@ func TestExtensions(t *testing.T) {
ctx.ctx = SetTraceStart(ctx.ctx, true)

return res, ctx,
`{"errors":[{"message":"Unauthorized request to Subgraph 'users' at Path 'query', Reason: test."},{"message":"Failed to fetch from Subgraph 'reviews' at Path 'query.me'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me"]}]}},{"message":"Failed to fetch from Subgraph 'products' at Path 'query.me.reviews.@.product'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me","reviews","@","product"]}]}}],"data":null,"extensions":{"authorization":{"missingScopes":[["read:users"]]},"rateLimit":{"Policy":"policy","Allowed":0,"Used":0},"trace":{"info":{"trace_start_time":"","trace_start_unix":0,"parse_stats":{"duration_nanoseconds":0,"duration_pretty":"","duration_since_start_nanoseconds":0,"duration_since_start_pretty":""},"normalize_stats":{"duration_nanoseconds":0,"duration_pretty":"","duration_since_start_nanoseconds":0,"duration_since_start_pretty":""},"validate_stats":{"duration_nanoseconds":0,"duration_pretty":"","duration_since_start_nanoseconds":0,"duration_since_start_pretty":""},"planner_stats":{"duration_nanoseconds":0,"duration_pretty":"","duration_since_start_nanoseconds":0,"duration_since_start_pretty":""}},"fetch":{"id":"00000000-0000-0000-0000-000000000000","type":"single","data_source_id":"users","datasource_load_trace":{"raw_input_data":{},"single_flight_used":false,"single_flight_shared_response":false,"load_skipped":false}},"node_type":"object","nullable":true,"fields":[{"name":"me","value":{"fetch":{"id":"00000000-0000-0000-0000-000000000000","type":"single","data_source_id":"reviews","datasource_load_trace":{"single_flight_used":false,"single_flight_shared_response":false,"load_skipped":false}},"node_type":"object","path":["me"],"fields":[{"name":"id","value":{"node_type":"string","path":["id"]},"data_source_ids":["users"]},{"name":"username","value":{"node_type":"string","path":["username"]},"data_source_ids":["users"]},{"name":"reviews","value":{"node_type":"array","path":["reviews"],"items":[{"node_type":"object","nullable":true,"fields":[{"name":"body","value":{"node_type":"string","path":["body"]},"data_source_ids":["reviews"]},{"name":"product","value":{"fetch":{"id":"00000000-0000-0000-0000-000000000000","type":"single","data_source_id":"products","datasource_load_trace":{"single_flight_used":false,"single_flight_shared_response":false,"load_skipped":false}},"node_type":"object","path":["product"],"fields":[{"name":"upc","value":{"node_type":"string","path":["upc"]},"data_source_ids":["products"]},{"name":"name","value":{"node_type":"string","path":["data","name"]},"data_source_ids":["products"]}]},"data_source_ids":["reviews"]}]}]},"data_source_ids":["reviews"]}]}}]}}}`,
`{"errors":[{"message":"Unauthorized request to Subgraph 'users' at Path 'query', Reason: test."},{"message":"Failed to fetch from Subgraph 'reviews' at Path 'query.me'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me"]}]}},{"message":"Failed to fetch from Subgraph 'products' at Path 'query.me.reviews.@.product'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me","reviews","@","product"]}]}}],"data":{"me":null},"extensions":{"authorization":{"missingScopes":[["read:users"]]},"rateLimit":{"Policy":"policy","Allowed":0,"Used":0},"trace":{"info":{"trace_start_time":"","trace_start_unix":0,"parse_stats":{"duration_nanoseconds":0,"duration_pretty":"","duration_since_start_nanoseconds":0,"duration_since_start_pretty":""},"normalize_stats":{"duration_nanoseconds":0,"duration_pretty":"","duration_since_start_nanoseconds":0,"duration_since_start_pretty":""},"validate_stats":{"duration_nanoseconds":0,"duration_pretty":"","duration_since_start_nanoseconds":0,"duration_since_start_pretty":""},"planner_stats":{"duration_nanoseconds":0,"duration_pretty":"","duration_since_start_nanoseconds":0,"duration_since_start_pretty":""}},"fetch":{"id":"00000000-0000-0000-0000-000000000000","type":"single","data_source_id":"users","datasource_load_trace":{"raw_input_data":{},"single_flight_used":false,"single_flight_shared_response":false,"load_skipped":false}},"node_type":"object","nullable":true,"fields":[{"name":"me","value":{"fetch":{"id":"00000000-0000-0000-0000-000000000000","type":"single","data_source_id":"reviews","datasource_load_trace":{"single_flight_used":false,"single_flight_shared_response":false,"load_skipped":false}},"node_type":"object","path":["me"],"fields":[{"name":"id","value":{"node_type":"string","path":["id"]},"data_source_ids":["users"]},{"name":"username","value":{"node_type":"string","path":["username"]},"data_source_ids":["users"]},{"name":"reviews","value":{"node_type":"array","path":["reviews"],"items":[{"node_type":"object","nullable":true,"fields":[{"name":"body","value":{"node_type":"string","path":["body"]},"data_source_ids":["reviews"]},{"name":"product","value":{"fetch":{"id":"00000000-0000-0000-0000-000000000000","type":"single","data_source_id":"products","datasource_load_trace":{"single_flight_used":false,"single_flight_shared_response":false,"load_skipped":false}},"node_type":"object","path":["product"],"fields":[{"name":"upc","value":{"node_type":"string","path":["upc"]},"data_source_ids":["products"]},{"name":"name","value":{"node_type":"string","path":["data","name"]},"data_source_ids":["products"]}]},"data_source_ids":["reviews"]}]}]},"data_source_ids":["reviews"]}]}}]}}}`,
func(t *testing.T) {}
}))
}
2 changes: 1 addition & 1 deletion v2/pkg/engine/resolve/ratelimit_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ func TestRateLimiter(t *testing.T) {
res := generateTestFederationGraphQLResponse(t, ctrl)

return res, &Context{ctx: context.Background(), Variables: nil, rateLimiter: limiter, RateLimitOptions: RateLimitOptions{Enable: true}},
`{"errors":[{"message":"Rate limit exceeded for Subgraph 'users' at Path 'query', Reason: rate limit exceeded."},{"message":"Failed to fetch from Subgraph 'reviews' at Path 'query.me'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me"]}]}},{"message":"Failed to fetch from Subgraph 'products' at Path 'query.me.reviews.@.product'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me","reviews","@","product"]}]}}],"data":null}`,
`{"errors":[{"message":"Rate limit exceeded for Subgraph 'users' at Path 'query', Reason: rate limit exceeded."},{"message":"Failed to fetch from Subgraph 'reviews' at Path 'query.me'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me"]}]}},{"message":"Failed to fetch from Subgraph 'products' at Path 'query.me.reviews.@.product'.","extensions":{"errors":[{"message":"could not render fetch input","path":["me","reviews","@","product"]}]}}],"data":{"me":null}}`,
func(t *testing.T) {
assert.Equal(t, int64(1), limiter.rateLimitPreFetchCalls.Load())
}
Expand Down
20 changes: 3 additions & 17 deletions v2/pkg/engine/resolve/resolvable.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,23 +127,9 @@ func (r *Resolvable) Resolve(ctx context.Context, root *Object, out io.Writer) e
r.printErr = nil
r.authorizationError = nil

// if we have errors and no data, we only print the errors and set data to null
// in this case, we're skipping the walk because it would lead to unnecessary non-null errors
if r.hasErrors() && !r.hasData() {
r.printBytes(lBrace)
r.printErrors()
r.printBytes(quote)
r.printBytes(literalData)
r.printBytes(quote)
r.printBytes(colon)
r.printBytes(null)
if r.hasExtensions() {
r.printBytes(comma)
r.printErr = r.printExtensions(ctx, root)
}
r.printBytes(rBrace)
return nil
}
/* @TODO: In the event of an error or failed fetch, propagate only the highest level errors.
* For example, if a fetch fails, only propagate that the fetch has failed; do not propagate nested non-null errors.
*/

err := r.walkObject(root, r.dataRoot)
if r.authorizationError != nil {
Expand Down
Loading

0 comments on commit 537f4d6

Please sign in to comment.