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

Prohibiting SSE via POST breaks graphql-sse usage (since 1.11.0) #224

Closed
dan-lee opened this issue Dec 21, 2021 · 6 comments
Closed

Prohibiting SSE via POST breaks graphql-sse usage (since 1.11.0) #224

dan-lee opened this issue Dec 21, 2021 · 6 comments

Comments

@dan-lee
Copy link
Contributor

dan-lee commented Dec 21, 2021

Since v1.11.0 text/event-stream / SSE requests are forbidden via POST:

POST requests that try to execute Subscription operations will now receive an error and 405 status code. This is not considered a breaking change as SSE is not doable over POST by the specification and was never officially supported.

In helix-flare tests we are using graphql-sse to create a subscription connection. It seems, that graphql-sse uses POST and body to instantiate a connection which is now refused by graphql-helix.

Tests are running fine for helix-flare with [email protected] but are failing for 1.11.0.

Am I wrong here by assuming that this should actually work fine with POST requests, or should I choose a different approach?

@n1ru4l
Copy link
Contributor

n1ru4l commented Dec 22, 2021

As mentioned on discord we never intended graphql-helix to be compatible with the protocol described via graphql-sse. The prior compatibility has been a pure coincidence.

The API for communicating with an SSE endpoint in browsers is EventSource. The EventSource ALWAYS sends a GET request to the SSE endpoint. There is no "official" SSE over HTTP spec yet. We made the assumption that SSE MUST be done over GET, however, this is assumption was wrong as the SSE specification does not really dictate the HTTP verb that should be used. It also seems like using GET for SSE is the common sense people agreed upon.

I don't really have an answer for this. I guess it might make sense to let people configure whether subscriptions are allowed via GET and/or POST?

This should probably also be a discussion on the GraphQL over HTTP repository

@kefniark
Copy link

kefniark commented Dec 22, 2021

Same here, using graphql-sse on few projects and running into the same issue.
I'm a bit confused by such modification, it looks quite arbitrary to me to enforce Get over Post for SSE Subscription, and I thought the whole point of graphql-helix was to be

Framework and runtime agnostic. Use whatever HTTP library you want

Anyway, as a workaround I just did what I was already doing with apollo before, moving the subscription out of the graphql-helix endpoint, and use a dedicated subscription endpoint managed by graphql-sse directly

    // graphql-helix on `/graphql`
    this.app.use(`/graphql`, async (req, res) => {
      const request = { body: req.body, headers: req.headers, method: req.method, query: req.query }

     // ... graphql-helix code

      sendResult(result, res, formatResult)
    })

    // graphql-sse on `/stream`
    const handler = createHandler({ schema, context: (req: Request) => ({ db, pubsub, req }) })
    this.app.use(`/stream`, [...middlewares], handler)

and on client side, the only modification is to change the subscription url.

@dan-lee
Copy link
Contributor Author

dan-lee commented Dec 22, 2021

@kefniark My problem is that I am using Cloudflare Workers and the graphql-sse server handler relies on Node.js Request/Response APIs.

I don't really have an answer for this. I guess it might make sense to let people configure whether subscriptions are allowed via GET and/or POST?

@n1ru4l I think that would be best for now instead of closing the door completely.
I also think this needs a broader discussion to provide a more common standard.

@n1ru4l
Copy link
Contributor

n1ru4l commented Dec 22, 2021

@dan-lee @kefniark PR welcome for lifting the constraint 😇 Sorry for causing the troubles, we discussed it internally ans we dont see why we should not allow it for now, as the official spec is still pretty vague (aka non-existing 😅)!

Slightly related: I would also appreciate any comments and thoughs on graphql/graphql-over-http#167

@dan-lee
Copy link
Contributor Author

dan-lee commented Dec 23, 2021

@n1ru4l I've opened PR #225 :)

@dan-lee
Copy link
Contributor Author

dan-lee commented Mar 19, 2022

Fixed in v1.12.0

@dan-lee dan-lee closed this as completed Mar 19, 2022
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

3 participants