diff --git a/.changeset/wicked-islands-march.md b/.changeset/wicked-islands-march.md new file mode 100644 index 000000000..587113978 --- /dev/null +++ b/.changeset/wicked-islands-march.md @@ -0,0 +1,65 @@ +--- +'@graphql-tools/batch-delegate': patch +--- + +Memoize the key arguments correctly; + +With the following schema and resolvers, `userById` should batch all the requests to `usersByIds`; + +```ts +{ + typeDefs: /* GraphQL */ ` + type User { + id: ID! + email: String! + } + type Query { + userById(id: ID!): User + usersByIds(ids: [ID!]): [User] + } + `, + resolvers: { + Query: { + usersByIds: (_root, args) => { + return args.ids.map((id: string) => users.find((user) => user.id === id)); + }, + userById: (root, args, context, info) => { + return batchDelegateToSchema({ + schema: userSubschema, + fieldName: 'usersByIds', + key: args.id, + rootValue: root, + context, + info, + }) + }, + }, + }, + } +``` + +This query should batch all the requests to `usersByIds`: + +```graphql +{ + userById(id: "1") { + id + email + } + userById(id: "2") { + id + email + } +} +``` + +The delegation request should be; + +```graphql +{ + usersByIds(ids: ["1", "2"]) { + id + email + } +} +``` \ No newline at end of file diff --git a/packages/batch-delegate/src/batchDelegateToSchema.ts b/packages/batch-delegate/src/batchDelegateToSchema.ts index e1fb4f2d2..71bd6f921 100644 --- a/packages/batch-delegate/src/batchDelegateToSchema.ts +++ b/packages/batch-delegate/src/batchDelegateToSchema.ts @@ -1,8 +1,8 @@ import { getLoader } from './getLoader.js'; import { BatchDelegateOptions } from './types.js'; -export function batchDelegateToSchema( - options: BatchDelegateOptions, +export function batchDelegateToSchema( + options: BatchDelegateOptions, ): any { const key = options.key; if (key == null) { diff --git a/packages/batch-delegate/src/getLoader.ts b/packages/batch-delegate/src/getLoader.ts index 1090caf99..50f781628 100644 --- a/packages/batch-delegate/src/getLoader.ts +++ b/packages/batch-delegate/src/getLoader.ts @@ -14,9 +14,10 @@ import DataLoader from 'dataloader'; import { getNamedType, GraphQLList, GraphQLSchema, print } from 'graphql'; import { BatchDelegateOptions } from './types.js'; +const DEFAULT_ARGS_FROM_KEYS = (keys: ReadonlyArray) => ({ ids: keys }); + function createBatchFn(options: BatchDelegateOptions) { - const argsFromKeys = - options.argsFromKeys ?? ((keys: ReadonlyArray) => ({ ids: keys })); + const argsFromKeys = options.argsFromKeys ?? DEFAULT_ARGS_FROM_KEYS; const fieldName = options.fieldName ?? options.info.fieldName; const { valuesFromResults, lazyOptionsFn } = options; @@ -103,6 +104,8 @@ export function getLoader( fieldNodes = info.fieldNodes[0] && getActualFieldNodes(info.fieldNodes[0]), selectionSet = fieldNodes?.[0]?.selectionSet, returnType = info.returnType, + argsFromKeys = DEFAULT_ARGS_FROM_KEYS, + key, } = options; const loaders = getLoadersMap(context ?? GLOBAL_CONTEXT, schema); @@ -119,7 +122,11 @@ export function getLoader( const fieldNode = fieldNodes?.[0]; if (fieldNode?.arguments) { - cacheKey += fieldNode.arguments.map((arg) => memoizedPrint(arg)).join(','); + const args = argsFromKeys([key]); + cacheKey += fieldNode.arguments + .filter((arg) => arg.name.value in args) + .map((arg) => memoizedPrint(arg)) + .join(','); } let loader = loaders.get(cacheKey); diff --git a/packages/batch-delegate/tests/basic.example.test.ts b/packages/batch-delegate/tests/basic.example.test.ts index 30b0d2aa3..419de7774 100644 --- a/packages/batch-delegate/tests/basic.example.test.ts +++ b/packages/batch-delegate/tests/basic.example.test.ts @@ -102,7 +102,6 @@ describe('batch delegation within basic stitching example', () => { const chirps: any = result.data!['trendingChirps']; expect(chirps[0].chirpedAtUser.email).not.toBe(null); }); - test('works with key arrays', async () => { let numCalls = 0; @@ -216,7 +215,6 @@ describe('batch delegation within basic stitching example', () => { ], }); }); - test('works with keys passed to lazyOptionsFn', async () => { let numCalls = 0; diff --git a/packages/batch-delegate/tests/cacheByArguments.test.ts b/packages/batch-delegate/tests/cacheByArguments.test.ts index 7f3718b32..c0398986f 100644 --- a/packages/batch-delegate/tests/cacheByArguments.test.ts +++ b/packages/batch-delegate/tests/cacheByArguments.test.ts @@ -1,9 +1,18 @@ import { batchDelegateToSchema } from '@graphql-tools/batch-delegate'; -import { execute, isIncrementalResult } from '@graphql-tools/executor'; +import { + createDefaultExecutor, + SubschemaConfig, +} from '@graphql-tools/delegate'; +import { + execute, + isIncrementalResult, + normalizedExecutor, +} from '@graphql-tools/executor'; import { makeExecutableSchema } from '@graphql-tools/schema'; import { stitchSchemas } from '@graphql-tools/stitch'; +import { Executor } from '@graphql-tools/utils'; import { OperationTypeNode, parse } from 'graphql'; -import { describe, expect, test } from 'vitest'; +import { afterEach, describe, expect, test, vi } from 'vitest'; describe('non-key arguments are taken into account when memoizing result', () => { test('memoizes non-key arguments as part of batch delegation', async () => { @@ -32,7 +41,8 @@ describe('non-key arguments are taken into account when memoizing result', () => const authorSchema = makeExecutableSchema({ typeDefs: /* GraphQL */ ` type User { - email: String! + id: ID! + email(obfuscated: Boolean): String! } type Query { usersByIds(ids: [ID!], obfuscateEmail: Boolean!): [User] @@ -43,10 +53,16 @@ describe('non-key arguments are taken into account when memoizing result', () => usersByIds: (_root, args) => { numCalls++; return args.ids.map((id: string) => ({ + id, email: args.obfuscateEmail ? '***' : `${id}@test.com`, })); }, }, + User: { + email(user, { obfuscated }) { + return obfuscated ? '***' : `${user.email}`; + }, + }, }, }); @@ -54,6 +70,10 @@ describe('non-key arguments are taken into account when memoizing result', () => extend type Chirp { chirpedAtUser(obfuscateEmail: Boolean!): User } + + extend type User { + friends(obfuscateEmail: Boolean!): [User] + } `; const stitchedSchema = stitchSchemas({ @@ -76,6 +96,27 @@ describe('non-key arguments are taken into account when memoizing result', () => }, }, }, + User: { + friends: { + selectionSet: `{ id }`, + resolve(user: { id: string }, args, context, info) { + return batchDelegateToSchema({ + schema: authorSchema, + operation: 'query' as OperationTypeNode, + fieldName: 'usersByIds', + key: user.id === '1' ? ['2', '4'] : ['1', '3'], + argsFromKeys: (friendIdsList) => { + return { + ids: [...new Set(friendIdsList.flat())], + ...args, + }; + }, + context, + info, + }); + }, + }, + }, }, }); @@ -83,10 +124,36 @@ describe('non-key arguments are taken into account when memoizing result', () => query { trendingChirps { withObfuscatedEmail: chirpedAtUser(obfuscateEmail: true) { + # Batch 1 + id email + friendsWithoutObfuscatedEmail: friends(obfuscateEmail: false) { + # Batch 3 + id + email + } + friendsWithObfuscatedEmail: friends(obfuscateEmail: true) { + # Batch 4 + id + email + } } withoutObfuscatedEmail: chirpedAtUser(obfuscateEmail: false) { + # Batch 2 + id email + friendsWithoutObfuscatedEmailArgButEmailObfuscatedArg: friends( + obfuscateEmail: false + ) { + # Batch 5 + id + email(obfuscated: true) + } + friendsWithObfuscatedEmail: friends(obfuscateEmail: true) { + # Batch 4 + id + email + } } } } @@ -97,17 +164,283 @@ describe('non-key arguments are taken into account when memoizing result', () => document: parse(query), }); - expect(numCalls).toEqual(2); + // According to the query, we expect 5 calls + expect(numCalls).toEqual(5); if (isIncrementalResult(result)) throw Error('result is incremental'); - expect(result.errors).toBeUndefined(); - - const chirps: any = result.data!['trendingChirps']; - expect(chirps[0].withObfuscatedEmail.email).toBe(`***`); - expect(chirps[1].withObfuscatedEmail.email).toBe(`***`); - - expect(chirps[0].withoutObfuscatedEmail.email).toBe(`1@test.com`); - expect(chirps[1].withoutObfuscatedEmail.email).toBe(`2@test.com`); + expect(result).toEqual({ + data: { + trendingChirps: [ + { + withObfuscatedEmail: { + email: '***', + friendsWithObfuscatedEmail: [ + { + email: '***', + id: '2', + }, + { + email: '***', + id: '4', + }, + ], + friendsWithoutObfuscatedEmail: [ + { + email: '2@test.com', + id: '2', + }, + { + email: '4@test.com', + id: '4', + }, + ], + id: '1', + }, + withoutObfuscatedEmail: { + email: '1@test.com', + friendsWithObfuscatedEmail: [ + { + email: '***', + id: '2', + }, + { + email: '***', + id: '4', + }, + ], + friendsWithoutObfuscatedEmailArgButEmailObfuscatedArg: [ + { + email: '***', + id: '2', + }, + { + email: '***', + id: '4', + }, + ], + id: '1', + }, + }, + { + withObfuscatedEmail: { + email: '***', + friendsWithObfuscatedEmail: [ + { + email: '***', + id: '1', + }, + { + email: '***', + id: '3', + }, + ], + friendsWithoutObfuscatedEmail: [ + { + email: '1@test.com', + id: '1', + }, + { + email: '3@test.com', + id: '3', + }, + ], + id: '2', + }, + withoutObfuscatedEmail: { + email: '2@test.com', + friendsWithObfuscatedEmail: [ + { + email: '***', + id: '1', + }, + { + email: '***', + id: '3', + }, + ], + friendsWithoutObfuscatedEmailArgButEmailObfuscatedArg: [ + { + email: '***', + id: '1', + }, + { + email: '***', + id: '3', + }, + ], + id: '2', + }, + }, + ], + }, + }); + }); + describe('memoizes key arguments as part of batch delegation', () => { + const users = [ + { id: '1', email: 'john@doe.com', friends: [{ id: '1' }], entity: true }, + { id: '2', email: 'jane@doe.com', friends: [{ id: '2' }], entity: true }, + ]; + const getUsersByIds = vi.fn((ids: string[]) => + ids.map((id: string) => users.find((user) => user.id === id)), + ); + const getArgsFromFriendIds = vi.fn(function ( + friendIdsList: readonly string[][], + ) { + return { + ids: [...new Set(friendIdsList.flat())], + }; + }); + const getFriendIdsFromUser = vi.fn((user: { friends: typeof users }) => + user.friends.map((friend) => friend.id), + ); + const userSchema = makeExecutableSchema({ + typeDefs: /* GraphQL */ ` + type User { + id: ID! + email: String! + friends: [User] + } + type Query { + userById(id: ID!): User + usersByIds(ids: [ID!]): [User] + } + `, + resolvers: { + Query: { + usersByIds: (_root, args) => { + return getUsersByIds(args.ids); + }, + userById: (root, args, context, info) => { + return batchDelegateToSchema({ + schema: userSubschema, + fieldName: 'usersByIds', + key: args.id, + rootValue: root, + context, + info, + }); + }, + }, + User: { + friends(root: { friends: typeof users }, _args, context, info) { + return batchDelegateToSchema({ + schema: userSubschema, + fieldName: 'usersByIds', + key: getFriendIdsFromUser(root), + argsFromKeys: getArgsFromFriendIds, + context, + info, + }); + }, + }, + }, + }); + const executorFn = vi.fn(createDefaultExecutor(userSchema)); + const userSubschema: SubschemaConfig = { + schema: userSchema, + executor: executorFn as Executor, + }; + afterEach(() => { + getUsersByIds.mockClear(); + executorFn.mockClear(); + }); + test('root level', async () => { + const result = await normalizedExecutor({ + schema: userSchema, + document: parse(/* GraphQL */ ` + query { + user1: userById(id: "1") { + email + } + user2: userById(id: "2") { + email + } + } + `), + }); + expect(result).toEqual({ + data: { + user1: { email: 'john@doe.com' }, + user2: { email: 'jane@doe.com' }, + }, + }); + expect(executorFn).toHaveBeenCalledTimes(1); + expect(getUsersByIds).toHaveBeenCalledTimes(1); + expect(getUsersByIds).toHaveBeenCalledWith(['1', '2']); + }); + test('nested level', async () => { + const result = await normalizedExecutor({ + schema: userSchema, + document: parse(/* GraphQL */ ` + query { + user1: userById(id: "1") { + id + email + friends { + id + email + friends { + id + email + } + } + } + user2: userById(id: "2") { + id + email + friends { + id + email + friends { + id + email + } + } + } + } + `), + }); + expect(result).toEqual({ + data: { + user1: { + email: 'john@doe.com', + friends: [ + { + email: 'john@doe.com', + friends: [ + { + email: 'john@doe.com', + id: '1', + }, + ], + id: '1', + }, + ], + id: '1', + }, + user2: { + email: 'jane@doe.com', + friends: [ + { + email: 'jane@doe.com', + friends: [ + { + email: 'jane@doe.com', + id: '2', + }, + ], + id: '2', + }, + ], + id: '2', + }, + }, + }); + // For each level of the query, we expect a single executor call + // So we have 3 levels in this query, so we expect 3 executor calls + expect(executorFn).toHaveBeenCalledTimes(3); + expect(getUsersByIds).toHaveBeenCalledWith(['1', '2']); + }); }); });