websocket client use websocket subprotocol header during handshake

[amondnet]

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

• Bugfix
• Feature
• Refactoring (no functional changes, no api changes)
• Build related changes
• Other (please describe):

Changes in this PR

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

WebSocketGraphQLClient client use Sec-WebSocket-Protocol during handshake.

WebSocket RFC defines the use of sub-protocols. During the handshake, the client and server can use the header Sec-WebSocket-Protocol to agree on a sub-protocol, i.e. a higher, application-level protocol to use. The use of a sub-protocol is not required, but even if not used, applications will still need to choose a message format that both the client and server can understand. That format can be custom, framework-specific, or a standard messaging protocol.

If you use subscriptions-transport-ws and protocol is not specified, the subscriptions-transport-ws closes the connection.
https://github.com/apollographql/subscriptions-transport-ws/blob/231c1b0d25b20090033140b83c8a39871d4ed88c/src/server.ts#L143

if (socket.protocol === undefined ||
        (socket.protocol.indexOf(GRAPHQL_WS) === -1 && socket.protocol.indexOf(GRAPHQL_SUBSCRIPTIONS) === -1)) {
        // Close the connection with an error code, ws v2 ensures that the
        // connection is cleaned up even when the closing handshake fails.
        // 1002: protocol error
        socket.close(1002);

        return;
      }

Alternatives considered

Describe alternative implementation you have considered

[srinivasankavitha]

Changes look good to me. We haven't encountered any issues so far due to not specifying the header. Looking at your PR description, sounds like it shouldn't work at all without the header?

[srinivasankavitha]

Thanks for making the updates. One question I still have - are there no other changes needed besides the addition header to support the newer graphql-ws protocol? Seems like we would need an entirely different protocol implementation, no? Or is this PR not intended to address that?

Looking through the existing tests, we still need coverage for :

1. WebsocketGraphQLClient talking to a server with older ws protocol
2. WebsocketGraphQLClient talking to a server running graphql-ws protocol

Something similar to existing SSE test basically - https://github.com/Netflix/dgs-framework/blob/master/graphql-dgs-client/src/test/kotlin/com/netflix/graphql/dgs/client/SSESubscriptionGraphQLClientTest.kt

Would be good to have this coverage before merging this feature.
I am probably not going to be able to add these tests right away, so feel free to add them if you have time. Else, I can do so probably over the next week.

[amondnet]

@srinivasankavitha Yes, this pr does not include implementation of newer graphql-ws protocol. The subprotocol currently being used by dgs-client is graphql-ws. The protocol name is very confusing, isn't it?

Sec-WebSocket-Protocol; websocket subprotocol:

• subscriptions-transport-ws ( older ): graphql-ws
• graphql over websocket; graphql-ws (newer): graphql-transport-ws

This pr makes dgs-client compatible with apollo-subscription + subscriptions-transport-ws. In this case, we only need to add the header. If we do that we will no longer be able to use dgs-subscription-websockets(below v4.9.1) + dgs-client.
Is it okay to break backwards compatibility?

[srinivasankavitha]

Generally, we cannot break backwards compatibility. It sounds like this header was a missing detail in the original websocket implementation implementation using subscriptions-transport-ws? I think we can make the case for this to be a breaking change in this scenario.

[srinivasankavitha]

Regarding this comment - This pr makes dgs-client compatible with apollo-subscription + subscriptions-transport-ws. In this case, we only need to add the header. , to confirm my understanding, are you saying it works with the dgs server running subscriptions-transport-ws but not the apollo server running subscriptions-transport-ws, and therefore this header is needed?

[amondnet]

@srinivasankavitha Yes, dgs-client does not work with the apollo server running subscriptions-transport-ws.
I am getting error onError(graphql.GraphQLException: Server closed the connection unexpectedly)) from dgs-client.
If protocol is not specified, apollo server will close the connection with error code 1002. 1002 indicates that an endpoint is terminating the connection due to a protocol error.

[srinivasankavitha]

Thanks for the explanation and for adding the test against the apollo server. That's a good scenario to add coverage for. Think we will also need to cover the following scenario, right?

1. DGS Websocket Client with header against DGS server with Websocket?

[srinivasankavitha]

@amondet - thanks a lot for adding more test coverage for the changes. I think the example test with the apollo-server - graphql-dgs-client-example-apollo would be a good one to separate out into a separate project rather than have it be part of the framework. We have a separate section in our docs for example projects. Currently it requires docker installation and would be good to avoid that dependency in the framework.

[srinivasankavitha]

The rest of the PR looks really good to me! Thanks for making the changes. I think we can merge once the apollo example is separated out to a different repo. We generally add OSS contribution sto examples in our docs here: https://netflix.github.io/dgs/examples/

[paulbakker]

@amondnet Although I agree with @srinivasankavitha that the test container setup is probably a bit more than we want/need for the framework tests, it does look really interesting. May I suggest writing a blog post about that approach?

[amondnet]

@srinivasankavitha I removed graphql-dgs-client-example-apollo module.

[srinivasankavitha]

Looks good! Thanks so much for all the work and your patience!