fix(cache): remove uninvalidate as it can undo invalidation in case of subsequent get (#2250)

* fix(cache): remove uninvalidate as it can undo invalidation in case of immediate get

GH-2249

* fix(cache): remove uninvalidate function from cache service

GH-2249

* feat(cache): add a few missing lines for a test case

GH-2249

Author: akshatdubeysf

packages/cache/src/__tests__/acceptance/cache-repository.mixin.acceptance.ts

@@ -11,7 +11,7 @@ import {setupEnv} from '../helpers';
 import {repoTestBuilder} from '../helpers/test-builder';
 
 dotenv.config();
-const DEFAULT_TIMEOUT = 5000;
+const DEFAULT_TIMEOUT = 15000;
 
 describe('CachedRepository: Acceptance', () => {
   let app: TestApp;
@@ -33,6 +33,8 @@ describe('CachedRepository: Acceptance', () => {
     if (!process.env.REDIS_HOST || !process.env.REDIS_PORT) {
       // eslint-disable-next-line @typescript-eslint/no-invalid-this
       this.skip();
+    } else {
+      mochaContext.timeout(DEFAULT_TIMEOUT);
     }
   });
   beforeEach(async () => {
@@ -87,11 +89,6 @@ describe('CachedRepository: Acceptance', () => {
     describe(testSuite.title, () => {
       testSuite.tests.forEach(test => {
         it(test.title, async function () {
-          if (test.timeout) {
-            mochaContext.timeout(test.timeout);
-          } else {
-            mochaContext.timeout(DEFAULT_TIMEOUT);
-          }
           await test.test(
             repo,
             mockData,

packages/cache/src/__tests__/fixtures/models/index.ts

@@ -1 +1,2 @@
+export * from './test-2.model';
 export * from './test.model';

packages/cache/src/__tests__/fixtures/models/test-2.model.ts

@@ -0,0 +1,17 @@
+import {Entity, model, property} from '@loopback/repository';
+
+@model()
+export class Test2 extends Entity {
+  @property({
+    type: 'number',
+    id: true,
+  })
+  id: number;
+  @property({
+    type: 'string',
+  })
+  name: string;
+  constructor(data?: Partial<Test2>) {
+    super(data);
+  }
+}

packages/cache/src/__tests__/fixtures/repositories/test-2-with-mixin.repository.ts

@@ -1,17 +1,17 @@
-import {DefaultCrudRepository, juggler} from '@loopback/repository';
-import {Test} from '../models';
 import {inject} from '@loopback/core';
+import {DefaultCrudRepository, juggler} from '@loopback/repository';
 import {CacheMixin} from '../../../mixins';
+import {Test2} from '../models';
 
 export class Test2WithMixinRepository extends CacheMixin(
-  DefaultCrudRepository<Test, number, {}>,
+  DefaultCrudRepository<Test2, number, {}>,
   {
     invalidationTags: ['TestTag', 'Test2Tag'],
     cachedItemTags: ['Test2Tag'],
   },
 ) {
   cacheIdentifier = 'testRepo2';
   constructor(@inject('datasources.memorydb2') dataSource: juggler.DataSource) {
-    super(Test, dataSource);
+    super(Test2, dataSource);
   }
 }

packages/cache/src/__tests__/helpers/test-builder.ts

@@ -13,7 +13,6 @@ type TestCase = {
     secondRepo?: DefaultCrudRepository<Test, number, {}>,
     tick?: TickPromise,
   ) => Promise<void>;
-  timeout?: 3000;
 };
 
 type TestSuite = {
@@ -170,7 +169,6 @@ export function repoTestBuilder(
             // one call for value and one for insertion time
             expect(cacheStoreSetSpy.callCount).to.equal(4);
           },
-          timeout: 3000,
         },
       ],
     },
@@ -375,7 +373,6 @@ export function repoTestBuilder(
             // one call for value and one for insertion time
             expect(cacheStoreSetSpy.callCount).to.equal(4);
           },
-          timeout: 3000,
         },
       ],
     },
@@ -673,6 +670,34 @@ export function repoTestBuilder(
             expect(cacheStoreSetSpy.callCount).to.equal(4);
           },
         },
+        {
+          title:
+            'should invalidate cache after updateById, and subsequent get with different argument should not effect this invalidation',
+          async test(
+            repo: DefaultCrudRepository<Test, number, {}>,
+            mockData: DataObject<Test>[],
+            cacheStoreGetSpy: sinon.SinonSpy,
+            cacheStoreSetSpy: sinon.SinonSpy,
+            secondRepo?: DefaultCrudRepository<Test, number, {}>,
+          ) {
+            const existing1 = await repo.findById(1);
+            await flushPromises();
+            expect(existing1).to.match(mockData[0]);
+            const existing2 = await repo.findById(2);
+            await flushPromises();
+            expect(existing2).to.match(mockData[1]);
+            await repo.updateById(1, {name: 'test-new-1'});
+            await flushPromises();
+            await repo.updateById(2, {name: 'test-new-2'});
+            await flushPromises();
+            const updatedLast = await repo.findById(2);
+            await flushPromises();
+            const updatedFirst = await repo.findById(1);
+            await flushPromises();
+            expect(updatedLast).to.match({id: 2, name: 'test-new-2'});
+            expect(updatedFirst).to.match({id: 1, name: 'test-new-1'});
+          },
+        },
         {
           title: 'should invalidate cache across repositories',
           async test(

packages/cache/src/__tests__/integration/cache-repository.mixin.integration.ts

@@ -27,12 +27,6 @@ describe('CachedRepository: Integration', () => {
   let cacheStoreGetSpy: sinon.SinonSpy;
   let cacheStoreSetSpy: sinon.SinonSpy;
   let clock: sinon.SinonFakeTimers | undefined = undefined;
-  let mochaContext: Mocha.Context;
-
-  before(function () {
-    // eslint-disable-next-line @typescript-eslint/no-invalid-this, @typescript-eslint/no-this-alias
-    mochaContext = this;
-  });
 
   beforeEach(async () => {
     setupEnv();
@@ -72,9 +66,6 @@ describe('CachedRepository: Integration', () => {
     describe(testSuite.title, () => {
       testSuite.tests.forEach(test => {
         it(test.title, async function () {
-          if (test.timeout) {
-            mochaContext.timeout(test.timeout);
-          }
           await test.test(
             repo,
             mockData,

packages/cache/src/services/cache.service.ts

@@ -161,11 +161,13 @@ export class CacheService implements ICacheService {
       }
     }
     const result = await fn(...args);
-    // eslint-disable-next-line @typescript-eslint/no-floating-promises
-    this.saveInCache(prefix, key, options?.tags ?? [], result, cacheOptions)
-      .catch(err => this.logger.error(err.message))
-      // to make sure new calls for same prefix or tags are not invalidated
-      .then(_ => this.uninvalidate(prefix, options?.tags));
+    this.saveInCache(
+      prefix,
+      key,
+      options?.tags ?? [],
+      result,
+      cacheOptions,
+    ).catch(err => this.logger.error(err.message));
     return result;
   }
 
@@ -202,27 +204,6 @@ export class CacheService implements ICacheService {
     await this.store.setMany([prefixDeletion, ...tagDeletions]);
   }
 
-  /**
-   * This function removes invalidatations of cache entries based on a given prefix and tags.
-   * @param {string} prefix - The `prefix` parameter is a string that is used to identify a group of
-   * items that need to be uninvalidated.
-   * @param {string[]} [tags] - The `tags` parameter in the `uninvalidate` method is an optional
-   * parameter of type `string[]`. It is an array of strings that represent tags that were invalidated. If
-   * this parameter is provided when calling the method, the corresponding tags will be uninvalidated along
-   * with the specified prefix. If the `
-   */
-  private async uninvalidate(prefix: string, tags?: string[]): Promise<void> {
-    const prefixDeletion = this.invalidatePrefix(prefix);
-    let tagDeletions: [string, number, number][] = [];
-    if (tags) {
-      tagDeletions = this.invalidateTags(tags);
-    }
-    await this.store.deleteMany([
-      prefixDeletion[0],
-      ...tagDeletions.map(record => record[0]),
-    ]);
-  }
-
   private invalidatePrefix(prefix: string): [string, number, number] {
     const deletionKey = this.buildKey(this.deletionMarkerPrefix, prefix);
     return [deletionKey, Date.now(), this.configuration.ttl];