feat: implement GraphQL over WebSocket Protocol by amondnet · Pull Request #686 · Netflix/dgs-framework

amondnet · 2021-10-08T06:06:23Z

Pull request checklist

Please read our contributor guide
Consider creating a discussion on the discussion forum
first
Make sure the PR doesn't introduce backward compatibility issues
Make sure to have sufficient test cases

Pull Request type

Changes in this PR

Describe the new behavior from this PR, and why it's needed
Issue #684

Alternatives considered

Describe alternative implementation you have considered

srinivasankavitha · 2021-10-08T16:31:33Z

Thanks for the PR @amondnet. We will test out the changes. My initial thought is that we need a better name for the module as it can be confusing with the existing websockets implementation that we already support. Having multiple websocket protocols can be very confusing for the user.

paulbakker

Thanks for starting the conversation about this! If this is the protocol that most folks use, we should obviously support that.

A few things we need to think about and/or test for:

Can this be used together with the existing implementation? E.g. can both modules be active at the same time? It does look like it, because the new module is registering on /graphql.
Both WebMVC and Webflux should be supported
Does this conflict with "normal" queries since it's on /graphql? (I think this should work, but we need to make sure).
This should be supported by the GraphQL client as well that was recently added.
Does this impact the Graphiql setup we have?

paulbakker · 2021-10-08T16:50:23Z

+package com.netflix.graphql.dgs.transports.websockets
+
+const val GRAPHQL_TRANSPORT_WS_PROTOCOL = "graphql-transport-ws" // graphql-ws subprotocol
+const val GRAPHQL_SUBSCRIPTIONS_WS_PROTOCOL = "graphql-ws" // subscriptions-transport-ws subprotocol


This is not actually supported by the new module right? It's what the existing websocket implementation does.

paulbakker · 2021-10-08T16:51:09Z

 include("graphql-dgs-pagination")
 include("graphql-dgs-subscription-types")
+include("graphql-dgs-transports-websockets")
+include("graphql-dgs-transports-websockets-autoconfigure")


Is this webflux specific? Can it be used with the webmvc starter as well?

@paulbakker It can be used with webmvc and webflux.

amondnet · 2021-10-10T02:48:28Z

@srinivasankavitha

My initial thought is that we need a better name for the module as it can be confusing with the existing websockets implementation that we already support. Having multiple websocket protocols can be very confusing for the user.

Yes, I agree. Actually I'm not sure how to name the module. graphql-dgs-subscriptions-websockets and graphql-dgs-subscriptions-sse support only Subscriptions.
GraphQL over WebSockets and GraphQL over SSE support all 3 GraphQL operations:Queries, Mutations, Subscriptions. These protocols aims to be standardised and become a part of GraphQL with the help of the foundation’s GraphQL over HTTP work group.

graphql/graphql-over-http#140
graphql/graphql-over-http#163

srinivasankavitha · 2021-10-11T18:05:12Z

I see, I missed the part about having support for Queries and mutations as well with websockets. Given that clarification, I think your existing module name does seem descriptive enough actually.

…

On Sat, Oct 9, 2021 at 7:48 PM amond ***@***.***> wrote: @srinivasankavitha <https://github.com/srinivasankavitha> My initial thought is that we need a better name for the module as it can be confusing with the existing websockets implementation that we already support. Having multiple websocket protocols can be very confusing for the user. Yes, I agree. Actually I'm not sure how to name the module. graphql-dgs-subscriptions-websockets and graphql-dgs-subscriptions-sse support only Subscriptions. GraphQL over WebSockets <https://github.com/enisdenjo/graphql-ws> and GraphQL over SSE support all 3 GraphQL operations:Queries, Mutations, Subscriptions. These protocols aims to be standardised and become a part of GraphQL with the help of the foundation’s GraphQL over HTTP <https://github.com/graphql/graphql-over-http> work group. graphql/graphql-over-http#140 <graphql/graphql-over-http#140> graphql/graphql-over-http#163 <graphql/graphql-over-http#163> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#686 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJ5JPXMFEIKSCTL5CIQ7K6DUGD5INANCNFSM5FS24BHQ> .

srinivasankavitha · 2021-10-11T18:13:12Z

I still need to review and test this PR, will do so this week. On Mon, Oct 11, 2021 at 11:05 AM Kavitha Srinivasan ***@***.***> wrote:

…

I see, I missed the part about having support for Queries and mutations as well with websockets. Given that clarification, I think your existing module name does seem descriptive enough actually. On Sat, Oct 9, 2021 at 7:48 PM amond ***@***.***> wrote: > @srinivasankavitha <https://github.com/srinivasankavitha> > > My initial thought is that we need a better name for the module as it can > be confusing with the existing websockets implementation that we already > support. Having multiple websocket protocols can be very confusing for the > user. > > Yes, I agree. Actually I'm not sure how to name the module. > graphql-dgs-subscriptions-websockets and graphql-dgs-subscriptions-sse > support only Subscriptions. > GraphQL over WebSockets <https://github.com/enisdenjo/graphql-ws> and GraphQL > over SSE support all 3 GraphQL operations:Queries, Mutations, > Subscriptions. These protocols aims to be standardised and become a part > of GraphQL with the help of the foundation’s GraphQL over HTTP > <https://github.com/graphql/graphql-over-http> work group. > > graphql/graphql-over-http#140 > <graphql/graphql-over-http#140> > graphql/graphql-over-http#163 > <graphql/graphql-over-http#163> > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#686 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AJ5JPXMFEIKSCTL5CIQ7K6DUGD5INANCNFSM5FS24BHQ> > . >

srinivasankavitha · 2021-10-12T05:13:08Z

+                )
+            }
+            is Message.PongMessage -> {
+                // TODO: onPong


Nothing to do here, right?

@srinivasankavitha According to the spec, it should call the onPong event handler if exists.

srinivasankavitha · 2021-10-12T05:20:40Z

+                if (session.isOpen) {
+                    session.sendMessage(jsonMessage)
+                }
+            }


Do we need to cleanup the context for this subscription id from the session upon error?
contexts[session.id]?.subscriptions?.remove(id)

srinivasankavitha · 2021-10-14T06:05:12Z

What set up are you using for the client to test the server? Are you using Apollo client with graphql-ws library?

srinivasankavitha · 2021-10-20T17:00:32Z

@amondnet - when you get a chance, could you describe your set up that you used to test this? Do you have an apollo client set up with the graphql-ws library or do you have your own client? If you have any client code that you can share, much appreciated.

amondnet · 2021-10-22T05:38:18Z

@srinivasankavitha This weekend I will share testable code.

srinivasankavitha · 2022-01-25T06:11:37Z

@amondnet - thanks for adding the test and that looks good. The PR overall looks good to me. I tried to test this with a real client with the DGS based on the docs here: https://github.com/enisdenjo/graphql-ws#use-the-client but it does not seem to work after enabling the new module. Have you been able to set up a client to test this end to end? Could you share what you have, if so? Thanks!
For reference, I am trying to set this up with the example app and ui that is part of the dgs framework repo here: https://github.com/Netflix/dgs-framework/tree/master/graphql-dgs-example-shared

michaelboyles · 2022-03-14T00:00:58Z

Happy to test my (closed source) application with an Apollo client if you like. Is there any config necessary on the DGS side, or does the client negotiate the subprotocol as part of the handshake or something?

srinivasankavitha · 2022-03-14T01:20:58Z

Happy to test my (closed source) application with an Apollo client if you like. Is there any config necessary on the DGS side, or does the client negotiate the subprotocol as part of the handshake or something?

That would be really helpful! Unfortunately, the testing will involve cloning and building the forked repo with the changes and publishing a local snapshot that you can use in your DGS app. You will also need to add the new module implementation(project(":graphql-dgs-transports-websockets")) in your DGS app to get this functionality.

amondnet · 2022-04-11T11:25:07Z

@paulbakker

Can this be used together with the existing implementation? E.g. can both modules be active at the same time? It does look like it, because the new module is registering on /graphql.

Yes, we can use together. Websocket uses http only during handshake.
https://developer.mozilla.org/en-US/docs/Web/API/WebSockets_API/Writing_WebSocket_servers#the_websocket_handshake

Both WebMVC and Webflux should be supported

Ok. I will add both.

Does this conflict with "normal" queries since it's on /graphql? (I think this should work, but we need to make sure).

No conflict.

michaelboyles · 2022-07-26T23:58:22Z

Sorry for the delay. I'm still happy to test this if it will help push it through. Unfortunately it's not currently building because of linter errors.

srinivasankavitha · 2022-07-29T04:05:22Z

@michaelboyles - I am now able to test this with Apollo client and have some fixes. Will push those in the next day. I will be doing some more testing and likely get this PR merged after next week. Will post updates here. Apologies again for the delay due to conflicting internal priorities.

srinivasankavitha · 2022-07-29T19:53:23Z

@amondnet - could you please give me access to push a branch to your fork? I have some fixes based on testing your PR.

amondnet · 2022-08-01T01:01:56Z

@srinivasankavitha I have invited you as a collaborator.

amondnet · 2022-08-11T01:42:05Z

https://github.com/Netflix/dgs-framework/tree/feat-subscriptions-graphql-ws

srinivasankavitha · 2022-08-29T17:30:21Z

Merged this PR with additional fixes, implementation for webflux and some refactoring: #1200.

I'll close this one out. Thanks @amondnet for all the contributions for this feature.

feat: implement GraphQL over WebSocket Protocol

125b065

amondnet force-pushed the grapqhl-transport-ws branch from 348cd13 to 125b065 Compare October 8, 2021 06:06

paulbakker reviewed Oct 8, 2021

View reviewed changes

srinivasankavitha requested changes Oct 12, 2021

View reviewed changes

amondnet mentioned this pull request Oct 12, 2021

websocket client use websocket subprotocol header during handshake #691

Merged

9 tasks

dgs client test

a337eab

srinivasankavitha mentioned this pull request Mar 13, 2022

feature: Add support for graphql-ws websocket protocol #933

Closed

amondnet added 2 commits April 12, 2022 02:25

wip: add test

d2c75a2

Merge branch 'master' into grapqhl-transport-ws

0b4ba40

amondnet force-pushed the grapqhl-transport-ws branch from 07d7587 to 3fcc7c9 Compare April 13, 2022 18:14

feat: add reactive websocket handler

f3d0e04

amondnet force-pushed the grapqhl-transport-ws branch from 3fcc7c9 to f3d0e04 Compare April 13, 2022 18:29

amondnet mentioned this pull request Apr 18, 2022

feat: add GraphQL over WebSocket Protocol amondnet/oss#2

Open

amondnet added 5 commits April 20, 2022 12:55

wip: add test

96c5f71

Merge branch 'master' into grapqhl-transport-ws

cff6c73

feat: add reactive websocket handler

15ea97a

feat: add reactive websocket handler

2cb558d

style: format code

8a129cc

amondnet force-pushed the grapqhl-transport-ws branch from 495243c to 8a129cc Compare April 20, 2022 09:26

feat: add reactive websocket handler

d357731

srinivasankavitha mentioned this pull request Aug 8, 2022

Graphql ws fixes #1185

Merged

5 tasks

srinivasankavitha mentioned this pull request Aug 23, 2022

Implement graphql-transport-ws protocol for websocket subscriptions (webmvc & webflux) #1200

Merged

5 tasks

srinivasankavitha closed this Aug 29, 2022

Uh oh!

Conversation

amondnet commented Oct 8, 2021

Pull request checklist

Pull Request type

Changes in this PR

Alternatives considered

Uh oh!

srinivasankavitha commented Oct 8, 2021

Uh oh!

paulbakker left a comment

Choose a reason for hiding this comment

Uh oh!

paulbakker Oct 8, 2021

Choose a reason for hiding this comment

Uh oh!

paulbakker Oct 8, 2021

Choose a reason for hiding this comment

Uh oh!

amondnet Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

amondnet commented Oct 10, 2021

Uh oh!

srinivasankavitha commented Oct 11, 2021 via email

Uh oh!

srinivasankavitha commented Oct 11, 2021 via email

Uh oh!

Uh oh!

srinivasankavitha Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

amondnet Jan 15, 2022

Choose a reason for hiding this comment

Uh oh!

srinivasankavitha Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

srinivasankavitha commented Oct 14, 2021

Uh oh!

srinivasankavitha commented Oct 20, 2021

Uh oh!

amondnet commented Oct 22, 2021

Uh oh!

srinivasankavitha commented Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelboyles commented Mar 14, 2022

Uh oh!

srinivasankavitha commented Mar 14, 2022

Uh oh!

amondnet commented Apr 11, 2022

Uh oh!

michaelboyles commented Jul 26, 2022

Uh oh!

srinivasankavitha commented Jul 29, 2022

Uh oh!

srinivasankavitha commented Jul 29, 2022

Uh oh!

amondnet commented Aug 1, 2022

Uh oh!

amondnet commented Aug 11, 2022

Uh oh!

srinivasankavitha commented Aug 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

srinivasankavitha commented Jan 25, 2022 •

edited

Loading