Fix XS missing 'ctx' into context

[Hugome]

Fix #72

[shawnmcknight]

I'm trying to get myself a bit more familiar with subscriptions and web sockets here, but I have a couple thoughts so far:

                  : {
                      service: connection.$service,
                    };
              },
              subscriptions: {
                onConnect: connectionParams => ({
                  ...connectionParams,
                  $service: this,
                }),
              },

In inspecting what happens here, the onConnect handler will add $service to the graphql context. However, this is not available at connection.$service later, but is instead available at connection.context.$service. Nothing appears to actually make use of this in the rest of the code, but I think this is incorrect and plays into my next thought.

2. Perhaps instead of establishing a new Context instance in the context assignment, it should instead be created when the subscription connects. Something like you already did, but add $ctx to the result of onConnect and then set ctx to connection.context.$ctx in the object construction.

3. I think setting the empty dataLoaders Map in both context objects would be important. If a child resolver had the dataLoader flag set, we would end up failing similarly to ctx being missing.

4. If we create a new Context here, it would seem that the asyncIteratorResolver could use ctx.call instead of broker.call which is a little smoother in my opinion.

5. If the WS logic is bypassing moleculer-web's assignment of the $ internal variables and creation of context, are we ultimately bypassing everything else moleculer-web abstracts away? That is, are we bypassing all of the lifecycles like authentication and authorization? I'm guessing we are, which is concerning. I'm not sure if there's a way to remedy that, but if it can be remedied, is there a manner in which the creation of context can be moved into moleculer-web's functionality in some way, similar to how the context is added to the req by moleculer-web?

As I mentioned in my opening, I'm not terribly familiar with GraphQL subscriptions or web sockets in general as I've not used either, but I wanted to get my initial thoughts on the table. I'll dig in some more.

[shawnmcknight]

If the WS logic is bypassing moleculer-web's assignment of the $ internal variables and creation of context, are we ultimately bypassing everything else moleculer-web abstracts away? That is, are we bypassing all of the lifecycles like authentication and authorization? I'm guessing we are, which is concerning. I'm not sure if there's a way to remedy that, but if it can be remedied, is there a manner in which the creation of context can be moved into moleculer-web's functionality in some way, similar to how the context is added to the req by moleculer-web?

So, after digging in a bit here, that's where onConnect is supposed to come into play. If you need custom authentication/authorization logic, that needs to occur here. It appears that you can intercept http headers for something like a token. So, I'm thinking in order to solve that, a different injectable function would need to be provided that could run as part of the connection lifecycle. Clearly that's out of scope for this PR.

With that in mind, I am more of the opinion that establishing context in onConnect is a more suitable location. Also, as I dig a bit more into how the service broker creates context, it uses the broker's factory method to do it, which I believe is important for custom contexts (we use one in our environment). so it would be risky to not ensure that is used. So maybe @icebob should weigh in here if there is a better way, but I'm thinking ctx = this.broker.ContextFactory.create(this, null, {}, {}); might be the more appropriate way to generate context at this level.

[icebob]

Yes, using the ContextFactory.create is the right way to create context. Moreover, if you don't define Endpoint it maybe causes problems. But you can't because the Context created outside actions. Maybe we can create an internal action like moleculer-web has also a rest internal action which is called with req & res. Although, I'm not familiar with Apollo subscription.

[Hugome]

@shawnmcknight

Perhaps instead of establishing a new Context instance in the context assignment, it should instead be created when the subscription connects. Something like you already did, but add $ctx to the result of onConnect and then set ctx to connection.context.$ctx in the object construction.

I think setting the empty dataLoaders Map in both context objects would be important. If a child resolver had the dataLoader flag set, we would end up failing similarly to ctx being missing.

If we create a new Context here, it would seem that the asyncIteratorResolver could use ctx.call instead of broker.call which is a little smoother in my opinion.

Yep you are totally right with these points.

@icebob

Maybe we can create an internal action like moleculer-web has also a rest internal action which is called with req & res. Although, I'm not familiar with Apollo subscription.

I think it can do the job quite well.

I have change the code, let me know what you think.
It use this idea of @icebob and improve the context (Now you can have access to the query params passed to the upgrade request if your need in you authentification/autorization)
And apply @shawnmcknight improvement on connection.context.$ctx/dataLoaders/asyncIteratorResolver

[icebob]

@shawnmcknight could you help to check it?

[icebob]

What's the status of this PR?

[shawnmcknight]

I apologize for the delay in this review. After the latest batch of changes, shouldn't the constuction of the Apollo Context for the WS connection no longer be looking at the .socket property for $params and $ctx, similar to how its not using it for $service?

[Hugome]

Absolutely right, now fixed

[Hugome]

Sorry I forgot to commit test file, done.

[shawnmcknight]

LGTM! Merging 🤖

[icebob]

Awesome, thanks @shawnmcknight & @Hugome!