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

Run middleware inside of GraphQL execution #363

Closed
1 of 5 tasks
spawnia opened this issue Oct 4, 2018 · 2 comments · Fixed by #395
Closed
1 of 5 tasks

Run middleware inside of GraphQL execution #363

spawnia opened this issue Oct 4, 2018 · 2 comments · Fixed by #395
Labels
enhancement A feature or improvement

Comments

@spawnia
Copy link
Collaborator

spawnia commented Oct 4, 2018

Is your feature request related to a problem? Please describe.

When querying a field that does use Laravel Middleware via @middleware, and an Exception is thrown in the Middleware, the Request fails before we are in the GraphQL execution context. The resulting Response is not a proper GraphQL response, it does not conform to the specification.

The problem with this becomes even more apparent when you consider that one might query multiple fields in a single request. A failure in one field would cause the whole query to abort. However, the correct behaviour would be to return an error for one field and resolve the other field as usual.

Describe the solution you'd like

We might use the Context, which is the third resolver argument, to call the Middleware on a per-field basis, while we are already during GraphQL execution.

This would also solve another issue: We are currently forced to load all fields of the root Query and Mutation types to evaluate if they have middleware. Deferring the Middleware execution would allow us to load those fields lazily, just like any other field.

Possible issues

  • We have to ensure that we always have the request available as part of the Context object
  • This would change the behaviour of Middleware execution in a possibly breaking way

TODOs

@spawnia spawnia added enhancement A feature or improvement discussion Requires input from multiple people labels Oct 4, 2018
@spawnia spawnia changed the title Run middleware inside of GraphQL execution [RFC] Run middleware inside of GraphQL execution Oct 4, 2018
@yaquawa
Copy link
Contributor

yaquawa commented Oct 8, 2018

Agree! I'm facing the same problem.
It's good to have the lazy loading too.❤️

@spawnia spawnia changed the title [RFC] Run middleware inside of GraphQL execution Run middleware inside of GraphQL execution Oct 16, 2018
@spawnia spawnia removed the discussion Requires input from multiple people label Oct 16, 2018
@spawnia
Copy link
Collaborator Author

spawnia commented Oct 16, 2018

@chrissm79 @yaquawa @olivernybroe @enzonotario @robsontenorio

Just put up a WIP PR #395 that will resolve this, looking for feedback on what i need to consider for the tests and to avoid breaking changes.

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

Successfully merging a pull request may close this issue.

2 participants