Skip to content

Commit

Permalink
fix: Correctly handle null values in JSON variations. (#569)
Browse files Browse the repository at this point in the history 
During the typescript implementation null values were removed during
JSON de-serialization. This was over-zealous as variations can be JSON
which contains null values.

This retains null value removal for everything aside from variations.

The reason this was originally done is to simplify all code which
interacts with the data model (It only needs to check undefined versus
null/undefined.). It also simplifies the ability to produce a compact
representation that omits any null fields.

In the future we may want to consider removing this behavior.

Fixes #568
  • Loading branch information
@kinyoklion
kinyoklion authored Sep 5, 2024
1 parent 4792391 commit 907d08b
Show file tree
Hide file tree
Showing 2 changed files with 220 additions and 17 deletions.
160 changes: 157 additions & 3 deletions packages/shared/sdk-server/__tests__/store/serialization.test.ts
View file
Original file line number Diff line number Diff line change
@@ -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';

Expand Down Expand Up @@ -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: {
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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',
Expand All @@ -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],
Expand Down Expand Up @@ -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);
});
77 changes: 63 additions & 14 deletions packages/shared/sdk-server/src/store/serialization.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Expand All @@ -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.
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down

0 comments on commit 907d08b

Please sign in to comment.