From b0e08a7c4d108a546da29edb19950dc02d2e4a5b Mon Sep 17 00:00:00 2001 From: Todd Anderson Date: Mon, 27 Jan 2025 14:57:25 -0600 Subject: [PATCH 1/3] adds unit tests for transactional persistent store --- .../store/PersistentStoreWrapper.test.ts | 138 ++++++++++++++++++ .../src/api/subsystems/LDFeatureStore.ts | 3 +- .../sdk-server/src/store/AsyncStoreFacade.ts | 10 ++ .../src/store/InMemoryFeatureStore.ts | 5 +- .../src/store/TransactionalPersistentStore.ts | 1 + packages/shared/sdk-server/src/store/index.ts | 8 +- 6 files changed, 160 insertions(+), 5 deletions(-) diff --git a/packages/shared/sdk-server/__tests__/store/PersistentStoreWrapper.test.ts b/packages/shared/sdk-server/__tests__/store/PersistentStoreWrapper.test.ts index efdc3d132a..eb9f99ecaa 100644 --- a/packages/shared/sdk-server/__tests__/store/PersistentStoreWrapper.test.ts +++ b/packages/shared/sdk-server/__tests__/store/PersistentStoreWrapper.test.ts @@ -499,5 +499,143 @@ describe.each(['caching', 'non-caching'])( const allValues = await asyncWrapper.all(VersionedDataKinds.Features); expect(allValues).toEqual({}); }); + + it('applyChanges with basis results in initialization', async () => { + await asyncWrapper.applyChanges( + true, + { + features: { + key1: { + version: 1, + }, + }, + }, + 'selector1', + ); + + expect(await asyncWrapper.initialized()).toBeTruthy(); + expect(await asyncWrapper.all(VersionedDataKinds.Features)).toEqual({ + key1: { + version: 1, + }, + }); + }); + + it('applyChanges with basis overwrites existing data', async () => { + await asyncWrapper.applyChanges( + true, + { + features: { + oldFeature: { + version: 1, + }, + }, + }, + 'selector1', + ); + + expect(await asyncWrapper.all(VersionedDataKinds.Features)).toEqual({ + oldFeature: { + version: 1, + }, + }); + + await asyncWrapper.applyChanges( + true, + { + features: { + newFeature: { + version: 1, + }, + }, + }, + 'selector1', + ); + + expect(await asyncWrapper.all(VersionedDataKinds.Features)).toEqual({ + newFeature: { + version: 1, + }, + }); + }); + + it('applyChanges callback fires after all upserts complete', async () => { + let callbackCount = 0; + jest + .spyOn(mockPersistentStore, 'upsert') + .mockImplementation(async (_kind, _key, _data, cb) => { + callbackCount += 1; + // this await gives chance for execution to continue elsewhere. If there is a bug, this will lead to a failure + await new Promise((f) => { + setTimeout(f, 1); + }); + cb(); + }); + + await asyncWrapper.applyChanges( + false, + { + features: { + key1: { + version: 1, + }, + key2: { + version: 1, + }, + key3: { + version: 1, + }, + }, + }, + 'selector', + ); + expect(callbackCount).toEqual(3); + }); + + it('applyChanges with basis=false merges correctly', async () => { + await asyncWrapper.applyChanges( + true, + { + features: { + key1: { + version: 1, + }, + key2: { + version: 1, + }, + }, + }, + 'selector', + ); + + await asyncWrapper.applyChanges( + false, + { + features: { + key1: { + version: 2, + }, + key3: { + version: 1, + }, + }, + }, + 'selector', + ); + + expect(await asyncWrapper.all(VersionedDataKinds.Features)).toEqual({ + key1: { + key: 'key1', + version: 2, + }, + key2: { + version: 1, + }, + key3: { + key: 'key3', + version: 1, + }, + }); + }); }, ); diff --git a/packages/shared/sdk-server/src/api/subsystems/LDFeatureStore.ts b/packages/shared/sdk-server/src/api/subsystems/LDFeatureStore.ts index bca038f52d..5c047a8651 100644 --- a/packages/shared/sdk-server/src/api/subsystems/LDFeatureStore.ts +++ b/packages/shared/sdk-server/src/api/subsystems/LDFeatureStore.ts @@ -139,7 +139,8 @@ export interface LDFeatureStore { /** * Applies the provided data onto the existing data, replacing all data or upserting depending - * on the basis parameter. + * on the basis parameter. Must call {@link applyChanges} providing basis before calling {@link applyChanges} + * that is not a basis. * * @param basis If true, completely overwrites the current contents of the data store * with the provided data. If false, upserts the items in the provided data. Upserts diff --git a/packages/shared/sdk-server/src/store/AsyncStoreFacade.ts b/packages/shared/sdk-server/src/store/AsyncStoreFacade.ts index 5d24d81997..792668ff25 100644 --- a/packages/shared/sdk-server/src/store/AsyncStoreFacade.ts +++ b/packages/shared/sdk-server/src/store/AsyncStoreFacade.ts @@ -57,6 +57,16 @@ export default class AsyncStoreFacade { }); } + async applyChanges( + basis: boolean, + data: LDFeatureStoreDataStorage, + selector: String | undefined, + ): Promise { + return promisify((cb) => { + this._store.applyChanges(basis, data, selector, cb); + }); + } + close(): void { this._store.close(); } diff --git a/packages/shared/sdk-server/src/store/InMemoryFeatureStore.ts b/packages/shared/sdk-server/src/store/InMemoryFeatureStore.ts index fcb78018d3..761057762e 100644 --- a/packages/shared/sdk-server/src/store/InMemoryFeatureStore.ts +++ b/packages/shared/sdk-server/src/store/InMemoryFeatureStore.ts @@ -93,12 +93,11 @@ export default class InMemoryFeatureStore implements LDFeatureStore { const item = items[key]; if (Object.hasOwnProperty.call(existingItems, key)) { const old = existingItems[key]; - // TODO: SDK-1046 - Determine if version check should be removed if (!old || old.version < item.version) { - existingItems[key] = item; + existingItems[key] = { key, ...item }; } } else { - existingItems[key] = item; + existingItems[key] = { key, ...item }; } }); }); diff --git a/packages/shared/sdk-server/src/store/TransactionalPersistentStore.ts b/packages/shared/sdk-server/src/store/TransactionalPersistentStore.ts index 39faf8e359..d4d95fd639 100644 --- a/packages/shared/sdk-server/src/store/TransactionalPersistentStore.ts +++ b/packages/shared/sdk-server/src/store/TransactionalPersistentStore.ts @@ -18,6 +18,7 @@ export default class TransactionalPersistentStore implements LDFeatureStore { private _activeStore: LDFeatureStore; constructor(private readonly _nonTransPersistenceStore: LDFeatureStore) { + // persistence store is inital active store this._activeStore = this._nonTransPersistenceStore; this._memoryStore = new InMemoryFeatureStore(); } diff --git a/packages/shared/sdk-server/src/store/index.ts b/packages/shared/sdk-server/src/store/index.ts index 047f32648f..6076e0f888 100644 --- a/packages/shared/sdk-server/src/store/index.ts +++ b/packages/shared/sdk-server/src/store/index.ts @@ -1,5 +1,11 @@ import AsyncStoreFacade from './AsyncStoreFacade'; import PersistentDataStoreWrapper from './PersistentDataStoreWrapper'; import { deserializePoll } from './serialization'; +import TransactionalPersistentStore from './TransactionalPersistentStore'; -export { AsyncStoreFacade, PersistentDataStoreWrapper, deserializePoll }; +export { + AsyncStoreFacade, + PersistentDataStoreWrapper, + TransactionalPersistentStore, + deserializePoll, +}; From 30275ce297438ce69afb1652dc482a6dc41bdb27 Mon Sep 17 00:00:00 2001 From: Todd Anderson Date: Mon, 27 Jan 2025 16:57:22 -0600 Subject: [PATCH 2/3] adding tests for TransactionalPersistentStore --- .../TransactionalPersistentStore.test.ts | 120 ++++++++++++++++++ 1 file changed, 120 insertions(+) create mode 100644 packages/shared/sdk-server/__tests__/store/TransactionalPersistentStore.test.ts diff --git a/packages/shared/sdk-server/__tests__/store/TransactionalPersistentStore.test.ts b/packages/shared/sdk-server/__tests__/store/TransactionalPersistentStore.test.ts new file mode 100644 index 0000000000..de4a468648 --- /dev/null +++ b/packages/shared/sdk-server/__tests__/store/TransactionalPersistentStore.test.ts @@ -0,0 +1,120 @@ +import { LDFeatureStore } from '../../src/api/subsystems'; +import AsyncStoreFacade from '../../src/store/AsyncStoreFacade'; +import InMemoryFeatureStore from '../../src/store/InMemoryFeatureStore'; +import TransactionalPersistentStore from '../../src/store/TransactionalPersistentStore'; +import VersionedDataKinds from '../../src/store/VersionedDataKinds'; + +describe('given a non transactional store', () => { + let mockNontransactionalStore: LDFeatureStore; + let transactionalStore: TransactionalPersistentStore; + + let nonTransactionalFacade: AsyncStoreFacade; + let transactionalFacade: AsyncStoreFacade; + + beforeEach(() => { + mockNontransactionalStore = new InMemoryFeatureStore(); + transactionalStore = new TransactionalPersistentStore(mockNontransactionalStore); + + // these two facades are used to make test writing easier + nonTransactionalFacade = new AsyncStoreFacade(mockNontransactionalStore); + transactionalFacade = new AsyncStoreFacade(transactionalStore); + }); + + afterEach(() => { + transactionalFacade.close(); + jest.restoreAllMocks(); + }); + + it('applies changes to non transactional store', async () => { + await transactionalFacade.applyChanges( + false, + { + features: { + key1: { + version: 2, + }, + }, + }, + 'selector1', + ); + expect(await nonTransactionalFacade.all(VersionedDataKinds.Features)).toEqual({ + key1: { + key: 'key1', + version: 2, + }, + }); + expect(await transactionalFacade.all(VersionedDataKinds.Features)).toEqual({ + key1: { + key: 'key1', + version: 2, + }, + }); + }); + + it('it reads through to non transactional store before basis is provided', async () => { + await nonTransactionalFacade.init({ + features: { + key1: { + version: 1, + }, + }, + }); + expect(await transactionalFacade.all(VersionedDataKinds.Features)).toEqual({ + key1: { + version: 1, + }, + }); + }); + + it('it switches to memory store when basis is provided', async () => { + // situate some mock data in non transactional store + await nonTransactionalFacade.init({ + features: { + nontransactionalFeature: { + version: 1, + }, + }, + }); + + await transactionalFacade.applyChanges( + true, + { + features: { + key1: { + version: 1, + }, + }, + }, + 'selector1', + ); + + expect(await nonTransactionalFacade.all(VersionedDataKinds.Features)).toEqual({ + key1: { + version: 1, + }, + }); + + expect(await transactionalFacade.all(VersionedDataKinds.Features)).toEqual({ + key1: { + version: 1, + }, + }); + + // corrupt non transactional store and then read from transactional store to prove it is not + // using underlying non transactional store for reads + await nonTransactionalFacade.init({ + features: { + nontransactionalFeature: { + version: 1, + }, + }, + }); + + // still should read from memory + expect(await transactionalFacade.all(VersionedDataKinds.Features)).toEqual({ + key1: { + version: 1, + }, + }); + }); +}); From 3c377f9de44e0de7b6aed8ba9223f1957bac26ba Mon Sep 17 00:00:00 2001 From: Todd Anderson Date: Wed, 29 Jan 2025 09:31:48 -0600 Subject: [PATCH 3/3] reverting TODO removal --- packages/shared/sdk-server/src/store/InMemoryFeatureStore.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/shared/sdk-server/src/store/InMemoryFeatureStore.ts b/packages/shared/sdk-server/src/store/InMemoryFeatureStore.ts index 761057762e..6357a81991 100644 --- a/packages/shared/sdk-server/src/store/InMemoryFeatureStore.ts +++ b/packages/shared/sdk-server/src/store/InMemoryFeatureStore.ts @@ -93,6 +93,7 @@ export default class InMemoryFeatureStore implements LDFeatureStore { const item = items[key]; if (Object.hasOwnProperty.call(existingItems, key)) { const old = existingItems[key]; + // TODO: SDK-1046 - Determine if version check should be removed if (!old || old.version < item.version) { existingItems[key] = { key, ...item }; }