diff --git a/packages/datafile-manager/CHANGELOG.md b/packages/datafile-manager/CHANGELOG.md index 02f0db724..2c2294af7 100644 --- a/packages/datafile-manager/CHANGELOG.md +++ b/packages/datafile-manager/CHANGELOG.md @@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## [Unreleased] Changes that have landed but are not yet released. +### Changed + +- Modified datafile manager to accept, process, and return the datafile's string representation instead of the datafile object. +- Remove JSON parsing of response received from datafile fetch request + - Responsibility of validating the datafile now solely belongs to the project config manager +- Modified React Native async storage cache and persistent value cache implementation to store strings instead of objects as values. + ## [0.7.0] - July 28, 2020 ### Changed diff --git a/packages/datafile-manager/__test__/httpPollingDatafileManager.spec.ts b/packages/datafile-manager/__test__/httpPollingDatafileManager.spec.ts index 5044b235f..9b52f3aa7 100644 --- a/packages/datafile-manager/__test__/httpPollingDatafileManager.spec.ts +++ b/packages/datafile-manager/__test__/httpPollingDatafileManager.spec.ts @@ -69,11 +69,11 @@ class TestDatafileManager extends HttpPollingDatafileManager { } const testCache: PersistentKeyValueCache = { - get(key: string): Promise { - let val = null; + get(key: string): Promise { + let val = ''; switch (key) { case 'opt-datafile-keyThatExists': - val = { name: 'keyThatExists' }; + val = JSON.stringify({ name: 'keyThatExists' }); break; } return Promise.resolve(val); @@ -109,11 +109,11 @@ describe('httpPollingDatafileManager', () => { describe('when constructed with sdkKey and datafile and autoUpdate: true,', () => { beforeEach(() => { - manager = new TestDatafileManager({ datafile: { foo: 'abcd' }, sdkKey: '123', autoUpdate: true }); + manager = new TestDatafileManager({ datafile: JSON.stringify({ foo: 'abcd' }), sdkKey: '123', autoUpdate: true }); }); it('returns the passed datafile from get', () => { - expect(manager.get()).toEqual({ foo: 'abcd' }); + expect(JSON.parse(manager.get())).toEqual({ foo: 'abcd' }); }); it('after being started, fetches the datafile, updates itself, and updates itself again after a timeout', async () => { @@ -134,7 +134,7 @@ describe('httpPollingDatafileManager', () => { manager.start(); expect(manager.responsePromises.length).toBe(1); await manager.responsePromises[0]; - expect(manager.get()).toEqual({ foo: 'bar' }); + expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' }); updateFn.mockReset(); await advanceTimersByTime(300000); @@ -142,20 +142,22 @@ describe('httpPollingDatafileManager', () => { expect(manager.responsePromises.length).toBe(2); await manager.responsePromises[1]; expect(updateFn).toBeCalledTimes(1); - expect(updateFn).toBeCalledWith({ - datafile: { fooz: 'barz' }, - }); - expect(manager.get()).toEqual({ fooz: 'barz' }); + expect(updateFn.mock.calls[0][0]).toEqual({ datafile: '{"fooz": "barz"}' }); + expect(JSON.parse(manager.get())).toEqual({ fooz: 'barz' }); }); }); describe('when constructed with sdkKey and datafile and autoUpdate: false,', () => { beforeEach(() => { - manager = new TestDatafileManager({ datafile: { foo: 'abcd' }, sdkKey: '123', autoUpdate: false }); + manager = new TestDatafileManager({ + datafile: JSON.stringify({ foo: 'abcd' }), + sdkKey: '123', + autoUpdate: false, + }); }); it('returns the passed datafile from get', () => { - expect(manager.get()).toEqual({ foo: 'abcd' }); + expect(JSON.parse(manager.get())).toEqual({ foo: 'abcd' }); }); it('after being started, fetches the datafile, updates itself once, but does not schedule a future update', async () => { @@ -167,7 +169,7 @@ describe('httpPollingDatafileManager', () => { manager.start(); expect(manager.responsePromises.length).toBe(1); await manager.responsePromises[0]; - expect(manager.get()).toEqual({ foo: 'bar' }); + expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' }); expect(getTimerCount()).toBe(0); }); }); @@ -179,7 +181,7 @@ describe('httpPollingDatafileManager', () => { describe('initial state', () => { it('returns null from get before becoming ready', () => { - expect(manager.get()).toBeNull(); + expect(manager.get()).toEqual(''); }); }); @@ -205,28 +207,7 @@ describe('httpPollingDatafileManager', () => { }); manager.start(); await manager.onReady(); - expect(manager.get()).toEqual({ foo: 'bar' }); - }); - - it('does not update if the response body is not valid json', async () => { - manager.queuedResponses.push( - { - statusCode: 200, - body: '{"foo" "', - headers: {}, - }, - { - statusCode: 200, - body: '{"foo": "bar"}', - headers: {}, - } - ); - manager.start(); - await manager.onReady(); - await advanceTimersByTime(1000); - expect(manager.responsePromises.length).toBe(2); - await manager.responsePromises[1]; - expect(manager.get()).toEqual({ foo: 'bar' }); + expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' }); }); describe('live updates', () => { @@ -275,22 +256,22 @@ describe('httpPollingDatafileManager', () => { manager.start(); await manager.onReady(); - expect(manager.get()).toEqual({ foo: 'bar' }); + expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' }); expect(updateFn).toBeCalledTimes(0); await advanceTimersByTime(1000); await manager.responsePromises[1]; expect(updateFn).toBeCalledTimes(1); - expect(updateFn.mock.calls[0][0]).toEqual({ datafile: { foo2: 'bar2' } }); - expect(manager.get()).toEqual({ foo2: 'bar2' }); + expect(updateFn.mock.calls[0][0]).toEqual({ datafile: '{"foo2": "bar2"}' }); + expect(JSON.parse(manager.get())).toEqual({ foo2: 'bar2' }); updateFn.mockReset(); await advanceTimersByTime(1000); await manager.responsePromises[2]; expect(updateFn).toBeCalledTimes(1); - expect(updateFn.mock.calls[0][0]).toEqual({ datafile: { foo3: 'bar3' } }); - expect(manager.get()).toEqual({ foo3: 'bar3' }); + expect(updateFn.mock.calls[0][0]).toEqual({ datafile: '{"foo3": "bar3"}' }); + expect(JSON.parse(manager.get())).toEqual({ foo3: 'bar3' }); }); describe('when the update interval time fires before the request is complete', () => { @@ -351,7 +332,7 @@ describe('httpPollingDatafileManager', () => { manager.start(); await manager.onReady(); - expect(manager.get()).toEqual({ foo: 'bar' }); + expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' }); advanceTimersByTime(1000); @@ -359,7 +340,7 @@ describe('httpPollingDatafileManager', () => { manager.stop(); await manager.responsePromises[1]; // Should not have updated datafile since manager was stopped - expect(manager.get()).toEqual({ foo: 'bar' }); + expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' }); }); it('calls abort on the current request if there is a current request when stop is called', async () => { @@ -399,7 +380,7 @@ describe('httpPollingDatafileManager', () => { // Trigger the update, should fetch the next response which should succeed, then we get ready advanceTimersByTime(1000); await manager.onReady(); - expect(manager.get()).toEqual({ foo: 'bar' }); + expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' }); }); describe('newness checking', () => { @@ -424,7 +405,7 @@ describe('httpPollingDatafileManager', () => { manager.start(); await manager.onReady(); - expect(manager.get()).toEqual({ foo: 'bar' }); + expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' }); // First response promise was for the initial 200 response expect(manager.responsePromises.length).toBe(1); // Trigger the queued update @@ -434,7 +415,7 @@ describe('httpPollingDatafileManager', () => { await manager.responsePromises[1]; // Since the response was 304, updateFn should not have been called expect(updateFn).toBeCalledTimes(0); - expect(manager.get()).toEqual({ foo: 'bar' }); + expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' }); }); it('sends if-modified-since using the last observed response last-modified', async () => { @@ -559,7 +540,7 @@ describe('httpPollingDatafileManager', () => { }); manager.start(); await manager.onReady(); - expect(manager.get()).toEqual({ foo: 'bar' }); + expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' }); }); it('does not schedule a live update after ready', async () => { @@ -659,9 +640,9 @@ describe('httpPollingDatafileManager', () => { manager.on('update', updateFn); manager.start(); await manager.onReady(); - expect(manager.get()).toEqual({ name: 'keyThatExists' }); + expect(JSON.parse(manager.get())).toEqual({ name: 'keyThatExists' }); await advanceTimersByTime(50); - expect(manager.get()).toEqual({ name: 'keyThatExists' }); + expect(JSON.parse(manager.get())).toEqual({ name: 'keyThatExists' }); expect(updateFn).toBeCalledTimes(0); }); @@ -676,10 +657,10 @@ describe('httpPollingDatafileManager', () => { manager.on('update', updateFn); manager.start(); await manager.onReady(); - expect(manager.get()).toEqual({ name: 'keyThatExists' }); + expect(JSON.parse(manager.get())).toEqual({ name: 'keyThatExists' }); expect(updateFn).toBeCalledTimes(0); await advanceTimersByTime(50); - expect(manager.get()).toEqual({ foo: 'bar' }); + expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' }); expect(updateFn).toBeCalledTimes(1); }); @@ -693,8 +674,9 @@ describe('httpPollingDatafileManager', () => { manager.start(); await manager.onReady(); await advanceTimersByTime(50); - expect(manager.get()).toEqual({ foo: 'bar' }); - expect(cacheSetSpy).toBeCalledWith('opt-datafile-keyThatExists', { foo: 'bar' }); + expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' }); + expect(cacheSetSpy.mock.calls[0][0]).toEqual('opt-datafile-keyThatExists'); + expect(JSON.parse(cacheSetSpy.mock.calls[0][1])).toEqual({ foo: 'bar' }); }); }); @@ -721,7 +703,7 @@ describe('httpPollingDatafileManager', () => { manager.start(); await advanceTimersByTime(50); await manager.onReady(); - expect(manager.get()).toEqual({ foo: 'bar' }); + expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' }); expect(updateFn).toBeCalledTimes(0); }); }); diff --git a/packages/datafile-manager/__test__/reactNativeAsyncStorageCache.spec.ts b/packages/datafile-manager/__test__/reactNativeAsyncStorageCache.spec.ts index 9084434de..0401a2065 100644 --- a/packages/datafile-manager/__test__/reactNativeAsyncStorageCache.spec.ts +++ b/packages/datafile-manager/__test__/reactNativeAsyncStorageCache.spec.ts @@ -24,46 +24,28 @@ describe('reactNativeAsyncStorageCache', () => { }); describe('get', function() { - it('should return correct object when item is found in cache', function() { - return cacheInstance.get('keyThatExists').then(v => expect(v).toEqual({ name: 'Awesome Object' })); + it('should return correct string when item is found in cache', function() { + return cacheInstance.get('keyThatExists').then(v => expect(JSON.parse(v)).toEqual({ name: 'Awesome Object' })); }); - it('should return null if item is not found in cache', function() { - return cacheInstance.get('keyThatDoesNotExist').then(v => expect(v).toBeNull()); - }); - - it('should reject promise error if string has an incorrect JSON format', function() { - return cacheInstance - .get('keyWithInvalidJsonObject') - .catch(() => 'exception caught') - .then(v => { - expect(v).toEqual('exception caught'); - }); + it('should return empty string if item is not found in cache', function() { + return cacheInstance.get('keyThatDoesNotExist').then(v => expect(v).toEqual('')); }); }); describe('set', function() { it('should resolve promise if item was successfully set in the cache', function() { const testObj = { name: 'Awesome Object' }; - return cacheInstance.set('testKey', testObj); - }); - - it('should reject promise if item was not set in the cache because of json stringifying error', function() { - const testObj: any = { name: 'Awesome Object' }; - testObj.myOwnReference = testObj; - return cacheInstance - .set('testKey', testObj) - .catch(() => 'exception caught') - .then(v => expect(v).toEqual('exception caught')); + return cacheInstance.set('testKey', JSON.stringify(testObj)); }); }); describe('contains', function() { - it('should return true if object with key exists', function() { + it('should return true if value with key exists', function() { return cacheInstance.contains('keyThatExists').then(v => expect(v).toBeTruthy()); }); - it('should return false if object with key does not exist', function() { + it('should return false if value with key does not exist', function() { return cacheInstance.contains('keyThatDoesNotExist').then(v => expect(v).toBeFalsy()); }); }); diff --git a/packages/datafile-manager/src/datafileManager.ts b/packages/datafile-manager/src/datafileManager.ts index 9e415bfc6..f93c8551f 100644 --- a/packages/datafile-manager/src/datafileManager.ts +++ b/packages/datafile-manager/src/datafileManager.ts @@ -16,7 +16,7 @@ import PersistentKeyValueCache from './persistentKeyValueCache'; export interface DatafileUpdate { - datafile: object; + datafile: string; } export interface DatafileUpdateListener { @@ -31,14 +31,14 @@ interface Managed { } export interface DatafileManager extends Managed { - get: () => object | null; + get: () => string; on: (eventName: string, listener: DatafileUpdateListener) => () => void; onReady: () => Promise; } export interface DatafileManagerConfig { autoUpdate?: boolean; - datafile?: object; + datafile?: string; sdkKey: string; updateInterval?: number; urlTemplate?: string; diff --git a/packages/datafile-manager/src/httpPollingDatafileManager.ts b/packages/datafile-manager/src/httpPollingDatafileManager.ts index 4c41adfc8..f8232c0bd 100644 --- a/packages/datafile-manager/src/httpPollingDatafileManager.ts +++ b/packages/datafile-manager/src/httpPollingDatafileManager.ts @@ -36,8 +36,8 @@ function isSuccessStatusCode(statusCode: number): boolean { } const noOpKeyValueCache: PersistentKeyValueCache = { - get(): Promise { - return Promise.resolve(null); + get(): Promise { + return Promise.resolve(''); }, set(): Promise { @@ -63,7 +63,7 @@ export default abstract class HttpPollingDatafileManager implements DatafileMana // Return any default configuration options that should be applied protected abstract getConfigDefaults(): Partial; - private currentDatafile: object | null; + private currentDatafile: string; private readonly readyPromise: Promise; @@ -131,7 +131,7 @@ export default abstract class HttpPollingDatafileManager implements DatafileMana this.resolveReadyPromise(); } } else { - this.currentDatafile = null; + this.currentDatafile = ''; } this.isStarted = false; @@ -152,7 +152,7 @@ export default abstract class HttpPollingDatafileManager implements DatafileMana this.syncOnCurrentRequestComplete = false; } - get(): object | null { + get(): string { return this.currentDatafile; } @@ -222,7 +222,7 @@ export default abstract class HttpPollingDatafileManager implements DatafileMana this.trySavingLastModified(response.headers); const datafile = this.getNextDatafileFromResponse(response); - if (datafile !== null) { + if (datafile !== '') { logger.info('Updating datafile from response'); this.currentDatafile = datafile; this.cache.set(this.cacheKey, datafile); @@ -305,35 +305,18 @@ export default abstract class HttpPollingDatafileManager implements DatafileMana }, nextUpdateDelay); } - private getNextDatafileFromResponse(response: Response): object | null { + private getNextDatafileFromResponse(response: Response): string { logger.debug('Response status code: %s', response.statusCode); if (typeof response.statusCode === 'undefined') { - return null; + return ''; } if (response.statusCode === 304) { - return null; + return ''; } if (isSuccessStatusCode(response.statusCode)) { - return this.tryParsingBodyAsJSON(response.body); + return response.body; } - return null; - } - - private tryParsingBodyAsJSON(body: string): object | null { - let parseResult: any; - try { - parseResult = JSON.parse(body); - } catch (err) { - logger.error('Error parsing response body: %s', err.message, err); - return null; - } - let datafileObj: object | null = null; - if (typeof parseResult === 'object' && parseResult !== null) { - datafileObj = parseResult; - } else { - logger.error('Error parsing response body: was not an object'); - } - return datafileObj; + return ''; } private trySavingLastModified(headers: Headers): void { @@ -346,7 +329,7 @@ export default abstract class HttpPollingDatafileManager implements DatafileMana setDatafileFromCacheIfAvailable(): void { this.cache.get(this.cacheKey).then(datafile => { - if (this.isStarted && !this.isReadyPromiseSettled && datafile) { + if (this.isStarted && !this.isReadyPromiseSettled && datafile !== '') { logger.debug('Using datafile from cache'); this.currentDatafile = datafile; this.resolveReadyPromise(); diff --git a/packages/datafile-manager/src/persistentKeyValueCache.ts b/packages/datafile-manager/src/persistentKeyValueCache.ts index 9b3ef9fac..08dfcf1fe 100644 --- a/packages/datafile-manager/src/persistentKeyValueCache.ts +++ b/packages/datafile-manager/src/persistentKeyValueCache.ts @@ -15,8 +15,7 @@ */ /** - * An Interface to implement a persistent key value cache which supports strings as keys - * and JSON Object as value. + * An Interface to implement a persistent key value cache which supports strings as keys and values. */ export default interface PersistentKeyValueCache { /** @@ -24,21 +23,21 @@ export default interface PersistentKeyValueCache { * @param key * @returns * Resolves promise with - * 1. Object if value found was stored as a JSON Object. + * 1. string if value found was stored as a string. * 2. null if the key does not exist in the cache. * Rejects the promise in case of an error */ - get(key: string): Promise; + get(key: string): Promise; /** - * Stores Object in the persistent cache against a key + * Stores string in the persistent cache against a key * @param key * @param val * @returns * Resolves promise without a value if successful * Rejects the promise in case of an error */ - set(key: string, val: any): Promise; + set(key: string, val: string): Promise; /** * Checks if a key exists in the cache diff --git a/packages/datafile-manager/src/reactNativeAsyncStorageCache.ts b/packages/datafile-manager/src/reactNativeAsyncStorageCache.ts index 73f0bb364..d0a6b46d0 100644 --- a/packages/datafile-manager/src/reactNativeAsyncStorageCache.ts +++ b/packages/datafile-manager/src/reactNativeAsyncStorageCache.ts @@ -14,35 +14,22 @@ * limitations under the License. */ -import { getLogger } from '@optimizely/js-sdk-logging'; import AsyncStorage from '@react-native-community/async-storage'; import PersistentKeyValueCache from './persistentKeyValueCache'; -const logger = getLogger('DatafileManager'); - export default class ReactNativeAsyncStorageCache implements PersistentKeyValueCache { - get(key: string): Promise { + get(key: string): Promise { return AsyncStorage.getItem(key).then((val: string | null) => { if (!val) { - return null; - } - try { - return JSON.parse(val); - } catch (ex) { - logger.error('Error Parsing Object from cache - %s', ex); - throw ex; + return ''; } + return val; }); } - set(key: string, val: any): Promise { - try { - return AsyncStorage.setItem(key, JSON.stringify(val)); - } catch (ex) { - logger.error('Error stringifying Object to Json - %s', ex); - return Promise.reject(ex); - } + set(key: string, val: string): Promise { + return AsyncStorage.setItem(key, val); } contains(key: string): Promise {