Fix empty block range (#2006)

* Draft fix

Add more tests

* remove test timeout

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

@@ -45,8 +45,10 @@ jest.mock('@subql/x-sequelize', () => {
       sequelize: {
         escape: (key: any) => key,
         query: (sql: string, option?: any) => jest.fn(),
-        fn: jest.fn().mockImplementation(() => {
-          return {fn: 'int8range', args: [41204769, null]};
+        fn: jest.fn().mockImplementation((_fn, ...args: unknown[]) => {
+          if (_fn === 'int8range') {
+            return {fn: _fn, args: [args[0], args[1] ?? null]};
+          }
         }),
         transaction,
       },
@@ -215,7 +217,219 @@ describe('cacheModel', () => {
       // should read from get cache, and entity should exclude __block_range
       expect(spyDbGet).not.toBeCalled();
       expect(JSON.stringify(entity)).not.toContain('__block_range');
-    }, 500000);
+    });
+
+    // TODO, getByFields still works
+
+    // Some edge cases for set get and remove
+    describe('set, remove and get', () => {
+      it('In different block, remove and set, should able to get', async () => {
+        testModel.remove('entity1_id_0x01', 4);
+        testModel.set(
+          'entity1_id_0x01',
+          {
+            id: 'entity1_id_0x01',
+            field1: 5,
+          },
+          6
+        );
+        const result = await testModel.get('entity1_id_0x01');
+        expect(result?.field1).toBe(5);
+      });
+
+      it('In same block, remove then set, should able to get', () => {
+        testModel.remove('entity1_id_0x01', 1);
+        testModel.set(
+          'entity1_id_0x01',
+          {
+            id: 'entity1_id_0x01',
+            field1: 1,
+          },
+          1
+        );
+        const result = testModel.get('entity1_id_0x01');
+        // data should be erased from removeCache
+        expect((testModel as any).removeCache.entity1_id_0x01).toBeUndefined();
+
+        expect(result).toBeDefined();
+      });
+
+      it('In different block, remove and set, then remove again, should get nothing', async () => {
+        testModel.remove('entity1_id_0x01', 4);
+        testModel.set(
+          'entity1_id_0x01',
+          {
+            id: 'entity1_id_0x01',
+            field1: 5,
+          },
+          6
+        );
+        testModel.remove('entity1_id_0x01', 8);
+        const result = await testModel.get('entity1_id_0x01');
+        expect((testModel as any).removeCache.entity1_id_0x01).toBeDefined();
+        // should match with last removed
+        expect((testModel as any).removeCache.entity1_id_0x01.removedAtBlock).toBe(8);
+
+        // check set cache, mark as removed
+        const latestSetRecord = (testModel as any).setCache.entity1_id_0x01.historicalValues[0];
+        expect(latestSetRecord.removed).toBeTruthy();
+        expect(latestSetRecord.endHeight).toBe(8);
+        expect(result).toBeUndefined();
+      });
+
+      it('In same block, remove and set, then remove again, should get nothing', async () => {
+        testModel.remove('entity1_id_0x01', 1);
+        testModel.set(
+          'entity1_id_0x01',
+          {
+            id: 'entity1_id_0x01',
+            field1: 1,
+          },
+          1
+        );
+        testModel.remove('entity1_id_0x01', 1);
+        const result = await testModel.get('entity1_id_0x01');
+        expect((testModel as any).removeCache.entity1_id_0x01).toBeDefined();
+
+        const latestSetRecord = (testModel as any).setCache.entity1_id_0x01.historicalValues[0];
+        // marked set record as removed
+        expect(latestSetRecord.removed).toBeTruthy();
+        expect(latestSetRecord.endHeight).toBe(1);
+        expect(result).toBeUndefined();
+      });
+
+      it('clean flushable records when applyBlockRange, if found set and removed happened in the same height', () => {
+        testModel.set(
+          'entity1_id_0x01',
+          {
+            id: 'entity1_id_0x01',
+            field1: 1,
+          },
+          1
+        );
+        testModel.remove('entity1_id_0x01', 1);
+
+        testModel.set(
+          'entity1_id_0x02',
+          {
+            id: 'entity1_id_0x02',
+            field1: 2,
+          },
+          2
+        );
+        expect((testModel as any).removeCache.entity1_id_0x01).toBeDefined();
+
+        const latestSetRecord = (testModel as any).setCache.entity1_id_0x01.historicalValues[0];
+        // marked set record as removed
+        expect(latestSetRecord.removed).toBeTruthy();
+        expect(latestSetRecord.startHeight).toBe(1);
+        expect(latestSetRecord.endHeight).toBe(1);
+        const records = (testModel as any).applyBlockRange((testModel as any).setCache);
+        // should filter out id 1
+        expect(records.length).toBe(1);
+        expect(records[0].id).toBe('entity1_id_0x02');
+      });
+
+      it('clean flushable records when applyBlockRange, pass if set and remove in the different height', () => {
+        testModel.set(
+          'entity1_id_0x01',
+          {
+            id: 'entity1_id_0x01',
+            field1: 1,
+          },
+          1
+        );
+        testModel.remove('entity1_id_0x01', 2);
+
+        testModel.set(
+          'entity1_id_0x02',
+          {
+            id: 'entity1_id_0x02',
+            field1: 2,
+          },
+          2
+        );
+        expect((testModel as any).removeCache.entity1_id_0x01).toBeDefined();
+        const records = (testModel as any).applyBlockRange((testModel as any).setCache);
+        expect(records.length).toBe(2);
+        expect(records[0].id).toBe('entity1_id_0x01');
+        expect(records[0].__block_range).toStrictEqual({args: [1, 2], fn: 'int8range'});
+        expect(records[1].id).toBe('entity1_id_0x02');
+        expect(records[1].__block_range).toStrictEqual({args: [2, null], fn: 'int8range'});
+      });
+
+      it('getFromCache could filter out removed data', async () => {
+        testModel.set(
+          'entity1_id_0x01',
+          {
+            id: 'entity1_id_0x01',
+            field1: 1,
+          },
+          1
+        );
+        testModel.remove('entity1_id_0x01', 1);
+
+        testModel.set(
+          'entity1_id_0x02',
+          {
+            id: 'entity1_id_0x02',
+            field1: 1,
+          },
+          2
+        );
+        const spyFindAll = jest.spyOn(testModel.model, 'findAll');
+        const result = await testModel.getByField('field1', 1, {offset: 0, limit: 50});
+        expect(spyFindAll).toBeCalledTimes(1);
+
+        expect(result).toStrictEqual([
+          {id: 'entity1_id_0x02', field1: 1}, //Filtered out removed record
+          {
+            id: 'apple-05-sequelize',
+            field1: 'set apple at block 5 with sequelize', // And mocked record
+          },
+        ]);
+      });
+
+      it('getFromCache with removed and set again data', async () => {
+        testModel.set(
+          'entity1_id_0x01',
+          {
+            id: 'entity1_id_0x01',
+            field1: 3,
+          },
+          1
+        );
+        testModel.remove('entity1_id_0x01', 1);
+        testModel.set(
+          'entity1_id_0x01',
+          {
+            id: 'entity1_id_0x01',
+            field1: 1,
+          },
+          1
+        );
+        const spyFindAll = jest.spyOn(testModel.model, 'findAll');
+        const result = await testModel.getByField('field1', 1, {offset: 0, limit: 50});
+        expect(spyFindAll).toBeCalledTimes(1);
+        expect(result).toStrictEqual([
+          {id: 'entity1_id_0x01', field1: 1},
+          {
+            id: 'apple-05-sequelize',
+            field1: 'set apple at block 5 with sequelize', // And mocked record
+          },
+        ]);
+
+        // Should not include any previous recorded value
+        const result3 = await testModel.getByField('field1', 3, {offset: 0, limit: 50});
+        // Expect only mocked
+        expect(result3).toStrictEqual([
+          {
+            id: 'apple-05-sequelize',
+            field1: 'set apple at block 5 with sequelize',
+          },
+        ]);
+      });
+    });
 
     describe('getByFields', () => {
       it('calls getByField if there is one filter', async () => {

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

@@ -72,13 +72,26 @@ export class CachedModel<
 
   async get(id: string): Promise<T | undefined> {
     // If this already been removed
+    const latestSetRecord = this.setCache[id]?.getLatest();
+
     if (this.removeCache[id]) {
+      // Entity has been set again after being removed
+      if (
+        latestSetRecord &&
+        !latestSetRecord.removed &&
+        this.removeCache[id].removedAtBlock <= latestSetRecord.startHeight
+      ) {
+        return latestSetRecord.data;
+      }
       return;
     }
     if (!this.getCache.has(id)) {
       // LFU getCache could remove record from due to it is least frequently used
       // Then we try look from setCache
-      let record = this.setCache[id]?.getLatest()?.data;
+      let record = latestSetRecord?.data;
+      if (latestSetRecord?.removed) {
+        return undefined;
+      }
       if (!record) {
         await this.mutex.waitForUnlock();
         record = (
@@ -219,6 +232,10 @@ export class CachedModel<
     // Experimental, this means getCache keeps duplicate data from setCache,
     // we can remove this once memory is too full.
     this.getCache.set(id, data);
+    // Handle remove cache, when removed data been created again
+    if (this.removeCache[id] && this.removeCache[id].removedAtBlock === blockHeight) {
+      delete this.removeCache[id];
+    }
     this.flushableRecordCounter += 1;
   }
 
@@ -379,17 +396,24 @@ export class CachedModel<
         if (!this.historical) {
           return v.getLatest()?.data;
         }
-
         // Historical
-        return v.getValues().map((historicalValue) => {
-          // Alternative: historicalValue.data.__block_range = [historicalValue.startHeight, historicalValue.endHeight];
-          historicalValue.data.__block_range = this.sequelize.fn(
-            'int8range',
-            historicalValue.startHeight,
-            historicalValue.endHeight
-          );
-          return historicalValue.data;
-        });
+        // Alternative: historicalValue.data.__block_range = [historicalValue.startHeight, historicalValue.endHeight];
+        return v
+          .getValues()
+          .map((historicalValue) => {
+            // prevent flush this record,
+            // this is happened when set and remove in same block
+            if (historicalValue.removed && historicalValue.startHeight === historicalValue.endHeight) {
+              return;
+            }
+            historicalValue.data.__block_range = this.sequelize.fn(
+              'int8range',
+              historicalValue.startHeight,
+              historicalValue.endHeight
+            );
+            return historicalValue.data;
+          })
+          .filter((r) => !!r);
       })
     ) as unknown as CreationAttributes<Model<T, T>>[];
   }
@@ -433,7 +457,7 @@ export class CachedModel<
     const unifiedIds: string[] = [];
     Object.entries(this.setCache).map(([, model]) => {
       if (model.isMatchData(field, value)) {
-        const latestData = model.getLatest()?.data;
+        const latestData = model.getLatest()?.removed ? undefined : model.getLatest()?.data;
         if (latestData) {
           unifiedIds.push(latestData.id);
           joinedData.push(latestData);

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

@@ -26,11 +26,14 @@ export class SetValueModel<T> {
       // Set multiple time within same block, replace with input data only
       if (this.historicalValues[latestIndex].startHeight === blockHeight) {
         this.historicalValues[latestIndex].data = data;
+        this.historicalValues[latestIndex].endHeight = null;
+        this.historicalValues[latestIndex].removed = false;
       } else if (this.historicalValues[latestIndex].startHeight > blockHeight) {
         throw new Error(
           `Can not set record with block height ${blockHeight} as data for future block heights has been set`
         );
       } else {
+        // close previous historicalValues record endHeight
         this.historicalValues[latestIndex].endHeight = blockHeight;
         this.create(data, blockHeight, operationIndex);
       }
@@ -51,6 +54,8 @@ export class SetValueModel<T> {
     }
   }
 
+  // the latest record could be mark as removed
+  // we need to handle return value in where this method called accordingly , rather than return undefined here.
   getLatest(): SetValue<T> | undefined {
     const latestIndex = this.latestIndex();
     if (latestIndex === -1) {
@@ -100,6 +105,7 @@ export class SetValueModel<T> {
       return;
     }
     this.historicalValues[latestIndex].endHeight = removeAtBlock;
+    this.historicalValues[latestIndex].removed = true;
   }
 
   isMatchData(field?: keyof T, value?: T[keyof T] | T[keyof T][]): boolean {
@@ -109,12 +115,19 @@ export class SetValueModel<T> {
     if (Array.isArray(value)) {
       return value.findIndex((v) => this.isMatchData(field, v)) > -1;
     } else {
+      if (this.getLatest()?.removed) return false;
       return isEqual(this.getLatest()?.data?.[field], value);
     }
   }
 
   private create(data: T, blockHeight: number, operationIndex: number): void {
-    this.historicalValues.push({data, startHeight: blockHeight, endHeight: null, operationIndex: operationIndex});
+    this.historicalValues.push({
+      data,
+      startHeight: blockHeight,
+      endHeight: null,
+      operationIndex: operationIndex,
+      removed: false,
+    });
     this._latestIndex += 1;
   }
 

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

@@ -55,6 +55,7 @@ export type SetValue<T> = {
   startHeight: number;
   endHeight: number | null;
   operationIndex: number;
+  removed: boolean;
 };
 
 export type SetData<T> = Record<string, SetValueModel<T>>;