Export a helper function to make data sources executable

[jlengstorf]

UPDATE 2017-12-19: After a lot of discussion we determined that it's a better idea to export a helper to make data sources executable rather than make each data source self-contained. See gramps-graphql/gramps#26 (comment) for the proposed helper function.

---

@jlengstorf commented on Mon Nov 20 2017

After a discussion today with @schickling, @kbrandwijk, and @nikolasburk about how GrAMPS data sources will fit with external tools (such as Graphcool and qewl), a couple things came up:

1. GrAMPS data sources should be exporting an executable schema, and not the typeDefs/resolvers/mocks as I was previously pushing for
2. GrAMPS data sources should not require the context to be attached externally (meaning there should be no intermediate step to attach a data source context to its resolvers)

(@ecwyne and @corycook, you're going to say, "I told you so.") 😄

[schickling]

2. GrAMPS data sources should not require the context to be attached externally (meaning there should be no intermediate step to attach a data source context to its resolvers)

I'm not sure about this. Probably needs more use cases to validate this.

[jlengstorf]

@schickling I should probably clarify.

What I mean is that the internal data access in GrAMPS is currently assigned to the model prop, which is exported in the data source.

Right now, the GraphQL server needs to take the GrAMPS schema + the GrAMPS context, or the data source won't work.

In many cases, this isn't necessary because the context is exported with the data source already — so why add the extra step unless we need to modify the model externally? (I think there are cases where that's necessary, but in looking at what we've built at IBM, and the use cases other devs have put forward, I believe this would be an exception, not the rule.)

So what I'm proposing is this:

Current Implementation

index.js

// Imports omitted for brevity

export default {
  namespace: 'XKCD',
  model: new Model({ connector: new Connector() }),
  typeDefs,
  resolvers,
  mocks,
};

resolvers.js

export default {
  Query: {
    getLatestComic: (_, __, context) =>
      new Promise((resolve, reject) => {
        context.XKCD
          .getLatestComic()
          .then(resolve)
          .catch(reject);
      }),
    // ...
  },
  // ...
};

Proposed Implementation

index.js

// Imports omitted for brevity

export default {
  namespace: 'XKCD', // this may not be necessary anymore
  schema, // export an executable schema instead of typeDefs/resolvers/mocks
};

Resolvers

+ import Model from './model';

+ const model = new Model({ connector: new Connector() });

  export default {
    Query: {
      getLatestComic: (_, __, context) =>
        new Promise((resolve, reject) => {
-         context.XKCD
+         model
            .getLatestComic()
            .then(resolve)
            .catch(reject);
        }),
      // ...
    },
    // ...
  };

---

In cases where the context needs to be modified outside the data source, GrAMPS has an extraContext prop that can be used to pass whatever is required into data sources.

Given that the intent for GrAMPS data sources is to be entirely isolated (e.g. their internal function should not rely on any external code, with the exception of headers/tokens, which can be passed through extraContext), can you think of a use case where encapsulating the core model would cause problems?

[ecwyne]

The only use case I see this not handling at this point is per-request caching (eg. dataloaders)

To my knowledge the only way to do this is creating a new context with each request.

[jlengstorf]

@ecwyne I think we can get around that by creating a new instance of the connector per-request. There's an issue open to chase down specifics in gramps-graphql/gramps-express#47

[ecwyne]

@jlengstorf That's exactly my point. The datasource itself cannot "create a new Instance of the connector per-request"

If the only thing a data source provides is an executable schema then there is no way to update the context per-request. There must be additionalContext or something like that for this case.

[jlengstorf]

Oh, good call. So probably needs to still provide a context in addition to the executable schema.

@kbrandwijk How is qewl handling context right now?

If you have other solutions, I'd be happy to hear them, but it looks like we'll need something like this:

import MyDataSource from '@gramps/data-source-mydata';

// ... setup here ...
graphql.use(
  schema(MyDataSource.schema),
);

// Add the data source context to the qewl context
graphql.use(
  (req, res, next) => {
    req.qewl.context = {
      ...req.qewl.context,
      [MyDataSource.namespace]: MyDataSource.context,
    };

    next();
  }
);

[kbrandwijk]

@jlengstorf That's exactly how I'm doing it now. The req from Express is passed in as context, so any Express middleware can modify the context by modifying the req. The qewl part of the req object is for internal processing of the schemas by qewl, you can just add it directly to req instead:

graphql.use(
  (req, res, next) => {
    req = {
      ...req,
      [MyDataSource.namespace]: MyDataSource.context,
    };

    next();
  }
);

That will make it available in any resolver using context.XCKD.

I intentionally use the entire req, instead of just a sub key, so things like express-jwt will also function as normal, by adding req.user that will be available as context.user inside the resolvers.

So maybe the datasource can export an Express middleware function for context?

export default {
  namespace: 'XKCD', // this may not be necessary anymore
  schema, // export an executable schema instead of typeDefs/resolvers/mocks,
  context: (req, res, next) => { ... }
};

Or, if that's an easier syntax for datasources, just a context: req => { ... } with a gramps wrapper helper:

graphql.use(schema(myDatasource.schema))
graphql.use(grampsContext(myDatasource.context)) 

[jlengstorf]

@schickling Excellent.

I think the simpler format would work well:

export default {
  namespace: 'XKCD',
  schema,
  context: req => { /* will `new Model(...)` work with DataLoader here? */ },
};

[jlengstorf]

Sorry, meant to tag @kbrandwijk and my brain shorted out. 😀

[kbrandwijk]

@jlengstorf The only trade-off is needing that wrapper.

[jlengstorf]

@kbrandwijk For data sources that don't need access to the full context, we could potentially just create a context middleware helper for gramps-express that would do something like this:

const grampsContext = dataSource => (req, res, next) => ({
  req = {
    ...req,
    [dataSource.namespace]: typeof dataSource.context === 'function'
      ? dataSource.context(req)
      : dataSource.context,
});

And that would make it possible to export a plain object as the context or a wrapper.

[jlengstorf]

I started work on this, and I'm remembering now why I wanted GrAMPS to build the executable schema instead of doing it per-data-source:

1. There are settings that tend to be gateway-wide, such as whether or not to add mock resolvers, that will need to be set per-data source now. The only way I can think to allow the gateway to control that is with env vars, which seems like a confusing way to manage that.

2. GrAMPS allows gateway-wide settings to pass through for makeExecutableSchema and addMockFunctionsToSchema, which we'll no longer be able to do if we're exporting executable schemas.

@kbrandwijk @schickling I think a solution that still allows flexibility and control for mocking might be to require all GrAMPS data sources to be run through gramps() before being considered usable:

import grampsExpress from '@gramps/gramps-express';
import MyDataSource from '@gramps/data-source-mydata';

// ... qewl setup here ...

// Convert any GrAMPS data sources to a single executable schema.
graphql.use(
  grampsExpress({
    dataSources: [MyDataSource],
    enableMockData: shouldMockData, // <-- assume this is defined in the setup
  })
});

// Use the executable schema generated by `gramps()`
graphql.use(
  req => schema(req.gramps.schema), // Does this work?
);

// Add the GrAMPS context
graphql.use(
  (req, res, next) => {
    req = {
      ...req,
      ...req.gramps.context, // This is optional; could use context.gramps.context.namespace.method
    };

    next();
  }
);

// ...

grampsExpress() attaches an executable schema and context (and error formatting) to req.gramps, which would make them accessible to all middleware later in the chain.

It's an extra hoop to jump through on setup, but it gives the developer finer-grained control without having to pass a bunch of env vars for their data sources.

Thoughts?

[kbrandwijk]

@jlengstorf I would just like to throw something out here that is in the complete opposite direction. What if the datasource would export a function, instead of an object with properties directly? And you would move the generation of the executable schema to the datasource too?

export default function dataSource(grampsOptions) {
  const executableSchema = makeExecutableSchema(schema)
  if (grampsOptions.enableMockData) {
    addMockFunctionsToSchema(executableSchema)
  }

  return { 
    namespace: 'XKCD',
    schema: executableSchema,
    context: req => { /* will `new Model(...)` work with DataLoader here? */ },
  }
};

Then use:

graphql.use(schema(
  myDataSource({ enableMockData: true })
))

You could reuse the options object passed in to the dataSource, so you get both fine-grained, per datasource control, and the options to use the same options for all dataSources.

This solutions would also push the gramps-express part back to the datasource, and out of the Gateway. So you don't introduce any additional dependencies.

That could of course be put in a library method that's called from the dataSource, but this is the general idea.

[jlengstorf]

@kbrandwijk I think that solution makes sense in the context of qewl, but I'm not sure how it would work with the standard Apollo server setup without the gramps() dependency to merge and stitch. Developers would be required to initialize and stitch together all data sources manually, which removes the core objective of GrAMPS.

The other concern I have about this is the addition of extra boilerplate to n data sources vs. 1 dependency in the gateway. If the API for makeExecutableSchema or addMockFunctionsToSchema changes, that's maintenance that will have to be done by every data source creator vs. a single GrAMPS maintainer.

All that being said, I do think there's a way we could write an adapter and put it in a helper package:

export default function makeExecutable({ namespace, typeDefs, resolvers, mocks, context }, options) {
  const executableSchema = makeExecutableSchema({
    typeDefs,
    resolvers,
    ...options.makeExecutableSchema,
  });

  if (options.enableMockData) {
    addMockFunctionsToSchema({
          schema: executableSchema,
          mocks,
          ...options.addMockFunctionsToSchema,
        })
  }

  return { 
    namespace,
    schema: executableSchema,
    context: typeof context === 'function' ? context(req) : context,
  };
};

And then:

graphql.use(schema(
  makeExecutable(myDataSource, { enableMockData: true })
));

If I'm missing something, let me know.

[kbrandwijk]

I agree with you that having this independent helper on top of the set of datasources offers better integration possibilities in a diversity of scenarios. So I actually do think now that your last option is best.

[jlengstorf]

Excellent. So with that, the current state of this repo is pretty much ready for 1.0 testing, I think, with the exception of moving over to Typescript.

We'll need to add the helper function (#33) as well, but that's not necessarily a blocker for 1.0. (Though I think the code above should almost work as-is. I hope.) 🤞

[jlengstorf]

@kbrandwijk @schickling @ecwyne @mfix22 @ryanomackey I just published this package under the beta tag on npm. If you'd like to start experimenting with the 1.0 build, you can install:

yarn add @gramps/gramps@beta

Here's a full example of a server using GrAMPS 1.0:

import Express from 'express';
import getPort from 'get-port';
import bodyParser from 'body-parser';
import gramps from '@gramps/gramps';
import { GraphQLSchema } from 'graphql';
import { graphqlExpress, graphiqlExpress } from 'apollo-server-express';

// Data sources
import XKCD from '@gramps/data-source-xkcd';

async function startServer(app) {
  const PORT = await getPort(8080);
  app.listen(PORT, () => {
    console.log(`=> http://localhost:${PORT}/graphiql`);
  });
}

const GraphQLOptions = gramps({
  dataSources: [XKCD],
});

const app = new Express();

app.use(bodyParser.json());
app.use('/graphql', graphqlExpress(GraphQLOptions));
app.use('/graphiql', graphiqlExpress({ endpointURL: '/graphql' }));

startServer(app);

I've updated @gramps/data-source-xkcd@beta to use the 1.0 data source structure.

If you've got downtime over the holidays, I'd really appreciate it if you could play with this and open issues with any feedback. Otherwise, I think we're pretty much ready to call 1.0 ready for a first stable release. 🎉

[mfix22]

@jlengstorf I think this is a very neat way to merge data sources. Unless I am reading this wrong, the only use case we have that this does not cover is for linking in a remote executable schema:

makeRemoteExecutableSchema({
  schema: builtSchema,
  link: HttpLink, // which points at my remote schema
});

Technically we could stitch this remote schema in after gramps() has been called but that seems messy.

[jlengstorf]

@mfix22 Yeah, that's a limitation of GrAMPS — in order to keep the data sources simple, we need to run all data sources through GrAMPS to get an executable schema. I suppose it would be possible to add an extra option to allow additional executable schemas to be passed in.

Right now, we create an executable schema right before returning.

We could modify this so you'd call something like:

const external = makeRemoteExecutableSchema({
  schema: builtSchema,
  link: HttpLink, // which points at my remote schema
});

const GraphQLOptions = gramps({
  dataSources: [XKCD],
  externalSchemas: [external],
});

And then we'd modify the gramps() function to spread that new prop as well:

  const schema = mergeSchemas({
    schemas: [...schemas, ...externalSchemas, ...linkTypeDefs],
    resolvers,
  });

Theoretically this would Just Work™ and would even allow schema stitching between GrAMPS data sources and remote executable schemas.

However, I haven't worked with remote schemas yet, so I'm not sure if this would do what I think it'll do. Let me know if this looks like it'll work and I'll push a new beta package so you can try it out.

[mfix22]

@jlengstorf This is exactly how I am currently handling the use case! I am pretty sure it will Just Work™ 😄 I think that option would fill in the gap perfectly while keeping data sources simple.

[kbrandwijk]

@mfix22 @jlengstorf An alternative approach would be to keep the gramps part contained to a single schema, and leave the stitching of other schemas to other emerging tooling (separation of concerns). I don't think this is messy at all.

[jlengstorf]

@kbrandwijk I agree, but I wonder if it's worth including this for simple use cases since it's really just two lines of code. I may end up eating these words later, but I can't really see how this could cause problems later on.

I do agree that advanced use cases should probably be handled outside of GrAMPS, but I see a good use case in something like this:

• I have a gateway that's built of my own custom data sources
• I'm stitching them together with GrAMPS
• I have a new requirement that means I need to include GitHub repos for users
• I add the GitHub schema as a remote schema and stitch it together with my User type via GrAMPS

If it went too far beyond that, other tools would be easier/recommended, but I can see this being valuable.

Any objections to including it?

[kbrandwijk]

@jlengstorf Well, I guess it wouldn't hurt anyone, but I would strongly advise against making this a best practice. Mainly because in most use cases, it will involve more than just calling makeRemoteExecutableSchema. For most endpoints, there's authentication, there's .graphqlconfig.yml for defining your endpoints, there's not wanting the entire remote GraphQL schema to become part of your final schema, etc.

[jlengstorf]

Hmmm... that's a pretty solid argument for not including it. I haven't worked on remote schemas, so I'm pretty much going with the experience of the people using it.

@mfix22, do you have a counterargument here?

If @kbrandwijk's point holds true for all but the most trivial use cases, it does seem smart to leave out the feature and instead add an example of how to do this outside of GrAMPS. Otherwise I'd worry we're inadvertently introducing something we'd have to either deprecate later or add a big warning recommending people don't use it. (And if either of those is true, we may as well support the proper path out of the gate.)

Thoughts?

[mfix22]

I do see @kbrandwijk's point about not wanting to expose the entire 3rd party's schema as a definite concern here. That was a question I still want to ask one of the Apollo guys: what is the best way to expose part of a schema, especially a remote one?

As for the other concerns, GrAMPs already supports adding context and you can still add headers to handle authorization use-cases (more Express middleware for example).

For initial release, i don't think GrAMPS needs to support remote executable schemas. The escape hatch to include them might help some users though. What is nice is that including them doesn't hurt the users that don't.

Maybe the best argument for not including them at first is that once they are included, it would be a breaking change to remove them, but that is not true the other way around (once we know more about exposing a subset of a schema).

[jlengstorf]

@mfix22 The Graphcool team just released a pretty interesting article on exposing subsets of underlying GraphQL schemas. There's a video that shows an approach to controlling data access. Check it out: https://blog.graph.cool/graphql-databases-a-preview-into-the-future-of-graphcool-c1d4981383d9

Maybe we can pull in some Apollo folks to weigh in? @stubailo, @peggyrayzis, or @jbaxleyiii — any thoughts on best practices for managing remote schemas as laid out in @kbrandwijk and @mfix22's comments above? (Also, 👋.)