chore: implement tombstoning (#328)

Implement tombstoning based on feedback frrom #326.

packages/shared/sdk-client/src/LDClientImpl.storage.test.ts

@@ -8,15 +8,13 @@ import { DeleteFlag, Flag, Flags, PatchFlag } from './types';
 
 jest.mock('@launchdarkly/js-sdk-common', () => {
   const actual = jest.requireActual('@launchdarkly/js-sdk-common');
-  const { MockStreamingProcessor: mockStreamer } = jest.requireActual(
-    '@launchdarkly/private-js-mocks',
-  );
+  const { MockStreamingProcessor } = jest.requireActual('@launchdarkly/private-js-mocks');
   return {
     ...actual,
     ...{
       internal: {
         ...actual.internal,
-        StreamingProcessor: mockStreamer,
+        StreamingProcessor: MockStreamingProcessor,
       },
     },
   };
@@ -330,7 +328,7 @@ describe('sdk-client storage', () => {
   test('delete should emit change event', async () => {
     const deleteResponse = {
       key: 'dev-test-flag',
-      version: defaultPutResponse['dev-test-flag'].version,
+      version: defaultPutResponse['dev-test-flag'].version + 1,
     };
 
     const allFlags = await identifyGetAllFlags(
@@ -344,13 +342,32 @@ describe('sdk-client storage', () => {
     expect(allFlags).not.toHaveProperty('dev-test-flag');
     expect(basicPlatform.storage.set).toHaveBeenCalledWith(
       'org:Testy Pizza',
-      expect.not.stringContaining('dev-test-flag'),
+      expect.stringContaining('dev-test-flag'),
     );
-    expect(flagsInStorage['dev-test-flag']).toBeUndefined();
+    expect(flagsInStorage['dev-test-flag']).toMatchObject({ ...deleteResponse, deleted: true });
     expect(emitter.emit).toHaveBeenCalledTimes(3);
     expect(emitter.emit).toHaveBeenNthCalledWith(3, 'change', context, ['dev-test-flag']);
   });
 
+  test('delete should not delete equal version', async () => {
+    const deleteResponse = {
+      key: 'dev-test-flag',
+      version: defaultPutResponse['dev-test-flag'].version,
+    };
+
+    const allFlags = await identifyGetAllFlags(
+      false,
+      defaultPutResponse,
+      undefined,
+      deleteResponse,
+      false,
+    );
+
+    expect(allFlags).toHaveProperty('dev-test-flag');
+    expect(basicPlatform.storage.set).toHaveBeenCalledTimes(1);
+    expect(emitter.emit).not.toHaveBeenCalledWith('change');
+  });
+
   test('delete should not delete newer version', async () => {
     const deleteResponse = {
       key: 'dev-test-flag',
@@ -370,15 +387,17 @@ describe('sdk-client storage', () => {
     expect(emitter.emit).not.toHaveBeenCalledWith('change');
   });
 
-  test('delete should ignore non-existing flag', async () => {
+  test('delete should add and tombstone non-existing flag', async () => {
     const deleteResponse = {
       key: 'does-not-exist',
       version: 1,
     };
 
     await identifyGetAllFlags(false, defaultPutResponse, undefined, deleteResponse, false);
+    const flagsInStorage = JSON.parse(basicPlatform.storage.set.mock.lastCall[1]) as Flags;
 
-    expect(basicPlatform.storage.set).toHaveBeenCalledTimes(1);
-    expect(emitter.emit).not.toHaveBeenCalledWith('change');
+    expect(basicPlatform.storage.set).toHaveBeenCalledTimes(2);
+    expect(flagsInStorage['does-not-exist']).toMatchObject({ ...deleteResponse, deleted: true });
+    expect(emitter.emit).toHaveBeenCalledWith('change', context, ['does-not-exist']);
   });
 });

packages/shared/sdk-client/src/LDClientImpl.test.ts

@@ -1,16 +1,13 @@
-import { LDContext } from '@launchdarkly/js-sdk-common';
-import {
-  basicPlatform,
-  logger,
-  MockStreamingProcessor,
-  setupMockStreamingProcessor,
-} from '@launchdarkly/private-js-mocks';
+import { clone, LDContext } from '@launchdarkly/js-sdk-common';
+import { basicPlatform, logger, setupMockStreamingProcessor } from '@launchdarkly/private-js-mocks';
 
 import * as mockResponseJson from './evaluation/mockResponse.json';
 import LDClientImpl from './LDClientImpl';
+import { Flags } from './types';
 
 jest.mock('@launchdarkly/js-sdk-common', () => {
   const actual = jest.requireActual('@launchdarkly/js-sdk-common');
+  const { MockStreamingProcessor } = jest.requireActual('@launchdarkly/private-js-mocks');
   return {
     ...actual,
     ...{
@@ -21,13 +18,16 @@ jest.mock('@launchdarkly/js-sdk-common', () => {
     },
   };
 });
-describe('sdk-client object', () => {
-  const testSdkKey = 'test-sdk-key';
-  const context: LDContext = { kind: 'org', key: 'Testy Pizza' };
-  let ldc: LDClientImpl;
 
+const testSdkKey = 'test-sdk-key';
+const context: LDContext = { kind: 'org', key: 'Testy Pizza' };
+let ldc: LDClientImpl;
+let defaultPutResponse: Flags;
+
+describe('sdk-client object', () => {
   beforeEach(() => {
-    setupMockStreamingProcessor(false, mockResponseJson);
+    defaultPutResponse = clone<Flags>(mockResponseJson);
+    setupMockStreamingProcessor(false, defaultPutResponse);
 
     ldc = new LDClientImpl(testSdkKey, basicPlatform, { logger, sendEvents: false });
     jest
@@ -95,8 +95,32 @@ describe('sdk-client object', () => {
     expect(devTestFlag).toBe(true);
   });
 
+  test('variationDetail flag not found', async () => {
+    await ldc.identify(context);
+    const flag = ldc.variationDetail('does-not-exist', 'not-found');
+
+    expect(flag).toEqual({
+      reason: { errorKind: 'FLAG_NOT_FOUND', kind: 'ERROR' },
+      value: 'not-found',
+      variationIndex: null,
+    });
+  });
+
+  test('variationDetail deleted flag not found', async () => {
+    await ldc.identify(context);
+    // @ts-ignore
+    ldc.flags['dev-test-flag'].deleted = true;
+    const flag = ldc.variationDetail('dev-test-flag', 'deleted');
+
+    expect(flag).toEqual({
+      reason: { errorKind: 'FLAG_NOT_FOUND', kind: 'ERROR' },
+      value: 'deleted',
+      variationIndex: null,
+    });
+  });
+
   test('identify success', async () => {
-    mockResponseJson['dev-test-flag'].value = false;
+    defaultPutResponse['dev-test-flag'].value = false;
     const carContext: LDContext = { kind: 'car', key: 'mazda-cx7' };
 
     await ldc.identify(carContext);

packages/shared/sdk-client/src/LDClientImpl.ts

@@ -79,7 +79,9 @@ export default class LDClientImpl implements LDClient {
   allFlags(): LDFlagSet {
     const result: LDFlagSet = {};
     Object.entries(this.flags).forEach(([k, r]) => {
-      result[k] = r.value;
+      if (!r.deleted) {
+        result[k] = r.value;
+      }
     });
     return result;
   }
@@ -154,8 +156,18 @@ export default class LDClientImpl implements LDClient {
         this.logger.debug(`Streamer DELETE ${JSON.stringify(dataJson, null, 2)}`);
         const existing = this.flags[dataJson.key];
 
-        if (existing && existing.version < dataJson.version) {
-          delete this.flags[dataJson.key];
+        // the deleted flag is saved as tombstoned
+        if (!existing || existing.version < dataJson.version) {
+          this.flags[dataJson.key] = {
+            ...dataJson,
+            deleted: true,
+            // props below are set to sensible defaults. they are irrelevant
+            // because this flag has been deleted.
+            flagVersion: 0,
+            value: undefined,
+            variation: 0,
+            trackEvents: false,
+          };
           await this.platform.storage?.set(canonicalKey, JSON.stringify(this.flags));
           const changedKeys = [dataJson.key];
           this.logger.debug(`Emitting changes from DELETE: ${changedKeys}`);
@@ -299,7 +311,7 @@ export default class LDClientImpl implements LDClient {
     const evalContext = Context.fromLDContext(this.context);
     const found = this.flags[flagKey];
 
-    if (!found) {
+    if (!found || found.deleted) {
       const defVal = defaultValue ?? null;
       const error = new LDClientError(
         `Unknown feature flag "${flagKey}"; returning default value ${defVal}`,

packages/shared/sdk-client/src/types/index.ts

@@ -9,6 +9,7 @@ export interface Flag {
   trackReason?: boolean;
   reason?: LDEvaluationReason;
   debugEventsUntilDate?: number;
+  deleted?: boolean;
 }
 
 export interface PatchFlag extends Flag {