Skip to content

Commit 2d20825

Browse files
btthomasclaude
andcommitted
refactor(flags): tighten numeric validation, document serialId/metadata (review M/L)
Remaining PR1 review follow-ups: - M1: integer flags require a whole number; number/float require a finite value (reject NaN/Infinity) so native parsers round-trip. - M2: document that serialId is intentionally not propagated (no FlagCacheEntry slot; native snapshot omits it too). - L3: comment that only flags (and obfuscated) are load-bearing; the rest of the response attributes are optional/ignored on purpose. - L1/L5: broaden round-trip fixture (number + nested object flags) and add tests for fractional integer, non-finite number, and a structurally broken response. L2 (distinguishing parse-failure from valid-but-empty) is intentionally left to PR2 (FFL-2688). L4 (configurationToString vs the reference's bug) is correct as-is. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent b509254 commit 2d20825

4 files changed

Lines changed: 59 additions & 2 deletions

File tree

packages/core/src/flags/configuration/__tests__/precomputed.test.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,35 @@ describe('decodePrecomputedFlags', () => {
178178
expect(decodePrecomputedFlags(responseWith({}))).toEqual({});
179179
});
180180

181+
it('omits an integer flag with a fractional value', () => {
182+
const cache = decodePrecomputedFlags(
183+
responseWith({
184+
frac: flag({ variationType: 'integer', variationValue: 7.9 })
185+
})
186+
);
187+
188+
expect(cache.frac).toBeUndefined();
189+
expect(InternalLog.log).toHaveBeenCalled();
190+
});
191+
192+
it('omits a number flag whose value is not finite', () => {
193+
const cache = decodePrecomputedFlags(
194+
responseWith({
195+
inf: flag({ variationType: 'number', variationValue: Infinity })
196+
})
197+
);
198+
199+
expect(cache.inf).toBeUndefined();
200+
});
201+
202+
it('returns an empty map for a structurally broken response', () => {
203+
expect(
204+
decodePrecomputedFlags(
205+
({} as unknown) as Parameters<typeof decodePrecomputedFlags>[0]
206+
)
207+
).toEqual({});
208+
});
209+
181210
it('rejects an array value for an object flag', () => {
182211
const cache = decodePrecomputedFlags(
183212
responseWith({

packages/core/src/flags/configuration/__tests__/wire.test.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,24 @@ const buildResponse = () => ({
2525
reason: 'STATIC',
2626
doLog: false,
2727
extraLogging: {}
28+
},
29+
'num-flag': {
30+
variationType: 'number',
31+
variationValue: 1.5,
32+
variationKey: '1.5',
33+
allocationKey: 'alloc-2',
34+
reason: 'STATIC',
35+
doLog: true,
36+
extraLogging: {}
37+
},
38+
'obj-flag': {
39+
variationType: 'object',
40+
variationValue: { nested: { a: 1 }, list: [1, 2] },
41+
variationKey: 'obj',
42+
allocationKey: 'alloc-3',
43+
reason: 'TARGETING_MATCH',
44+
doLog: false,
45+
extraLogging: { extra: 'x' }
2846
}
2947
}
3048
}

packages/core/src/flags/configuration/precomputed.ts

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,9 @@ const toFlagCacheEntry = (
9090
return null;
9191
}
9292

93+
// `serialId` is intentionally not propagated: `FlagCacheEntry` has no slot for it
94+
// and the native CDN-fetched snapshot omits it too, so dropping it keeps
95+
// offline/online parity.
9396
return {
9497
key,
9598
value: variationValue,
@@ -113,9 +116,13 @@ const valueMatchesVariationType = (
113116
case 'string':
114117
return typeof value === 'string';
115118
case 'number':
116-
case 'integer':
117119
case 'float':
118-
return typeof value === 'number';
120+
// Reject NaN/Infinity: native parsers can't round-trip them.
121+
return typeof value === 'number' && Number.isFinite(value);
122+
case 'integer':
123+
// A fractional value under an integer flag would be truncated/mis-parsed
124+
// natively, so require a whole number.
125+
return Number.isInteger(value);
119126
case 'object':
120127
// Object flags are a JSON object at the root; arrays are not valid values.
121128
return (

packages/core/src/flags/configuration/types.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,9 @@ export interface PrecomputedConfigurationResponse {
6565
id?: string;
6666
type?: string;
6767
attributes: {
68+
// Only `flags` is load-bearing (and `obfuscated`, which gates support). The
69+
// remaining fields are metadata the decoder ignores; typed loosely/optional
70+
// on purpose so payload variation across environments doesn't matter.
6871
obfuscated?: boolean;
6972
createdAt?: string;
7073
format?: string;

0 commit comments

Comments
 (0)