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

graphql-ws example: Invalid query returns "Subscriptions should be sent over WebSocket" #232

Open
gabubarks opened this issue Feb 11, 2022 · 2 comments

Comments

@gabubarks
Copy link

Since v1.11.0, for some reason invalid queries (for example querying a field on an object that does not exist, or forgetting to add a required argument) always result in a PUSH responses from processRequest if the client has text/event-stream in the accepts header.

But if the server does not actually implement PUSH responses, for example when using websockets for subscriptions and implementing that as in the graphql-ws example:

} else {
res.status(422);
res.json({
errors: [new GraphQLError("Subscriptions should be sent over WebSocket.")],
});
}

Then instead of getting a GraphQL validation error you get that "Subscriptions should be sent over WebSocket" message. Notably the version of GraphiQL supplied by graphql-helix has text/event-stream in the accepts header.

My server roughly follows the graphql-ws example, but with some differences like using fastify and having the ws handler in a separate POST-only route, but the issue stems from processRequest returning a PUSH response for GraphQL validation errors when my server does not even implement those otherwise.

To reproduce:

Start the graphql-ws example and execute the (invalid) query in GraphiQL:

mutation { echo(someArgThatDoesNotExist: 1) }

The response will be:

{
  "errors": [
    {
      "message": "Subscriptions should be sent over WebSocket."
    }
  ]
}

Whereas executing the query from a client that only accepts application/json the response will be:

{
    "errors": [
        {
            "locations": [
                {
                    "column": 17,
                    "line": 1
                }
            ],
            "message": "Unknown argument \"someArgThatDoesNotExist\" on field \"Mutation.echo\"."
        }
    ]
}

Is it a bug that GraphQL validation errors are sent by graphql-helix as PUSH responses in the first place?

Or alternatively, is there some option missing in the graphql-ws example to stop the server from sending a PUSH response?

If I implement sendPushResult then obviously the error is correctly returned, but then a client can also query subscriptions over HTTP, which is not my intention.

@n1ru4l
Copy link
Collaborator

n1ru4l commented Jul 9, 2022

I just run this example (https://codesandbox.io/s/github/contrawork/graphql-helix/tree/main/examples/express?initialpath=/graphql&file=/server.ts) and followed your instructions. I received the following response, which seems to be correct:

{
  "errors": [
    {
      "message": "Unknown argument \"someArgThatDoesNotExist\" on field \"Mutation.echo\".",
      "locations": [
        {
          "line": 2,
          "column": 8
        }
      ]
    }
  ]
}

Can you provide a reproduction for your setup?

@gabubarks
Copy link
Author

gabubarks commented Jul 28, 2022

Ah, interesting. You're running the express example which does not allow websocket connections and wouldn't have the issue I'm describing, but my issue should occur with the graphql-ws example. And if I try that in the sandbox then you're right, it doesn't seem to occur there either.

I explicitly tested it with the graphql-helix example to make sure it wasn't some issue with my project, and when I tried it before submitting this issue it happened there too.

I will make a minimal reproduction later and post it here. Thanks for checking it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants