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

Cloning a resolver evaluates its arguments' thunks #231

Closed
toverux opened this issue Feb 5, 2020 · 7 comments
Closed

Cloning a resolver evaluates its arguments' thunks #231

toverux opened this issue Feb 5, 2020 · 7 comments
Labels

Comments

@toverux
Copy link
Contributor

toverux commented Feb 5, 2020

I have a cyclic dependency between two TypeComposers. Usually, this kind of scenario does not cause any problem when using thunks for resolvers types and resolvers' arguments' types, like this:

schemaComposer.createResolver({
    type: () => userTypeComposer.getResolver('createOne').getType(),
    args: {
        record: () => userTypeComposer.getResolver('createOne').getArgType('record')
    }
    // ...
})

However, I have a middleware system (very much inspired by the one from this library), and that system makes use of Resolver.clone() to wrap the original resolver and expose a derived resolver with a wrapper resolve function.

Here is how it looks:

export const domainUserMutations = {
    ...allWithMiddlewares([someMiddleware()], {
        createOne: schemaComposer.createResolver({ /* ... */ })
    })
};

The allWithMiddlewares function eventually makes a call to the withMiddlewares function, that is very similar to graphql-compose's Resolver.withMiddlewares() method:

export function withMiddlewares(resolver: Resolver, middlewares: ResolverMiddleware[]): Resolver {
    return middlewares.reduceRight((parent, mw) => {
        const name = mw.name ?? mw.constructor?.name ?? 'middleware';

        const newResolver = parent.clone({ name: `${name}::${parent.name}`, parent });

        const resolve = parent.getResolve();

        newResolver.setResolve(params => mw(resolve, params, parent));

        return newResolver;
    }, resolver);
}

Now I get this stack trace:

error: Unhandled error: TypeError: TypeError[Resolver.checkDocumentMutateAccessMiddleware::createOne.record]: Cannot read property 'getResolver' of undefined
    at record (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/projects/app.server/graphql/domain/domain-user-graphql.ts:43:48)
    at /home/morgan/Documents/MitM/core/node_modules/graphql-compose/lib/TypeMapper.js:348:100
    at getFC (/home/morgan/Documents/MitM/core/node_modules/graphql-compose/lib/utils/createThunkedObjectProxy.js:17:48)
    at Object.get (/home/morgan/Documents/MitM/core/node_modules/graphql-compose/lib/utils/createThunkedObjectProxy.js:28:14)
    at isFunction (/home/morgan/Documents/MitM/core/node_modules/graphql-compose/lib/utils/is.js:25:28)
    at TypeMapper.convertArgConfig (/home/morgan/Documents/MitM/core/node_modules/graphql-compose/lib/TypeMapper.js:347:30)
    at /home/morgan/Documents/MitM/core/node_modules/graphql-compose/lib/TypeMapper.js:393:39
    at Array.forEach (<anonymous>)
    at TypeMapper.convertArgConfigMap (/home/morgan/Documents/MitM/core/node_modules/graphql-compose/lib/TypeMapper.js:392:41)
    at Resolver.setArgs (/home/morgan/Documents/MitM/core/node_modules/graphql-compose/lib/Resolver.js:174:48)
    at new Resolver (/home/morgan/Documents/MitM/core/node_modules/graphql-compose/lib/Resolver.js:72:10)
    at Resolver.clone (/home/morgan/Documents/MitM/core/node_modules/graphql-compose/lib/Resolver.js:698:12)
    at /home/morgan/Documents/MitM/core/dist/app.server/webpack:/projects/app.server/graphql/common.ts:97:36
    at Array.reduceRight (<anonymous>)
    at withMiddlewares (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/projects/app.server/graphql/common.ts:94:24)
    at /home/morgan/Documents/MitM/core/dist/app.server/webpack:/projects/app.server/graphql/common.ts:125:29
    at Array.forEach (<anonymous>)
    at allWithMiddlewares (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/projects/app.server/graphql/common.ts:124:28)
    at Module.<anonymous> (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/projects/app.server/graphql/domain/domain-user-graphql.ts:32:26)
    at __webpack_require__ (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/webpack/bootstrap:19:1)
    at Module.<anonymous> (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/projects/app.server/graphql/domain/index.ts:1:1)
    at __webpack_require__ (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/webpack/bootstrap:19:1)
    at Module.<anonymous> (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/projects/app.server/graphql/user/user-graphql.ts:1:1)
    at __webpack_require__ (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/webpack/bootstrap:19:1)
    at Module.<anonymous> (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/projects/app.server/graphql/user/index.ts:1:1)
    at __webpack_require__ (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/webpack/bootstrap:19:1)
    at Module.<anonymous> (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/projects/app.server/graphql/save/save-graphql.ts:1:1)
    at __webpack_require__ (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/webpack/bootstrap:19:1)
    at Module.<anonymous> (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/projects/app.server/graphql/save/index.ts:1:1)
    at __webpack_require__ (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/webpack/bootstrap:19:1)
    at Module.<anonymous> (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/projects/app.server/graphql/collection/collection-graphql.ts:1:1)
    at __webpack_require__ (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/webpack/bootstrap:19:1)
    at Module.<anonymous> (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/projects/app.server/graphql/collection/index.ts:1:1)
    at __webpack_require__ (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/webpack/bootstrap:19:1)
    at Module.<anonymous> (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/projects/app.server/graphql/brand/brand-graphql.ts:1:1)
    at __webpack_require__ (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/webpack/bootstrap:19:1)
    at Module.<anonymous> (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/projects/app.server/graphql/brand/index.ts:1:1)
    at __webpack_require__ (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/webpack/bootstrap:19:1)
    at Module.<anonymous> (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/projects/app.server/graphql/applet-access-point/applet-access-point-graphql.ts:1:1)
    at __webpack_require__ (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/webpack/bootstrap:19:1)
    at Module.<anonymous> (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/projects/app.server/graphql/applet-access-point/index.ts:1:1)
    at __webpack_require__ (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/webpack/bootstrap:19:1)
    at Module.<anonymous> (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/projects/app.server/graphql/index.ts:1:1)
    at __webpack_require__ (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/webpack/bootstrap:19:1)
    at Module.<anonymous> (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/projects/app.server/main.ts:1:1)
    at __webpack_require__ (/home/morgan/Documents/MitM/core/dist/app.server/webpack:/webpack/bootstrap:19:1)
    at /home/morgan/Documents/MitM/core/dist/app.server/webpack:/webpack/bootstrap:83:1
    at Object.<anonymous> (/home/morgan/Documents/MitM/core/dist/app.server/main.js:88:10)
    at Module._compile (internal/modules/cjs/loader.js:1151:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1171:10)
    at Module.load (internal/modules/cjs/loader.js:1000:32)
    at Function.Module._load (internal/modules/cjs/loader.js:899:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
    at internal/main/run_main_module.js:17:47

Decoding it, we can see that there's this cyclic dependency: user => domain => user.

Here is the code that actually crashes (see the line with a comment):

export const domainUserMutations = {
    ...allWithMiddlewares([
        attachToken(), attachUser(), attachDomain(),
        checkDocumentMutateAccess(or(
            hasDomainSecretKey(params => params.args.domainId),
            hasBrandRole(params => params.args.domainId)))
    ], {
        createOne: schemaComposer.createResolver<unknown, MutationDomainUserCreateOneArgs>({
            name: 'createOne',
            type: () => userTypeComposer.getResolver('createOne').getType(),
            args: {
                domainId: GraphQLNonNull(GraphQLMongoID),
                // THE ERROR OCCURS IN THE THUNK BELOW, userTypeComposer is undefined because it is still initializing when the thunk is called
                record: () => userTypeComposer.getResolver('createOne').getArgType('record')
            },
            resolve: fromRootResolveCreateOne
        })
    })
};

Interestingly, this happens because Resolver.clone() is used in withMiddlewares(). For some reason, the cloning function calls a lot of other functions, and eventually it ends up in the TypeMapper, where the thunk is evaluated.

Not cloning the resolver and just changing its resolve function works.

I'd consider this as bug and an undesired side effect, as I should be able to clone a Resolver object without evaluating its thunked configuration (ie. just make a copy of the object).

I'd guess the library's Resolver.withMiddlewares() method has the same problem since it does basically the same thing.

Now, I'd understand if that's too complicated to fix, and I know how to workaround this, but I'd really like to still be able to clone my resolvers to apply middlewares on top of them.

I hope I am clear enough. Cyclic dependencies are always a source of fun.

As always - thanks a lot for your library.

@nodkz
Copy link
Member

nodkz commented Feb 6, 2020

If you use graphql-compose v7

Try to modify your code to this

schemaComposer.createResolver({
-    type: () => userTypeComposer.getResolver('createOne').getType(),
+    type: () => userTypeComposer.getResolver('createOne').type,
    args: {
-        record: () => userTypeComposer.getResolver('createOne').getArgType('record')
+        record: () => userTypeComposer.getResolver('createOne').getArg('record').type
    }
    // ...
})

When you call getType() method it starts creating a GraphQL type. But we need to work with TypeComposers as long as possible to avoid hoisting problems. So .type returns TypeComposers. Hope that it helps to resolve your issue.

@toverux
Copy link
Contributor Author

toverux commented Feb 6, 2020

Hi, thanks for your answer.

I changed .getType/.getArgType occurrences everywhere but it doesn't seem to help.

The crash happens sooner in the thunk, when it tries to read the userTypeComposer reference, that is undefined because it's still initializing at that point.

@toverux
Copy link
Contributor Author

toverux commented Feb 6, 2020

This is my current workaround, I use this function instead of Resolver.clone():

function cloneResolverIssue231(resolver: compose.Resolver, opts?: Partial<compose.ResolverDefinition<unknown, Context>>) {
    const oldOpts: compose.ResolverDefinition<unknown, Context> = {};

    for (const key in resolver) {
        if (resolver.hasOwnProperty(key)) {
            // @ts-ignore
            oldOpts[key] = resolver[key];
        }
    }

    // Delete args property, or Resolver ctor calls setArgs, that crashes
    delete oldOpts.args;

    const clonedResolver = new compose.Resolver({ ...oldOpts, ...opts }, resolver.schemaComposer);

    clonedResolver.args = resolver.args;

    return clonedResolver;
}

Right now it seems there a no undesirable side effects. The key is to avoid Resolver.setArgs() that is called in Resolver's constructor.

@nodkz
Copy link
Member

nodkz commented Feb 14, 2020

I don't like workarounds )))
Can you provide any working example which does not work?

It will be cool if you open PR with a broken test like these https://github.com/graphql-compose/graphql-compose/tree/master/src/__tests__/github_issues

Tnx!

toverux added a commit to toverux/graphql-compose that referenced this issue Feb 14, 2020
nodkz added a commit to toverux/graphql-compose that referenced this issue Feb 15, 2020
…eDefinition> stability in complex schemas and provide stronger & better DX

Closes graphql-compose#231
nodkz pushed a commit that referenced this issue Feb 15, 2020
@nodkz nodkz closed this as completed in 2b56ace Feb 15, 2020
@nodkz
Copy link
Member

nodkz commented Feb 15, 2020

@toverux please check a new version of graphql-compose with your app. It should properly work without workarounds.

If any other errors occur, please let me know. Thanks.

@nodkz
Copy link
Member

nodkz commented Feb 15, 2020

🎉 This issue has been resolved in version 7.12.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nodkz nodkz added the released label Feb 15, 2020
@toverux
Copy link
Contributor Author

toverux commented Feb 17, 2020

Thanks @nodkz, I'll try it out the next time I run an upgrade pass on our app, if there's a problem I'll let you know.

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

No branches or pull requests

2 participants