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

Document directives #27

Closed
gmencz opened this issue Jan 30, 2021 · 13 comments
Closed

Document directives #27

gmencz opened this issue Jan 30, 2021 · 13 comments

Comments

@gmencz
Copy link

gmencz commented Jan 30, 2021

I'm trying to use directives to implement this rate limiting middleware https://github.com/ravangen/graphql-rate-limit but I'm not sure how to do this, the types are telling me that the first argument for builder.toSchema({}) accepts an array of directives but how would I then attach these directives to specific fields?

@hayes
Copy link
Owner

hayes commented Jan 31, 2021

This is probably something I should have added explicitly in the docs. The inclusion of directives in the builder.toSchema method was probably an oversight on my part that resulted from just copying options from the GraphQL.js options.

Unfortunately, directives are not actually supported properly in GiraphQL. The reason it isn't supported is because of how directives are supported in graphql js. directives don't have any runtime behavior as part of the graphql spec, and the graphql js library only represents them in the ast nodes of a parsed schema file. I don't think directives were ever intended to be used as a way to add functionality to a code first API.

All that being said, there are lots of tools and libraries out there that have evolved around directives, and is probably something that should be supported. I need to look a bit more into how exactly directives work, and work out the details of the API, but taking an initial look just now, it should be possible to generate fake ast nodes for the schema as it's being built, and injecting the directives that way.

The API would probably consist of a few different pieces:

const builder = new SchemaBuilder<{
  ObjectDirectives: {
     rateLimit: { limit: number, duration: number }
  };
  FieldDirectives: {
     rateLimit: { limit: number, duration: number }
  };
}>({});


builder.addDirective('rateLimit', createRateLimitDirective(rateLimitOptions), {
  // TODO, not sure exactly what all we need to know here to generate directives correctly
});

builder.objectType('Example',  {
  // TODO making this an array is awkward, but directives are ordered, and repeatable, so an object doesn't really work.
  directives: [{
    name: 'rateLimit',
    args: { limit: 123, duration: 456 },
  }],
  // What I would prefer (maybe we can allow both).
  directives: {
    rateLimit: { limit: 123, duration: 456 },
  },
  fields: (t) => ({
    foo: t.string({
      directives: [{
        name: 'rateLimit',
        args: { limit: 123, duration: 456 },
      }],
      resolve: () => 'bar
    })
  })
});

@hayes
Copy link
Owner

hayes commented Jan 31, 2021

I'll try to find some time tomorrow to try to implement this. I'm not sure how complex the implementation actually is, and what kinds of blockers and issues I'll run into. If this is easy, I might be able to have something usable by the end of the weekend, if it ends up being more complex, its possible this will either take significantly longer to get done, or may not end up making sense to implement at all. I should at least have a better idea about what is actually involved in getting this working some time tomorrow.

@gmencz
Copy link
Author

gmencz commented Jan 31, 2021

I don't really like the idea of directives implementing functionality either but as you said there's a lot of tooling around it so it would be nice having them.

If it doesn't make sense to implement them, a plugin could be useful here for this field-scoped rate limiting functionality since I believe it's pretty common to use some sort of rate limiting like that for a GraphQL API, I'd be willing to contribute to this plugin (If it's even possible to implement it).

@hayes
Copy link
Owner

hayes commented Jan 31, 2021 via email

@hayes
Copy link
Owner

hayes commented Feb 1, 2021

@gmencz I've got a PR up for adding a plugin which adds directives (#29). I didn't have enough time today to add documentation, but there is a pre-release version you can install (you'll need to install the preview version of giraphql/core and giraphql/plugin-directives).

To get this working with a Directive that uses the schemaVisitor pattern like therate limiter one:

const builder = new Builder<{
  Directives: {
    rateLimit: {
      locations: 'OBJECT' | 'FIELD_DEFINITION';
      args: { limit: number, duration: number };
    }
}>({
  schemaDirectives: {
    rateLimit: createRateLimitDirective()
  }
});

builder.queryType({
  directives: {
    rateLimit: { limit: 5, duration: 10 }
  },
  fields: (t) => ({
    exampleField: t.string({
      directives: {
        rateLimit: { limit: 100, duration: 60 }
      },
      resolve() => 'hi',
    })
  })
})

My syntax above might not be exactly right, but it should be close enough. I'll try to find some time this week to write docs, but not sure when I'll have time. I am a lot more busy during the week.

Let me know if you have any questions, or run into any issues.

@gmencz
Copy link
Author

gmencz commented Feb 1, 2021

I installed the preview version of @giraphql/core and @giraphql/plugin-directives and tried what you suggested but the directive isn't added to the GraphQL schema so it doesn't seem to be working.

Here's what I tried:

const builder = new Builder<{
  Context: {
    prisma: PrismaClient;
    req: NextApiRequest & { session: Session };
  };
  Root: {};
  Objects: {
    Member: Member;
    RegisterPayload: RegisterPayload;
  };
  Interfaces: {};
  Scalars: {
    ID: { Input: string; Output: string };
    Date: { Input: Date; Output: Date };
  };
  Directives: {
    rateLimit: {
      locations: "OBJECT" | "FIELD_DEFINITION";
      args: { limit: number; duration: number };
    };
  };
}>({
  plugins: ["GiraphQLRelay", "GiraphQLDirectives"],
  relayOptions: {
    nodeQueryOptions: {},
    nodesQueryOptions: {},
    nodeTypeOptions: {},
    pageInfoTypeOptions: {},
  },
  schemaDirectives: {
    rateLimit: createRateLimitDirective(),
  },
});

builder.queryType({
  directives: {
    rateLimit: { limit: 5, duration: 60 },
  },
});

Am I missing something?

@hayes
Copy link
Owner

hayes commented Feb 1, 2021 via email

@gmencz
Copy link
Author

gmencz commented Feb 1, 2021

I tried doing that, I exceeded the rate limit and nothing happened so it's sadly not working, let me know if you need any other details.

@hayes
Copy link
Owner

hayes commented Feb 1, 2021

Looks like this is an issue in graphql-tools. I've opened a ticket, but I can probably add a flag to the extension for format things in a way that is compatible. I'll probably have some time to work on this tonight after work.

Opened an issue with graphql-tools here: ardatan/graphql-tools#2534

@hayes
Copy link
Owner

hayes commented Feb 1, 2021

This should work now. I don't think it makes sense to giraphql to actually apply the directives, since that is somewhat dependent on the implementation and there are a few competing conventions.

If you update to the latest preview version, and use a new flag in the options like:
https://github.com/hayes/giraphql/pull/29/files#diff-4a7891da6cc29d97fe21c02e6d1b44261d4ac61396503519868452d4b16a9d70R62

and then then apply the schema directives yourself like:
https://github.com/hayes/giraphql/pull/29/files#diff-d05bfe96369579b3b01a790056e16af95dea91c8e3e6b546dd62b4f34d921336R99-R103

It should work correctly.

@gmencz
Copy link
Author

gmencz commented Feb 1, 2021

It works! 🎉

@hayes
Copy link
Owner

hayes commented Feb 5, 2021

You can continue to use the preview release for now. The final version will need to wait until 2.0 is released. There are some changes I needed to make to get directives working correctly that are technically breaking changes to the type system. Work for 2.0 will be tracked in #32

@hayes
Copy link
Owner

hayes commented Feb 16, 2021

directives plugin is live along with the rest of v2.0. Make sure to read the migration guide when upgrading. Let me know if you run into any issues

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

No branches or pull requests

2 participants