apollographql / apollo-server

🌍  Spec-compliant and production ready JavaScript GraphQL server that lets you develop in a schema-first way. Built for Express, Connect, Hapi, Koa, and more.
https://www.apollographql.com/docs/apollo-server/
MIT License
13.78k stars 2.03k forks source link

ws Network Interface Integration #272

Closed DxCx closed 7 years ago

DxCx commented 7 years ago

Hi All,

I've been speaking with @helfer and we got into agreement that i should add a pure websocket integration. before writing anything, i want to post here some mini-design to share my thoughts and hear some feedback.

package name: i was thinking about graphql-server-ws, then the function will be called graphqlWs.

how to use it: So, i don't see any reason why not having the same interface as our other packages, therefore:

import { graphqlWs } from "graphql-server-ws";
import { Server as WsServer } from "ws";
import { Schema } from './schema';
import { graphiqlExpress } from "graphql-server-express";

let app = express();
app.use("/graphiql", graphiqlExpress({
    endpointURL: `ws://localhost:3000/graphql`,
}));
let server = app.listen(3000, () => {
    let wss = new WsServer({ server: server });
    wss.on("connection", graphqlWs((ws) => {
        // The user can basically use any information he pleases too from ws connection object.
        const location = url.parse(ws.upgradeReq.url, true);

        // Multiplex ws connections by path.
        switch ( location.pathname ) {
           case "/graphql": // Same path graphiql is pointed to
               return {
                   context: {},
                   schema: Schema,
               };
           default:
               ws.terminate();
               return undefined;
        }
    });
});

NOTE: One more options is to do the multiplexing internally, but it breaks abit the similarity to other interfaces, and it will look like:

    wss.on("connection", graphqlWs({
      "/graphql": (ws) => {
          // user can still use any information he pleases to make descisions.
          return {
               context: {},
               schema: Schema,
          };
        },
    });

underlaying protocol: so, as i can see it, there is no much difference between using HTTP POST as a transport, or a websocket.

the main difference are:

  1. websocket remains opens therefore you can send more then one request.
  2. because of the above, batching is redundant and there is no reason to support it.
  3. websocket needs the ability to update older response.
  4. client of the websocket should have the ability to cancel/close open request.

to solve this, i suggest to add the following fields to request:

  1. id field - shall be used to respond.
  2. action field - one of the following values:
    • request - default value incase action is missing, will trigger a new request, when used, query, variables and operationName will be used.
    • cancel - cancel or stop updates for Query Id, only id field is being used.
      interface WebsocketGraphqlRequest {
      id: number; // Per socket increasing number
      action?: "request" | "cancel";
      query?: string; // GraphQL Printed Query.
      variables?: any; // GraphQL variables.
      operationName?: string; // GraphQL operationName
      }

also, the response should also add response type and id, so it will look like:

interface WebsocketGraphqlResponse {
  id: number;
  type: "init" | "update" | "error" | "complete"
  payload: any; // standard object (data/errors) incase of init, 
                         // Error for error type
                         // diff object incase of update
                         // empty incase of complete.
}

therefore, if we request a query with 1 result, the server will send:

  1. init + payload
  2. complete

but if we request a query with more then 1 result, the server can send diff updates, and the client can construct the full immutable object over and over.

how diffing will be done: i really liked the deep-diff library, so i'm thinking of using it and keep the diff as is at the moment.

This is more or less what i had on my mind, feel free to comment and give feedback :)

NeoPhi commented 7 years ago

This is something that I'm interested in as well. A few thoughts below, not as fleshed out as yours at the moment.

I'd propose graphql-server-module-ws as I can see this being easily added to any one of the graphql-sever-* implementations, like GraphiQL.

I can't say that I'm a fan of the location semantics for configuration. I'd use graphqlOptions: GraphQLOptions passed into the constructor. The optional async functional version would be passed information about the socket/connection to allow inspection for dynamic options.

I think the request and response messages should be structured the same. Each would have id, type, and payload, where the message type defines the expected payload. Most common payload being query, variables, operationName. This provides a path for future message types and using subscriptions-transport-ws with a little adapter. Some initial types: QUERY_REQUEST, QUERY_RESPONSE, QUERY_CANCEL, and QUERY_CANCELED.

My hope is that code in graphql-server-core such as runQuery could be used to handle most of the low-level handling.

I'm unclear about the use-case for the diffing that you mentioned.

DxCx commented 7 years ago

@NeoPhi Thanks!

i was already thinking about diffing because in the future i want to subscribe / @live / @defer / @stream with it. for that, you need diff capability ;)

wmertens commented 7 years ago

Very nice!

I wanted to point out one extra advantage of using ws for graphql: Since the connection remain live for a long time, you only need to verify the user session once, so that saves on a bunch of database lookups.

NeoPhi commented 7 years ago

Keep in mind you have some other mechanism to invalidate the WebSocket connection should the user's session become invalid for some reason.

DxCx commented 7 years ago

Hi,

I've tried to review how can i support the current subscription manager for the new approach. the best thing i could think about is to add another key named engine to the options. so, when the options returned it will have it

               return {
                   context: {},
                   schema: Schema,
                   engine: new SubscriptionManagerEngine({ pubsub, setupFunctions }),
               };

the idea is to do the new in a static location and not inside a callback which calls every time ofcourse.

this way we can support this approach for legacy users who wants to switch, and in the future add more engines, what do you think about this approach?

CC: @helfer / @stubailo / @NeoPhi / @Urigo / @davidyaha

NeoPhi commented 7 years ago

@DxCx I'm having a little trouble fitting the pieces together. I presume the need for this would be to support @live like directives. How would the engine be accessed or used?

DxCx commented 7 years ago

Well, the engine should export a reactive execution function. Which gets the same parameters as execute but returns an observable interface.

Usig this aproach, in the future we can write an executaion engine that supports the reactive directives, for now only subscription will be supported by the same interface as subscription manager as i exampled above

helfer commented 7 years ago

I like most of your proposal here @DxCx. 🙂

I think it's fine to keep the same interface for the websocket server as we did for the other servers until we see a need to change it.

For the websocket protocol, I agree with @NeoPhi that we should just extend (and maybe harmonize) what we already have for subscriptions. So messages both to and from the server should have { id: <operation id>, type: <START/DATA/COMPLETE/ERROR/STOP>}, payload

START is sent to start an operation (query, mutation or subscription) and contains payload of { query, variables, operationName }.

DATA contains the usual graphql response or an update to previous data in case of multiple results. I don't think this payload should have any fancy diff format at first, it should just contain all the data from the root upwards and overwrite whatever's in the store. I know this isn't necessarily efficient, but I don't believe we should lock ourselves into a diff format this early on.

COMPLETE is sent when no more results are to be expected from the server. This could also be STOP, but I kind of like to keep them separate because there's the small distinction that STOP could also be sent when not all data has been received. We could also roll this type up into DATA if we add some meta field to that message, but keeping it separate leaves us the option of saying that there is no more data even when no data was sent at all. You could imagine this use-case for a subscription, which only sends data on certain events. When it knows for sure no more of those events will happen, it can send COMPLETE without having to send any data. This also aligns more closely with observables.

ERROR is sent when there's an error that didn't happen inside graphql validation or execution. An error also means this particular operation is stopped and no further results will be received.

STOP is sent by the client whenever it isn't interested in more data. I don't think we need to differentiate between stop and cancel, so just one type will do.

Maybe we also need some sort of keepalive / heartbeat message to keep the socket connection open.

As for actually executing the GraphQL query on the server, I assume that this will eventually be taken care of by graphql-js entirely, but for now we should have our own engine (I prefer to call it executor, because that's what we're naming it on the client). That function should have the same function signature as the graphql-js graphql function, but it should return an observable instead of a promise. At the moment we still need to special-case subscriptions though, so we could call it ExecutorWithSubscriptions. I don't think we should call it SubscriptionManagerEngine, because that suggests that it contains only subscriptions, when it fact it also runs queries and mutations.

NeoPhi commented 7 years ago

@DxCx Thinking more about sending diffs, I would suggest using an implementation that adheres to RFC6902 aka JSON Patch. There are many conforming implementations which would give the server and client a choice of the implementation to use.

DxCx commented 7 years ago

nice! thanks @NeoPhi i didn't know this format, well if there is a diffing format for JSON then i totally agree with you this is the one we should use. i want initially to release the first version without any diff, and then later on i'll add it 👍

DxCx commented 7 years ago

hey @helfer i wanted to test compatibility between the existing websocket implementation and this one here, they are not exactly the same:

  1. Message names are not exactly the same:

    'subscription_fail' => `error`
    'subscription_end' => `stop`
    'subscription_data' => `data`
    'subscription_start' => `start`
  2. subscription_start doesn't hold query & variables inside payload, but as a key in the root.

  3. there is no subscription_success for graphql-server-ws

  4. there is no complete (No more data from server) for subscription-transport-ws

how do you want to tackle those? align subscription-transport-ws to the protocol you described here? or make sure graphql-server-ws does exactly as subscription-transport-ws? we can upgrade the server (of subscription-transport-ws) to support both formats, while newer client will use the cleaner alternative..

  1. subscription_start => will be handled as start (moving query & variables into payload), and marking oldProtocol flag.
  2. subscription_success will be sent only if oldProtocol is set.
  3. subscription_end => will be handled as stop
  4. subscription_data => will be handled as data
vladshcherbin commented 7 years ago

@DxCx hey, any news on graphql queries over ws support?

DxCx commented 7 years ago

@VladShcherbin @MrGekko there is already an open PR for the initial protocol changes, you can track it here: https://github.com/apollographql/subscriptions-transport-ws/pull/108

helfer commented 7 years ago

Closing this for now as we've gone into a different direction for now 🤷‍♂️