Merge pull request #350 from splitio/refactor_segments_cache_update_method

Refactor: segments cache `update` method

Author: EmilianoSanchez

src/storages/AbstractSegmentsCacheSync.ts …/storages/AbstractMySegmentsCacheSync.tssrc/storages/AbstractSegmentsCacheSync.ts renamed to src/storages/AbstractMySegmentsCacheSync.ts src/storages/AbstractSegmentsCacheSync.ts renamed to src/storages/AbstractMySegmentsCacheSync.ts

@@ -1,5 +1,3 @@
-/* eslint-disable @typescript-eslint/no-unused-vars */
-/* eslint-disable no-unused-vars */
 import { IMySegmentsResponse } from '../dtos/types';
 import { MySegmentsData } from '../sync/polling/types';
 import { ISegmentsCacheSync } from './types';
@@ -8,18 +6,11 @@ import { ISegmentsCacheSync } from './types';
  * This class provides a skeletal implementation of the ISegmentsCacheSync interface
  * to minimize the effort required to implement this interface.
  */
-export abstract class AbstractSegmentsCacheSync implements ISegmentsCacheSync {
-  /**
-   * For server-side synchronizer: add `segmentKeys` list of keys to `name` segment.
-   * For client-side synchronizer: add `name` segment to the cache. `segmentKeys` is undefined.
-   */
-  abstract addToSegment(name: string, segmentKeys?: string[]): boolean
+export abstract class AbstractMySegmentsCacheSync implements ISegmentsCacheSync {
 
-  /**
-   * For server-side synchronizer: remove `segmentKeys` list of keys from `name` segment.
-   * For client-side synchronizer: remove `name` segment from the cache. `segmentKeys` is undefined.
-   */
-  abstract removeFromSegment(name: string, segmentKeys?: string[]): boolean
+  protected abstract addSegment(name: string): boolean
+  protected abstract removeSegment(name: string): boolean
+  protected abstract setChangeNumber(changeNumber?: number): boolean | void
 
   /**
    * For server-side synchronizer: check if `key` is in `name` segment.
@@ -34,11 +25,10 @@ export abstract class AbstractSegmentsCacheSync implements ISegmentsCacheSync {
     this.resetSegments({});
   }
 
-  /**
-   * For server-side synchronizer: add the given list of segments to the cache, with an empty list of keys. The segments that already exist are not modified.
-   * For client-side synchronizer: the method is not used.
-   */
-  registerSegments(names: string[]): boolean { return false; }
+
+  // No-op. Not used in client-side.
+  registerSegments(): boolean { return false; }
+  update() { return false; }
 
   /**
    * For server-side synchronizer: get the list of segments to fetch changes.
@@ -52,31 +42,26 @@ export abstract class AbstractSegmentsCacheSync implements ISegmentsCacheSync {
    */
   abstract getKeysCount(): number
 
-  /**
-   * For server-side synchronizer: change number of `name` segment.
-   * For client-side synchronizer: change number of mySegments.
-   */
-  abstract setChangeNumber(name?: string, changeNumber?: number): boolean | void
   abstract getChangeNumber(name: string): number
 
   /**
    * For server-side synchronizer: the method is not used.
    * For client-side synchronizer: it resets or updates the cache.
    */
   resetSegments(segmentsData: MySegmentsData | IMySegmentsResponse): boolean {
-    this.setChangeNumber(undefined, segmentsData.cn);
+    this.setChangeNumber(segmentsData.cn);
 
     const { added, removed } = segmentsData as MySegmentsData;
 
     if (added && removed) {
       let isDiff = false;
 
       added.forEach(segment => {
-        isDiff = this.addToSegment(segment) || isDiff;
+        isDiff = this.addSegment(segment) || isDiff;
       });
 
       removed.forEach(segment => {
-        isDiff = this.removeFromSegment(segment) || isDiff;
+        isDiff = this.removeSegment(segment) || isDiff;
       });
 
       return isDiff;
@@ -97,11 +82,11 @@ export abstract class AbstractSegmentsCacheSync implements ISegmentsCacheSync {
 
     // Slowest path => add and/or remove segments
     for (let removeIndex = index; removeIndex < storedSegmentKeys.length; removeIndex++) {
-      this.removeFromSegment(storedSegmentKeys[removeIndex]);
+      this.removeSegment(storedSegmentKeys[removeIndex]);
     }
 
     for (let addIndex = index; addIndex < names.length; addIndex++) {
-      this.addToSegment(names[addIndex]);
+      this.addSegment(names[addIndex]);
     }
 
     return true;

src/storages/inLocalStorage/MySegmentsCacheInLocal.ts

@@ -1,10 +1,10 @@
 import { ILogger } from '../../logger/types';
 import { isNaNNumber } from '../../utils/lang';
-import { AbstractSegmentsCacheSync } from '../AbstractSegmentsCacheSync';
+import { AbstractMySegmentsCacheSync } from '../AbstractMySegmentsCacheSync';
 import type { MySegmentsKeyBuilder } from '../KeyBuilderCS';
 import { LOG_PREFIX, DEFINED } from './constants';
 
-export class MySegmentsCacheInLocal extends AbstractSegmentsCacheSync {
+export class MySegmentsCacheInLocal extends AbstractMySegmentsCacheSync {
 
   private readonly keys: MySegmentsKeyBuilder;
   private readonly log: ILogger;
@@ -16,7 +16,7 @@ export class MySegmentsCacheInLocal extends AbstractSegmentsCacheSync {
     // There is not need to flush segments cache like splits cache, since resetSegments receives the up-to-date list of active segments
   }
 
-  addToSegment(name: string): boolean {
+  protected addSegment(name: string): boolean {
     const segmentKey = this.keys.buildSegmentNameKey(name);
 
     try {
@@ -29,7 +29,7 @@ export class MySegmentsCacheInLocal extends AbstractSegmentsCacheSync {
     }
   }
 
-  removeFromSegment(name: string): boolean {
+  protected removeSegment(name: string): boolean {
     const segmentKey = this.keys.buildSegmentNameKey(name);
 
     try {
@@ -81,7 +81,7 @@ export class MySegmentsCacheInLocal extends AbstractSegmentsCacheSync {
     return 1;
   }
 
-  setChangeNumber(name?: string, changeNumber?: number) {
+  protected setChangeNumber(changeNumber?: number) {
     try {
       if (changeNumber) localStorage.setItem(this.keys.buildTillKey(), changeNumber + '');
       else localStorage.removeItem(this.keys.buildTillKey());

src/storages/inLocalStorage/__tests__/MySegmentsCacheInLocal.spec.ts

@@ -22,7 +22,11 @@ test('SEGMENT CACHE / in LocalStorage', () => {
   });
 
   caches.forEach(cache => {
-    cache.removeFromSegment('mocked-segment');
+    // @ts-expect-error
+    cache.resetSegments({
+      added: [],
+      removed: ['mocked-segment']
+    });
 
     expect(cache.isInSegment('mocked-segment')).toBe(false);
     expect(cache.getRegisteredSegments()).toEqual(['mocked-segment-2']);

src/storages/inMemory/MySegmentsCacheInMemory.ts

@@ -1,23 +1,23 @@
-import { AbstractSegmentsCacheSync } from '../AbstractSegmentsCacheSync';
+import { AbstractMySegmentsCacheSync } from '../AbstractMySegmentsCacheSync';
 
 /**
  * Default MySegmentsCacheInMemory implementation that stores MySegments in memory.
  * Supported by all JS runtimes.
  */
-export class MySegmentsCacheInMemory extends AbstractSegmentsCacheSync {
+export class MySegmentsCacheInMemory extends AbstractMySegmentsCacheSync {
 
   private segmentCache: Record<string, boolean> = {};
   private cn?: number;
 
-  addToSegment(name: string): boolean {
+  protected addSegment(name: string): boolean {
     if (this.segmentCache[name]) return false;
 
     this.segmentCache[name] = true;
 
     return true;
   }
 
-  removeFromSegment(name: string): boolean {
+  protected removeSegment(name: string): boolean {
     if (!this.segmentCache[name]) return false;
 
     delete this.segmentCache[name];
@@ -30,7 +30,7 @@ export class MySegmentsCacheInMemory extends AbstractSegmentsCacheSync {
   }
 
 
-  setChangeNumber(name?: string, changeNumber?: number) {
+  protected setChangeNumber(changeNumber?: number) {
     this.cn = changeNumber;
   }
 

src/storages/inMemory/SegmentsCacheInMemory.ts

@@ -1,36 +1,25 @@
-import { AbstractSegmentsCacheSync } from '../AbstractSegmentsCacheSync';
 import { ISet, _Set } from '../../utils/lang/sets';
 import { isIntegerNumber } from '../../utils/lang';
+import { ISegmentsCacheSync } from '../types';
 
 /**
- * Default ISplitsCacheSync implementation that stores split definitions in memory.
- * Supported by all JS runtimes.
+ * Default ISplitsCacheSync implementation for server-side that stores segments definitions in memory.
  */
-export class SegmentsCacheInMemory extends AbstractSegmentsCacheSync {
+export class SegmentsCacheInMemory implements ISegmentsCacheSync {
 
   private segmentCache: Record<string, ISet<string>> = {};
   private segmentChangeNumber: Record<string, number> = {};
 
-  addToSegment(name: string, segmentKeys: string[]): boolean {
-    const values = this.segmentCache[name];
-    const keySet = values ? values : new _Set<string>();
+  update(name: string, addedKeys: string[], removedKeys: string[], changeNumber: number) {
+    const keySet = this.segmentCache[name] || new _Set<string>();
 
-    segmentKeys.forEach(k => keySet.add(k));
+    addedKeys.forEach(k => keySet.add(k));
+    removedKeys.forEach(k => keySet.delete(k));
 
     this.segmentCache[name] = keySet;
+    this.segmentChangeNumber[name] = changeNumber;
 
-    return true;
-  }
-
-  removeFromSegment(name: string, segmentKeys: string[]): boolean {
-    const values = this.segmentCache[name];
-    const keySet = values ? values : new _Set<string>();
-
-    segmentKeys.forEach(k => keySet.delete(k));
-
-    this.segmentCache[name] = keySet;
-
-    return true;
+    return addedKeys.length > 0 || removedKeys.length > 0;
   }
 
   isInSegment(name: string, key: string): boolean {
@@ -74,16 +63,13 @@ export class SegmentsCacheInMemory extends AbstractSegmentsCacheSync {
     }, 0);
   }
 
-  setChangeNumber(name: string, changeNumber: number) {
-    this.segmentChangeNumber[name] = changeNumber;
-
-    return true;
-  }
-
   getChangeNumber(name: string) {
     const value = this.segmentChangeNumber[name];
 
     return isIntegerNumber(value) ? value : -1;
   }
 
+  // No-op. Not used in server-side
+  resetSegments() { return false; }
+
 }

src/storages/inMemory/SplitsCacheInMemory.ts

@@ -5,7 +5,6 @@ import { ISet, _Set } from '../../utils/lang/sets';
 
 /**
  * Default ISplitsCacheSync implementation that stores split definitions in memory.
- * Supported by all JS runtimes.
  */
 export class SplitsCacheInMemory extends AbstractSplitsCacheSync {
 

src/storages/inMemory/__tests__/SegmentsCacheInMemory.spec.ts

@@ -2,24 +2,18 @@ import { SegmentsCacheInMemory } from '../SegmentsCacheInMemory';
 
 describe('SEGMENTS CACHE IN MEMORY', () => {
 
-  test('isInSegment, set/getChangeNumber, add/removeFromSegment, getKeysCount', () => {
+  test('isInSegment, getChangeNumber, update, getKeysCount', () => {
     const cache = new SegmentsCacheInMemory();
 
-    cache.addToSegment('mocked-segment', [
-      'a', 'b', 'c'
-    ]);
-
-    cache.setChangeNumber('mocked-segment', 1);
-
-    cache.removeFromSegment('mocked-segment', ['d']);
+    cache.update('mocked-segment', [ 'a', 'b', 'c'], [], 1);
+    cache.update('mocked-segment', [], ['d'], 1);
 
     expect(cache.getChangeNumber('mocked-segment') === 1).toBe(true);
 
-    cache.addToSegment('mocked-segment', ['d', 'e']);
+    cache.update('mocked-segment', [ 'd', 'e'], [], 2);
+    cache.update('mocked-segment', [], ['a', 'c'], 2);
 
-    cache.removeFromSegment('mocked-segment', ['a', 'c']);
-
-    expect(cache.getChangeNumber('mocked-segment') === 1).toBe(true);
+    expect(cache.getChangeNumber('mocked-segment') === 2).toBe(true);
 
     expect(cache.isInSegment('mocked-segment', 'a')).toBe(false);
     expect(cache.isInSegment('mocked-segment', 'b')).toBe(true); // b
@@ -29,7 +23,7 @@ describe('SEGMENTS CACHE IN MEMORY', () => {
 
     // getKeysCount
     expect(cache.getKeysCount()).toBe(3);
-    cache.addToSegment('mocked-segment-2', ['a', 'b', 'c', 'd', 'e']);
+    cache.update('mocked-segment-2', ['a', 'b', 'c', 'd', 'e'], [], 2);
     expect(cache.getKeysCount()).toBe(8);
     cache.clear();
     expect(cache.getKeysCount()).toBe(0);

src/storages/inRedis/SegmentsCacheInRedis.ts

@@ -17,24 +17,21 @@ export class SegmentsCacheInRedis implements ISegmentsCacheAsync {
     this.keys = keys;
   }
 
-  addToSegment(name: string, segmentKeys: string[]) {
+  /**
+   * Update the given segment `name` with the lists of `addedKeys`, `removedKeys` and `changeNumber`.
+   * The returned promise is resolved if the operation success, with `true` if the segment was updated (i.e., some key was added or removed),
+   * or rejected if it fails (e.g., Redis operation fails).
+   */
+  update(name: string, addedKeys: string[], removedKeys: string[], changeNumber: number) {
     const segmentKey = this.keys.buildSegmentNameKey(name);
 
-    if (segmentKeys.length) {
-      return this.redis.sadd(segmentKey, segmentKeys).then(() => true);
-    } else {
-      return Promise.resolve(true);
-    }
-  }
-
-  removeFromSegment(name: string, segmentKeys: string[]) {
-    const segmentKey = this.keys.buildSegmentNameKey(name);
-
-    if (segmentKeys.length) {
-      return this.redis.srem(segmentKey, segmentKeys).then(() => true);
-    } else {
-      return Promise.resolve(true);
-    }
+    return Promise.all([
+      addedKeys.length && this.redis.sadd(segmentKey, addedKeys),
+      removedKeys.length && this.redis.srem(segmentKey, removedKeys),
+      this.redis.set(this.keys.buildSegmentTillKey(name), changeNumber + '')
+    ]).then(() => {
+      return addedKeys.length > 0 || removedKeys.length > 0;
+    });
   }
 
   isInSegment(name: string, key: string) {
@@ -43,12 +40,6 @@ export class SegmentsCacheInRedis implements ISegmentsCacheAsync {
     ).then(matches => matches !== 0);
   }
 
-  setChangeNumber(name: string, changeNumber: number) {
-    return this.redis.set(
-      this.keys.buildSegmentTillKey(name), changeNumber + ''
-    ).then(status => status === 'OK');
-  }
-
   getChangeNumber(name: string) {
     return this.redis.get(this.keys.buildSegmentTillKey(name)).then((value: string | null) => {
       const i = parseInt(value as string, 10);

src/storages/inRedis/__tests__/SegmentsCacheInRedis.spec.ts

@@ -9,25 +9,21 @@ const keys = new KeyBuilderSS(prefix, metadata);
 
 describe('SEGMENTS CACHE IN REDIS', () => {
 
-  test('isInSegment, set/getChangeNumber, add/removeFromSegment', async () => {
+  test('isInSegment, getChangeNumber, update', async () => {
     const connection = new RedisAdapter(loggerMock);
     const cache = new SegmentsCacheInRedis(loggerMock, keys, connection);
 
-    await cache.addToSegment('mocked-segment', ['a', 'b', 'c']);
-
-    await cache.setChangeNumber('mocked-segment', 1);
-
-    await cache.removeFromSegment('mocked-segment', ['d']);
+    await cache.update('mocked-segment', ['a', 'b', 'c'], ['d'], 1);
 
     expect(await cache.getChangeNumber('mocked-segment') === 1).toBe(true);
 
     expect(await cache.getChangeNumber('inexistent-segment')).toBe(-1); // -1 if the segment doesn't exist
 
-    await cache.addToSegment('mocked-segment', ['d', 'e']);
+    await cache.update('mocked-segment', ['d', 'e'], [], 2);
 
-    await cache.removeFromSegment('mocked-segment', ['a', 'c']);
+    await cache.update('mocked-segment', [], ['a', 'c'], 2);
 
-    expect(await cache.getChangeNumber('mocked-segment') === 1).toBe(true);
+    expect(await cache.getChangeNumber('mocked-segment') === 2).toBe(true);
 
     expect(await cache.isInSegment('mocked-segment', 'a')).toBe(false);
     expect(await cache.isInSegment('mocked-segment', 'b')).toBe(true);