fix: Correctly handle null values in JSON variations.

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);
+});

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;
   }