diff --git a/API_DOCS.md b/API_DOCS.md index f80b3ea..d07d8af 100644 --- a/API_DOCS.md +++ b/API_DOCS.md @@ -93,6 +93,7 @@ resources: nestedPath: ?string (can only use if isBatchResource=true) commaSeparatedBatchKey: ?string (can only use if isBatchResource=true) isResponseDictionary: ?boolean (can only use if isBatchResource=true) + isBatchKeyASet: ?boolean (can only use if isBatchResource=true) typings: language: flow @@ -113,16 +114,17 @@ Describes the shape and behaviour of the resources object you will pass to `getL #### `resources` Parameters -| Key | Value Description | -| ------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `isBatchResource` | Is this a batch resource? (Can you pass it a list of keys and get a list of results back?) | -| `docsLink` | The URL for the documentation of the resource. Useful for others to verify information is correct, and may be used in stack traces. | -| `batchKey` | The argument to the resource that represents the list of entities we want to fetch. (e.g. 'user_ids') | -| `newKey` | The argument we'll replace the batchKey with - should be a singular version of the `batchKey` (e.g. 'user_id') | -| `reorderResultsByKey` | (Optional) If the resource itself does not guarantee ordering, use this to specify which key in the response objects corresponds to an element in `batchKey`. Transforms and re-order the response to the same order as requested from the DataLoaders. | -| `nestedPath` | (Optional) If the resource returns the list of results in a nested path (e.g. `{ results: [ 1, 2, 3 ] }`), this tells the DataLoader where in the response to find the results. (e.g. 'results'). | -| `commaSeparatedBatchKey` | (Optional) Set to true if the interface of the resource takes the batch key as a comma separated list (rather than an array of IDs, as is more common). Default: false | -| `isResponseDictionary` | (Optional) Set to true if the batch resource returns the results as a dictionary with key mapped to values (instead of a list of items). If this option is supplied `reorderResultsByKey` should not be. Default: false | +| Key | Value Description | +| ------------------------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `isBatchResource` | Is this a batch resource? (Can you pass it a list of keys and get a list of results back?) | +| `docsLink` | The URL for the documentation of the resource. Useful for others to verify information is correct, and may be used in stack traces. | +| `batchKey` | The argument to the resource that represents the list of entities we want to fetch. (e.g. 'user_ids') | +| `newKey` | The argument we'll replace the batchKey with - should be a singular version of the `batchKey` (e.g. 'user_id') | +| `reorderResultsByKey` | (Optional) If the resource itself does not guarantee ordering, use this to specify which key in the response objects corresponds to an element in `batchKey`. Transforms and re-order the response to the same order as requested from the DataLoaders. | +| `nestedPath` | (Optional) If the resource returns the list of results in a nested path (e.g. `{ results: [ 1, 2, 3 ] }`), this tells the DataLoader where in the response to find the results. (e.g. 'results'). | +| `commaSeparatedBatchKey` | (Optional) Set to true if the interface of the resource takes the batch key as a comma separated list (rather than an array of IDs, as is more common). Default: false | +| `isResponseDictionary` | (Optional) Set to true if the batch resource returns the results as a dictionary with key mapped to values (instead of a list of items). If this option is supplied `reorderResultsByKey` should not be. Default: false | +| `isBatchKeyASet` | (Optional) Set to true if the interface of the resource takes the batch key as a set (rather than an array). For example, when using a generated clientlib based on swagger where `uniqueItems: true` is set for the batchKey parameter. Default: false. | ### `typings` diff --git a/__tests__/genTypeFlow.test.js b/__tests__/genTypeFlow.test.js index 4cd8fe9..ccd0539 100644 --- a/__tests__/genTypeFlow.test.js +++ b/__tests__/genTypeFlow.test.js @@ -1,7 +1,20 @@ -import { getResourceTypeReference } from '../src/genTypeFlow'; +import { getResourceTypeReference, getNewKeyTypeFromBatchKeySetType } from '../src/genTypeFlow'; it('getResourceTypeReference converts a resource path to a valid reference', () => { expect(getResourceTypeReference(null, ['foo', 'bar', 'baz'])).toBe( "$PropertyType<$PropertyType<$PropertyType, 'bar'>, 'baz'>", ); }); + +it('getNewKeyTypeFromBatchKeySetType returns a newKey type with a valid value', () => { + expect( + getNewKeyTypeFromBatchKeySetType( + 'bKey', + "$PropertyType<$PropertyType<$PropertyType, 'bar'>, 'baz'>", + ), + ).toBe(`\ + $Call< + ExtractArg, + [$PropertyType<$PropertyType<$PropertyType<$PropertyType<$PropertyType, 'bar'>, 'baz'>, 'bKey'>, 'has'>] + >`); +}); diff --git a/__tests__/implementation.test.js b/__tests__/implementation.test.js index c366e87..f4e3767 100644 --- a/__tests__/implementation.test.js +++ b/__tests__/implementation.test.js @@ -843,6 +843,59 @@ test('batch endpoint with isResponseDictionary handles a response that returns a }); }); +test('batch endpoint with isBatchKeyASet handles a response', async () => { + const config = { + resources: { + foo: { + isBatchResource: true, + docsLink: 'example.com/docs/bar', + batchKey: 'foo_ids', + newKey: 'foo_id', + isBatchKeyASet: true, + }, + }, + }; + + const resources = { + foo: ({ foo_ids, include_extra_info }) => { + if (_.isEqual(foo_ids, [1, 2])) { + expect(include_extra_info).toBe(false); + return Promise.resolve([ + { foo_id: 1, foo_value: 'hello' }, + { foo_id: 2, foo_value: 'world' }, + ]); + } + + if (_.isEqual(foo_ids, [3])) { + expect(include_extra_info).toBe(true); + return Promise.resolve([ + { + foo_id: 3, + foo_value: 'greetings', + extra_stuff: 'lorem ipsum', + }, + ]); + } + }, + }; + + await createDataLoaders(config, async (getLoaders) => { + const loaders = getLoaders(resources); + + const results = await loaders.foo.loadMany([ + { foo_id: 1, include_extra_info: false }, + { foo_id: 2, include_extra_info: false }, + { foo_id: 3, include_extra_info: true }, + ]); + + expect(results).toEqual([ + { foo_id: 1, foo_value: 'hello' }, + { foo_id: 2, foo_value: 'world' }, + { foo_id: 3, foo_value: 'greetings', extra_stuff: 'lorem ipsum' }, + ]); + }); +}); + test('batch endpoint with isResponseDictionary handles a response that returns a dictionary, with a missing item', async () => { const config = { resources: { diff --git a/examples/swapi/swapi-loaders.js b/examples/swapi/swapi-loaders.js index 12e23dd..6381a26 100644 --- a/examples/swapi/swapi-loaders.js +++ b/examples/swapi/swapi-loaders.js @@ -146,6 +146,36 @@ export type LoadersType = $ReadOnly<{| // This third argument is the cache key type. Since we use objectHash in cacheKeyOptions, this is "string". string, >, + getFilms: DataLoader< + {| + ...$Diff< + $Call]>, + { + film_ids: $PropertyType<$Call]>, 'film_ids'>, + }, + >, + ...{| + film_id: $Call< + ExtractArg, + [ + $PropertyType< + $PropertyType<$Call]>, 'film_ids'>, + 'has', + >, + ], + >, + |}, + |}, + $ElementType< + $Call< + ExtractPromisedReturnValue<[$Call]>]>, + $PropertyType, + >, + 0, + >, + // This third argument is the cache key type. Since we use objectHash in cacheKeyOptions, this is "string". + string, + >, getRoot: DataLoader< $Call]>, $Call< @@ -978,6 +1008,284 @@ export default function getLoaders(resources: ResourcesType, options?: DataLoade ...cacheKeyOptions, }, ), + getFilms: new DataLoader< + {| + ...$Diff< + $Call]>, + { + film_ids: $PropertyType< + $Call]>, + 'film_ids', + >, + }, + >, + ...{| + film_id: $Call< + ExtractArg, + [ + $PropertyType< + $PropertyType< + $Call]>, + 'film_ids', + >, + 'has', + >, + ], + >, + |}, + |}, + $ElementType< + $Call< + ExtractPromisedReturnValue<[$Call]>]>, + $PropertyType, + >, + 0, + >, + // This third argument is the cache key type. Since we use objectHash in cacheKeyOptions, this is "string". + string, + >( + /** + * =============================================================== + * Generated DataLoader: getFilms + * =============================================================== + * + * Resource Config: + * + * ```json + * { + * "docsLink": "https://swapi.dev/documentation#films", + * "isBatchResource": true, + * "batchKey": "film_ids", + * "newKey": "film_id", + * "isBatchKeyASet": true + * } + * ``` + */ + async (keys) => { + invariant( + typeof resources.getFilms === 'function', + [ + '[dataloader-codegen :: getFilms] resources.getFilms is not a function.', + 'Did you pass in an instance of getFilms to "getLoaders"?', + ].join(' '), + ); + + /** + * Chunk up the "keys" array to create a set of "request groups". + * + * We're about to hit a batch resource. In addition to the batch + * key, the resource may take other arguments too. When batching + * up requests, we'll want to look out for where those other + * arguments differ, and send multiple requests so we don't get + * back the wrong info. + * + * In other words, we'll potentially want to send _multiple_ + * requests to the underlying resource batch method in this + * dataloader body. + * + * ~~~ Why? ~~~ + * + * Consider what happens when we get called with arguments where + * the non-batch keys differ. + * + * Example: + * + * ```js + * loaders.foo.load({ foo_id: 2, include_private_data: true }); + * loaders.foo.load({ foo_id: 3, include_private_data: false }); + * loaders.foo.load({ foo_id: 4, include_private_data: false }); + * ``` + * + * If we collected everything up and tried to send the one + * request to the resource as a batch request, how do we know + * what the value for "include_private_data" should be? We're + * going to have to group these up up and send two requests to + * the resource to make sure we're requesting the right stuff. + * + * e.g. We'd need to make the following set of underlying resource + * calls: + * + * ```js + * foo({ foo_ids: [ 2 ], include_private_data: true }); + * foo({ foo_ids: [ 3, 4 ], include_private_data: false }); + * ``` + * + * ~~~ tl;dr ~~~ + * + * When we have calls to .load with differing non batch key args, + * we'll need to send multiple requests to the underlying + * resource to make sure we get the right results back. + * + * Let's create the request groups, where each element in the + * group refers to a position in "keys" (i.e. a call to .load) + * + * Example: + * + * ```js + * partitionItems([ + * { bar_id: 7, include_extra_info: true }, + * { bar_id: 8, include_extra_info: false }, + * { bar_id: 9, include_extra_info: true }, + * ], 'bar_id') + * ``` + * + * Returns: + * `[ [ 0, 2 ], [ 1 ] ]` + * + * We'll refer to each element in the group as a "request ID". + */ + const requestGroups = partitionItems('film_id', keys); + + // Map the request groups to a list of Promises - one for each request + const groupedResults = await Promise.all( + requestGroups.map(async (requestIDs) => { + /** + * Select a set of elements in "keys", where all non-batch + * keys should be identical. + * + * We're going to smoosh all these together into one payload to + * send to the resource as a batch request! + */ + const requests = requestIDs.map((id) => keys[id]); + + // For now, we assume that the dataloader key should be the first argument to the resource + // @see https://github.com/Yelp/dataloader-codegen/issues/56 + const resourceArgs = [ + { + ..._.omit(requests[0], 'film_id'), + ['film_ids']: requests.map((k) => k['film_id']), + }, + ]; + + let response = await (async (_resourceArgs) => { + // Make a re-assignable variable so flow/eslint doesn't complain + let __resourceArgs = _resourceArgs; + + if (options && options.resourceMiddleware && options.resourceMiddleware.before) { + __resourceArgs = await options.resourceMiddleware.before(['getFilms'], __resourceArgs); + } + + let _response; + try { + // Finally, call the resource! + _response = await resources.getFilms(...__resourceArgs); + } catch (error) { + const errorHandler = + options && typeof options.errorHandler === 'function' + ? options.errorHandler + : defaultErrorHandler; + + /** + * Apply some error handling to catch and handle all errors/rejected promises. errorHandler must return an Error object. + * + * If we let errors here go unhandled here, it will bubble up and DataLoader will return an error for all + * keys requested. We can do slightly better by returning the error object for just the keys in this batch request. + */ + _response = await errorHandler(['getFilms'], error); + + // Check that errorHandler actually returned an Error object, and turn it into one if not. + if (!(_response instanceof Error)) { + _response = new Error( + [ + `[dataloader-codegen :: getFilms] Caught an error, but errorHandler did not return an Error object.`, + `Instead, got ${typeof _response}: ${util.inspect(_response)}`, + ].join(' '), + ); + } + } + + if (options && options.resourceMiddleware && options.resourceMiddleware.after) { + _response = await options.resourceMiddleware.after(['getFilms'], _response); + } + + return _response; + })(resourceArgs); + + if (!(response instanceof Error)) { + } + + if (!(response instanceof Error)) { + if (!Array.isArray(response)) { + response = new Error( + ['[dataloader-codegen :: getFilms]', 'Expected response to be an array!'].join(' '), + ); + } + } + + if (!(response instanceof Error)) { + /** + * Check to see the resource contains the same number + * of items that we requested. If not, since there's + * no "reorderResultsByKey" specified for this resource, + * we don't know _which_ key's response is missing. Therefore + * it's unsafe to return the response array back. + */ + if (response.length !== requests.length) { + /** + * We must return errors for all keys in this group :( + */ + response = new BatchItemNotFoundError( + [ + `[dataloader-codegen :: getFilms] Resource returned ${response.length} items, but we requested ${requests.length} items.`, + 'Add reorderResultsByKey to the config for this resource to be able to handle a partial response.', + ].join(' '), + ); + + // Tell flow that BatchItemNotFoundError extends Error. + // It's an issue with flowgen package, but not an issue with Flow. + // @see https://github.com/Yelp/dataloader-codegen/pull/35#discussion_r394777533 + invariant(response instanceof Error, 'expected BatchItemNotFoundError to be an Error'); + } + } + + /** + * If the resource returns an Error, we'll want to copy and + * return that error as the return value for every request in + * this group. + * + * This allow the error to be cached, and allows the rest of the + * requests made by this DataLoader to succeed. + * + * @see https://github.com/graphql/dataloader#caching-errors + */ + if (response instanceof Error) { + response = requestIDs.map((requestId) => { + /** + * Since we're returning an error object and not the + * expected return type from the resource, this element + * would be unsortable, since it wouldn't have the + * "reorderResultsByKey" attribute. + * + * Let's add it to the error object, as "reorderResultsByValue". + * + * (If we didn't specify that this resource needs + * sorting, then this will be "null" and won't be used.) + */ + const reorderResultsByValue = null; + + // Tell flow that "response" is actually an error object. + // (This is so we can pass it as 'cause' to CaughtResourceError) + invariant(response instanceof Error, 'expected response to be an error'); + + return new CaughtResourceError( + `[dataloader-codegen :: getFilms] Caught error during call to resource. Error: ${response.stack}`, + response, + reorderResultsByValue, + ); + }); + } + + return response; + }), + ); + + // Split the results back up into the order that they were requested + return unPartitionResults(requestGroups, groupedResults); + }, + { + ...cacheKeyOptions, + }, + ), getRoot: new DataLoader< $Call]>, $Call< diff --git a/examples/swapi/swapi-server.js b/examples/swapi/swapi-server.js index f81b292..ed63433 100644 --- a/examples/swapi/swapi-server.js +++ b/examples/swapi/swapi-server.js @@ -14,8 +14,15 @@ const createSWAPIServer = () => { diameter: String } + type Film { + title: String + episodeNumber: Int + director: String + } + type Query { planet(id: Int): Planet + film(id: Int): Film } `); @@ -63,10 +70,57 @@ const createSWAPIServer = () => { } } + class FilmModel { + id: number; + + constructor(id: number) { + this.id = id; + } + + async title() { + const response = await swapiLoaders.getFilms.load({ film_id: this.id }); + + if (response instanceof Error) { + return response; + } + + if (response) { + return response.title; + } + } + + async episodeNumber() { + const response = await swapiLoaders.getFilms.load({ film_id: this.id }); + + if (response instanceof Error) { + return response; + } + + if (response) { + return response.episode_id; + } + } + + async director() { + const response = await swapiLoaders.getFilms.load({ film_id: this.id }); + + if (response instanceof Error) { + return response; + } + + if (response) { + return response.director; + } + } + } + const root = { planet: ({ id }) => { return new PlanetModel(id); }, + film: ({ id }) => { + return new FilmModel(id); + }, }; return { schema, root }; @@ -90,6 +144,17 @@ runQuery(/* GraphQL */ ` dagobah: planet(id: 5) { name } + + meh: film(id: 1) { + title + episodeNumber + director + } + theBest: film(id: 4) { + title + episodeNumber + director + } } `).then((result) => { console.log(JSON.stringify(result, null, 4)); diff --git a/examples/swapi/swapi.dataloader-config.yaml b/examples/swapi/swapi.dataloader-config.yaml index 6694107..6e35f3a 100644 --- a/examples/swapi/swapi.dataloader-config.yaml +++ b/examples/swapi/swapi.dataloader-config.yaml @@ -22,6 +22,12 @@ resources: isBatchResource: true batchKey: vehicle_ids newKey: vehicle_id + getFilms: + docsLink: https://swapi.dev/documentation#films + isBatchResource: true + batchKey: film_ids + newKey: film_id + isBatchKeyASet: true getRoot: docsLink: https://swapi.dev/documentation#root isBatchResource: false diff --git a/examples/swapi/swapi.js b/examples/swapi/swapi.js index 68e11df..83945f2 100644 --- a/examples/swapi/swapi.js +++ b/examples/swapi/swapi.js @@ -43,6 +43,23 @@ export type SWAPI_Person = $ReadOnly<{| url: string, |}>; +export type SWAPI_Film = $ReadOnly<{| + title: string, + episode_id: number, + opening_crawl: string, + director: string, + producer: string, + release_date: string, + species: $ReadOnlyArray, + starships: $ReadOnlyArray, + vehicles: $ReadOnlyArray, + characters: $ReadOnlyArray, + planets: $ReadOnlyArray, + url: string, + created: string, + edited: string, +|}>; + export type SWAPI_Vehicle = $ReadOnly<{| name: string, key: string, @@ -61,6 +78,7 @@ export type SWAPIClientlibTypes = {| getPlanets: ({| planet_ids: $ReadOnlyArray |}) => Promise<$ReadOnlyArray>, getPeople: ({| people_ids: $ReadOnlyArray |}) => Promise<$ReadOnlyArray>, getVehicles: ({| vehicle_ids: $ReadOnlyArray |}) => Promise<$ReadOnlyArray>, + getFilms: ({| film_ids: Set |}) => Promise<$ReadOnlyArray>, getRoot: ({||}) => Promise, |}; @@ -78,6 +96,10 @@ module.exports = function (): SWAPIClientlibTypes { Promise.all( vehicle_ids.map((id) => fetch(url.resolve(SWAPI_URL, `vehicles/${id}`)).then((res) => res.json())), ), + getFilms: ({ film_ids }) => + Promise.all( + [...film_ids].map((id) => fetch(url.resolve(SWAPI_URL, `films/${id}`)).then((res) => res.json())), + ), getRoot: ({}) => fetch(SWAPI_URL).then((res) => res.json()), }; }; diff --git a/schema.json b/schema.json index faacc95..c4bfe37 100644 --- a/schema.json +++ b/schema.json @@ -93,6 +93,10 @@ "isResponseDictionary": { "type": "boolean", "description": "(Optional) Set to true if the batch resource returns the results as a dictionary with key mapped to values (instead of a list of items). If this option is supplied `reorderResultsByKey` should not be. Default: false" + }, + "isBatchKeyASet": { + "type": "boolean", + "description": "(Optional) Set to true if the interface of the resource takes the batch key as a set (rather than an array). For example, when using a generated clientlib based on swagger where `uniqueItems: true` is set for the batchKey parameter. Default: false" } } }, diff --git a/src/config.ts b/src/config.ts index 965576e..ffe7301 100644 --- a/src/config.ts +++ b/src/config.ts @@ -25,6 +25,7 @@ export interface BatchResourceConfig { commaSeparatedBatchKey?: boolean; // TODO: Assert somehow/somewhere that both isResponseDictionary and reorderResultsByKey aren't set isResponseDictionary?: boolean; + isBatchKeyASet?: boolean; } export interface NonBatchResourceConfig { diff --git a/src/genTypeFlow.ts b/src/genTypeFlow.ts index 6484cb3..8b0cbf3 100644 --- a/src/genTypeFlow.ts +++ b/src/genTypeFlow.ts @@ -36,18 +36,47 @@ function getResourceArg(resourceConfig: ResourceConfig, resourcePath: ReadonlyAr >`; } +/** + * Extract the type T from a Set resource (in this case a batchKey's resource) + * using its `.has(T)`'s function paremeter type + */ +export function getNewKeyTypeFromBatchKeySetType(batchKey: string, resourceArgs: string) { + return `\ + $Call< + ExtractArg, + [$PropertyType<$PropertyType<${resourceArgs}, '${batchKey}'>, 'has'>] + >`; +} + export function getLoaderTypeKey(resourceConfig: ResourceConfig, resourcePath: ReadonlyArray) { // TODO: We assume that the resource accepts a single dict argument. Let's // make this configurable to handle resources that use seperate arguments. const resourceArgs = getResourceArg(resourceConfig, resourcePath); - return resourceConfig.isBatchResource - ? ` - {| - ...$Diff<${resourceArgs}, { ${resourceConfig.batchKey}: $PropertyType<${resourceArgs}, '${resourceConfig.batchKey}'> }>, - ...{| ${resourceConfig.newKey}: $ElementType<$PropertyType<${resourceArgs}, '${resourceConfig.batchKey}'>, 0> |} - |}` - : resourceArgs; + if (resourceConfig.isBatchResource) { + // Extract newKeyType from the batch key's Array's type + let newKeyType = `${resourceConfig.newKey}: $ElementType<$PropertyType<${resourceArgs}, '${resourceConfig.batchKey}'>, 0>`; + + if (resourceConfig.isBatchKeyASet) { + /** + * New key's type has to be extracted differently if the batch key's resource + * expects them in the form of a Set + */ + newKeyType = `${resourceConfig.newKey}: ${getNewKeyTypeFromBatchKeySetType( + resourceConfig.batchKey, + resourceArgs, + )}`; + } + + return `{| + ...$Diff<${resourceArgs}, { + ${resourceConfig.batchKey}: $PropertyType<${resourceArgs}, '${resourceConfig.batchKey}'> + }>, + ...{| ${newKeyType} |} + |}`; + } + + return resourceArgs; } export function getLoaderTypeVal(resourceConfig: ResourceConfig, resourcePath: ReadonlyArray) {