Fix: Propagate graphql response regardless of the subgraph HTTP status code. #1664

o0Ignition0o · 2022-08-31T16:26:37Z

Fix: Propagate graphql response regardless of the subgraph HTTP status code.

fixes: #1335, fixes #1662

Subgraph service calls used to return an error when the received HTTP status code isn't 200.
There's however no mention in the GraphQL specification that leads us to assume any intent behind the HTTP status code returned by a GraphQL server.

This commit removes our HTTP status code check in the subgraph_service.

…s code. fixes: #1335, #1662 Subgraph service calls used to return an error when the received HTTP status code isn't 200. There's however no mention in the GraphQL specification that leads us to assume anything about the HTTP status code returned by a GraphQL server. This commit removes our HTTP status code check in the `subgraph_service`.

garypen

This all looks good and it's nice that it fixes #1335. I guess we still have to resolve the header propagation part in #1284...

abernix · 2022-08-31T19:04:20Z

what does the gateway do right now?

o0Ignition0o · 2022-08-31T19:53:55Z

what does the gateway do right now?

Good question.

The gateway uses make-fetch-happen under the hood, which invokes fetch AFAICT.

make-fetch-happen's fetch function returns a Promise if the network call succeeded (and if the status code isn't one of the retryable ones, unless we have provided it with options such as an integrity check)

Make fetch happen also explicitely mentions they don't throw on errors:

    // don't reject for http errors, just return them

It's worth noting the gateway's fetcher explicitly disables retries.

@abernix you probably know your way around the gateway better than I do, please correct me if I'm wrong

abernix · 2022-08-31T20:07:12Z

Sounds about right!

I was curious if we'd tried running a query through it using the same subgraphs with failing status code conditions on it and observed the behavior (in case we wrap its errors or anything).

o0Ignition0o · 2022-08-31T20:09:51Z

Thanks for calling it out! I'll spin up a gateway tomorrow and make sure it behaves the same!

o0Ignition0o · 2022-09-01T07:12:37Z

I just tried to compare the router and the gateway's behavior if a subgraph returns an error:

CURL command:

curl --request POST \
    --header 'content-type: application/json' \
    --url http://localhost:<ROUTER_OR_GATEWAY_PORT> \
    --data '{"query":"query Me {\n  me {\n    name\n    username\n  }\n}"}' | jq

gateway response:

{
  "errors": [
    {
      "message": "400: Bad Request",
      "extensions": {
        "code": "INTERNAL_SERVER_ERROR",
        "response": {
          "url": "http://localhost:8000/",
          "status": 400,
          "statusText": "Bad Request",
          "body": {
            "errors": [
              {
                "message": "Cannot query field \"me\" on type \"Query\".",
                "extensions": {
                  "code": "GRAPHQL_VALIDATION_FAILED",
                  "exception": {
                    "stacktrace": [
                      "GraphQLError: Cannot query field \"me\" on type \"Query\".",
                      "    at Object.Field (/Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/subgraphs/users/node_modules/graphql/validation/rules/FieldsOnCorrectTypeRule.js:51:13)",
                      "    at Object.enter (/Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/subgraphs/users/node_modules/graphql/language/visitor.js:301:32)",
                      "    at Object.enter (/Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/subgraphs/users/node_modules/graphql/utilities/TypeInfo.js:391:27)",
                      "    at visit (/Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/subgraphs/users/node_modules/graphql/language/visitor.js:197:21)",
                      "    at validate (/Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/subgraphs/users/node_modules/graphql/validation/validate.js:91:24)",
                      "    at validate (/Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/subgraphs/users/node_modules/apollo-server-core/dist/requestPipeline.js:186:39)",
                      "    at processGraphQLRequest (/Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/subgraphs/users/node_modules/apollo-server-core/dist/requestPipeline.js:98:34)",
                      "    at processTicksAndRejections (node:internal/process/task_queues:96:5)",
                      "    at async processHTTPRequest (/Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/subgraphs/users/node_modules/apollo-server-core/dist/runHttpQuery.js:220:30)"
                    ]
                  }
                }
              }
            ]
          }
        },
        "exception": {
          "stacktrace": [
            "Error: 400: Bad Request",
            "    at RemoteGraphQLDataSource.errorFromResponse (/Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/gateway/node_modules/@apollo/gateway/dist/datasources/RemoteGraphQLDataSource.js:158:21)",
            "    at RemoteGraphQLDataSource.sendRequest (/Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/gateway/node_modules/@apollo/gateway/dist/datasources/RemoteGraphQLDataSource.js:98:34)",
            "    at processTicksAndRejections (node:internal/process/task_queues:96:5)",
            "    at async RemoteGraphQLDataSource.process (/Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/gateway/node_modules/@apollo/gateway/dist/datasources/RemoteGraphQLDataSource.js:73:26)",
            "    at async sendOperation (/Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/gateway/node_modules/@apollo/gateway/dist/executeQueryPlan.js:225:26)",
            "    at async /Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/gateway/node_modules/@apollo/gateway/dist/executeQueryPlan.js:166:49",
            "    at async executeNode (/Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/gateway/node_modules/@apollo/gateway/dist/executeQueryPlan.js:129:17)",
            "    at async /Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/gateway/node_modules/@apollo/gateway/dist/executeQueryPlan.js:29:35",
            "    at async /Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/gateway/node_modules/@apollo/gateway/dist/index.js:124:38",
            "    at async execute (/Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/gateway/node_modules/apollo-server-core/dist/requestPipeline.js:202:20)"
          ]
        }
      }
    }
  ],
  "data": {
    "me": null
  }
}

router response:

{
  "data": null,
  "errors": [
    {
      "message": "Cannot query field \"me\" on type \"Query\".",
      "locations": [],
      "path": null,
      "extensions": {
        "code": "GRAPHQL_VALIDATION_FAILED",
        "exception": {
          "stacktrace": [
            "GraphQLError: Cannot query field \"me\" on type \"Query\".",
            "    at Object.Field (/Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/subgraphs/users/node_modules/graphql/validation/rules/FieldsOnCorrectTypeRule.js:51:13)",
            "    at Object.enter (/Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/subgraphs/users/node_modules/graphql/language/visitor.js:301:32)",
            "    at Object.enter (/Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/subgraphs/users/node_modules/graphql/utilities/TypeInfo.js:391:27)",
            "    at visit (/Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/subgraphs/users/node_modules/graphql/language/visitor.js:197:21)",
            "    at validate (/Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/subgraphs/users/node_modules/graphql/validation/validate.js:91:24)",
            "    at validate (/Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/subgraphs/users/node_modules/apollo-server-core/dist/requestPipeline.js:186:39)",
            "    at processGraphQLRequest (/Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/subgraphs/users/node_modules/apollo-server-core/dist/requestPipeline.js:98:34)",
            "    at processTicksAndRejections (node:internal/process/task_queues:96:5)",
            "    at async processHTTPRequest (/Users/ignition/projects/apollo/router/dockerfiles/federation2-demo/subgraphs/users/node_modules/apollo-server-core/dist/runHttpQuery.js:220:30)"
          ]
        }
      }
    }
  ]
}

The "response" field is missing in the router, but we can follow up on that (I suspect it's because of the way we deal with errors in the include_subgraph_errors plugin). The shape of the error looks good though.

abernix · 2022-09-02T09:25:35Z

The "response" field is missing in the router, but we can follow up on that (I suspect it's because of the way we deal with errors in the include_subgraph_errors plugin). The shape of the error looks good though.

@o0Ignition0o Are we tracking this 😄 follow-up anywhere? (I'm on the right issue now!)

o0Ignition0o · 2022-09-02T09:26:31Z

Oh I see, Opening an issue!

This comment has been minimized.

Sign in to view

o0Ignition0o added 4 commits August 31, 2022 18:59

wip

23d1dd3

test

e281a88

changelog

d1d47c5

o0Ignition0o force-pushed the igni/subgraph_graphql_error_propagation branch from b969803 to d1d47c5 Compare August 31, 2022 17:03

o0Ignition0o marked this pull request as ready for review August 31, 2022 17:04

o0Ignition0o requested review from abernix, garypen and Geal August 31, 2022 17:04

garypen approved these changes Aug 31, 2022

View reviewed changes

o0Ignition0o mentioned this pull request Sep 1, 2022

Router / Gateway: different data returned on subgraph errors #1670

Closed

Geal approved these changes Sep 1, 2022

View reviewed changes

o0Ignition0o merged commit 01014d7 into main Sep 1, 2022

o0Ignition0o deleted the igni/subgraph_graphql_error_propagation branch September 1, 2022 16:35

abernix mentioned this pull request Sep 2, 2022

Status propagation plugin #1662

Closed

o0Ignition0o mentioned this pull request Sep 2, 2022

Error propagation, missing field response #1686

Closed

abernix mentioned this pull request Sep 2, 2022

release: v1.0.0-alpha.1 #1689

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Propagate graphql response regardless of the subgraph HTTP status code. #1664

Fix: Propagate graphql response regardless of the subgraph HTTP status code. #1664

o0Ignition0o commented Aug 31, 2022 •

edited by abernix

Loading

This comment has been minimized.

garypen left a comment

abernix commented Aug 31, 2022

o0Ignition0o commented Aug 31, 2022 •

edited

Loading

abernix commented Aug 31, 2022

o0Ignition0o commented Aug 31, 2022

o0Ignition0o commented Sep 1, 2022

abernix commented Sep 2, 2022

o0Ignition0o commented Sep 2, 2022

Fix: Propagate graphql response regardless of the subgraph HTTP status code. #1664

Fix: Propagate graphql response regardless of the subgraph HTTP status code. #1664

Conversation

o0Ignition0o commented Aug 31, 2022 • edited by abernix Loading

This comment has been minimized.

garypen left a comment

Choose a reason for hiding this comment

abernix commented Aug 31, 2022

o0Ignition0o commented Aug 31, 2022 • edited Loading

abernix commented Aug 31, 2022

o0Ignition0o commented Aug 31, 2022

o0Ignition0o commented Sep 1, 2022

abernix commented Sep 2, 2022

o0Ignition0o commented Sep 2, 2022

o0Ignition0o commented Aug 31, 2022 •

edited by abernix

Loading

o0Ignition0o commented Aug 31, 2022 •

edited

Loading