From 65cb69d25d7cd47b04b013ee02d2c8453e3d3e72 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Thu, 5 Sep 2024 10:24:41 -0700 Subject: [PATCH 1/4] fix: Correctly handle null values in JSON variations. --- packages/shared/sdk-server/src/store/serialization.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/shared/sdk-server/src/store/serialization.ts b/packages/shared/sdk-server/src/store/serialization.ts index aaba1bc4de..d3e254d5df 100644 --- a/packages/shared/sdk-server/src/store/serialization.ts +++ b/packages/shared/sdk-server/src/store/serialization.ts @@ -54,6 +54,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; From 9d0c7ab5dee41730cb2ccbc97a1005606a57f5ec Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Thu, 5 Sep 2024 12:44:19 -0700 Subject: [PATCH 2/4] fix: Correctly handle nulls within JSON variations. --- .../__tests__/store/serialization.test.ts | 151 +++++++++++++++++- .../sdk-server/src/store/serialization.ts | 75 +++++++-- 2 files changed, 209 insertions(+), 17 deletions(-) diff --git a/packages/shared/sdk-server/__tests__/store/serialization.test.ts b/packages/shared/sdk-server/__tests__/store/serialization.test.ts index 4656ea5513..471cf09e1f 100644 --- a/packages/shared/sdk-server/__tests__/store/serialization.test.ts +++ b/packages/shared/sdk-server/__tests__/store/serialization.test.ts @@ -1,11 +1,13 @@ +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, serializeSegment, } from '../../src/store/serialization'; @@ -152,6 +154,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 +273,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 +360,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 +412,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 of null value', () => { + expect(() => { + nullReplacer(null); + nullReplacer(undefined); + }).not.toThrow(); +}); + it.each([ [flagWithAttributeNameInClause, undefined], [flagWithAttributeReferenceInClause, undefined], diff --git a/packages/shared/sdk-server/src/store/serialization.ts b/packages/shared/sdk-server/src/store/serialization.ts index d3e254d5df..798c2afb6c 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,63 @@ 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; + skip: boolean; + }[] = []; + + if (target === null || target === undefined) { + return; + } + + stack.push( + ...Object.entries(target).map(([key, value]) => ({ + skip: excludeKeys?.includes(key) ?? false, + key, + value, + parent: target, + })), + ); + + while (stack.length) { + const item = stack.pop()!; + if (item.skip) { + // eslint-disable-next-line no-continue + continue; + } + // 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. @@ -112,6 +156,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); @@ -135,6 +181,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; @@ -187,7 +234,7 @@ export function processSegment(segment: Segment) { function tryParse(data: string): any { try { - return JSON.parse(data, reviver); + return JSON.parse(data); } catch { return undefined; } From 46855760914564336980f7a2de72de135768c98b Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Thu, 5 Sep 2024 13:09:15 -0700 Subject: [PATCH 3/4] Typo --- .../shared/sdk-server/__tests__/store/serialization.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/shared/sdk-server/__tests__/store/serialization.test.ts b/packages/shared/sdk-server/__tests__/store/serialization.test.ts index 471cf09e1f..7e7716f060 100644 --- a/packages/shared/sdk-server/__tests__/store/serialization.test.ts +++ b/packages/shared/sdk-server/__tests__/store/serialization.test.ts @@ -448,7 +448,7 @@ it('does remove null from objects that are inside of arrays', () => { }); }); -it('can handle attempting to replace nulls for an undefined of null value', () => { +it('can handle attempting to replace nulls for an undefined or null value', () => { expect(() => { nullReplacer(null); nullReplacer(undefined); From c823bf3bb5ea0f1aed487675548a15013fb6746c Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Thu, 5 Sep 2024 13:42:59 -0700 Subject: [PATCH 4/4] PR feedback. Additional test. --- .../sdk-server/__tests__/store/serialization.test.ts | 9 +++++++++ .../shared/sdk-server/src/store/serialization.ts | 12 +++++------- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/packages/shared/sdk-server/__tests__/store/serialization.test.ts b/packages/shared/sdk-server/__tests__/store/serialization.test.ts index 7e7716f060..56e41e964b 100644 --- a/packages/shared/sdk-server/__tests__/store/serialization.test.ts +++ b/packages/shared/sdk-server/__tests__/store/serialization.test.ts @@ -8,6 +8,7 @@ import { deserializePatch, nullReplacer, replacer, + serializeFlag, serializeSegment, } from '../../src/store/serialization'; @@ -595,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 798c2afb6c..4af09bb832 100644 --- a/packages/shared/sdk-server/src/store/serialization.ts +++ b/packages/shared/sdk-server/src/store/serialization.ts @@ -38,16 +38,18 @@ export function nullReplacer(target: any, excludeKeys?: string[]): void { key: string; value: any; parent: any; - skip: boolean; }[] = []; if (target === null || target === undefined) { return; } + const filteredEntries = Object.entries(target).filter( + ([key, _value]) => !excludeKeys?.includes(key), + ); + stack.push( - ...Object.entries(target).map(([key, value]) => ({ - skip: excludeKeys?.includes(key) ?? false, + ...filteredEntries.map(([key, value]) => ({ key, value, parent: target, @@ -56,10 +58,6 @@ export function nullReplacer(target: any, excludeKeys?: string[]): void { while (stack.length) { const item = stack.pop()!; - if (item.skip) { - // eslint-disable-next-line no-continue - continue; - } // Do not remove items from arrays. if (item.value === null && !Array.isArray(item.parent)) { delete item.parent[item.key];