Fix getByFields including removed cache data (#2571)

* Fix getByFields including removed cache data

* Update changelog

packages/node-core/CHANGELOG.md

@@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Issues with setting a large block range for bypass blocks (#2566)
 - Test runner not setting lastProcessedHeight leading to data not being flushed (#2569)
 - Unable to rewind unfinalized blocks on startup (#2570)
+- Store `getByFields` returning removed cache data (#2571)
+
 ### Changed
 - Throw error when store getByField(s) options.limit exceeds queryLimit option (#2567)
 

packages/node-core/src/indexer/storeCache/cacheModel.spec.ts

@@ -229,6 +229,11 @@ describe('cacheModel', () => {
       testModel = new CachedModel(sequelize.model('entity1'), true, {} as NodeConfig, () => i++);
     });
 
+    it('throws when trying to set undefined', () => {
+      expect(() => testModel.set('0x01', undefined as any, 1)).toThrow();
+      expect(() => testModel.set('0x01', null as any, 1)).toThrow();
+    });
+
     // it should keep same behavior as hook we used
     it('when get data after flushed, it should exclude block range', async () => {
       const spyDbGet = jest.spyOn(testModel.model, 'findOne');

packages/node-core/src/indexer/storeCache/cacheModel.test.ts

@@ -373,12 +373,28 @@ describe('cacheModel integration', () => {
     cacheModel = new CachedModel(model, false, new NodeConfig({} as any), () => i++);
   });
 
+  it('behaves as expected with removals', async () => {
+    // Create data but dont flush, this puts it in the cache
+    await setDefaultData('0x01', 1, undefined, false);
+
+    // Remove the data, this could happen at any block > 1 but before flushing.
+    cacheModel.remove('0x01', 1);
+
+    // The result should be undefined as it will be marked as removed
+    const res0 = await cacheModel.get('0x01');
+    expect(res0).toBeUndefined();
+
+    // The data will still be in the set cache but it should not be included in the result
+    const res = await cacheModel.getByFields([], {offset: 0, limit: 10, orderBy: 'selfStake', orderDirection: 'ASC'});
+    expect(res).toEqual([]);
+  });
+
   afterAll(async () => {
     await sequelize.dropSchema(schema, {logging: false});
     await sequelize.close();
   });
 
-  async function setDefaultData(id: string, height: number, data?: any): Promise<void> {
+  async function setDefaultData(id: string, height: number, data?: any, flushData = true): Promise<void> {
     cacheModel.set(
       id,
       data ?? {
@@ -390,7 +406,9 @@ describe('cacheModel integration', () => {
       },
       height
     );
-    await flush(height + 1);
+    if (flushData) {
+      await flush(height + 1);
+    }
   }
 
   describe('cached data and db data compare', () => {

packages/node-core/src/indexer/storeCache/cacheModel.ts

@@ -166,6 +166,7 @@ export class CachedModel<
       [(value: SetValueModel<T>) => value.getLatest()?.data?.[fullOptions.orderBy]],
       [fullOptions.orderDirection.toLowerCase() as 'asc' | 'desc']
     )
+      .filter((value) => !value.getLatest()?.removed) // Ensure the data has not been marked for removal
       .filter((value) => value.matchesFields(filters)) // This filters out any removed/undefined
       .map((value) => value.getLatest()?.data)
       .map((value) => cloneDeep(value)) as T[];
@@ -216,9 +217,10 @@ export class CachedModel<
   }
 
   set(id: string, data: T, blockHeight: number): void {
-    if (this.setCache[id] === undefined) {
-      this.setCache[id] = new SetValueModel();
+    if (data === undefined || data === null) {
+      throw new Error('Cannot set undefined or null data. If you wish to remove data, use remove()');
     }
+    this.setCache[id] ??= new SetValueModel();
     const copiedData = cloneDeep(data);
     this.setCache[id].set(copiedData, blockHeight, this.getNextStoreOperationIndex());
     // Experimental, this means getCache keeps duplicate data from setCache,