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

Support Route level hooks for grafserv/fastify/v4 #1999

Open
4 of 6 tasks
jcgsville opened this issue Mar 20, 2024 · 4 comments
Open
4 of 6 tasks

Support Route level hooks for grafserv/fastify/v4 #1999

jcgsville opened this issue Mar 20, 2024 · 4 comments

Comments

@jcgsville
Copy link
Contributor

Feature description

In the fastify grafserv server, there are several calls to app.route() to set up the relevant routes with fastify. I think it would be useful to be able to register fastify hooks to individual routes.

To do this, I think you'd need some server-specific settings in Grafserv options? I'm not sure if that's already a pattern somewhere, or whether that adds coupling where we want grafserv and the specific servers to remain decoupled.

I'm thinking the grafserv config would include something like

{
    // ... other grafserv config properties
    fastifyV4: {
        routeHooks: {
            '/graphql': {
                preHandler: () => {}
            }
        }
    }
}

Motivating example

I'm trying to find the cleanest way to add a hook to my postgraphile + fastify application that validates the access token present in request cookies and returns a 401 before the request starts executing grafast/postgraphile-related things.

Because grafserv's calls to app.route() are inaccessible to my code, I think my best option without this feature request would be to create a server-wide hook and check the path within the hook to determine whether the call is to /graphql or some other route. If it's a call to /graphql, then I do the validation.

Supporting development

I:

  • am interested in building this feature myself
  • am interested in collaborating on building this feature
  • am willing to help testing this feature before it's released
  • am willing to write a test-driven test suite for this feature (before it exists)
  • am a Graphile sponsor ❤️
  • have an active support or consultancy contract with Graphile
@benjie
Copy link
Member

benjie commented Mar 25, 2024

I'm not super familiar with Fastify, so I'd need a few people who use it to confirm that's the right approach. For now, you should be able to write your own fastify adaptor: just take a copy of the fastify adaptor that already exists, and then apply your changes to it:

https://github.com/graphile/crystal/blob/main/grafast/grafserv/src/servers/fastify/v4/index.ts

Hopefully everything it imports is available as an export from grafserv? 🤞

There should be no other files you'd need to touch to add configuration options for fastify within grafserv, just use declaration merging:

interface FastifyV4GrafservOptions {
  // ...
}
declare global {
  namespace GraphileConfig {
    interface GrafservOptions {
      fastifyV4?: FastifyV4GrafservOptions
    }
  }
}

If you think lots of plugins will want to be able to hook into this then you'll want to use graphile-config's methods for handling plugins which I don't think are documented yet but you can find throughout the codebase. You probably want AsyncHooks to enable your plugins to do async things? Note addTo is already async, so waiting on hooks in there should be fine.

@jcgsville
Copy link
Contributor Author

For future readers, I took a slightly different approach for now which I think it a bit less idiomatic, but works fine. I think it has the small cost of running the extra javascript of the hook on every route, but it's just a trivial if statement for routes other than the ones I want to authenticate

.addHook('onRequest', async (request, reply) => {
    if (request.routeOptions.url === '/graphql') {
        return authenticateRequest(request, reply)
    }
})

where authenticateRequest() contains my authn logic. Hope that helps others.

The addition of this to the fastify adaptor is simple enough that I believe I would be comfortable with taking this on and willing to put in the time to do so if you decide that it's a worthwhile change pending feedback from others who are more deeply familiar with fastify. Just lmk!

@benjie
Copy link
Member

benjie commented Mar 31, 2024

I’m happy adding things so long as they don’t have a noticeable performance impact for people not using the feature, and so long as the feature actually seems desirable and isn’t better served via an alternative solution. 👍

@benjie
Copy link
Member

benjie commented Mar 31, 2024

(But as I say; you can experiment with this external to core, even share the code so others can try it, and then we can merge it once general consensus supports it.)

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