onSubscribe and use middleware to modify runtime SubscriptionOptions #78

srtucker22 · 2017-02-20T06:38:08Z

TODO:

Make sure all of the significant new logic is covered by tests
Rebase your changes on master so that they can be merged easily
Make sure all tests and linter rules pass
Update CHANGELOG.md with your change

srtucker22 · 2017-02-20T06:53:06Z

This is related to #75 -- while it doesn't create a default context, it instead allows you to override the SubscriptionOptions passed into subscribe much like applyMiddleware overrides the request in networkInterface.

It's a very basic solution to the broad challenge I'm facing -- having some macro controls over subscription creation with apollo-client. A typical example of this is setting authorization context for new subscriptions after a connection is made. We want to make a connection right away without worrying about authorization, and then run all queries through the same logic to establish auth context when subscribing to events.

NeoPhi · 2017-03-03T15:56:41Z

I feel this would be better handled in the glue layer between the networkInterface and the wsClient. An alternative implementation of addGraphQLSubscriptions with hooks would allow you to modify the options being passed to subscribe.

srtucker22 · 2017-03-04T02:32:46Z

so something like?

import { SubscriptionClient } from './client';
import { print } from 'graphql-tag/printer';

// Quick way to add the subscribe and unsubscribe functions to the network interface
export function addGraphQLSubscriptions(networkInterface: any, wsClient: SubscriptionClient): any {
  return Object.assign(networkInterface, {
    subscribe(request: any, handler: any): number {
      if(networkInterface.onSubscribe) {
        request = networkInterface.onSubscribe(request);
      }
      
      return wsClient.subscribe({
        query: print(request.query),
        variables: request.variables,
        context: request.context,
      }, handler);
    },
    unsubscribe(id: number): void {
      wsClient.unsubscribe(id);
    },
  });
}

At the very least I think the subscribe function should be able to pass context.

Just to be clear about the use case I'm running into, which I don't think is that uncommon:
I want to be able to login/logout and have that auth context passed to my subscriptions.
If using apollo-client or react-apollo, there is no way of affecting context.
you can pass context when first connecting to socket, but it would be inefficient to open/close connection every login/logout.

Even if we exposed context in subscribe, you wouldn't want to have to explicitly pass context in every subscribe or subscribeToMore call if context is something high level like a JWT token. I also imagine there are other things someone might need to do before every subscribe call just like it's import to have middleware before a gql request sent with networkInterface.

Urigo · 2017-03-08T15:25:54Z

@srtucker22 looks really good and I agree that this is a common use case that would be beneficial to support.

If we choose to support it then it will be easier then @NeoPhi solution overriding the subscribe method.

Maybe a better way of supporting it would be to add another client life-cycle hook called onSubscribe and this transport will call this hook right after this and extend the options with the return value of the hook.

That means that the hooks can return only the part of the options that it want to override or extend and not making it necessary to build the whole options object again from scratch.

In the client there is already lifecycle hook event emitter we can use for that, here is an example - https://github.com/apollographql/subscriptions-transport-ws/blob/master/src/client.ts#L147-L157

Would love to hear both of your thoughts..

Urigo · 2017-03-08T18:11:33Z

also, maybe the best thing will be to make it as similar as possible to that existing API - http://dev.apollodata.com/core/network.html#networkInterfaceMiddleware

srtucker22 · 2017-03-13T12:49:02Z

Hey @Urigo, good to hear from you!

I'm all for networkInterfaceMiddleware style middleware. Are you thinking it would look something like this?

SubscriptionClient.use([{
  applyMiddleware(options, next) {
    // modify options for SubscriptionClient.subscribe here
    next();
  }
}]);

My only concern with adding middlware this way (as it's written in apollo-client) is it introduces promises and async. I like how in the current setup, subscribe is a synchronous function that returns the subscription ID or throws. I think it would be best to still return an ID synchronously, but I wouldn't want to use promises in the background, especially when things throw errors. We could restrict middleware funcs to sync only, but that kind of defeats the purpose of using next(). Maybe can be overcome with iterators and generators?

srtucker22 · 2017-03-14T05:01:47Z

ok so this latest push will use either client.onSubscribe or client.use, but both are syncronous and so very basic.

i can forsee use cases where someone would like to run an async function to modify options before subscribing, and this doesn't address that. but anything with async middleware before subscribe will essentially demand breaking changes whereas these changes don't.

happy to modify more if we want to go down the breaking changes path :)

Urigo · 2017-03-17T13:03:15Z

Thank you for your recent changes.
I think that adding support for async is important, and I think it might be simpler.
Can you please explain why the changes will break the API?

Thanks @srtucker22

srtucker22 · 2017-03-21T14:43:10Z

so i guess if you do it this way (latest commit) the changes aren't breaking per se -- before this change you already had to catch errors (1) when subscribing and (2) in the handler, but now some of those errors (like having a query) will also be caught in the handler where they weren't before, like if your middleware accidentally deleted the query.

however, this code has an issue --> if you call client.unsubscribe before client.subscribe finishes (as was written in a few tests), you will not unsubscribe because unsub will run first in the queue / run before the graphqlSubId is created and is necessary to call unsubscribe on the server. not sure how you want to tackle this

apologies for being touch and go. i've been away in Bali these past few weeks and will be traveling for another couple months or so. just want to give you a heads up i might not respond for a few days in between coms

srtucker22 · 2017-03-22T02:59:13Z

ok, this version tracks unsubs and waits to sendMessage for unsubscribe if it comes before sendMessage for init for a given sub. doesn't feel elegant, but gets the job done

…ns-transport-ws

srtucker22 · 2017-04-01T02:55:24Z

a little cleaner now. lemme know what you think

in tokyo now so better internet sitch

NeoPhi · 2017-04-03T14:52:05Z

src/client.ts

-    const { query, variables, operationName, context } = options;
+  public subscribe(opts: SubscriptionOptions, handler: (error: Error[], result?: any) => void) {
+    // this.eventEmitter.emit('subscribe', options);
+    this.checkSubscriptionParams(opts, handler);


I would just rely on the this. applyMiddlewares handling instead of checking it twice. This also avoids the case that sometimes the handler is called with the error and not.

so if we change this, it will break the following tests as they currently stand:

should throw an exception when query is not provided

should throw an exception when query is not valid

because those errors currently throw outside the handler

i'm all for deferring all error handling to the handler (aside from throwing if there isn't a handler), but was trying to make this feature without making breaking changes. but i agree it's cleaner -- so if you confirm this is the route to go, i'm happy to make the change

NeoPhi · 2017-04-03T14:54:03Z

src/client.ts

+    this.applyMiddlewares(opts).then(options => {
+      try {
+        this.checkSubscriptionParams(options, handler);
+      } catch (e) {


Feels like this should just be a catch on the promise to catch any issues.

NeoPhi · 2017-04-03T14:55:13Z

src/client.ts

+        this.checkSubscriptionParams(options, handler);
+      } catch (e) {
+        handler([e]);
+        this.unsubscribe(subId);


We should unsubscribe before calling the handler.

NeoPhi · 2017-04-03T14:55:40Z

src/client.ts

+      setTimeout( () => {
+        if (this.waitingSubscriptions[subId]) {
+          handler([new Error('Subscription timed out - no response from server')]);
+          this.unsubscribe(subId);


We should unsubscribe before calling the handler.

NeoPhi · 2017-04-03T15:01:58Z

src/client.ts

@@ -165,9 +172,14 @@ export class SubscriptionClient {
  }

  public unsubscribe(id: number) {
+    if (!this.subscriptions[id] && !this.waitingSubscriptions[id]) {
+      this.waitingUnsubscribes[id] = true;


I'm not sure what waitingUnsubscribes is giving us. Each subscribe call is going to generate a new id and I don't see this being used elsewhere to set a timeout to ensure that it was cleaned up.

the issue waitingUnsubscribes tries to resolve is if you unsubscribe before the middleware promise resolves. you're supposed to be able to call client.unsubscribe synchronously (as it stands currently -- and i'd like to keep it this way), but if we remove waitingUnsubscribes, the following would fail:

should trigger onUnsubscribe when client unsubscribes

SUBSCRIPTION_END gets sent before SUBSCRIPTION_START and so we don't actually properly unsubscribe. we could check for an unsubscribe call before we send SUBSCRIPTION_START which would prevent a needless START immediately followed by END, but I didn't want to break the existing tests (again was trying to keep this a non-breaking feature addition). let me know if this makes sense and which direction you'd like me to go forward with

helfer · 2017-04-18T17:57:09Z

@Urigo could you take another look at this? Thanks! 🙂

Urigo · 2017-04-18T22:57:32Z

hi, I'm sorry for the delay but we have a huge important change here that should land soon and after that I will address the rest of the PRs.
@srtucker22 also be aware of that change because probably after there will be a need for rebase and changes

srtucker22 · 2017-04-19T06:58:17Z

thanks for the heads up -- looks cool. also looks like i'm gonna have to rewrite my subscription tutorial :)

giautm · 2017-04-29T17:51:53Z

src/client.ts

+  public applyMiddlewares(options: SubscriptionOptions): Promise<SubscriptionOptions> {
+    return new Promise((resolve, reject) => {
+      const queue = (funcs: MiddlewareInterface[], scope: any) => {
+        const next = () => {


Can be reject with next(error)?

Following lines should be change to:

const queue = (funcs: MiddlewareInterface[], scope: any) => { const next = (error) => { if (error) { reject(error); } else { if (funcs.length > 0) { const f = funcs.shift(); if (f) { f.applyMiddleware.apply(scope, [options, next]); } } else { resolve(options); } } }; next(); };

And this line update to:

}).catch(e => handler([e]));

giautm · 2017-04-29T18:01:16Z

src/client.ts

+  public applyMiddlewares(options: SubscriptionOptions): Promise<SubscriptionOptions> {
+    return new Promise((resolve, reject) => {
+      const queue = (funcs: MiddlewareInterface[], scope: any) => {
+        const next = () => {


Following lines should be change to:

const queue = (funcs: MiddlewareInterface[], scope: any) => { const next = (error) => { if (error) { reject(error); } else { if (funcs.length > 0) { const f = funcs.shift(); if (f) { f.applyMiddleware.apply(scope, [options, next]); } } else { resolve(options); } } }; next(); };

And this line update to:

}).catch(e => handler([e]));

giautm · 2017-05-05T09:31:38Z

Is this ready for merge, wait too long. :(

srtucker22 · 2017-05-15T02:55:31Z

SO, here is my proposal with the latest:

Everything works, but I've changed up a few tests since we're in the spirit of major code overhaul:

All query, variables, and operationName related errors are thrown in the handler. Since we're giving users the option to use asynchronous middleware to change all of these options, they should really only get checked once they have been updated. Plus, I think it's much more intuitive that all errors get handled by the handler! (changed tests for throwing exceptions to queries to get handled by handler)
Because client.subscribe returns the opId synchronously, but sends the first message asynchronously (after middleware), there is a period in between where the user might try and unsubscribe before the first message is sent. I think the most reasonable thing to do if someone tries to unsubscribe before they've ever actually sent a subscription message to the server is to just cancel the subscription message altogether. In this scenario, the GQL_START message won't make it to the queue (1st changed test), and on the server, onUnsubscribe won't get triggered (2nd changed test).

Let me know what's good!

Urigo · 2017-05-16T13:35:27Z

@srtucker22 thank you so much for an amazing work and your patience keeping this PR updated.

srtucker22 · 2017-05-16T18:39:08Z

no worries!

DxCx · 2017-05-19T11:56:08Z

src/server.ts

@@ -423,7 +424,7 @@ export class SubscriptionServer {
              query: parsedMessage.payload.query,
              variables: parsedMessage.payload.variables,
              operationName: parsedMessage.payload.operationName,
-              context: Object.assign({}, isObject(initResult) ? initResult : {}),
+              context: Object.assign({}, isObject(initResult) ? initResult : {}, parsedMessage.payload.context),


Why is context sent over the network?
@Urigo @helfer @Stubilo - Thats a serious security issue, you shall not release the package with that

quadsurf · 2017-08-26T12:40:00Z

This is still not working for me... I'm still getting "Insufficient Permissions" when I have permissions enabled... however, it works fine when permissions are disabled. I have even hard coded in the server auth token, and it still doesn't work.

There seems to be some property naming inconsistency in this library:

some use:
connectionParams: {
Authorization: getToken()
}
some use:
connectionParams: {
authToken: getToken()
}

So is the correct property name "Authorization" or "authToken"? Either way, I've tested both, and both fail.

HERE ARE MY VERSIONS
react-native@latest
subscriptions-transport-ws@0.8.2
react-apollo@1.4.8

HERE IS MY ERROR
Unhandled GraphQL subscription error Array [
Object {
"code": 3008,
"locations": Array [
Object {
"column": 7,
"line": 4,
},
],
"message": "Insufficient Permissions",
"path": Array [
"User",
"node",
"id",
],
"requestId": "subscription:cj6...:1",
},
Object {
"code": 3008,
"locations": Array [
Object {
"column": 7,
"line": 5,
},
],
"message": "Insufficient Permissions",
"path": Array [
"User",
"node",
"type",
],
"requestId": "subscription:cj6...:1",
},
]

ISSUES I'VE REVIEWED
#171 (comment) (the one recommended by @lewisblackwood)
https://github.com/graphcool/feature-requests/issues/317
#102

* log server events to logFunction * update changelog

srtucker22 mentioned this pull request Mar 3, 2017

authorizing subscriptions -- set a default context for client #75

Closed

Urigo mentioned this pull request Mar 8, 2017

Expose connectionParams of subscriptions-transport-ws to subscribeToMore apollographql/apollo-client#1355

Closed

onSubscribe and use middleware

966480f

srtucker22 changed the title ~~beforeSubscribe to ClientOptions to modify runtime SubscriptionOptions~~ onSubscribe and use middleware to modify runtime SubscriptionOptions Mar 14, 2017

srtucker22 added 2 commits March 21, 2017 21:37

middleware with async

a140286

need to fix race condition with unsub

513c4f3

with unsub hotfix

b28b3bc

ervorel mentioned this pull request Mar 30, 2017

Middleware for connection params #102

Closed

srtucker22 added 3 commits April 1, 2017 11:38

cleaner error testing

4a5b473

cleanup

b575639

Merge branch 'master' of https://github.com/apollographql/subscriptio…

12e6eb4

…ns-transport-ws

NeoPhi reviewed Apr 3, 2017

View reviewed changes

giautm mentioned this pull request Apr 29, 2017

[FEATURE REQUEST] connectionParams accept async function or a function return promise. #113

Closed

giautm reviewed Apr 29, 2017

View reviewed changes

giautm suggested changes Apr 29, 2017

View reviewed changes

srtucker22 added 3 commits April 30, 2017 03:49

merged

2b7a9df

add context to baseParams

e2b27c8

hotfix sendMessage after adding subscription

07f7b26

srtucker22 mentioned this pull request May 1, 2017

Cannot read property 'handler' of undefined at WebSocket.client.onmessage #93

Closed

maybe fixes race condition in #93

f4ce20c

srtucker22 added 2 commits May 14, 2017 22:24

merged with middleware and unsub enhancements

f882b36

all suboption errors handled by handler

3cbcbac

update changelog

278524c

Urigo merged commit 9889859 into apollographql:master May 16, 2017

Urigo mentioned this pull request May 16, 2017

WIP: Graphql Transport WS Protocol #108

Merged

5 tasks

mistic mentioned this pull request May 19, 2017

API Docs #134

Merged

DxCx reviewed May 19, 2017

View reviewed changes

mistic mentioned this pull request May 19, 2017

refact(NA): onsubscribe middleware pr #138

Merged

baconz pushed a commit to PhiloInc/subscriptions-transport-ws that referenced this pull request Apr 24, 2018

log server events to logFunction (apollographql#78)

adf5727

* log server events to logFunction * update changelog

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

onSubscribe and use middleware to modify runtime SubscriptionOptions #78

onSubscribe and use middleware to modify runtime SubscriptionOptions #78

srtucker22 commented Feb 20, 2017 •

edited

Loading

srtucker22 commented Feb 20, 2017

NeoPhi commented Mar 3, 2017

srtucker22 commented Mar 4, 2017

Urigo commented Mar 8, 2017

Urigo commented Mar 8, 2017

srtucker22 commented Mar 13, 2017

srtucker22 commented Mar 14, 2017

Urigo commented Mar 17, 2017

srtucker22 commented Mar 21, 2017 •

edited

Loading

srtucker22 commented Mar 22, 2017

srtucker22 commented Apr 1, 2017

NeoPhi Apr 3, 2017

srtucker22 Apr 4, 2017

NeoPhi Apr 3, 2017

NeoPhi Apr 3, 2017

NeoPhi Apr 3, 2017

NeoPhi Apr 3, 2017 •

edited

Loading

srtucker22 Apr 4, 2017

helfer commented Apr 18, 2017

Urigo commented Apr 18, 2017

srtucker22 commented Apr 19, 2017

giautm Apr 29, 2017

giautm Apr 29, 2017

giautm Apr 29, 2017

giautm commented May 5, 2017

srtucker22 commented May 15, 2017

Urigo commented May 16, 2017

srtucker22 commented May 16, 2017

DxCx May 19, 2017

quadsurf commented Aug 26, 2017 •

edited

Loading

onSubscribe and use middleware to modify runtime SubscriptionOptions #78

onSubscribe and use middleware to modify runtime SubscriptionOptions #78

Conversation

srtucker22 commented Feb 20, 2017 • edited Loading

srtucker22 commented Feb 20, 2017

NeoPhi commented Mar 3, 2017

srtucker22 commented Mar 4, 2017

Urigo commented Mar 8, 2017

Urigo commented Mar 8, 2017

srtucker22 commented Mar 13, 2017

srtucker22 commented Mar 14, 2017

Urigo commented Mar 17, 2017

srtucker22 commented Mar 21, 2017 • edited Loading

srtucker22 commented Mar 22, 2017

srtucker22 commented Apr 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NeoPhi Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helfer commented Apr 18, 2017

Urigo commented Apr 18, 2017

srtucker22 commented Apr 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giautm commented May 5, 2017

srtucker22 commented May 15, 2017

Urigo commented May 16, 2017

srtucker22 commented May 16, 2017

Choose a reason for hiding this comment

quadsurf commented Aug 26, 2017 • edited Loading

srtucker22 commented Feb 20, 2017 •

edited

Loading

srtucker22 commented Mar 21, 2017 •

edited

Loading

NeoPhi Apr 3, 2017 •

edited

Loading

quadsurf commented Aug 26, 2017 •

edited

Loading