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

Docs: document schema by http header (#913) #979

Merged

Conversation

brainrepo
Copy link
Contributor

@brainrepo brainrepo commented Apr 14, 2023

closes #913
This PR explains exposing two different mercurius instances to the same route using the fastify/find-my-way custom constraints.

  • Add a working example to the examples folder
  • Add a new paragraph to the FAQ section

@brainrepo brainrepo marked this pull request as draft April 14, 2023 10:55
@brainrepo brainrepo marked this pull request as ready for review April 14, 2023 14:59
Copy link
Collaborator

@simoneb simoneb left a comment

Choose a reason for hiding this comment

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

nice finding, I didn't know this was possible. although this may work, to me it seems a little hack-ish and I think we should try to come up with something better.

the issues I see at the moment are:

  • the fastify docs discourage the creation of custom constraints because of the performance impact on the router. maybe by inspiring from the built-in ones we can come up with an implementation without any performance implications?
  • the fact that you're registering two different instances of mercurius may still be necessary, but it would be nice it there wasn't so much work involved. maybe there's a way to build something into mercurius which would allow to achieve this without so much code? e.g. if we solved the first issue above and mercurius supported constraints natively, then maybe we can make it work without having to register routes explicitly because mercurius would know that unless the constraint is matched it wouldn't have to run?

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Fastify is amazing from time to time ;)

@brainrepo
Copy link
Contributor Author

brainrepo commented Apr 19, 2023

I am exploring alternative ways to solve this issue.
The idea is to avoid instantiating multiple Mercurius instances.
I developed an idea by looking at @mercurius-js/auth's schema filtering and pruning features.
We can use the preExecution hook to change the schema on the fly for the current request.
Doing that, we have just one limit. If the schemas differ, the validation is done using the original (default ) schema, and the validation step fails.
We can avoid that by giving the preValidation hook the ability to change the schema like done in the preExecution hook.
@mcollina @simoneb, do you think this is feasible?

Here a simple example

const app = Fastify()
const schema1 = makeExecutableSchema({
  typeDefs: ...
  resolvers: ... 
})
const schema2 = makeExecutableSchema({
  typeDefs: ...
  resolvers: ... 
})

app.register(mercurius, {
  schema: schema1,
})

app.ready(err => {
  if (err) throw err
  app.graphql.addHook('preExecution', (schema, document, context) => {

    if (context.reply.request.headers.schema === "schema2") {
      return { schema: schema2}
    }

    return { schema }
  })
})

Not sure which is the impact on loaders and jit. Do you have any clue?

@mcollina
Copy link
Collaborator

I think using the constraints is the best way to solve this. Adding support for multiple schemas in Mercurius will make the code harder to read, add a bunch if edge case we don't know about, and increase the maintenance cost.

@simoneb
Copy link
Collaborator

simoneb commented Apr 24, 2023

@mcollina sounds good, thanks for the feedback. @brainrepo let's go ahead and document this option for the time being, while we think about other options to do this. Based on Matteo's feedback, implementing support for this within mercurius may not be a compelling option, on which case we may consider building a plugin instead, which hides away the implementation details.

@brainrepo
Copy link
Contributor Author

brainrepo commented Apr 24, 2023

Thanks @mcollina , @simoneb .
@simoneb I think I haven't the right to merge.

@simoneb simoneb merged commit daa6462 into mercurius-js:master Apr 24, 2023
10 checks passed
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.

Execute against different schemas based on request headers
3 participants