deleteFromCacheById does not clear DataLoader cache

[ash0080]

Today I found that I can't get the updated data with this package In fact, I don't think this package currently works, plz check the comments I added

const loader = new DataLoader(ids => {
const filter = {
  _id: {
    $in: ids  // <== ids = [ObjectId]
  }
}
const promise = model
  ? model.find(filter).exec()
  : collection.find(filter).toArray()

return promise.then(orderDocs(ids))
})

const cachePrefix = `mongo-${getCollection(collection).collectionName}-`

const methods = {
findOneById: async (id, { ttl } = {}) => {
  const key = cachePrefix + idToString(id)

  const cacheDoc = await cache.get(key)
  if (cacheDoc) {
    return EJSON.parse(cacheDoc)
  }

  const doc = await loader.load(id) // <== ids = [ObjectId]
  if (Number.isInteger(ttl)) {
    // https://github.com/apollographql/apollo-server/tree/master/packages/apollo-server-caching#apollo-server-caching
    cache.set(key, EJSON.stringify(doc), { ttl })
  }

  return doc
},
findManyByIds: (ids, { ttl } = {}) => {
  return Promise.all(ids.map(id => methods.findOneById(id, { ttl })))
},
deleteFromCacheById: async id => {
  const stringId = idToString(id)  // <== id = string??? can never delete a cache? because key is not a same thing
  loader.clear(stringId)
  await cache.delete(cachePrefix + stringId)
}
}

[ash0080]

yep, it is confirmed, the keys are not the same thing, In the current code, the key of the dataloader is always the ObjectID, but you are trying to delete it by a string key. so it doesn't work

[lorensr]

DataLoader is separate from the cache. The cache is a key-value store, where the key is the cachePrefix + stringId. DataLoader deduplicates and batches using the ObjectId

[ash0080]

@lorensr Please try it out yourself， I wrote a test, which has confirmed that the updated data cannot be retrieved without modifying the current code, because the key is used incorrectly. Sorry for the the post title, You're right, it's not about 'cache' here, it's about your wrong use of dataloader.

dataloader in your code now using ObjectID as keys, so

 loader.clear(stringId) // wrong

doesn't work simply console.dir to check what exactly is stored in the dataloader

Besides, I have a question, why use EJSON to store the data as string in cache? I don't understand, InMemoryLRUCache is a Map() actually, it can store anything, so why add this two-way conversion? This seems inefficient

[lorensr]

Is there a way to modify this test to get it to fail?

https://github.com/GraphQLGuide/apollo-datasource-mongodb/blob/master/src/__tests__/cache.test.js#L101-L111

Besides, I have a question, why use EJSON to store the data as string in cache? I don't understand, InMemoryLRUCache is a Map() actually, it can store anything, so why add this two-way conversion? This seems inefficient

The dev may be using a different cache. We need to match this signature:

[ash0080]

No, the problem is, Your test didn't cover this situation at all, it's about dataloader, not cache make a real mongo test, or if you add a break point in debug mode, you will find dateloader.clear() didn't release anything

const doc = ｛_id: OBJECTID, value: 0｝

1. findOneById(OBJECTID)

2. collection.updateOne({_id: OBJECTID},{value:{$inc:10}}) this.deleteFromCacheById(OBJECTID)

3. findOneById(OBJECTID) what you get here is Value===0

[lorensr]

Tests continue to pass after this commit:

https://github.com/GraphQLGuide/apollo-datasource-mongodb/commit/6ce707e458116477ef3935d4c440394b6a319339

[ash0080]

it(`deletes from cache`, async () => {
    await api.findOneById(docs.id2._id, { ttl: 1 })

    const valueBefore = await cache.get(cacheKey(docs.id2._id))
    expect(valueBefore).toEqual(EJSON.stringify(docs.id2))

    await api.deleteFromCacheById(docs.id2._id)

    const valueAfter = await cache.get(cacheKey(docs.id2._id))
   /** todo: this test also needs to make sure that the dataloader is cleared
      because when the next findOneById is called, 
      you can't sure it's not returned from the dataloader's cache
      this is where the bug really lies
   **/
    expect(valueAfter).toBeUndefined()
  })

[9at8]

@lorensr I think these two lines is where the problem stems from:

https://github.com/GraphQLGuide/apollo-datasource-mongodb/blob/master/src/cache.js#L43

The data loader stores an instance of ObjectId

https://github.com/GraphQLGuide/apollo-datasource-mongodb/blob/master/src/cache.js#L56

But we call clear with a string

The fix would be to convert ObjectId to string in both methods before calling methods on the data loader.

[ash0080]

@9at8 Yes, That's the issue exactly is.

Is there something wrong with my English? After communicating for so long, someone finally saw the point :man_shrugging:

[lorensr]

Ah, I see. Thank you @ash0080 for reporting and @9at8 for clarifying! Fix released in https://github.com/GraphQLGuide/apollo-datasource-mongodb/releases/tag/0.2.11

Hopefully 0.2.11 works for you, @ash0080 ?

[ash0080]

@lorensr @9at8 Thank you guys, haven't tested it yet, but it should work, I've modified the local folk before and added some other features.

[ash0080]

Oh, yes, I forgot, the last question According to the official documentation, the dataloader is supposed to be a per request cache, so how could this problem happen, shouldn't a new dataloader instance be generated for each request?

[lorensr]

Yes. A new instance is created each request if this library is used like this:

[9at8]

Yes, a new dataloader is initialized on every request.

This is a problem when do the following in the same request:

1. Find a thing
2. Update it
3. Find the same thing again later on

[ash0080]

I see, I was using dataSources incorrectly in my test (which was correct in the actual ApolloServer) But by mistake I found a bug :smiley: