diff --git a/packages/shared/sdk-server/__tests__/store/serialization.test.ts b/packages/shared/sdk-server/__tests__/store/serialization.test.ts index 4656ea5513..56e41e964b 100644 --- a/packages/shared/sdk-server/__tests__/store/serialization.test.ts +++ b/packages/shared/sdk-server/__tests__/store/serialization.test.ts @@ -1,11 +1,14 @@ +import { AttributeReference } from '@launchdarkly/js-sdk-common'; + import { Flag } from '../../src/evaluation/data/Flag'; import { Segment } from '../../src/evaluation/data/Segment'; import { deserializeAll, deserializeDelete, deserializePatch, + nullReplacer, replacer, - reviver, + serializeFlag, serializeSegment, } from '../../src/store/serialization'; @@ -152,6 +155,38 @@ const segmentWithBucketBy = { deleted: false, }; +const flagWithNullInJsonVariation = { + key: 'flagName', + on: true, + fallthrough: { variation: 1 }, + variations: [[true, null, 'potato'], [null, null], { null: null }, { arr: [null] }], + version: 1, +}; + +const flagWithManyNulls = { + key: 'test-after-value1', + on: true, + rules: [ + { + variation: 0, + id: 'ruleid', + clauses: [ + { + attribute: 'attrname', + op: 'after', + values: ['not valid'], + negate: null, + }, + ], + trackEvents: null, + }, + ], + offVariation: null, + fallthrough: { variation: 1 }, + variations: [true, false], + version: 1, +}; + function makeAllData(flag?: any, segment?: any): any { const allData: any = { data: { @@ -239,6 +274,42 @@ describe('when deserializing all data', () => { const ref = parsed?.data.flags.flagName.rules?.[0].rollout?.bucketByAttributeReference; expect(ref?.isValid).toBeTruthy(); }); + + it('does not replace null in Objects or array JSON variations', () => { + const jsonString = makeSerializedAllData(flagWithNullInJsonVariation); + const parsed = deserializeAll(jsonString); + + expect(parsed?.data.flags.flagName.variations).toStrictEqual( + flagWithNullInJsonVariation.variations, + ); + }); + + it('removes null values outside variations', () => { + const jsonString = makeSerializedAllData(flagWithManyNulls); + const parsed = deserializeAll(jsonString); + + expect(parsed?.data.flags.flagName).toStrictEqual({ + key: 'test-after-value1', + on: true, + rules: [ + { + variation: 0, + id: 'ruleid', + clauses: [ + { + attribute: 'attrname', + attributeReference: new AttributeReference('attrname'), + op: 'after', + values: ['not valid'], + }, + ], + }, + ], + fallthrough: { variation: 1 }, + variations: [true, false], + version: 1, + }); + }); }); describe('when deserializing patch data', () => { @@ -290,9 +361,45 @@ describe('when deserializing patch data', () => { const ref = (parsed?.data as Flag).rules?.[0].rollout?.bucketByAttributeReference; expect(ref?.isValid).toBeTruthy(); }); + + it('does not replace null in Objects or array JSON variations', () => { + const jsonString = makeSerializedPatchData(flagWithNullInJsonVariation); + const parsed = deserializePatch(jsonString); + + expect((parsed?.data as Flag)?.variations).toStrictEqual( + flagWithNullInJsonVariation.variations, + ); + }); + + it('removes null values outside variations', () => { + const jsonString = makeSerializedPatchData(flagWithManyNulls); + const parsed = deserializePatch(jsonString); + + expect(parsed?.data as Flag).toStrictEqual({ + key: 'test-after-value1', + on: true, + rules: [ + { + variation: 0, + id: 'ruleid', + clauses: [ + { + attribute: 'attrname', + attributeReference: new AttributeReference('attrname'), + op: 'after', + values: ['not valid'], + }, + ], + }, + ], + fallthrough: { variation: 1 }, + variations: [true, false], + version: 1, + }); + }); }); -it('removes null elements', () => { +it('removes null elements that are not part of arrays', () => { const baseData = { a: 'b', b: 'c', @@ -306,10 +413,49 @@ it('removes null elements', () => { polluted.c.f = null; const stringPolluted = JSON.stringify(polluted); - const parsed = JSON.parse(stringPolluted, reviver); + const parsed = JSON.parse(stringPolluted); + nullReplacer(parsed); expect(parsed).toStrictEqual(baseData); }); +it('does not remove null in arrays', () => { + const data = { + a: ['b', null, { arr: [null] }], + c: { + d: ['e', null, { arr: [null] }], + }, + }; + + const parsed = JSON.parse(JSON.stringify(data)); + nullReplacer(parsed); + expect(parsed).toStrictEqual(data); +}); + +it('does remove null from objects that are inside of arrays', () => { + const data = { + a: ['b', null, { null: null, notNull: true }], + c: { + d: ['e', null, { null: null, notNull: true }], + }, + }; + + const parsed = JSON.parse(JSON.stringify(data)); + nullReplacer(parsed); + expect(parsed).toStrictEqual({ + a: ['b', null, { notNull: true }], + c: { + d: ['e', null, { notNull: true }], + }, + }); +}); + +it('can handle attempting to replace nulls for an undefined or null value', () => { + expect(() => { + nullReplacer(null); + nullReplacer(undefined); + }).not.toThrow(); +}); + it.each([ [flagWithAttributeNameInClause, undefined], [flagWithAttributeReferenceInClause, undefined], @@ -450,3 +596,11 @@ it('serialization converts sets back to arrays for includedContexts/excludedCont expect(jsonDeserialized.includedContexts[0].generated_valuesSet).toBeUndefined(); expect(jsonDeserialized.excludedContexts[0].generated_valuesSet).toBeUndefined(); }); + +it('serializes null values without issue', () => { + const jsonString = makeSerializedAllData(flagWithNullInJsonVariation); + const parsed = deserializeAll(jsonString); + const serialized = serializeFlag(parsed!.data.flags.flagName); + // After serialization nulls should still be there, and any memo generated items should be gone. + expect(JSON.parse(serialized)).toEqual(flagWithNullInJsonVariation); +}); diff --git a/packages/shared/sdk-server/src/store/serialization.ts b/packages/shared/sdk-server/src/store/serialization.ts index aaba1bc4de..4af09bb832 100644 --- a/packages/shared/sdk-server/src/store/serialization.ts +++ b/packages/shared/sdk-server/src/store/serialization.ts @@ -13,19 +13,6 @@ import VersionedDataKinds, { VersionedDataKind } from './VersionedDataKinds'; // The max size where we use an array instead of a set. const TARGET_LIST_ARRAY_CUTOFF = 100; -/** - * @internal - */ -export function reviver(this: any, key: string, value: any): any { - // Whenever a null is included we want to remove the field. - // In this way validation checks do not have to consider null, only undefined. - if (value === null) { - return undefined; - } - - return value; -} - export interface FlagsAndSegments { flags: { [name: string]: Flag }; segments: { [name: string]: Segment }; @@ -35,6 +22,61 @@ export interface AllData { data: FlagsAndSegments; } +/** + * Performs deep removal of null values. + * + * Does not remove null values from arrays. + * + * Note: This is a non-recursive implementation for performance and to avoid + * potential stack overflows. + * + * @param target The target to remove null values from. + * @param excludeKeys A list of top-level keys to exclude from null removal. + */ +export function nullReplacer(target: any, excludeKeys?: string[]): void { + const stack: { + key: string; + value: any; + parent: any; + }[] = []; + + if (target === null || target === undefined) { + return; + } + + const filteredEntries = Object.entries(target).filter( + ([key, _value]) => !excludeKeys?.includes(key), + ); + + stack.push( + ...filteredEntries.map(([key, value]) => ({ + key, + value, + parent: target, + })), + ); + + while (stack.length) { + const item = stack.pop()!; + // Do not remove items from arrays. + if (item.value === null && !Array.isArray(item.parent)) { + delete item.parent[item.key]; + } else if (typeof item.value === 'object' && item.value !== null) { + // Add all the children to the stack. This includes array children. + // The items in the array could themselves be objects which need nulls + // removed from them. + stack.push( + ...Object.entries(item.value).map(([key, value]) => ({ + key, + value, + parent: item.value, + skip: false, + })), + ); + } + } +} + /** * For use when serializing flags/segments. This will ensure local types * are converted to the appropriate JSON representation. @@ -54,6 +96,10 @@ export function replacer(this: any, key: string, value: any): any { return undefined; } } + // Allow null/undefined values to pass through without modification. + if (value === null || value === undefined) { + return value; + } if (value.generated_includedSet) { value.included = [...value.generated_includedSet]; delete value.generated_includedSet; @@ -108,6 +154,8 @@ function processRollout(rollout?: Rollout) { * @internal */ export function processFlag(flag: Flag) { + nullReplacer(flag, ['variations']); + if (flag.fallthrough && flag.fallthrough.rollout) { const rollout = flag.fallthrough.rollout!; processRollout(rollout); @@ -131,6 +179,7 @@ export function processFlag(flag: Flag) { * @internal */ export function processSegment(segment: Segment) { + nullReplacer(segment); if (segment?.included?.length && segment.included.length > TARGET_LIST_ARRAY_CUTOFF) { segment.generated_includedSet = new Set(segment.included); delete segment.included; @@ -183,7 +232,7 @@ export function processSegment(segment: Segment) { function tryParse(data: string): any { try { - return JSON.parse(data, reviver); + return JSON.parse(data); } catch { return undefined; }