[KeyvAdapter] The Array of values must be the same length as the Array of keys

[Grmiade]

We have this error when the queries are not yet cached: 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 of the same length as the Array of keys.

const server = new ApolloServer({
  ...,
  cache: new KeyvAdapter(new Keyv(process.env.CACHE_REDIS_URL)),
}

See https://github.com/apollographql/apollo-utils/blob/main/packages/keyvAdapter/src/index.ts#L27 As mentioned in the DataLoader documentation, the Array of values must be the same length as the Array of keys. And it seems this.keyv.get does not ensure that. We should probably ensure that in the KeyvAdapter.

Workaround: set disableBatchReads to true

[dcrdev]

We're also experiencing this

[trevor-scheer]

Can one of you help me out with a reproduction? I've tried the edge cases I can reasonably imagine and these don't seem to break the usage of dataloader. Something that I can clone and run, or a failing test case in this repo that demonstrates the problem would be great.

Working on this PR: https://github.com/apollographql/apollo-utils/pull/167 Here's what I've tried as a test to reproduce (within that PR): https://github.com/apollographql/apollo-utils/pull/167/commits/ee48aeecc734d1e352cb203edccec2be0adbd1bf

[trevor-scheer]

Of course the lightbulb goes off immediately after I give up, seems obvious after the fact. Reproduction here: https://github.com/apollographql/apollo-utils/pull/167/commits/4f4aa340307647c75f6dd21e8c760fbe184be3dc

So if the getMany function returns an array shorter than the length of keys, DataLoader gets upset. This condition seems like an issue with the Redis client. In a get many scenario, shouldn't Redis be returning null/undefined in array slots where there was no entry? There's nothing "smart" we can do at this point in the cache to handle this failure mode.

This issue should probably bubble up to keyv or ioredis.

[trevor-scheer]

Determined this is almost certainly due to Keyv's getMany implementation returning an empty array in the all-cache-miss case. More info in the linked issue mention^.

[Grmiade]

@trevor-scheer Since when all keys are missing getMany will return an empty array, maybe we could use this logic in order to ensure the array of values has always the same length as the array of keys.

new DataLoader(
  async (keys) => {
    const values = await this.keyv.get(keys as string[]);
    if (values.length === 0) return keys.map(() => undefined);
    return values;
  },
  // We're not actually using `DataLoader` for its caching
  // capabilities, we're only interested in batching functionality
  { cache: false },
);

[dcrdev]

Or

new DataLoader(
  async (keys) => {
    const values = await this.keyv.get(keys as string[]);
    if (!values.length) return Array(keys.length ?? 0).fill(null);
    return values;
  },
  // We're not actually using `DataLoader` for its caching
  // capabilities, we're only interested in batching functionality
  { cache: false },
);

Which is about 30% faster

---

I would say that the way keyv has implemented getMany is a fairly standard way of implementing getMany, or atleast that's how I've seen it implemented in the world of ORMs; which I know is a slightly different space.

[trevor-scheer]

We'll see what @jaredwray thinks about my comment on the PR and go from there. If they are happy with their current implementation then we will fix it here.

[jaredwray]

@trevor-scheer - thanks for the notice and we are going to look at this and the current implementation. It sounds like what would work better for you is if you did a call to getMany() it will return a full array of nulls if they were all cache misses. Correct?

[trevor-scheer]

@jaredwray thats correct. It looks like that would significantly simplify the getMany implementations as well by removing that special case.

[jaredwray]

@trevor-scheer - agreed that we could look to just send back an array of nulls [null, null, null] instead. Is that what you are thinking? If so, I can put a feature on this and look getting that in place in August 22.

[trevor-scheer]

@jaredwray yep, exactly that (or possibly [undefined, undefined, ...], depending on how the cache client behaves - I'd err on the side of returning whatever the client returns i.e. just returning data here, whatever it may be: https://github.com/jaredwray/keyv/blob/main/packages/keyv/src/index.js#L140-L148).

I'm not sure whether you'd call this a bug fix or breaking change - something to consider when versioning the change. If you'd like any help from me, I'd be happy to open a PR.

[trevor-scheer]

@Grmiade / @dcrdev, if this bug is blocking for you, you can open a PR with the workaround either of you have proposed and use the CodeSandbox builds for keyvadapter in the meantime. It'll give you a usable build based on the changes in your PR.

[jaredwray]

@trevor-scheer @Grmiade and @dcrdev - adding this bug fix here and starting to work on it next week: https://github.com/jaredwray/keyv/issues/431 🎉

[jaredwray]

@trevor-scheer @Grmiade @dcrdev - Fix has been submitted and accepted. We will plan to release it in the next 10 days. https://github.com/jaredwray/keyv/issues/431

[trevor-scheer]

Resolved via https://github.com/jaredwray/keyv/pull/432