Redis KeyvAdapter DataLoader throws TypeError because the result is undefined

[HishamAli81]

When using the Redis Keyv and if the Redis client is unable to connect to the Redis cluster, the keyv.get() function will returned undefined.

This in turn would cause the DataLoader used by the KeyvAdapter to throw the following error since returning undefined violates the DataLoader contract:

DataLoader must be constructed with a function which accepts Array<key> and returns Promise<Array<value>>, but the function did not return a Promise of an Array: undefined.

Would suggest changing the following KeyvAdapter code to better handle this error scenario:

this.dataLoader = options?.disableBatchReads
      ? undefined
      : new DataLoader(
          (keys) => this.keyv.get([...keys]),
          // We're not actually using `DataLoader` for its caching
          // capabilities, we're only interested in batching functionality
          { cache: false },
        );

For example, to something like this:

this.dataLoader = options?.disableBatchReads
      ? undefined
      : new DataLoader(
          async (keys) => (await this.keyv.get([...keys])) ?? keys.map(() => null),
          // We're not actually using `DataLoader` for its caching
          // capabilities, we're only interested in batching functionality
          { cache: false },
        );

I haven't tested this with other Keyv implementations so this issue exist with other caches besides Redis. I've also only noticed this issue occurring when Redis can't connect to the cluster, but could possible occur for other type of Redis errors as well.

[trevor-scheer]

Hope this is helpful, let me know if this solves your issue. Can we document this better to make it more discoverable?

#211 (comment)

[HishamAli81]

@trevor-scheer Thanks for the quick reply!

I'm not sure #211 this solves the issue, rather it's the cause of the issue I'm reporting 😄

I agree that an error should not be thrown if the connection is lost, and that using the ErrorsAreMissesCache helps to gracefully handle that error.

The issue occurs when an array of keys are provided to DataLoader, and DataLoader then expects that an array with the same amount of elements are returned by the batch loading function, but since the ErrorsAreMissesCache returns undefined, DataLoader ends up throwing a type error.

So for example, if keys ['foo', 'bar] were passed into the DataLoader batch loading function, and the connection to the cache is severed, then the batch loading function should return [null, null] or [undefined, undefined] instead of undefined.

The DataLoader documentation mentions these constraints on the batch loading function:

There are a few constraints this function must uphold:

• The Array of values must be the same length as the Array of keys.
• Each index in the Array of values must correspond to the same index in the Array of keys.

So, an alternative to the initial suggested fix, would be to instead fix it in ErrorsAreMissesCache, so it checks if an array of keys are provided, then return an array with the same amount of elements that contain null/undefined values. Otherwise return undefined if a single key is provided.

[trevor-scheer]

Thanks for the additional details! I'll have a closer look at this near the end of the week.

[trevor-scheer]

@HishamAli81 thanks for that. I was able to reproduce trivially. I've got a PR open to resolve (as well an open issue on the Keyv repo - there's a bug in either the typings or the runtime).

[HishamAli81]

@trevor-scheer Thanks for the quick turnaround!