diff --git a/packages/shared/sdk-server/__tests__/LDClient.allFlags.test.ts b/packages/shared/sdk-server/__tests__/LDClient.allFlags.test.ts index 830f25100b..a9fdc91acf 100644 --- a/packages/shared/sdk-server/__tests__/LDClient.allFlags.test.ts +++ b/packages/shared/sdk-server/__tests__/LDClient.allFlags.test.ts @@ -1,5 +1,8 @@ import { LDClientImpl } from '../src'; import TestData from '../src/integrations/test_data/TestData'; +import AsyncStoreFacade from '../src/store/AsyncStoreFacade'; +import InMemoryFeatureStore from '../src/store/InMemoryFeatureStore'; +import VersionedDataKinds from '../src/store/VersionedDataKinds'; import { createBasicPlatform } from './createBasicPlatform'; import TestLogger, { LogLevel } from './Logger'; import makeCallbacks from './makeCallbacks'; @@ -369,3 +372,124 @@ describe('given an offline client', () => { }); }); }); + +describe('given an LDClient with a flag the evaluator cannot read', () => { + let client: LDClientImpl; + let td: TestData; + let onError: jest.Mock; + + beforeEach(async () => { + td = new TestData(); + onError = jest.fn(); + client = new LDClientImpl( + 'sdk-key-all-flags-malformed', + createBasicPlatform(), + { + updateProcessor: td.getFactory(), + sendEvents: false, + }, + { ...makeCallbacks(true), onError }, + ); + + await client.waitForInitialization({ timeout: 10 }); + // LaunchDarkly never sends a flag without variations, but a custom store or a data file + // can. A healthy flag sits next to it to show the rest of the state is still built. + await td.usePreconfiguredFlag({ + key: 'malformed', + version: 1, + on: true, + fallthrough: { variation: 0 }, + }); + await td.usePreconfiguredFlag({ + key: 'healthy', + version: 1, + on: false, + offVariation: 0, + variations: ['a'], + }); + }); + + afterEach(() => { + client.close(); + }); + + it('resolves allFlagsState, reports the flag through the error callback, and keeps the rest', async () => { + const state = await client.allFlagsState(defaultUser, { withReasons: true }); + + expect(state.valid).toBe(true); + expect(state.getFlagValue('healthy')).toBe('a'); + expect(state.getFlagValue('malformed')).toBeNull(); + expect(state.getFlagReason('malformed')).toEqual({ + kind: 'ERROR', + errorKind: 'MALFORMED_FLAG', + }); + expect(onError).toHaveBeenCalledTimes(1); + expect(onError.mock.calls[0][0].message).toMatch( + /^Error for feature flag "malformed" while evaluating all flags: /, + ); + }); +}); + +describe('given an LDClient whose store holds an item that fails after evaluation', () => { + let client: LDClientImpl; + let onError: jest.Mock; + + beforeEach(async () => { + const store = new InMemoryFeatureStore(); + const td = new TestData(); + onError = jest.fn(); + client = new LDClientImpl( + 'sdk-key-all-flags-unreadable', + createBasicPlatform(), + { + updateProcessor: td.getFactory(), + sendEvents: false, + featureStore: store, + }, + { ...makeCallbacks(true), onError }, + ); + await client.waitForInitialization({ timeout: 10 }); + + // The evaluator turns a definition it cannot read into an error result, which allFlagsState + // handles like any other error. This item evaluates cleanly and then throws when its event + // settings are read while the state is assembled, which stands in for any unexpected + // failure outside of evaluation. Before the error path existed, the promise never settled. + const unreadable = new Proxy( + { key: 'unreadable', version: 1, on: false, offVariation: 0, variations: ['a'] }, + { + get(target, property) { + if (property === 'trackEvents') { + throw new Error('unreadable item'); + } + return target[property as keyof typeof target]; + }, + }, + ); + // The store keeps the object handed to init, while upsert copies it and would drop the trap. + await new AsyncStoreFacade(store).init({ + [VersionedDataKinds.Features.namespace]: { unreadable }, + [VersionedDataKinds.Segments.namespace]: {}, + }); + }); + + afterEach(() => { + client.close(); + }); + + it('reports the failure through the error callback and resolves with an invalid state', async () => { + const state = await client.allFlagsState(defaultUser); + + expect(state.valid).toBe(false); + expect(state.allValues()).toEqual({}); + expect(onError).toHaveBeenCalledTimes(1); + expect(onError.mock.calls[0][0].message).toBe('unreadable item'); + }); + + it('delivers the invalid state to a callback', (done) => { + client.allFlagsState(defaultUser, {}, (err, state) => { + expect(err).toBeNull(); + expect(state.valid).toBe(false); + done(); + }); + }); +}); diff --git a/packages/shared/sdk-server/__tests__/LDClient.evaluation.test.ts b/packages/shared/sdk-server/__tests__/LDClient.evaluation.test.ts index 1711068410..fa0ff07746 100644 --- a/packages/shared/sdk-server/__tests__/LDClient.evaluation.test.ts +++ b/packages/shared/sdk-server/__tests__/LDClient.evaluation.test.ts @@ -34,6 +34,27 @@ describe('given an LDClient with test data', () => { client.close(); }); + it('returns the default value with a malformed flag reason for a flag it cannot read', async () => { + // A target without values throws while the flag is read. LaunchDarkly never sends one, + // but a custom store or a data file can. + await td.usePreconfiguredFlag({ + key: 'malformed', + version: 1, + on: true, + targets: [{ variation: 0 }], + fallthrough: { variation: 1 }, + variations: ['a', 'b'], + }); + + const detail = await client.variationDetail('malformed', defaultUser, 'default'); + expect(detail).toEqual({ + value: 'default', + variationIndex: null, + reason: { kind: 'ERROR', errorKind: 'MALFORMED_FLAG' }, + }); + expect(await client.variation('malformed', defaultUser, 'default')).toBe('default'); + }); + it('evaluates a flag which has a fallthrough and a rule', async () => { const testId = 'abcd'.repeat(8); const flagKey = 'testFlag'; diff --git a/packages/shared/sdk-server/__tests__/evaluation/Evaluator.malformed.test.ts b/packages/shared/sdk-server/__tests__/evaluation/Evaluator.malformed.test.ts new file mode 100644 index 0000000000..0d4d458674 --- /dev/null +++ b/packages/shared/sdk-server/__tests__/evaluation/Evaluator.malformed.test.ts @@ -0,0 +1,273 @@ +import { AttributeReference, Context } from '@launchdarkly/js-sdk-common'; + +import { BigSegmentStoreMembership } from '../../src/api/interfaces'; +import { Clause } from '../../src/evaluation/data/Clause'; +import { Flag } from '../../src/evaluation/data/Flag'; +import { Segment } from '../../src/evaluation/data/Segment'; +import EvalResult from '../../src/evaluation/EvalResult'; +import Evaluator from '../../src/evaluation/Evaluator'; +import { Queries } from '../../src/evaluation/Queries'; +import EventFactory from '../../src/events/EventFactory'; +import { createBasicPlatform } from '../createBasicPlatform'; + +// LaunchDarkly never sends these shapes, but a custom store, a data file, or an override can +// hand the evaluator anything. Each test awaits the evaluation directly: if the evaluator threw, +// the promise would reject, and if it never delivered a result, the test would time out. + +const userContext = Context.fromLDContext({ key: 'user-key' }); + +// The definitions are deliberately malformed, so they are built as plain objects and cast +// rather than typed as Flag or Segment. +function makeFlag(overrides: Record): Flag { + return { + key: 'malformed-flag', + version: 1, + on: true, + fallthrough: { variation: 1 }, + variations: ['zero', 'one'], + ...overrides, + } as unknown as Flag; +} + +function makeClause(overrides: Record): Clause { + return { + attribute: 'key', + attributeReference: new AttributeReference('key'), + op: 'in', + values: ['user-key'], + contextKind: 'user', + ...overrides, + } as unknown as Clause; +} + +function makeSegment(overrides: Record): Segment { + return { key: 'segment', version: 1, ...overrides } as unknown as Segment; +} + +function makeFlagMatchingSegment(segmentKey: string): Flag { + return makeFlag({ + rules: [ + { + id: 'rule', + variation: 0, + clauses: [makeClause({ op: 'segmentMatch', values: [segmentKey] })], + }, + ], + }); +} + +class TestQueries implements Queries { + constructor( + private readonly _data: { + flags?: Flag[]; + segments?: Segment[]; + membership?: BigSegmentStoreMembership; + }, + private readonly _callsBackAsync: boolean = false, + ) {} + + getFlag(key: string, cb: (flag: Flag | undefined) => void): void { + this._deliver(() => cb(this._data.flags?.find((flag) => flag.key === key))); + } + + getSegment(key: string, cb: (segment: Segment | undefined) => void): void { + this._deliver(() => cb(this._data.segments?.find((segment) => segment.key === key))); + } + + getBigSegmentsMembership(): Promise<[BigSegmentStoreMembership | null, string] | undefined> { + const result: [BigSegmentStoreMembership | null, string] = [ + this._data.membership ?? null, + 'HEALTHY', + ]; + return Promise.resolve(result); + } + + // A store backed by a database calls back after its own promise resolves, which takes the + // evaluator off the stack of the call that started the evaluation. + private _deliver(fn: () => void): void { + if (this._callsBackAsync) { + Promise.resolve().then(fn); + } else { + fn(); + } + } +} + +function expectMalformed(result: EvalResult) { + expect(result.isError).toBe(true); + expect(result.detail).toMatchObject({ + value: null, + variationIndex: null, + reason: { kind: 'ERROR', errorKind: 'MALFORMED_FLAG' }, + }); +} + +describe('given an evaluator and flag definitions it cannot read', () => { + let evaluator: Evaluator; + + beforeEach(() => { + evaluator = new Evaluator(createBasicPlatform(), new TestQueries({})); + }); + + it('returns a malformed flag result for a flag with no variations', async () => { + const result = await evaluator.evaluate(makeFlag({ variations: undefined }), userContext); + expectMalformed(result); + expect(result.message).toBe('Flag variations are not an array'); + }); + + it('returns a malformed flag result when variations are not an array', async () => { + // A string has a length and can be indexed, so without an explicit check one of its + // characters would be served as the flag value. + const result = await evaluator.evaluate(makeFlag({ variations: 'abc' }), userContext); + expectMalformed(result); + expect(result.message).toBe('Flag variations are not an array'); + }); + + it('returns a malformed flag result for a rule whose clauses are not an array', async () => { + // A rule with no clauses at all is treated as not matching. Clauses that are present but + // not a list cannot be iterated, and an object would otherwise match every context. + const flag = makeFlag({ rules: [{ id: 'rule', variation: 0, clauses: {} }] }); + const result = await evaluator.evaluate(flag, userContext); + expectMalformed(result); + expect(result.message).toContain('Flag "malformed-flag" has a malformed definition'); + expect(result.message).toContain('Expected an array but received object'); + }); + + it('returns a malformed flag result for a clause with no values', async () => { + const flag = makeFlag({ + rules: [{ id: 'rule', variation: 0, clauses: [makeClause({ values: undefined })] }], + }); + const result = await evaluator.evaluate(flag, userContext); + expectMalformed(result); + expect(result.message).toContain('Flag "malformed-flag" has a malformed definition'); + }); + + it('returns a malformed flag result for a target with no values', async () => { + const flag = makeFlag({ targets: [{ variation: 0 }] }); + const result = await evaluator.evaluate(flag, userContext); + expectMalformed(result); + expect(result.message).toContain('Flag "malformed-flag" has a malformed definition'); + }); + + it('returns a malformed flag result for a context target with no values', async () => { + // A context target is only read for a context of its kind. + const flag = makeFlag({ contextTargets: [{ contextKind: 'org', variation: 0 }] }); + const orgContext = Context.fromLDContext({ kind: 'org', key: 'org-key' }); + const result = await evaluator.evaluate(flag, orgContext); + expectMalformed(result); + expect(result.message).toContain('Flag "malformed-flag" has a malformed definition'); + }); + + it('delivers the result once and propagates an exception thrown by the receiving callback', () => { + // The guards are for exceptions raised while reading the definition. One raised by the + // caller after the result was delivered is not a flag problem and must stay visible, and + // it must not cause the result to be delivered a second time on its way out. + const flag = makeFlag({ + rules: [{ id: 'rule', variation: 0, clauses: [makeClause({ values: undefined })] }], + }); + const cb = jest.fn(() => { + throw new Error('from the caller'); + }); + expect(() => { + evaluator.evaluateCb(flag, userContext, cb); + }).toThrow('from the caller'); + expect(cb).toHaveBeenCalledTimes(1); + }); +}); + +describe.each([ + ['synchronously', false], + ['asynchronously', true], +])('given a store that calls back %s', (_description, callsBackAsync) => { + it('returns a malformed flag result for a segment whose included list is not an array', async () => { + const segment = makeSegment({ included: {} }); + const evaluator = new Evaluator( + createBasicPlatform(), + new TestQueries({ segments: [segment] }, callsBackAsync), + ); + const result = await evaluator.evaluate(makeFlagMatchingSegment(segment.key), userContext); + expectMalformed(result); + expect(result.message).toContain('Flag "malformed-flag" has a malformed definition'); + }); + + it('returns a malformed flag result for a segment rule clause with no values', async () => { + const segment = makeSegment({ rules: [{ clauses: [makeClause({ values: undefined })] }] }); + const evaluator = new Evaluator( + createBasicPlatform(), + new TestQueries({ segments: [segment] }, callsBackAsync), + ); + const result = await evaluator.evaluate(makeFlagMatchingSegment(segment.key), userContext); + expectMalformed(result); + expect(result.message).toContain('Flag "malformed-flag" has a malformed definition'); + }); + + it('returns a malformed flag result for the parent of a malformed prerequisite', async () => { + const prerequisite = makeFlag({ key: 'prerequisite', targets: [{ variation: 0 }] }); + const parent = makeFlag({ + key: 'parent', + prerequisites: [{ key: 'prerequisite', variation: 1 }], + }); + const evaluator = new Evaluator( + createBasicPlatform(), + new TestQueries({ flags: [prerequisite] }, callsBackAsync), + ); + const result = await evaluator.evaluate(parent, userContext, new EventFactory(true)); + expectMalformed(result); + expect(result.message).toContain('Flag "parent" has a malformed definition'); + // The prerequisite never produced a result, so there is no evaluation to record for it + // and no event to send. Recording it without an event would misreport what ran. + expect(result.prerequisites).toBeUndefined(); + expect(result.events).toBeUndefined(); + }); +}); + +describe('given an unbounded segment whose rules cannot be read', () => { + // Big segment membership is looked up through a promise, so the rest of the evaluation + // runs inside promise continuations rather than on the caller's stack. + const segment = makeSegment({ + unbounded: true, + generation: 1, + rules: [{ clauses: [makeClause({ values: undefined })] }], + }); + let evaluator: Evaluator; + + beforeEach(() => { + evaluator = new Evaluator( + createBasicPlatform(), + new TestQueries({ segments: [segment], membership: {} }), + ); + }); + + it('returns a malformed flag result instead of an unhandled rejection', async () => { + const result = await evaluator.evaluate(makeFlagMatchingSegment(segment.key), userContext); + expectMalformed(result); + expect(result.message).toContain('Flag "malformed-flag" has a malformed definition'); + // The error result carries the state gathered before the failure, like other errors do. + expect(result.detail.reason.bigSegmentsStatus).toBe('HEALTHY'); + }); +}); + +describe('given a big segment lookup that rejects', () => { + // The big segments manager turns store failures into a status, so a rejection here would be + // a bug. Before the guard it was an unhandled rejection and the evaluation never finished. + const segment = makeSegment({ unbounded: true, generation: 1 }); + + class RejectingQueries extends TestQueries { + override getBigSegmentsMembership(): Promise< + [BigSegmentStoreMembership | null, string] | undefined + > { + return Promise.reject(new Error('lookup failed')); + } + } + + it('returns a malformed flag result carrying the rejection message', async () => { + const evaluator = new Evaluator( + createBasicPlatform(), + new RejectingQueries({ segments: [segment] }), + ); + const result = await evaluator.evaluate(makeFlagMatchingSegment(segment.key), userContext); + expectMalformed(result); + expect(result.message).toContain('Flag "malformed-flag" has a malformed definition'); + expect(result.message).toContain('lookup failed'); + }); +}); diff --git a/packages/shared/sdk-server/__tests__/evaluation/collection.test.ts b/packages/shared/sdk-server/__tests__/evaluation/collection.test.ts new file mode 100644 index 0000000000..14b8b29fc7 --- /dev/null +++ b/packages/shared/sdk-server/__tests__/evaluation/collection.test.ts @@ -0,0 +1,139 @@ +import { allAsync, allSeriesAsync, firstSeriesAsync } from '../../src/evaluation/collection'; + +// The series helpers recurse synchronously for small collections and defer to a resolved +// promise once a collection has more than 50 items, so a throw after that point happens off the +// caller's stack. Both sizes are exercised. Each run settles on whichever callback fires first, +// so a result delivered where an error was expected fails the assertion rather than hanging. If +// a helper swallowed an exception and called neither callback, the test would time out. +const smallCollection = [0, 1, 2]; +const largeCollection = Array.from({ length: 60 }, (_, index) => index); + +type SeriesHelper = typeof allSeriesAsync; +type SeriesCheck = Parameters[1]; +type Outcome = { result: boolean | null | undefined } | { error: unknown }; + +function runSeries( + helper: SeriesHelper, + collection: unknown, + check: SeriesCheck, +): Promise { + return new Promise((resolve) => { + helper( + collection as number[], + check, + (result) => resolve({ result }), + (error) => resolve({ error }), + ); + }); +} + +function runAll(collection: number[], check: Parameters[1]): Promise { + return new Promise((resolve) => { + allAsync( + collection, + check, + (result) => resolve({ result }), + (error) => resolve({ error }), + ); + }); +} + +describe.each<[string, SeriesHelper, boolean]>([ + // The third column is the value a check must produce for iteration to move to the next item. + ['allSeriesAsync', allSeriesAsync, true], + ['firstSeriesAsync', firstSeriesAsync, false], +])('%s', (_name, helper, continueValue) => { + it('calls cb with the aggregate result when no check throws', async () => { + const outcome = await runSeries(helper, largeCollection, (_item, _index, itemCb) => + itemCb(continueValue), + ); + expect(outcome).toEqual({ result: continueValue }); + }); + + it.each<[string, number[], number]>([ + ['a small', smallCollection, 1], + ['a large', largeCollection, 55], + ])( + 'reports an exception from a check in %s collection through onError', + async (_description, collection, throwAt) => { + const outcome = await runSeries(helper, collection, (item, _index, itemCb) => { + if (item === throwAt) { + throw new Error(`boom at ${item}`); + } + itemCb(continueValue); + }); + expect(outcome).toEqual({ error: new Error(`boom at ${throwAt}`) }); + }, + ); + + it('reports an exception from the completion callback after an asynchronous check', async () => { + // Once a check calls back from a promise, the rest of the iteration runs in that + // continuation, where the try/catch around the original call is no longer on the stack. + // The first item ends the iteration early, so the completion callback runs there directly. + const error = await new Promise((resolve) => { + helper( + smallCollection, + (_item, _index, itemCb) => { + Promise.resolve().then(() => itemCb(!continueValue)); + }, + () => { + throw new Error('from cb after async callback'); + }, + resolve, + ); + }); + expect(error).toEqual(new Error('from cb after async callback')); + }); + + it('reports an exception from the completion callback through onError', async () => { + const error = await new Promise((resolve) => { + helper( + smallCollection, + (_item, _index, itemCb) => itemCb(continueValue), + () => { + throw new Error('from cb'); + }, + resolve, + ); + }); + expect(error).toEqual(new Error('from cb')); + }); + + it('reports a collection that is not an array through onError without running a check', async () => { + const check = jest.fn((_item, _index, itemCb) => itemCb(continueValue)); + const outcome = await runSeries(helper, {}, check); + expect(outcome).toEqual({ error: new TypeError('Expected an array but received object') }); + expect(check).not.toHaveBeenCalled(); + }); +}); + +describe('allAsync', () => { + it('calls cb with true when every check passes', async () => { + const outcome = await runAll(largeCollection, (_item, itemCb) => itemCb(true)); + expect(outcome).toEqual({ result: true }); + }); + + it('reports an exception from a check through onError', async () => { + const outcome = await runAll(smallCollection, (item, itemCb) => { + if (item === 1) { + throw new Error('boom at 1'); + } + itemCb(true); + }); + expect(outcome).toEqual({ error: new Error('boom at 1') }); + }); + + it('reports an exception from the completion callback through onError', async () => { + const error = await new Promise((resolve) => { + allAsync( + smallCollection, + (_item, itemCb) => itemCb(true), + () => { + throw new Error('from cb'); + }, + resolve, + ); + }); + expect(error).toEqual(new Error('from cb')); + }); +}); diff --git a/packages/shared/sdk-server/src/LDClientImpl.ts b/packages/shared/sdk-server/src/LDClientImpl.ts index 38329c97b7..1e82398952 100644 --- a/packages/shared/sdk-server/src/LDClientImpl.ts +++ b/packages/shared/sdk-server/src/LDClientImpl.ts @@ -1152,6 +1152,7 @@ export default class LDClientImpl implements LDClient { const clientOnly = !!options?.clientSideOnly; const detailsOnlyIfTracked = !!options?.detailsOnlyForTrackedFlags; + let delivered = false; allAsync( Object.values(allFlags), (storeItem, iterCb) => { @@ -1183,10 +1184,26 @@ export default class LDClientImpl implements LDClient { }); }, () => { + delivered = true; const res = builder.build(); callback?.(null, res); resolve(res); }, + (err) => { + if (delivered) { + // The state has already been handed to the caller, so this exception came from + // the caller's callback. Let it propagate as it did before. + throw err; + } + delivered = true; + // The evaluator reports a malformed flag as an error result, which is handled + // above, so this is an unexpected failure while assembling the state. Report it + // and return an invalid state rather than leaving the promise pending. + this._onError(err instanceof Error ? err : new Error(String(err))); + const res = new FlagsStateBuilder(false, false).build(); + callback?.(null, res); + resolve(res); + }, ); }); if (!this.initialized()) { diff --git a/packages/shared/sdk-server/src/evaluation/Evaluator.ts b/packages/shared/sdk-server/src/evaluation/Evaluator.ts index b3acc25369..097f0e5fdb 100644 --- a/packages/shared/sdk-server/src/evaluation/Evaluator.ts +++ b/packages/shared/sdk-server/src/evaluation/Evaluator.ts @@ -70,6 +70,19 @@ interface EvalState { bigSegmentsStatus?: BigSegmentStoreStatusString; bigSegmentsMembership?: Record; + + /** + * True once the top-level result has been handed to the caller. Nothing is delivered twice. + */ + delivered: boolean; + + /** + * Receives an exception raised while reading the flag, or a prerequisite or segment it + * references, so that it can be delivered as an error result instead of escaping. The state + * is shared with prerequisite evaluations, so an exception in a prerequisite fails the + * top-level flag. + */ + onError: (err: unknown) => void; } interface Match { @@ -92,6 +105,44 @@ function makeError(result: EvalResult): MatchError { return { error: true, isMatch: false, result }; } +/** + * Run part of an evaluation that may execute outside of the try/catch in evaluateCb, such as + * the continuation of a store lookup, routing any exception to the state's error handler. + */ +function guarded(state: EvalState, fn: () => void): void { + try { + fn(); + } catch (err) { + state.onError(err); + } +} + +/** + * Deliver the top-level result to the caller exactly once, decorated with the state + * accumulated during the evaluation. + */ +function deliverResult(state: EvalState, res: EvalResult, cb: (res: EvalResult) => void): void { + if (state.delivered) { + return; + } + state.delivered = true; + if (state.bigSegmentsStatus) { + res.detail.reason = { + ...res.detail.reason, + bigSegmentsStatus: state.bigSegmentsStatus, + }; + } + if (state.prerequisites) { + res.prerequisites = state.prerequisites; + } + res.events = state.events; + cb(res); +} + +function errorMessage(err: unknown): string { + return err instanceof Error ? err.message : String(err); +} + /** * MatchOrError effectively creates a discriminated union for the segment * matching process. Allowing encoding a true/false match result, or an @@ -124,28 +175,40 @@ export default class Evaluator { cb: (res: EvalResult) => void, eventFactory?: EventFactory, ) { - const state: EvalState = {}; - this._evaluateInternal( - flag, - context, - state, - [], - (res) => { - if (state.bigSegmentsStatus) { - res.detail.reason = { - ...res.detail.reason, - bigSegmentsStatus: state.bigSegmentsStatus, - }; - } - if (state.prerequisites) { - res.prerequisites = state.prerequisites; + const state: EvalState = { + delivered: false, + onError: (err) => { + if (state.delivered) { + // The result has already been handed to the caller, so this exception came from the + // caller's callback or from work that ran after delivery, not from reading the flag. + // Let it propagate as it did before. + throw err; } - res.events = state.events; - cb(res); + deliverResult( + state, + EvalResult.forError( + ErrorKinds.MalformedFlag, + `Flag "${flag.key}" has a malformed definition, or references a malformed` + + ` prerequisite or segment: ${errorMessage(err)}`, + ), + cb, + ); }, - true, - eventFactory, - ); + }; + // Reading a malformed definition throws, and most of an evaluation runs synchronously, so + // this guard turns those exceptions into an error result. Steps that resume after a store + // callback or a promise are guarded where they resume. + guarded(state, () => { + this._evaluateInternal( + flag, + context, + state, + [], + (res) => deliverResult(state, res, cb), + true, + eventFactory, + ); + }); } /** @@ -250,50 +313,55 @@ export default class Evaluator { return; } const updatedVisitedFlags = [...visitedFlags, prereq.key]; - this._queries.getFlag(prereq.key, (prereqFlag) => { - if (!prereqFlag) { - prereqResult = getOffVariation(flag, Reasons.prerequisiteFailed(prereq.key)); - iterCb(false); - return; - } + this._queries.getFlag(prereq.key, (prereqFlag) => + // The store may call back asynchronously, after the try/catch around the top-level + // evaluation has left the stack, so the continuation is guarded here. + guarded(state, () => { + if (!prereqFlag) { + prereqResult = getOffVariation(flag, Reasons.prerequisiteFailed(prereq.key)); + iterCb(false); + return; + } - this._evaluateInternal( - prereqFlag, - context, - state, - updatedVisitedFlags, - (res) => { - state.events ??= []; - if (topLevel) { - state.prerequisites ??= []; - - state.prerequisites.push(prereqFlag.key); - } - if (eventFactory) { - state.events.push( - eventFactory.evalEventServer(prereqFlag, context, res.detail, null, flag), - ); - } + this._evaluateInternal( + prereqFlag, + context, + state, + updatedVisitedFlags, + (res) => { + state.events ??= []; + if (topLevel) { + state.prerequisites ??= []; + + state.prerequisites.push(prereqFlag.key); + } + if (eventFactory) { + state.events.push( + eventFactory.evalEventServer(prereqFlag, context, res.detail, null, flag), + ); + } - if (res.isError) { - prereqResult = res; - return iterCb(false); - } + if (res.isError) { + prereqResult = res; + return iterCb(false); + } - if (res.isOff || res.detail.variationIndex !== prereq.variation) { - prereqResult = getOffVariation(flag, Reasons.prerequisiteFailed(prereq.key)); - return iterCb(false); - } - return iterCb(true); - }, - false, // topLevel false evaluating the prerequisite. - eventFactory, - ); - }); + if (res.isOff || res.detail.variationIndex !== prereq.variation) { + prereqResult = getOffVariation(flag, Reasons.prerequisiteFailed(prereq.key)); + return iterCb(false); + } + return iterCb(true); + }, + false, // topLevel false evaluating the prerequisite. + eventFactory, + ); + }), + ); }, () => { cb(prereqResult); }, + state.onError, ); } @@ -323,6 +391,7 @@ export default class Evaluator { }); }, () => cb(ruleResult), + state.onError, ); } @@ -338,30 +407,33 @@ export default class Evaluator { firstSeriesAsync( clause.values, (value, _index, iterCb) => { - this._queries.getSegment(value, (segment) => { - if (segment) { - if (segmentsVisited.includes(segment.key)) { - errorResult = EvalResult.forError( - ErrorKinds.MalformedFlag, - `Segment rule referencing segment ${segment.key} caused a circular reference. ` + - 'This is probably a temporary condition due to an incomplete update', - ); - // There was an error, so stop checking further segments. - iterCb(true); - return; - } - - const newVisited = [...segmentsVisited, segment?.key]; - this.segmentMatchContext(segment, context, state, newVisited, (res) => { - if (res.error) { - errorResult = res.result; + this._queries.getSegment(value, (segment) => + // The store may call back asynchronously; see _checkPrerequisites. + guarded(state, () => { + if (segment) { + if (segmentsVisited.includes(segment.key)) { + errorResult = EvalResult.forError( + ErrorKinds.MalformedFlag, + `Segment rule referencing segment ${segment.key} caused a circular reference. ` + + 'This is probably a temporary condition due to an incomplete update', + ); + // There was an error, so stop checking further segments. + iterCb(true); + return; } - iterCb(res.error || res.isMatch); - }); - } else { - iterCb(false); - } - }); + + const newVisited = [...segmentsVisited, segment?.key]; + this.segmentMatchContext(segment, context, state, newVisited, (res) => { + if (res.error) { + errorResult = res.result; + } + iterCb(res.error || res.isMatch); + }); + } else { + iterCb(false); + } + }), + ); }, (match) => { if (errorResult) { @@ -370,6 +442,7 @@ export default class Evaluator { return cb(makeMatch(maybeNegate(clause, match))); }, + state.onError, ); return; } @@ -429,6 +502,7 @@ export default class Evaluator { } return cb(undefined); }, + state.onError, ); } @@ -556,6 +630,7 @@ export default class Evaluator { return cb(makeMatch(false)); }, + state.onError, ); } @@ -590,6 +665,7 @@ export default class Evaluator { return cb(makeMatch(matched)); }, + state.onError, ); } @@ -637,32 +713,40 @@ export default class Evaluator { segment, context, state, - ).then(cb); + ) + .then(cb) + .catch(state.onError); return; } - this._queries.getBigSegmentsMembership(keyForBigSegment).then((result) => { - state.bigSegmentsMembership = state.bigSegmentsMembership || {}; - if (result) { - const [membership, status] = result; - state.bigSegmentsMembership[keyForBigSegment] = membership; - state.bigSegmentsStatus = computeUpdatedBigSegmentsStatus( - state.bigSegmentsStatus, - status as BigSegmentStoreStatusString, - ); - } else { - state.bigSegmentsStatus = computeUpdatedBigSegmentsStatus( - state.bigSegmentsStatus, - 'NOT_CONFIGURED', + this._queries + .getBigSegmentsMembership(keyForBigSegment) + .then((result) => { + state.bigSegmentsMembership = state.bigSegmentsMembership || {}; + if (result) { + const [membership, status] = result; + state.bigSegmentsMembership[keyForBigSegment] = membership; + state.bigSegmentsStatus = computeUpdatedBigSegmentsStatus( + state.bigSegmentsStatus, + status as BigSegmentStoreStatusString, + ); + } else { + state.bigSegmentsStatus = computeUpdatedBigSegmentsStatus( + state.bigSegmentsStatus, + 'NOT_CONFIGURED', + ); + } + return this.bigSegmentMatchContext( + state.bigSegmentsMembership[keyForBigSegment], + segment, + context, + state, ); - } - this.bigSegmentMatchContext( - state.bigSegmentsMembership[keyForBigSegment], - segment, - context, - state, - ).then(cb); - }); + }) + .then(cb) + // The rest of the evaluation runs inside these continuations, so an exception there would + // otherwise be an unhandled rejection and the result would never be delivered. + .catch(state.onError); } bigSegmentMatchContext( diff --git a/packages/shared/sdk-server/src/evaluation/collection.ts b/packages/shared/sdk-server/src/evaluation/collection.ts index 709acde953..cc20b3f699 100644 --- a/packages/shared/sdk-server/src/evaluation/collection.ts +++ b/packages/shared/sdk-server/src/evaluation/collection.ts @@ -18,6 +18,13 @@ export function firstResult( return res; } +/** + * Receives an exception thrown by a check or completion callback during asynchronous + * iteration. Iteration stops and the completion callback is not called, so the caller + * can report the failure instead of waiting for a result that would never arrive. + */ +export type IterationErrorHandler = (err: unknown) => void; + const ITERATION_RECURSION_LIMIT = 50; function seriesAsync( @@ -26,35 +33,51 @@ function seriesAsync( all: boolean, index: number, cb: (res: boolean) => void, + onError: IterationErrorHandler, ): void { - if (!collection) { - cb(false); - return; - } - if (index < collection?.length) { + try { + if (!collection) { + cb(false); + return; + } + if (!Array.isArray(collection)) { + // A string would be indexed character by character and an object would be treated as + // an empty collection, so a value that is not an array is reported instead of iterated. + throw new TypeError(`Expected an array but received ${typeof collection}`); + } + if (index >= collection.length) { + cb(all); + return; + } check(collection[index], index, (res) => { - if (all) { - if (!res) { - cb(false); + // The check may call back asynchronously, after the try/catch around this function has + // left the stack, so the continuation is guarded on its own. + try { + if (all) { + if (!res) { + cb(false); + return; + } + } else if (res) { + cb(true); return; } - } else if (res) { - cb(true); - return; - } - if (collection.length > ITERATION_RECURSION_LIMIT) { - // When we hit the recursion limit we defer execution - // by using a resolved promise. This is similar to using setImmediate - // but more portable. - Promise.resolve().then(() => { - seriesAsync(collection, check, all, index + 1, cb); - }); - } else { - seriesAsync(collection, check, all, index + 1, cb); + if (collection.length > ITERATION_RECURSION_LIMIT) { + // When we hit the recursion limit we defer execution + // by using a resolved promise. This is similar to using setImmediate + // but more portable. + Promise.resolve().then(() => { + seriesAsync(collection, check, all, index + 1, cb, onError); + }); + } else { + seriesAsync(collection, check, all, index + 1, cb, onError); + } + } catch (err) { + onError(err); } }); - } else { - cb(all); + } catch (err) { + onError(err); } } @@ -63,13 +86,15 @@ function seriesAsync( * @param collection The collection to iterate. * @param check The check to perform for each item in the container. * @param cb Called with true if all items pass the check. + * @param onError Called instead of cb if a check or cb throws. */ export function allSeriesAsync( collection: T[] | undefined, check: (val: T, index: number, cb: (res: boolean) => void) => void, cb: (res: boolean) => void, + onError: IterationErrorHandler, ): void { - seriesAsync(collection, check, true, 0, cb); + seriesAsync(collection, check, true, 0, cb, onError); } /** @@ -78,13 +103,15 @@ export function allSeriesAsync( * @param check The check to perform for each item in the container. * @param cb called with true on the first item that passes the check. False * means no items passed the check. + * @param onError Called instead of cb if a check or cb throws. */ export function firstSeriesAsync( collection: T[] | undefined, check: (val: T, index: number, cb: (res: boolean) => void) => void, cb: (res: boolean) => void, + onError: IterationErrorHandler, ): void { - seriesAsync(collection, check, false, 0, cb); + seriesAsync(collection, check, false, 0, cb, onError); } /** @@ -95,25 +122,35 @@ export function firstSeriesAsync( * @param cb Callback executed when all items have been checked. The callback * will be called with true if each item resulted in true, otherwise it will * be called with false. + * @param onError Called instead of cb if a check or cb throws. */ export function allAsync( collection: T[] | undefined, check: (val: T, cb: (res: boolean) => void) => void, cb: (res: boolean | null | undefined) => void, + onError: IterationErrorHandler, ): void { - if (!collection) { - cb(false); - return; - } + try { + if (!collection) { + cb(false); + return; + } - Promise.all( - collection?.map( - (item) => - new Promise((resolve) => { - check(item, resolve); - }), - ), - ).then((results) => { - cb(results.every((success) => success)); - }); + Promise.all( + collection.map( + (item) => + new Promise((resolve) => { + check(item, resolve); + }), + ), + ) + .then((results) => { + cb(results.every((success) => success)); + }) + // A throw inside a check rejects its promise, and a throw inside cb rejects the chain. + // Without this handler either one would be an unhandled rejection and cb would never run. + .catch(onError); + } catch (err) { + onError(err); + } } diff --git a/packages/shared/sdk-server/src/evaluation/variations.ts b/packages/shared/sdk-server/src/evaluation/variations.ts index 9095162308..842a3ef200 100644 --- a/packages/shared/sdk-server/src/evaluation/variations.ts +++ b/packages/shared/sdk-server/src/evaluation/variations.ts @@ -23,6 +23,11 @@ const KEY_ATTR_REF = new AttributeReference('key'); * @internal */ export function getVariation(flag: Flag, index: number, reason: LDEvaluationReason): EvalResult { + if (!Array.isArray(flag.variations)) { + // A flag from a custom store or a data file may have no variations, or a string in their + // place, which would otherwise be read as an array of characters. + return EvalResult.forError(ErrorKinds.MalformedFlag, 'Flag variations are not an array'); + } if (TypeValidators.Number.is(index) && index >= 0 && index < flag.variations.length) { return EvalResult.forSuccess(flag.variations[index], reason, index); }