Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v4.2 regression: variables: null (etc) should be allowed #7200

Closed
mabuyo opened this issue Nov 24, 2022 · 6 comments · Fixed by #7203
Closed

v4.2 regression: variables: null (etc) should be allowed #7200

mabuyo opened this issue Nov 24, 2022 · 6 comments · Fixed by #7203

Comments

@mabuyo
Copy link
Contributor

mabuyo commented Nov 24, 2022

Description

When running rover dev on a subgraph using Apollo Server 4, I get error [E004].

Steps to reproduce

  1. Clone https://github.com/apollographql-education/lift-off-supergraph-demo-users
  2. On the main branch, run npm i and npm start.
  3. In another terminal, run rover dev --url http://localhost:4002 --name users.
  4. This should work perfectly, can query the router through Sandbox. The main branch uses Apollo Server 3.

Now let's try Apollo Server 4.

  1. Stop any running processes from above.
  2. Checkout the upgrade/as4 branch.
  3. Run npm i and npm start
  4. In another terminal, run rover dev --url http://localhost:4002 --name users.

Expected result

Rover dev works correctly, as in the first AS3 example. The local router runs, able to query the router through Sandbox on port 3000.

Actual result

rover dev --url http://localhost:4002 --name users
WARN: could not detect a schema in the current working directory. to watch a schema, pass the `--schema <PATH>` argument
👂 polling http://localhost:4002/ every 1 second
error: could not run `rover graph introspect http://localhost:4002/` or `rover subgraph introspect http://localhost:4002/`
        error[E004]: HTTP status client error (400 Bad Request) for url (http://localhost:4002/)
        Make sure the endpoint is accepting connections and is spelled correctly

Running rover subgraph introspect http://localhost:4002/ gets me:

error[E004]: HTTP status client error (400 Bad Request) for url (http://localhost:4002/)
        Make sure the endpoint is accepting connections and is spelled correctly

Environment

Rover Info:
Version: 0.10.0
Install Location: /Users/michelle-apollo/.rover/bin/rover
OS: Mac OS 11.6.4 [64-bit]
Shell: /bin/zsh

@dane-stevens
Copy link

This is the error message when i --log trace the end of the rover introspect <endpoint> command:

{
  "errors": [
    {
      "message": "`variables` in a POST body must be an object if provided.",
      "extensions": {
        "code": "BAD_REQUEST",
        "stacktrace": [
          "BadRequestError: `variables` in a POST body must be an object if provided.",
          "    at new GraphQLErrorWithCode (/home/skyf4ll/Documents/Websites/NAPA/napa-api/node_modules/@apollo/server/src/internalErrorClasses.ts:15:5)",
          "    at new BadRequestError (/home/skyf4ll/Documents/Websites/NAPA/napa-api/node_modules/@apollo/server/src/internalErrorClasses.ts:116:5)",
          "    at runHttpQuery (/home/skyf4ll/Documents/Websites/NAPA/napa-api/node_modules/@apollo/server/src/runHttpQuery.ts:169:15)",
          "    at runPotentiallyBatchedHttpQuery (/home/skyf4ll/Documents/Websites/NAPA/napa-api/node_modules/@apollo/server/src/httpBatching.ts:85:30)",
          "    at ApolloServer.executeHTTPGraphQLRequest (/home/skyf4ll/Documents/Websites/NAPA/napa-api/node_modules/@apollo/server/src/ApolloServer.ts:1038:50)",
          "    at processTicksAndRejections (node:internal/process/task_queues:96:5)"
        ]
      }
    }
  ]
}

@dane-stevens
Copy link

The issue seems to be that rover is passing "variables": null in the POST request. Apollo server doesn't like this.

I am able to intercept the POST request through ngrok, then manually change the "variables": null to an empty object (ie. "variables": {} and I get a 200 OK response after that.

@dane-stevens
Copy link

dane-stevens commented Nov 24, 2022

This is caused by the 4.2.0 release of @apollo/server: https://github.com/apollographql/apollo-server/releases/tag/%40apollo%2Fserver%404.2.0

@eunchurn
Copy link

Please remove variables: null in GraphIntrospectQuery.

@EverlastingBugstopper
Copy link
Contributor

Thanks all for all of the wonderful context on this issue. I'm going to transfer it to the apollo-server repository since this is a regression on the server side. Essentially apollo-server is a bit over-eager to reject requests (it is valid to send variables: null in a GraphQL request).

@glasser glasser transferred this issue from apollographql/rover Nov 28, 2022
@glasser glasser changed the title rover dev not working with Apollo Server 4 subgraph v4.2 regression: variables: null (etc) should be allowed Nov 28, 2022
@glasser
Copy link
Member

glasser commented Nov 28, 2022

Right, this was an unintentional regression in v4.2. We added some reasonable type validation in that change but banning variables: null should not have happened (even according to the spec). Going to first improve the spec test suite (graphql/graphql-http#26) and then fix. Thanks all.

glasser added a commit that referenced this issue Nov 28, 2022
In v4.2.0 (#7171) we changed POST handling to be stricter if
`operationName`, `variables`, or `extensions` were provided with a
surprising data type. This was intended to pass more of the optional
recommendations of the GraphQL Over HTTP spec as tested by the
graphql-http audit suite. However, we were overzealous and also banned
providing these parameters as an explicit `null`, which is documented by
the spec as legitimate. (And some clients, such as FIXME, actually send
`variables: null` in practice.)

We added explicit tests for this to the `graphql-http` test suite
(graphql/graphql-http#28) and this commit allows
these `null`s again.

Fixes #7200.
glasser added a commit that referenced this issue Nov 28, 2022
In v4.2.0 (#7171) we changed POST handling to be stricter if
`operationName`, `variables`, or `extensions` were provided with a
surprising data type. This was intended to pass more of the optional
recommendations of the GraphQL Over HTTP spec as tested by the
graphql-http audit suite. However, we were overzealous and also banned
providing these parameters as an explicit `null`, which is documented by
the spec as legitimate. (And some clients, such as FIXME, actually send
`variables: null` in practice.)

We added explicit tests for this to the `graphql-http` test suite
(graphql/graphql-http#28) and this commit allows
these `null`s again.

Fixes #7200.
glasser added a commit that referenced this issue Nov 28, 2022
In v4.2.0 (#7171) we changed POST handling to be stricter if
`operationName`, `variables`, or `extensions` were provided with a
surprising data type. This was intended to pass more of the optional
recommendations of the GraphQL Over HTTP spec as tested by the
graphql-http audit suite. However, we were overzealous and also banned
providing these parameters as an explicit `null`, which is documented by
the spec as legitimate. (And some clients, such as FIXME, actually send
`variables: null` in practice.)

We added explicit tests for this to the `graphql-http` test suite
(graphql/graphql-http#28) and this commit allows
these `null`s again.

Fixes #7200.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants