Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding schema injection #448

Closed
wants to merge 5 commits into from
Closed

Adding schema injection #448

wants to merge 5 commits into from

Conversation

mikeburgh
Copy link

Looking for some feedback at this stage on the approach, as a way to solve #300

Still todo

  • Write some tests
  • Clean up loadInjections.ts
  • Validate the injections to ensure they are the right format (eg contain a schema).

You can load injections using a config param/CLI option that contains the path (supports globs) to your additional schema you want to inject: --schema-injection './injections/**/*.js'

Each file matched is exported to export the following format as an object:

{
    name: "testMutation1",
    type: "mutation",
    schema: {... a valid GraphQL Schema and resolver}
}

The type can be either a mutation or a query, and will be injected into the appropriate part of the GraphQL schema.

@mgallagher
Copy link

Really excited about this!

@@ -38,6 +38,9 @@ function createMutationGqlType (buildToken: BuildToken): GraphQLObjectType | und
.map(collection => createCollectionMutationFieldEntries(buildToken, collection))
.reduce((a, b) => a.concat(b), [])
),
...(loadInjections(options.schemaInjection, 'mutation')

),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird whitespace?

@benjie
Copy link
Member

benjie commented May 3, 2017

This is pretty cool and seems simple (I've not looked at it in depth yet). I feel like the files should export a function that returns the given format rather than returning the format directly because this allows us to pass options (e.g. PostGraphQLOptions) to the injected schemas which can, for example, give it access to the PostgreSQL database.

Or is this already accounted for?

@benjie
Copy link
Member

benjie commented May 3, 2017

Is there another system that does something like this that we can compare to?

@mikeburgh
Copy link
Author

Thanks for the feedback.. I have not been able to find another GraphQL implementation that lets you inject into the schema, and I cannot think of any other systems that let you do similar. I am sure they are out there, just not something I have come across.

Exporting as a function is a lot better, I will sort that out, and clean this up.

I also want to look at getting access to the decoded/verified JWT in the methods, so the function approach will solve that.

@benjie benjie mentioned this pull request May 5, 2017
@mikeburgh
Copy link
Author

mikeburgh commented May 9, 2017

@benjie I have cleaned up the loading process, and refactored it to expect a function from the loaded injections, rather than the resolved schema.

In terms of passing something into the schema function to allow for reuse, I passed the main postgraphql object, although not sure that is the best option given it's more of a constructor. To pass in the options object from the CLI might require some larger re working if your okay with that?

A couple of issues I have found:

  • When a custom injection is executed, commands for the transaction and claims are still sent to the DB. This may not be an issue if the injection is going to call into the DB (although getting access to the client from the context is not straight forward), but perhaps we need to let the injections specify if they need that, so in the case they are not going to call the DB directly that overhead is not present.

  • There is a pgClient in the context object passed to the resolver, it's just hard to get to..

import { $$pgClient } from '../postgres/inventory/pgClientFromContext'
const pgClient = context[$$pgClient];

Not sure exactly why it's like that ? it means the injection files need to import that invetory to retrieve it

For reference here is a file that can be used as an injection

var graphQL = require('graphql');
module.exports = {
  name: "testMutation",
  type: "mutation",
  schema: function (postgraphQL) {
    return {
      args: {
        input: {
          description: 'the input'
          , type: new graphQL.GraphQLNonNull(graphQL.GraphQLString)
        }
      }
      , description: "test"
      , resolve: function (_source, args, context, info) {
       //context at the moment contains pgRole and jwtClaims if a valid jwt was supplied (hidden inside is a pgClient )
        return {i: args.input, t: 're'};
      }
      , type: new graphQL.GraphQLObjectType({
          name: "testMutationPayload"
          , description: "The output of our mutation."
          , fields: {
            i: {type: graphQL.GraphQLString, description: 'Something out'},
            t: {type: graphQL.GraphQLString, description: 'Something else out'},
          }
        }
      )
    }
  }
}

@benjie
Copy link
Member

benjie commented May 24, 2017

@mikeburgh Really sorry for the lack of communications on this - I've not had a chance to play with it but definitely extending the schema is something we (and specifically I!) want to do - so I will be taking a look at some point. If I've not got back to you by 7th May, please add a comment below to give me a nudge!

@brysgo
Copy link
Contributor

brysgo commented May 27, 2017

I played with this a little. Would love to figure out a way to use autogenerated types in injected schema.

@aaronc
Copy link

aaronc commented May 27, 2017

Hey, I think this is a really cool idea and would be great to have. Would solve some cases I'm trying to handle.

I'm wondering why not implement this with a simple middleware function? This could be done by by adding middleware?: (schema, pgClient, pgSchemas, options) => Promise<GraphQLSchema> to PostGraphQLOptions and then calling it after right https://github.com/postgraphql/postgraphql/blob/master/src/postgraphql/postgraphql.ts#L138 like this:

const newGqlSchema = await createPostGraphQLSchema(pgClient, pgSchemas, options)
const enhancedGqlSchema = await middleware(newGqlSchema, pgClient, pgSchemas, options)
exportGqlSchema(enhancedGqlSchema)

I think this would be the simplest and most flexible route.

@mikeburgh
Copy link
Author

@benjie no problems at all, I have not had a chance to get back to it (hoping this week). I am also keen on extending the schema as I think it gets you a really solid backend without having to do all the usual scaffolding of building a rest api that maps to a bunch of stored procs etc... the only thing missing is input validation, but I see talk about adding that into graphQL (graphql/graphql-js#361)

@brysgo do you mean the other graphQL qureries and mutations that were discovered.. I think once we get the right object passed into the schema generator, you should be able to call those in a similar way to the custom execution example

@aaronc my original plan for using this was going to be via the command line, but I have since ended up wrapping it with express... so having the command line capability is not needed for me any more, so I guess it depends what the overall feeling on use case for this is ?

The big issue for me with this still (middleware or command line) is finding a neat way to mark methods as not needing the database and hence preventing a connection being opened to the DB.. in some cases it might make sense to have that, but not all.

@brysgo
Copy link
Contributor

brysgo commented May 27, 2017

@mikeburgh - first I have to say great work so far, though I think it is important to get at the API from inside the injection, I was actually talking about defining fields on existing types and defining an existing type as the return type of an injected query.

@aaronc
Copy link

aaronc commented May 28, 2017

@mikeburgh yeah, it makes sense to take a directory string if you want to support command line. I would say maybe the config option could take either a string for cli usage or a function for embedded? The reason I'm suggesting middleware is because this would address some of the cases @brysgo is describing - you would get the full generated schema and be able to modify that if needed and or reuse some of the fields and types elsewhere.

The idea of not opening a pg client if not needed sounds useful. One approach would be to have context contain a function which opens a connection if needed. Would certainly require a bit of refactoring of the current code, but may allow more flexibility. I could see a scenario where a schema is shared between say pg and some other data store and you may want to inject support for that other data store into context.

@mikeburgh
Copy link
Author

@aaronc I was just checking out the middleware idea, with making the command line a file to require that exports a function (so both library and cli work as middleware). The problem with the suggestion for where to hook up the middleware is that the gqlSchema is already built there and they don't have a way to add to at that stage (graphql/graphql-js#314).

I think we need to actually tie it in here around here (https://github.com/postgraphql/postgraphql/blob/master/src/graphql/schema/createGqlSchema.ts#L44) which then brings back the use of hooks which @calebmer mentioned in #300 I am just not clear on how he sees those being used to actually extend it, and still not sure this would get you access to all the types that were discovered during introspection from the DB

@chadfurman
Copy link
Collaborator

chadfurman commented Jun 1, 2017

The way I'd envision a useful middleware layer working would be:

  1. I start postgraphql on the command-line and it looks for scripts in a folder and loads them by registering special endpoints (or queries? or mutations? Or both depending on config?) based on whatever is specified by the plugin file via an exported variable name or .json config file
  2. a call to /api/plugin (or a graphQL query for the query/mutation) would hit the script first (like an express endpoint) which would then have the ability to trigger graphQL calls, get their results, and respond.

I think this would be a super clean and simple method of allowing us to write code in JS w/ all the power of libraries, external API calls, etc. while still maintaining the magic that is GraphQL.

What you've made so far seems like it would accomplish the above, so I'm very excited to try it out.

@benjie benjie mentioned this pull request Jun 6, 2017
@chadfurman
Copy link
Collaborator

So, on second thought, I believe this is wasted effort in light of https://github.com/postgraphql/postgraphql/blob/master/docs/library.md

I can put a /graphql endpoint in a nodejs API, and then everything Postgraph can't do, my Node API can.

@mgallagher
Copy link

@chadfurman I think serving PostGraphQL through Node and adding any needed REST endpoints is a great solution for some things, but don't we need a way to actually extend the GraphQL schema generated by PostGraphQL? I thought that was more of the goal here.

@chadfurman
Copy link
Collaborator

I don't think PostGraphQL should do everything for the user. I think there's 5 pages of active tickets on GitHub and, from what I can tell, two active developers. I think the goal of this PR is to just have a way to deliver data from an external api to the user on request -- synchronously, without having to trigger a NOTIFY event, and as part of the same "API"

If, at that point, you want to bring the GraphQL library into your API to keep the request structure consistent, you could do that.

This was a feature that was asked for quite a bit, but I believe it's due to lack of knowledge / understanding. That was certainly the case for me -- I don't expect Postgres to ever be able to run certain JS events, and PostGraph is about auto-generating a GraphQL API by introspecting schemas in a database. At this point, I don't see why PostgraphQL would need to run any additional JS on the side -- injections, middleware, whatever -- because PostgraphQL itself can be the middleware and the JS handlers can simply wrap around Postgraph.

@mikeburgh
Copy link
Author

My initial needs for this functionality was to extend the GraphQL schema so that some JS functions to fetch remote data (and not touch the PG database) could be included.

@chadfurman you are right, you can do this by consuming postgraphql as a lib, and then mixing GraphQL with another API (rest or even another graphQL endpoint).

As some comments here have pointed out, there are some short falls with this currently:

  • you cannot use any of the types the introspection creates. So if you want to inject methods that say augment the incoming data before going to PG.. you have to re define the types.

  • a connection to PG is opened, even if the method is not going to use it.

I have not had a chance to get back to this, but I should in a week or two.. @benjie you mentioned to give you a nudge on it if you had not got back to it by June 7..

@benjie
Copy link
Member

benjie commented Jun 15, 2017

I can't help wondering if there's a better way to extend the GraphQL schema... I'm working up an idea at the moment.

@cellis
Copy link

cellis commented Jun 18, 2017

@benjie @mikeburgh @chadfurman you may want to look at graphql-compose for inspiration of how to extend graphql schemas.

@benjie
Copy link
Member

benjie commented Jun 21, 2017

Thanks @cellis, I saw that on Awesome GraphQL. It makes me a bit uncomfortable though because it uses internal GraphQL.js features (rather than just the public API) - there's no public API as far as I know to remove a field from a GraphQL schema.

@bradleyayers
Copy link
Contributor

I'm very enthusiastic about this issue, @benjie did you work up a plan for how this would work? Being able to contribute mutation queries backed by JavaScript logic would be great, but I'm curious if you've given any thought to subscription queries?

@benjie
Copy link
Member

benjie commented Jul 7, 2017

@bradleyayers Yep, I've worked up a solution; I just want to get Caleb's take on it before I announce it...

As for subscriptions; I've not done any work on this yet... But I believe they can be implemented on top of my schema extending stuff 👍

@benjie
Copy link
Member

benjie commented Jul 8, 2017

Caleb has given me his blessing, so here it is folks: #506

@mikeburgh
Copy link
Author

Closing this in favor of the v4 approach!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants