Skip to content
This repository has been archived by the owner on Oct 2, 2024. It is now read-only.

Middleware resolvers #81

Closed
wants to merge 3 commits into from
Closed

Middleware resolvers #81

wants to merge 3 commits into from

Conversation

afallou
Copy link

@afallou afallou commented Nov 23, 2018

There are action that we may want to perform for all (or almost all) resolvers in an app.
Currently the only way to do that is to add this action to every resolver. Middleware resolvers allow us to instead centrally define both those actions and the resolvers to which they shouldn't apply.

  • Create the MiddlewareResolver class and its interface
  • Execute middleware resolvers in any resolver pipeline, before the actual resolver logic.

There are action that we may want to perform for all (or almost all) resolvers in an app.
Currently the only way to do that is to add this action to every resolver. Middleware resolvers allow us to instead centrally define both those actions and the resolvers to which they shouldn't apply.
- Create the MiddlewareResolver class and its interface
- Execute middleware resolvers in any resolver pipeline, before the actual resolver logic.
/* If any of the keys in the resolver pipeline should be ignored, ignore the whole pipeline,
* making the middleware the no-op for this request.
*
*/
Copy link
Author

@afallou afallou Nov 23, 2018

Choose a reason for hiding this comment

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

An example is authorization: we may want to restrict write operations to a given role by default, but allow some specific write operations to all roles.

).toEqual(3);
});

it('run middlewares sequentially in order', async() => {
Copy link
Author

Choose a reason for hiding this comment

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

This test succeeds in isolation but fails when running with the others. The issue is improper cleanup of the graphql.resolverMiddlewares binding, but I don't fully understand it yet.

@jessemyers
Copy link
Contributor

@afallou You may want to look a bit at what, if anything, the core GraphQL layer provides.

I don't mind this approach so much as to block it, but I worry about coupling. We're layering a new middleware abstraction on top of our own resolver pipeline abstraction, which means we're going further and further in a direction that doesn't play nice with any other approach to writing resolvers.

At the time I wrote the initial createResolver/createStrictResolver/ResolverPipeline code, writing our own building blocks was justified as there wasn't much out there. I suspect that things have advanced since then, but I have no proof.

@afallou
Copy link
Author

afallou commented Dec 3, 2018

@jessemyers I looked through the changelog of graphql-js and didn't see anything pointing in this direction. Somebody actually filed a feature request for resolver middlewares back in September, and it has a reply from one of the main contributors; so it seems like nothing like this exists in the core GraphQL layer yet.

@afallou
Copy link
Author

afallou commented Dec 3, 2018

It turns out that the state cleanup issue I'm seeing in one test comes from the fact that nodule-config's clearBinding is broken for nested bindings (e.g. foo.bar). So far we've only used it to clear the config, so the issue has never surfaced.

@cdm-ium cdm-ium closed this Oct 7, 2019
@cdm-ium cdm-ium deleted the feature/middleware-resolver branch October 7, 2019 16:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants