@defer: duplicated errors across incremental items

[alessbell]

Describe the bug In https://github.com/apollographql/router-defer-e2e-tests/pull/8, some tests were added for error cases.

For the following query:

query TestQuery {
  allProducts {
    ...ErrorFragment @defer
    sku
    id
  }
}
fragment ErrorFragment on Product {
  promiseNonNullErrorField
}

the response is

--graphql
content-type: application/json

{"data":{"allProducts":[{"__typename":"Product","sku":"federation","id":"apollo-federation"},{"__typename":"Product","sku":"studio","id":"apollo-studio"},{"__typename":"Product","sku":"client","id":"apollo-client"}]},"hasNext":true}
--graphql
content-type: application/json

{"hasNext":false,"incremental":[{"data":null,"path":["allProducts",0],"errors":[{"message":"Cannot return null for non-nullable field Product.promiseNonNullErrorField.","locations":[{"line":1,"column":126}],"path":["allProducts","@","_entities",0,"promiseNonNullErrorField"],"extensions":{"code":"INTERNAL_SERVER_ERROR","exception":{"stacktrace":["Error: Cannot return null for non-nullable field Product.promiseNonNullErrorField.","    at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:594:13)","    at /usr/src/app/node_modules/graphql/execution/execute.js:486:9","    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)","    at async Promise.all (index 1)","    at async Promise.all (index 0)","    at async Promise.all (index 0)","    at async execute (/usr/src/app/node_modules/apollo-server-core/dist/requestPipeline.js:205:20)","    at async processGraphQLRequest (/usr/src/app/node_modules/apollo-server-core/dist/requestPipeline.js:148:28)","    at async processHTTPRequest (/usr/src/app/node_modules/apollo-server-core/dist/runHttpQuery.js:220:30)"]}}},{"message":"Cannot return null for non-nullable field Product.promiseNonNullErrorField.","locations":[{"line":1,"column":126}],"path":["allProducts","@","_entities",1,"promiseNonNullErrorField"],"extensions":{"code":"INTERNAL_SERVER_ERROR","exception":{"stacktrace":["Error: Cannot return null for non-nullable field Product.promiseNonNullErrorField.","    at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:594:13)","    at /usr/src/app/node_modules/graphql/execution/execute.js:486:9","    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)","    at async Promise.all (index 1)","    at async Promise.all (index 1)","    at async Promise.all (index 0)","    at async execute (/usr/src/app/node_modules/apollo-server-core/dist/requestPipeline.js:205:20)","    at async processGraphQLRequest (/usr/src/app/node_modules/apollo-server-core/dist/requestPipeline.js:148:28)","    at async processHTTPRequest (/usr/src/app/node_modules/apollo-server-core/dist/runHttpQuery.js:220:30)"]}}},{"message":"Cannot return null for non-nullable field Product.promiseNonNullErrorField.","locations":[{"line":1,"column":126}],"path":["allProducts","@","_entities",2,"promiseNonNullErrorField"],"extensions":{"code":"INTERNAL_SERVER_ERROR","exception":{"stacktrace":["Error: Cannot return null for non-nullable field Product.promiseNonNullErrorField.","    at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:594:13)","    at /usr/src/app/node_modules/graphql/execution/execute.js:486:9","    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)","    at async Promise.all (index 1)","    at async Promise.all (index 2)","    at async Promise.all (index 0)","    at async execute (/usr/src/app/node_modules/apollo-server-core/dist/requestPipeline.js:205:20)","    at async processGraphQLRequest (/usr/src/app/node_modules/apollo-server-core/dist/requestPipeline.js:148:28)","    at async processHTTPRequest (/usr/src/app/node_modules/apollo-server-core/dist/runHttpQuery.js:220:30)"]}}},{"message":"Cannot return null for non-nullable field ProductItf.promiseNonNullErrorField","path":["allProducts",0]},{"message":"Cannot return null for non-nullable field ProductItf.promiseNonNullErrorField","path":["allProducts",1]},{"message":"Cannot return null for non-nullable field ProductItf.promiseNonNullErrorField","path":["allProducts",2]}]},{"data":null,"path":["allProducts",1],"errors":[{"message":"Cannot return null for non-nullable field Product.promiseNonNullErrorField.","locations":[{"line":1,"column":126}],"path":["allProducts","@","_entities",0,"promiseNonNullErrorField"],"extensions":{"code":"INTERNAL_SERVER_ERROR","exception":{"stacktrace":["Error: Cannot return null for non-nullable field Product.promiseNonNullErrorField.","    at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:594:13)","    at /usr/src/app/node_modules/graphql/execution/execute.js:486:9","    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)","    at async Promise.all (index 1)","    at async Promise.all (index 0)","    at async Promise.all (index 0)","    at async execute (/usr/src/app/node_modules/apollo-server-core/dist/requestPipeline.js:205:20)","    at async processGraphQLRequest (/usr/src/app/node_modules/apollo-server-core/dist/requestPipeline.js:148:28)","    at async processHTTPRequest (/usr/src/app/node_modules/apollo-server-core/dist/runHttpQuery.js:220:30)"]}}},{"message":"Cannot return null for non-nullable field Product.promiseNonNullErrorField.","locations":[{"line":1,"column":126}],"path":["allProducts","@","_entities",1,"promiseNonNullErrorField"],"extensions":{"code":"INTERNAL_SERVER_ERROR","exception":{"stacktrace":["Error: Cannot return null for non-nullable field Product.promiseNonNullErrorField.","    at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:594:13)","    at /usr/src/app/node_modules/graphql/execution/execute.js:486:9","    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)","    at async Promise.all (index 1)","    at async Promise.all (index 1)","    at async Promise.all (index 0)","    at async execute (/usr/src/app/node_modules/apollo-server-core/dist/requestPipeline.js:205:20)","    at async processGraphQLRequest (/usr/src/app/node_modules/apollo-server-core/dist/requestPipeline.js:148:28)","    at async processHTTPRequest (/usr/src/app/node_modules/apollo-server-core/dist/runHttpQuery.js:220:30)"]}}},{"message":"Cannot return null for non-nullable field Product.promiseNonNullErrorField.","locations":[{"line":1,"column":126}],"path":["allProducts","@","_entities",2,"promiseNonNullErrorField"],"extensions":{"code":"INTERNAL_SERVER_ERROR","exception":{"stacktrace":["Error: Cannot return null for non-nullable field Product.promiseNonNullErrorField.","    at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:594:13)","    at /usr/src/app/node_modules/graphql/execution/execute.js:486:9","    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)","    at async Promise.all (index 1)","    at async Promise.all (index 2)","    at async Promise.all (index 0)","    at async execute (/usr/src/app/node_modules/apollo-server-core/dist/requestPipeline.js:205:20)","    at async processGraphQLRequest (/usr/src/app/node_modules/apollo-server-core/dist/requestPipeline.js:148:28)","    at async processHTTPRequest (/usr/src/app/node_modules/apollo-server-core/dist/runHttpQuery.js:220:30)"]}}},{"message":"Cannot return null for non-nullable field ProductItf.promiseNonNullErrorField","path":["allProducts",0]},{"message":"Cannot return null for non-nullable field ProductItf.promiseNonNullErrorField","path":["allProducts",1]},{"message":"Cannot return null for non-nullable field ProductItf.promiseNonNullErrorField","path":["allProducts",2]}]},{"data":null,"path":["allProducts",2],"errors":[{"message":"Cannot return null for non-nullable field Product.promiseNonNullErrorField.","locations":[{"line":1,"column":126}],"path":["allProducts","@","_entities",0,"promiseNonNullErrorField"],"extensions":{"code":"INTERNAL_SERVER_ERROR","exception":{"stacktrace":["Error: Cannot return null for non-nullable field Product.promiseNonNullErrorField.","    at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:594:13)","    at /usr/src/app/node_modules/graphql/execution/execute.js:486:9","    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)","    at async Promise.all (index 1)","    at async Promise.all (index 0)","    at async Promise.all (index 0)","    at async execute (/usr/src/app/node_modules/apollo-server-core/dist/requestPipeline.js:205:20)","    at async processGraphQLRequest (/usr/src/app/node_modules/apollo-server-core/dist/requestPipeline.js:148:28)","    at async processHTTPRequest (/usr/src/app/node_modules/apollo-server-core/dist/runHttpQuery.js:220:30)"]}}},{"message":"Cannot return null for non-nullable field Product.promiseNonNullErrorField.","locations":[{"line":1,"column":126}],"path":["allProducts","@","_entities",1,"promiseNonNullErrorField"],"extensions":{"code":"INTERNAL_SERVER_ERROR","exception":{"stacktrace":["Error: Cannot return null for non-nullable field Product.promiseNonNullErrorField.","    at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:594:13)","    at /usr/src/app/node_modules/graphql/execution/execute.js:486:9","    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)","    at async Promise.all (index 1)","    at async Promise.all (index 1)","    at async Promise.all (index 0)","    at async execute (/usr/src/app/node_modules/apollo-server-core/dist/requestPipeline.js:205:20)","    at async processGraphQLRequest (/usr/src/app/node_modules/apollo-server-core/dist/requestPipeline.js:148:28)","    at async processHTTPRequest (/usr/src/app/node_modules/apollo-server-core/dist/runHttpQuery.js:220:30)"]}}},{"message":"Cannot return null for non-nullable field Product.promiseNonNullErrorField.","locations":[{"line":1,"column":126}],"path":["allProducts","@","_entities",2,"promiseNonNullErrorField"],"extensions":{"code":"INTERNAL_SERVER_ERROR","exception":{"stacktrace":["Error: Cannot return null for non-nullable field Product.promiseNonNullErrorField.","    at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:594:13)","    at /usr/src/app/node_modules/graphql/execution/execute.js:486:9","    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)","    at async Promise.all (index 1)","    at async Promise.all (index 2)","    at async Promise.all (index 0)","    at async execute (/usr/src/app/node_modules/apollo-server-core/dist/requestPipeline.js:205:20)","    at async processGraphQLRequest (/usr/src/app/node_modules/apollo-server-core/dist/requestPipeline.js:148:28)","    at async processHTTPRequest (/usr/src/app/node_modules/apollo-server-core/dist/runHttpQuery.js:220:30)"]}}},{"message":"Cannot return null for non-nullable field ProductItf.promiseNonNullErrorField","path":["allProducts",0]},{"message":"Cannot return null for non-nullable field ProductItf.promiseNonNullErrorField","path":["allProducts",1]},{"message":"Cannot return null for non-nullable field ProductItf.promiseNonNullErrorField","path":["allProducts",2]}]}]}
--graphql--

With a products array containing 3 items, the second chunk's incremental field contains 3 objects, each with data: null as expected as well as the correct path, but each object's errors array contains 6 errors: one per product returned (3) * 2 different ways the error is formatted. This results 18 errors total (the same 6 errors returned for each item).

There seems to be a second issue here as well which is that a single error is returned two different ways: "INTERNAL_SERVER_ERROR" with stacktrace: "Error: Cannot return null for non-nullable field Product.promiseNonNullErrorField." and "Cannot return null for non-nullable field ProductItf.promiseNonNullErrorField" (in our schema, type Product implements ProductItf).

To Reproduce Steps to reproduce the behavior:

1. Checkout https://github.com/apollographql/router-defer-e2e-tests
2. cd CRA-demo && npm run router:up
3. npm i && npm start
4. Visit http://localhost:3000/error-async-non-nullable-in-deferred-fragment and view response in network tab
5. npm run router:down to bring router down after testing

Expected behavior Still determining the expected output per the spec.

Desktop (please complete the following information):

• OS: [e.g. iOS]
• Version [e.g. 22]

[Geal]

so #1892 fixes it, but it needs a bit more testing. Right now I don't know what happens with errors on requests where the primary and deferred responses are both computed from the same subgraph query. If you see the nullability error appear in two ways, that is because the first one comes from the subgraph, and the second from the router's own nullability check. Right now there's no way to merge both

[alessbell]

Thanks @Geal, just one follow-up here:

I tested against the 1.2.1 release and I'm seeing a difference in the errors returned with/without @defer:

Without @defer

query TestQuery {
  allProducts {
    ...ErrorFragment
    sku
    id
  }
}
fragment ErrorFragment on Product {
  promiseNonNullErrorField
}

With @defer

query TestQuery {
  allProducts {
    ...ErrorFragment @defer
    sku
    id
  }
}
fragment ErrorFragment on Product {
  promiseNonNullErrorField
}

With @defer, there is a second error Cannot return null for non-nullable field ProductItf.promiseNonNullErrorField returned for each item. (Type Product implements ProductItf.)

I don't think this is a problem for the clients, per se, but it seemed like a discrepancy worth noting. Let me know if you'd like me to open a separate issue for this, thanks!

Edit: to clarify, it seems like this is what you could have been referring to here, in which case we're on the same page:

If you see the nullability error appear in two ways, that is because the first one comes from the subgraph, and the second from the router's own nullability check. Right now there's no way to merge both

Just wanted to be sure :) Thanks!