Subscription should guarantee at least one gqlData response to a gqlStart request

[alankm]

The Problem

According to what I've read, it seems that if a client sends a "gqlStart" to the server then the server should guarantee at least one "gqlData" response. Currently this doesn't happen, as "gqlData" responses are only sent when the server logic explicitly calls "Subscription.SendData()", which may never happen if the subscribed content isn't being updated.

I have two proposals for fixing this issue:

Solution 1: execute the query within this library within the "AddSubscription" function.

This would leave the exported functions of the library unchanged, but denies the server logic the ability to customize behaviour such as adding objects to the graphql query context.

Solution 2: allow or require the server logic to provide OnConnect callback functions

If the server logic could provide callback functions to the SubscriptionManager then the solution is entirely in the hands of the developer. As one possible way this could be approached, this would be near-identical to the existing implementation, except that instead of being created with a single argument, it would now also need a callback argument.

Before:

NewSubscriptionManager(schema *graphql.Schema) SubscriptionManager

After:

NewSubscriptionManager(schema *graphql.Schema, onConnectCallback func(s *Subscription)) SubscriptionManager

It would even be possible to make this change to the existing implementation of SubscriptionManager directly without breaking any existing code by accepting a variadic number of callbacks, as the callback definitions would then be completely optional.

Alternative:

NewSubscriptionManager(schema *graphql.Schema, onConnectCallbacks ...func(s *Subscription)) SubscriptionManager

[Jannis]

You're referring to https://github.com/apollographql/subscriptions-transport-ws/blob/master/PROTOCOL.md#gql_data I assume. I agree, this is something we are not doing yet but should do.

[ccamel]

@Jannis Won't the solution 2 proposed by @alankm be fixed by b829e73 in branch subscription-listener? Do you plan to merge it soon?

[alankm]

That does look like it would solve my issue. I hope it can be merged as soon as possible.

[Jannis]

The reason I haven't merged the subscription-listener branch yet is that there already is way to achieve the same thing: by wrapping the default subscriptionManager in your own SubscriptionManager interface implementation and adding the "on connect" functionality to the AddSubscription wrapper function.

I can see that this is not obvious and certainly not the prettiest solution. The subscription-listener branch provides a more obvious way, so yes, let's move that branch forward.

Would it be reasonable to make it mandatory for listeners to return an initial gqlData message back from SubscriptionAdded? That would make it harder for users of graphqlws to violate the GraphQL over WS spec. @ccamel, @alankm: What do you think?

[ccamel]

@Jannis I get your point. As the SubscriptionManager type is actually an interface, we could easily decorate it to introduce the functionality of "changes listening". However, I think it would greatly help users to have it available and ready to use in that library.

After having a good time on the subject, I think it's fine to have the SubscriptionAdded callback to have a return value (a *DataMessagePayload).

We can also accept the implementator to return a nil value, for the sake of convenience, in such case the graphql payload returned (through the frame gqlData) will be data: null. Indeed, we may have the use case where the first data returned is empty (for instance if you want to subscribe to a Kafka topic which is empty).

[Jannis]

@ccamel That sounds like a good approach. It will force people to think about why they'd return nil and we can add a doc string pointing at the spec.

[alankm]

@Jannis & @ccamel I'm all for this solution.