From c1f1d70e2dde80ff11530ebfc43f2c13757c0b05 Mon Sep 17 00:00:00 2001 From: bquinn Date: Mon, 12 Oct 2020 10:38:57 -0700 Subject: [PATCH] tighten argument passing semantics for delegateToComponent and use clonedeep instead of Object.assign when importing resolvers to avoid modifying the original resolver map in certain exclusion scenarios --- lib/delegate/__tests__.js | 390 ++++++++++++++++++++++++++++++++++--- lib/delegate/index.js | 67 +++---- lib/resolvers/__tests__.js | 17 +- lib/resolvers/index.js | 3 +- package.json | 1 + 5 files changed, 408 insertions(+), 70 deletions(-) diff --git a/lib/delegate/__tests__.js b/lib/delegate/__tests__.js index 8205d1d..229e1d3 100644 --- a/lib/delegate/__tests__.js +++ b/lib/delegate/__tests__.js @@ -833,7 +833,14 @@ Test('delegateToComponent - nested abstract type is resolved without error', asy t.end(); }) -Test('delegateToComponent - calling resolver arg is not passed when target root field does not have matching arg', async (t) => { +// argument forwarding tests: + +/* + * case 1: target field has no arguments and calling resolver has no arguments + * result: no arguments are forwarded even if caller of delegateToComponent + * provides them +*/ +Test('delegateToComponent - case 1 - no args provided to delegateToComponent', async (t) => { const reviews = new GraphQLComponent({ types: ` type Review { @@ -842,13 +849,157 @@ Test('delegateToComponent - calling resolver arg is not passed when target root } type Query { - reviewsByPropertyId: [Review] + reviews: [Review] } `, resolvers: { Query: { - reviewsByPropertyId(_root, args) { - t.equals(Object.keys(args).length, 0, 'property id arg is not passed'); + reviews(_root, args) { + t.equals(Object.keys(args).length, 0, 'no args forwarded to target field'); + return [{ id: 'revid', content: 'some review content'}]; + } + } + } + }); + + const property = new GraphQLComponent({ + types: ` + type Property { + id: ID + reviews: [Review] + } + + type Query { + property: Property + } + `, + resolvers: { + Query: { + async property(_root, _args, context, info) { + const revs = await GraphQLComponent.delegateToComponent(reviews, { + targetRootField: 'reviews', + subPath: 'reviews', + info, + contextValue: context + }) + return { id: '1', reviews: revs }; + } + } + }, + imports: [reviews] + }); + + const result = await graphql.execute({ + document: gql` + query { + property { + id + reviews { + id + content + } + } + } + `, + schema: property.schema, + contextValue: {} + }); + t.deepEqual(result.data, { property: { id: '1', reviews: [{ id: 'revid', content: 'some review content'}]}}, 'propery reviews successfully resolved'); + t.end(); +}); + +Test('delegateToComponent - case 1 - args provided to delegateToComponent', async (t) => { + const reviews = new GraphQLComponent({ + types: ` + type Review { + id: ID + content: String + } + + type Query { + reviews: [Review] + } + `, + resolvers: { + Query: { + reviews(_root, args) { + t.equals(Object.keys(args).length, 0, 'no args forwarded to target field'); + return [{ id: 'revid', content: 'some review content'}]; + } + } + } + }); + + const property = new GraphQLComponent({ + types: ` + type Property { + id: ID + reviews: [Review] + } + + type Query { + property: Property + } + `, + resolvers: { + Query: { + async property(_root, _args, context, info) { + const revs = await GraphQLComponent.delegateToComponent(reviews, { + targetRootField: 'reviews', + subPath: 'reviews', + info, + contextValue: context, + args: { + foo: 'bar' + } + }) + return { id: '1', reviews: revs }; + } + } + }, + imports: [reviews] + }); + + const result = await graphql.execute({ + document: gql` + query { + property { + id + reviews { + id + content + } + } + } + `, + schema: property.schema, + contextValue: {} + }); + t.deepEqual(result.data, { property: { id: '1', reviews: [{ id: 'revid', content: 'some review content'}]}}, 'propery reviews successfully resolved'); + t.end(); +}) + +/* + * case 2: target field has no arguments and calling resolver has arguments + * result: no arguments are forwarded from calling resolver or from the caller + * of delegateToComponent if provided +*/ +Test('delegateToComponent - case 2 - no args provided to delegateToComponent', async (t) => { + const reviews = new GraphQLComponent({ + types: ` + type Review { + id: ID + content: String + } + + type Query { + reviews: [Review] + } + `, + resolvers: { + Query: { + reviews(_root, args) { + t.equals(Object.keys(args).length, 0, 'no args forwarded to target field'); return [{ id: 'revid', content: 'some review content'}]; } } @@ -868,14 +1019,15 @@ Test('delegateToComponent - calling resolver arg is not passed when target root `, resolvers: { Query: { - async propertyById(root, args, context, info) { + async propertyById(_root, args, context, info) { + t.ok(args.id, 'argument present in delegating resolver'); const revs = await GraphQLComponent.delegateToComponent(reviews, { - targetRootField: 'reviewsByPropertyId', + targetRootField: 'reviews', subPath: 'reviews', info, contextValue: context }) - return { id: args.id, reviews: revs } + return { id: '1', reviews: revs }; } } }, @@ -901,7 +1053,7 @@ Test('delegateToComponent - calling resolver arg is not passed when target root t.end(); }); -Test('delegateToComponent - calling resolver arg is passed if target root field has matching arg', async (t) => { +Test('delegateToComponent - case 2 - args provided to delegateToComponent', async (t) => { const reviews = new GraphQLComponent({ types: ` type Review { @@ -910,13 +1062,13 @@ Test('delegateToComponent - calling resolver arg is passed if target root field } type Query { - reviewsByPropertyId(id: ID): [Review] + reviews: [Review] } `, resolvers: { Query: { - reviewsByPropertyId(_root, args) { - t.equals(args.id, '1', 'property id from calling resolver is passed'); + reviews(_root, args) { + t.equals(Object.keys(args).length, 0, 'no args forwarded to target field'); return [{ id: 'revid', content: 'some review content'}]; } } @@ -936,14 +1088,18 @@ Test('delegateToComponent - calling resolver arg is passed if target root field `, resolvers: { Query: { - async propertyById(root, args, context, info) { + async propertyById(_root, args, context, info) { + t.ok(args.id, 'argument present in delegating resolver'); const revs = await GraphQLComponent.delegateToComponent(reviews, { - targetRootField: 'reviewsByPropertyId', + targetRootField: 'reviews', subPath: 'reviews', info, - contextValue: context + contextValue: context, + args: { + foo: 'bar' + } }) - return { id: args.id, reviews: revs } + return { id: '1', reviews: revs }; } } }, @@ -969,7 +1125,14 @@ Test('delegateToComponent - calling resolver arg is passed if target root field t.end(); }); -Test('delegateToComponent - user arg is passed and overrides calling resolver arg', async (t) => { +/* +* case 3: target field has arguments and calling resolver has arguments +* result: matching args to the target field provided by the caller of +* delegateToComponent take priority and are forwarded, otherwise falling back +* to matching args from the calling resolver, no other args are forwarded +*/ + +Test('delegateToComponent - case 3 - calling resolver has matching args/extra args, no args provided to delegateToComponent', async (t) => { const reviews = new GraphQLComponent({ types: ` type Review { @@ -984,8 +1147,9 @@ Test('delegateToComponent - user arg is passed and overrides calling resolver ar resolvers: { Query: { reviewsByPropertyId(_root, args) { - t.equals(Object.keys(args).length, 1, 'only 1 arg is passed'); - t.equals(args.id, '2', 'id arg from delegateToComponent call is passed'); + t.equals(Object.keys(args).length, 1, '1 arg forwarded to target field'); + t.ok(args.id, 'id arg from calling resolver forwarded'); + t.notOk(args.cached, 'cached arg from calling resolver is not forwarded'); return [{ id: 'revid', content: 'some review content'}]; } } @@ -1000,20 +1164,21 @@ Test('delegateToComponent - user arg is passed and overrides calling resolver ar } type Query { - propertyById(id: ID): Property + propertyById(id: ID!, cached: Boolean!): Property } `, resolvers: { Query: { - async propertyById(root, args, context, info) { + async propertyById(_root, args, context, info) { + t.ok(args.id, 'id argument present in delegating resolver'); + t.ok(args.cached, 'cached argument present in resolver'); const revs = await GraphQLComponent.delegateToComponent(reviews, { targetRootField: 'reviewsByPropertyId', subPath: 'reviews', info, contextValue: context, - args: {id: 2} }) - return { id: args.id, reviews: revs }; + return { id: '1', reviews: revs }; } } }, @@ -1023,7 +1188,7 @@ Test('delegateToComponent - user arg is passed and overrides calling resolver ar const result = await graphql.execute({ document: gql` query { - propertyById(id: 1) { + propertyById(id: 1, cached: true) { id reviews { id @@ -1039,7 +1204,7 @@ Test('delegateToComponent - user arg is passed and overrides calling resolver ar t.end(); }); -Test('delegateToComponent - calling resolver and user provided arg are passed', async (t) => { +Test('delegateToComponent - case 3 - calling resolver has matching args/extra args, rest of target args provided by delegateToComponent caller', async (t) => { const reviews = new GraphQLComponent({ types: ` type Review { @@ -1048,14 +1213,18 @@ Test('delegateToComponent - calling resolver and user provided arg are passed', } type Query { - reviewsByPropertyId(id: ID, limit: Int): [Review] + reviewsByPropertyId(id: ID, foo: String, bar: String): [Review] } `, resolvers: { Query: { reviewsByPropertyId(_root, args) { - t.equals(args.id, '1', 'property id from calling resolver is passed'); - t.equals(args.limit, 10, 'limit arg from delegateToComponent is passed'); + t.equals(Object.keys(args).length, 3, '3 args forwarded to target field'); + t.ok(args.id, 'id arg from calling resolver forwarded'); + t.equals(args.id, '1', 'args.id value is 1 from calling resolver'); + t.equals(args.foo, 'foo', 'args.foo provided by delegateToComponent caller is passed with expected value'); + t.equals(args.bar, 'bar', 'args.bar provided by delegateToComponent caller is passed with expected value'); + t.notOk(args.cached, 'args.cached from calling resolver is not forwarded'); return [{ id: 'revid', content: 'some review content'}]; } } @@ -1070,20 +1239,98 @@ Test('delegateToComponent - calling resolver and user provided arg are passed', } type Query { - propertyById(id: ID): Property + propertyById(id: ID!, cached: Boolean!): Property } `, resolvers: { Query: { - async propertyById(root, args, context, info) { + async propertyById(_root, args, context, info) { + t.ok(args.id, 'id argument present in delegating resolver'); + t.ok(args.cached, 'cached argument present in resolver'); const revs = await GraphQLComponent.delegateToComponent(reviews, { targetRootField: 'reviewsByPropertyId', subPath: 'reviews', info, contextValue: context, - args: { limit: 10 } + args: { + foo: 'foo', + bar: 'bar' + } + }); + return { id: '1', reviews: revs }; + } + } + }, + imports: [reviews] + }); + + const result = await graphql.execute({ + document: gql` + query { + propertyById(id: 1, cached: true) { + id + reviews { + id + content + } + } + } + `, + schema: property.schema, + contextValue: {} + }); + t.deepEqual(result.data, { propertyById: { id: '1', reviews: [{ id: 'revid', content: 'some review content'}]}}, 'propery reviews successfully resolved'); + t.end(); +}); + +Test('delegateToComponent - case 3 - calling resolver has a matching arg/no extra args, but matching arg is overridden by arg passed to delegateToComponent', async (t) => { + const reviews = new GraphQLComponent({ + types: ` + type Review { + id: ID + content: String + } + + type Query { + reviewsByPropertyId(id: ID): [Review] + } + `, + resolvers: { + Query: { + reviewsByPropertyId(_root, args) { + t.equals(Object.keys(args).length, 1, '1 arg forwarded to target field'); + t.equals(args.id, '2', 'id arg from calling resolver forwarded and has overridden value'); + return [{ id: 'revid', content: 'some review content'}]; + } + } + } + }); + + const property = new GraphQLComponent({ + types: ` + type Property { + id: ID + reviews: [Review] + } + + type Query { + propertyById(id: ID!): Property + } + `, + resolvers: { + Query: { + async propertyById(_root, args, context, info) { + t.ok(args.id, 'id argument present in delegating resolver'); + const revs = await GraphQLComponent.delegateToComponent(reviews, { + targetRootField: 'reviewsByPropertyId', + subPath: 'reviews', + info, + contextValue: context, + args: { + id: '2' + } }) - return { id: args.id, reviews: revs }; + return { id: '1', reviews: revs }; } } }, @@ -1109,6 +1356,87 @@ Test('delegateToComponent - calling resolver and user provided arg are passed', t.end(); }); +/* +* case 4: target field has arguments and calling resolver has no arguments +* result: caller of delegateToComponent must provide args to forward and will +* be forwarded if provided +*/ + +Test('delegateToComponent - case 4 - delegateToComponent caller provides all args to target field', async (t) => { + const reviews = new GraphQLComponent({ + types: ` + type Review { + id: ID + content: String + } + + type Query { + reviewsById(id: ID!, foo: String!): [Review] + } + `, + resolvers: { + Query: { + reviewsById(_root, args) { + t.equals(Object.keys(args).length, 2, '2 args forwarded to target field'); + t.equals(args.id, '2', 'id arg forwarded and has value passed to delegateToComponent'); + t.equals(args.foo, 'foo', 'foo arg forwarded and has value passed to delegateToComponent'); + return [{ id: 'revid', content: 'some review content'}]; + } + } + } + }); + + const property = new GraphQLComponent({ + types: ` + type Property { + id: ID + reviews: [Review] + } + + type Query { + property: Property + } + `, + resolvers: { + Query: { + async property(_root, args, context, info) { + t.equals(Object.keys(args).length, 0, 'no args present in delegateing resolver'); + const revs = await GraphQLComponent.delegateToComponent(reviews, { + targetRootField: 'reviewsById', + subPath: 'reviews', + info, + contextValue: context, + args: { + id: '2', + foo: 'foo' + } + }) + return { id: '1', reviews: revs }; + } + } + }, + imports: [reviews] + }); + + const result = await graphql.execute({ + document: gql` + query { + property { + id + reviews { + id + content + } + } + } + `, + schema: property.schema, + contextValue: {} + }); + t.deepEqual(result.data, { property: { id: '1', reviews: [{ id: 'revid', content: 'some review content'}]}}, 'propery reviews successfully resolved'); + t.end(); +}); + Test('delegateToComponent - user provided args of various types: ID (as Int), ID (as string), String, Int, Float, Boolean, enum, input object are passed', async (t) => { const reviewsComponent = new GraphQLComponent({ types: ` diff --git a/lib/delegate/index.js b/lib/delegate/index.js index 4a92906..ce6555d 100644 --- a/lib/delegate/index.js +++ b/lib/delegate/index.js @@ -93,44 +93,39 @@ const createSubOperationDocument = function (component, targetRootField, args, s } } - // if the user didn't provide args, default to the calling resolver's args - if (Object.keys(args).length === 0) { - targetRootFieldArguments.push(...callingResolverArgs); - } - else { - // for each target root field defined argument, see if the user provided - // an argument of the same name, if so, construct the argument node - // and remove the calling resolver's matching argument if present - // so that the user provided arg overrides calling resolver args - for (const definedArg of definedRootFieldArgs) { - if (args[definedArg.name]) { - const definedArgNamedType = getNamedType(definedArg.type); - // this provides us some type safety by trying to coerce the user's - // argument value to the type defined by the target field's matching - // argument - if they dont match, it will throw a meaningful error. - // without this astFromValue would coerce things we dont want coerced - const coercedArgValue = coerceInputValue(args[definedArg.name], definedArgNamedType); - const argValueNode = astFromValue(coercedArgValue, definedArgNamedType); - targetRootFieldArguments.push({ - kind: Kind.ARGUMENT, - name: { kind: Kind.NAME, value: definedArg.name }, - value: argValueNode - }); - - if (callingResolverArgs.length > 0) { - // if the calling resolver args has a matching argument (same name) - // remove it so that user provided args "override" - const matchingArgIdx = callingResolverArgs.findIndex((argNode) => { - return argNode.name.value === definedArg.name; - }); - if (matchingArgIdx !== -1) { - callingResolverArgs.splice(matchingArgIdx, 1); - } - } + // for each argument defined for the target root field + // check if the caller of delegateToComponent provided an argument of + // the same name (and type) and forward it on if so + // if not - check the calling resolver's args for an argument of the + // same name and forward it if so. + for (const definedArg of definedRootFieldArgs) { + // a caller of delegateToComponent provided an argument that matches + // the target root field's argument name + if (args[definedArg.name]) { + const definedArgNamedType = getNamedType(definedArg.type); + // this provides us some type safety by trying to coerce the user's + // argument value to the type defined by the target field's matching + // argument - if they dont match, it will throw a meaningful error. + // without this astFromValue would coerce things we dont want coerced + const coercedArgValue = coerceInputValue(args[definedArg.name], definedArgNamedType); + const argValueNode = astFromValue(coercedArgValue, definedArgNamedType); + targetRootFieldArguments.push({ + kind: Kind.ARGUMENT, + name: { kind: Kind.NAME, value: definedArg.name }, + value: argValueNode + }); + } + else { + // search the calling resolver's arguments for an arg with the same + // name as the target root field's defined argument + const matchingArgIdx = callingResolverArgs.findIndex((argNode) => { + return argNode.name.value === definedArg.name; + }); + + if (matchingArgIdx !== -1) { + targetRootFieldArguments.push(callingResolverArgs[matchingArgIdx]); } } - // append any remaining calling resolver args - targetRootFieldArguments.push(...callingResolverArgs); } } diff --git a/lib/resolvers/__tests__.js b/lib/resolvers/__tests__.js index f5382ed..7123134 100644 --- a/lib/resolvers/__tests__.js +++ b/lib/resolvers/__tests__.js @@ -134,7 +134,6 @@ Test('filterResolvers()', (t) => { } } const transformedResolvers = filterResolvers(resolvers); - st.equal(transformedResolvers, resolvers, 'object reference returned from filterResolvers is equal to input reference'); st.deepEqual(transformedResolvers, resolvers, 'object content returned from filterResolvers is equal to input resolver object content'); st.end(); }); @@ -146,7 +145,6 @@ Test('filterResolvers()', (t) => { } } const transformedResolvers = filterResolvers(resolvers, []); - st.equal(transformedResolvers, resolvers, 'object reference returned from filterResolvers is equal to input reference'); st.deepEqual(transformedResolvers, resolvers, 'object content returned from filterResolvers is equal to input resolver object content'); st.end(); }); @@ -166,6 +164,9 @@ Test('filterResolvers()', (t) => { const transformedResolvers = filterResolvers(resolvers, [['*']]); st.deepEqual(transformedResolvers, {}, 'results in an empty resolver map being returned'); + st.ok(resolvers.Query.foo, 'original resolver map maintains structure despite exclusions'); + st.ok(resolvers.Mutation.baz, 'original resolver map maintains structure despite exclusions') + st.ok(resolvers.SomeType.bar, 'original resolver map maintains structure despite exclusions'); st.end(); }); @@ -182,6 +183,8 @@ Test('filterResolvers()', (t) => { const transformedResolvers = filterResolvers(resolvers, [['SomeType']]); st.notOk(transformedResolvers.SomeType, 'entire specified type is excluded'); st.ok(transformedResolvers.Query.foo, 'other non-excluded type remains'); + st.ok(resolvers.Query.foo, 'original resolver map maintains structure despite exclusions'); + st.ok(resolvers.SomeType.bar, 'original resolver map maintains structure despite exclusions'); st.end(); }); @@ -198,6 +201,8 @@ Test('filterResolvers()', (t) => { const transformedResolvers = filterResolvers(resolvers, [['SomeType', '']]); st.notOk(transformedResolvers.SomeType,'entire specified type is excluded'); st.ok(transformedResolvers.Query.foo, 'other non-excluded type remains'); + st.ok(resolvers.Query.foo, 'original resolver map maintains structure despite exclusions'); + st.ok(resolvers.SomeType.bar, 'original resolver map maintains structure despite exclusions'); st.end(); }); @@ -214,6 +219,8 @@ Test('filterResolvers()', (t) => { const transformedResolvers = filterResolvers(resolvers, [['SomeType', '*']]); st.notOk(transformedResolvers.SomeType, 'entire specified type is excluded'); st.ok(transformedResolvers.Query.foo, 'other non-excluded type remains'); + st.ok(resolvers.Query.foo, 'original resolver map maintains structure despite exclusions'); + st.ok(resolvers.SomeType.bar, 'original resolver map maintains structure despite exclusions'); st.end(); }); @@ -232,6 +239,9 @@ Test('filterResolvers()', (t) => { st.notOk(transformedResolvers.SomeType.bar, 'specified field on specified type is removed'); st.ok(transformedResolvers.SomeType.a, 'non-excluded field on specified type remains'); st.ok(transformedResolvers.Query.foo, 'non-exluded type remains'); + st.ok(resolvers.Query.foo, 'original resolver map maintains structure despite exclusions'); + st.ok(resolvers.SomeType.bar, 'original resolver map maintains structure despite exclusions'); + st.ok(resolvers.SomeType.a, 'original resolver map maintains structure despite exclusions'); st.end(); }); @@ -249,6 +259,9 @@ Test('filterResolvers()', (t) => { const transformedResolvers = filterResolvers(resolvers, [['SomeType', 'bar'], ['SomeType', 'a']]); st.notOk(transformedResolvers.SomeType, 'specified type is completely removed because all of its fields were removed'); st.ok(transformedResolvers.Query.foo, 'non-exluded type remains'); + st.ok(resolvers.Query.foo, 'original resolver map maintains structure despite exclusions'); + st.ok(resolvers.SomeType.bar, 'original resolver map maintains structure despite exclusions'); + st.ok(resolvers.SomeType.a, 'original resolver map maintains structure despite exclusions'); st.end(); }) }); diff --git a/lib/resolvers/index.js b/lib/resolvers/index.js index 4e44c7c..86650e8 100644 --- a/lib/resolvers/index.js +++ b/lib/resolvers/index.js @@ -2,6 +2,7 @@ const debug = require('debug')('graphql-component:resolver'); const { GraphQLScalarType } = require('graphql'); +const cloneDeep = require('lodash.clonedeep'); /** * memoizes resolver functions such that calls of an identical resolver (args/context/path) within the same request context are avoided @@ -61,7 +62,7 @@ const filterResolvers = function (resolvers, excludes) { return resolvers; } - let filteredResolvers = Object.assign({}, resolvers); + let filteredResolvers = cloneDeep(resolvers); for (const [type, field] of excludes) { if (type === '*') { diff --git a/package.json b/package.json index a489021..b16e7be 100644 --- a/package.json +++ b/package.json @@ -25,6 +25,7 @@ "cuid": "^2.1.8", "debug": "^4.1.1", "graphql-tools": "^6.0.10", + "lodash.clonedeep": "^4.5.0", "lodash.get": "^4.4.2", "lodash.set": "^4.3.2" },