diff --git a/src/feature-flags/evaluator.spec.ts b/src/feature-flags/evaluator.spec.ts index fe47a8ae7..eaac42b61 100644 --- a/src/feature-flags/evaluator.spec.ts +++ b/src/feature-flags/evaluator.spec.ts @@ -1,10 +1,15 @@ import { Evaluator } from './evaluator'; import { InMemoryStore } from './in-memory-store'; -import { FlagPollEntry } from './interfaces'; +import { + EvaluationContext, + FlagPollEntry, + RuntimeClientLogger, +} from './interfaces'; describe('Evaluator', () => { let store: InMemoryStore; let evaluator: Evaluator; + let logger: jest.Mocked; const enabledFlag: FlagPollEntry = { slug: 'enabled-flag', @@ -30,16 +35,40 @@ describe('Evaluator', () => { { id: 'user_456', enabled: true }, { id: 'user_blocked', enabled: false }, ], + custom_targets: [ + { type: 'workspace', id: 'ws_123', enabled: true }, + { type: 'workspace', id: 'ws_off', enabled: false }, + { type: 'region', id: 'us-east-1', enabled: true }, + ], + }, + }; + + // Simulates a default-on flag with a disabled override, which the API + // cannot produce yet: the row must not turn the flag off. + const defaultOnFlag: FlagPollEntry = { + slug: 'default-on-flag', + enabled: true, + default_value: true, + targets: { + users: [{ id: 'user_blocked', enabled: false }], + organizations: [], }, }; beforeEach(() => { store = new InMemoryStore(); - evaluator = new Evaluator(store); + logger = { + debug: jest.fn(), + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + }; + evaluator = new Evaluator(store, logger); store.swap({ 'enabled-flag': enabledFlag, 'disabled-flag': disabledFlag, 'targeted-flag': targetedFlag, + 'default-on-flag': defaultOnFlag, }); }); @@ -53,31 +82,36 @@ describe('Evaluator', () => { expect(evaluator.isEnabled('disabled-flag')).toBe(false); }); - it('returns target.enabled for matching organization', () => { + it('returns true for a matching enabled organization target', () => { expect( evaluator.isEnabled('targeted-flag', { organizationId: 'org_123' }), ).toBe(true); }); - it('returns target.enabled for matching user', () => { + it('returns true for a matching enabled user target', () => { expect(evaluator.isEnabled('targeted-flag', { userId: 'user_456' })).toBe( true, ); }); - it('returns false for user target with enabled=false', () => { + it('treats targets with enabled=false as not present', () => { + // No enabled match, so the flag falls back to its default value — + // false here, but crucially the target does not force the flag off. expect( evaluator.isEnabled('targeted-flag', { userId: 'user_blocked' }), ).toBe(false); + expect( + evaluator.isEnabled('default-on-flag', { userId: 'user_blocked' }), + ).toBe(true); }); - it('prioritizes user target over organization target', () => { + it('matches any enabled target with no precedence between types', () => { expect( evaluator.isEnabled('targeted-flag', { userId: 'user_blocked', organizationId: 'org_123', }), - ).toBe(false); + ).toBe(true); }); it('falls back to organization target when user target does not match', () => { @@ -100,6 +134,109 @@ describe('Evaluator', () => { }); }); + describe('typed evaluation contexts', () => { + it('matches custom targets by exact type and id', () => { + expect( + evaluator.isEnabled('targeted-flag', { workspace: { id: 'ws_123' } }), + ).toBe(true); + expect( + evaluator.isEnabled('targeted-flag', { region: { id: 'us-east-1' } }), + ).toBe(true); + + // A missing or mistyped target is a valid empty match, not an error. + expect( + evaluator.isEnabled('targeted-flag', { workspace: { id: 'ws_456' } }), + ).toBe(false); + expect(logger.warn).not.toHaveBeenCalled(); + }); + + it('accepts built-in types in the typed form', () => { + expect( + evaluator.isEnabled('targeted-flag', { user: { id: 'user_456' } }), + ).toBe(true); + expect( + evaluator.isEnabled('targeted-flag', { + organization: { id: 'org_123' }, + }), + ).toBe(true); + }); + + it('treats custom targets with enabled=false as not present', () => { + expect( + evaluator.isEnabled('targeted-flag', { workspace: { id: 'ws_off' } }), + ).toBe(false); + }); + + it('evaluates safely when the payload has no custom_targets field', () => { + expect( + evaluator.isEnabled('enabled-flag', { workspace: { id: 'ws_123' } }), + ).toBe(true); + expect( + evaluator.isEnabled('default-on-flag', { + workspace: { id: 'ws_123' }, + }), + ).toBe(true); + }); + + it('rejects a context mixing legacy and typed keys', () => { + const hybridContext: EvaluationContext = { + userId: 'user_456', + workspace: { id: 'ws_123' }, + }; + + expect(evaluator.isEnabled('targeted-flag', hybridContext)).toBe(false); + expect(logger.warn).toHaveBeenCalledTimes(1); + }); + + it('does not treat unset keys as part of the context shape', () => { + expect( + evaluator.isEnabled('targeted-flag', { + userId: undefined, + workspace: { id: 'ws_123' }, + }), + ).toBe(true); + expect(logger.warn).not.toHaveBeenCalled(); + }); + + it('ignores scalar extra fields on a legacy context', () => { + const legacyWithExtras = { + userId: 'user_456', + requestId: 'req_1', + } as EvaluationContext; + + expect(evaluator.isEnabled('targeted-flag', legacyWithExtras)).toBe(true); + expect(logger.warn).not.toHaveBeenCalled(); + }); + + it('ignores invalid target type keys with a warning', () => { + expect( + evaluator.isEnabled('targeted-flag', { Workspace: { id: 'ws_123' } }), + ).toBe(false); + expect(logger.warn).toHaveBeenCalledTimes(1); + }); + + it('ignores typed entries with invalid ids with a warning', () => { + expect( + evaluator.isEnabled('targeted-flag', { workspace: { id: '..' } }), + ).toBe(false); + expect( + evaluator.isEnabled('targeted-flag', { workspace: { id: 'ws 123' } }), + ).toBe(false); + expect(logger.warn).toHaveBeenCalledTimes(2); + }); + + it('never throws on malformed context values', () => { + const malformedContext = { + workspace: 'ws_123', + } as unknown as EvaluationContext; + + expect(evaluator.isEnabled('targeted-flag', malformedContext)).toBe( + false, + ); + expect(logger.warn).toHaveBeenCalledTimes(1); + }); + }); + describe('getAllFlags', () => { it('evaluates all flags for the given context', () => { const result = evaluator.getAllFlags({ userId: 'user_456' }); @@ -108,9 +245,27 @@ describe('Evaluator', () => { 'enabled-flag': true, 'disabled-flag': false, 'targeted-flag': true, + 'default-on-flag': true, }); }); + it('evaluates all flags for a typed context', () => { + const result = evaluator.getAllFlags({ workspace: { id: 'ws_123' } }); + + expect(result).toEqual({ + 'enabled-flag': true, + 'disabled-flag': false, + 'targeted-flag': true, + 'default-on-flag': true, + }); + }); + + it('warns once per call for an invalid context, not once per flag', () => { + evaluator.getAllFlags({ Workspace: { id: 'ws_123' } }); + + expect(logger.warn).toHaveBeenCalledTimes(1); + }); + it('works with empty context', () => { const result = evaluator.getAllFlags(); @@ -118,6 +273,7 @@ describe('Evaluator', () => { 'enabled-flag': true, 'disabled-flag': false, 'targeted-flag': false, + 'default-on-flag': true, }); }); }); diff --git a/src/feature-flags/evaluator.ts b/src/feature-flags/evaluator.ts index 0a91684a2..1089293a7 100644 --- a/src/feature-flags/evaluator.ts +++ b/src/feature-flags/evaluator.ts @@ -1,16 +1,63 @@ import { InMemoryStore } from './in-memory-store'; -import { EvaluationContext } from './interfaces'; +import { + EvaluationContext, + EvaluationResource, + FlagPollEntry, + RuntimeClientLogger, +} from './interfaces'; + +// Mirror of the API's validation rules for custom target types and IDs. +const TARGET_TYPE_PATTERN = /^[a-z][a-z0-9_-]{0,63}$/; +const TARGET_ID_PATTERN = /^[A-Za-z0-9._:-]{1,255}$/; + +const LEGACY_KEY_TO_TARGET_TYPE = new Map([ + ['userId', 'user'], + ['organizationId', 'organization'], +]); + +const isEvaluationResource = (value: unknown): value is EvaluationResource => + typeof value === 'object' && + value !== null && + 'id' in value && + typeof value.id === 'string'; export class Evaluator { - constructor(private readonly store: InMemoryStore) {} + constructor( + private readonly store: InMemoryStore, + private readonly logger?: RuntimeClientLogger, + ) {} isEnabled( flagKey: string, context: EvaluationContext = {}, defaultValue: boolean = false, ): boolean { - const entry = this.store.get(flagKey); + return this.evaluate( + this.store.get(flagKey), + this.normalizeContext(context), + defaultValue, + ); + } + + getAllFlags(context: EvaluationContext = {}): Record { + // Normalized once so an invalid context warns once per call, not once + // per flag. + const normalizedContext = this.normalizeContext(context); + const flags = this.store.getAll(); + const result: Record = {}; + + for (const slug of Object.keys(flags)) { + result[slug] = this.evaluate(flags[slug], normalizedContext, false); + } + + return result; + } + private evaluate( + entry: FlagPollEntry | undefined, + normalizedContext: Map, + defaultValue: boolean, + ): boolean { if (!entry) { return defaultValue; } @@ -19,35 +66,124 @@ export class Evaluator { return false; } - if (context.userId) { - const userTarget = entry.targets.users.find( - (t) => t.id === context.userId, - ); - if (userTarget) { - return userTarget.enabled; + // Evaluation is enable-only: any enabled target matching the context + // turns the flag on, with no precedence between target types. + for (const [targetType, targetId] of normalizedContext) { + if (this.hasEnabledTarget(entry, targetType, targetId)) { + return true; } } - if (context.organizationId) { - const orgTarget = entry.targets.organizations.find( - (t) => t.id === context.organizationId, + return entry.default_value; + } + + /** + * Reduces either context form to target type → ID pairs. Evaluation must + * never throw in application code, so every invalid piece of context + * degrades to "matches no targets" with a logged warning instead of an + * error. + */ + private normalizeContext(context: EvaluationContext): Map { + const normalized = new Map(); + const record: Record = context; + + const legacyEntries: Array<[string, string]> = []; + const typedKeys: string[] = []; + + for (const [key, value] of Object.entries(record)) { + // Unset values never influence which shape the context is in, so + // optional spreading (`userId: maybeId`) stays safe. + if (value === undefined || value === null) { + continue; + } + + const legacyTargetType = LEGACY_KEY_TO_TARGET_TYPE.get(key); + if (legacyTargetType) { + if (typeof value === 'string' && value !== '') { + legacyEntries.push([legacyTargetType, value]); + } + continue; + } + + typedKeys.push(key); + } + + // The legacy and typed shapes cannot be mixed: inventing a precedence + // between them would guess at caller intent, so a genuinely hybrid + // context matches no targets at all. Only resource-shaped values signal + // typed intent here — a scalar extra field on a legacy context is + // ignored, as it always has been. + const resourceShapedKeys = typedKeys.filter( + (key) => typeof record[key] === 'object', + ); + + if (legacyEntries.length > 0 && resourceShapedKeys.length > 0) { + this.logger?.warn( + 'Evaluation context mixes legacy keys (userId/organizationId) with typed target keys; no targets will match', + { keys: Object.keys(record) }, ); - if (orgTarget) { - return orgTarget.enabled; + return normalized; + } + + if (legacyEntries.length > 0) { + for (const [targetType, targetId] of legacyEntries) { + normalized.set(targetType, targetId); } + return normalized; } - return entry.default_value; + for (const key of typedKeys) { + if (!TARGET_TYPE_PATTERN.test(key)) { + this.logger?.warn( + `Ignoring invalid target type in evaluation context: ${key}`, + ); + continue; + } + + const value = record[key]; + if (!isEvaluationResource(value)) { + this.logger?.warn( + `Ignoring target type with a missing or invalid resource id in evaluation context: ${key}`, + ); + continue; + } + + const { id } = value; + if (!TARGET_ID_PATTERN.test(id) || id === '.' || id === '..') { + this.logger?.warn( + `Ignoring invalid target id in evaluation context for type: ${key}`, + ); + continue; + } + + normalized.set(key, id); + } + + return normalized; } - getAllFlags(context: EvaluationContext = {}): Record { - const flags = this.store.getAll(); - const result: Record = {}; + /** + * A target participates in evaluation only while its `enabled` is true. A + * `false` value is reserved for future disabled overrides and is treated + * as if the target were absent. + */ + private hasEnabledTarget( + entry: FlagPollEntry, + targetType: string, + targetId: string, + ): boolean { + if (targetType === 'user') { + return entry.targets.users.some((t) => t.id === targetId && t.enabled); + } - for (const slug of Object.keys(flags)) { - result[slug] = this.isEnabled(slug, context); + if (targetType === 'organization') { + return entry.targets.organizations.some( + (t) => t.id === targetId && t.enabled, + ); } - return result; + return (entry.targets.custom_targets ?? []).some( + (t) => t.type === targetType && t.id === targetId && t.enabled, + ); } } diff --git a/src/feature-flags/interfaces/evaluation-context.interface.ts b/src/feature-flags/interfaces/evaluation-context.interface.ts index 29fd5ec4a..3c0a1c38a 100644 --- a/src/feature-flags/interfaces/evaluation-context.interface.ts +++ b/src/feature-flags/interfaces/evaluation-context.interface.ts @@ -1,4 +1,36 @@ -export interface EvaluationContext { +/** + * A single resource in a typed evaluation context. V1 carries only the exact + * resource ID; attribute matching is a future capability layered onto this + * same shape. + */ +export interface EvaluationResource { + id: string; +} + +/** + * Legacy evaluation context, accepted for backward compatibility and + * normalized internally to the typed form: `userId` matches `user` targets + * and `organizationId` matches `organization` targets. + */ +export type LegacyEvaluationContext = { userId?: string; organizationId?: string; -} +}; + +/** + * Typed evaluation context: a direct map of target type slug to the resource + * being evaluated, e.g. + * `{ user: { id: 'user_123' }, workspace: { id: 'ws_1' } }`. + * A context contains at most one resource of each type; callers needing a + * decision per resource should evaluate once per resource. + */ +export type TypedEvaluationContext = Record; + +/** + * Either evaluation context form. The two shapes cannot be mixed in a single + * call: a hybrid context (a legacy key alongside a typed resource entry) is + * rejected at evaluation time with a logged warning and matches no targets, + * so the flag falls back to its default value. + */ +export type EvaluationContext = + LegacyEvaluationContext | TypedEvaluationContext; diff --git a/src/feature-flags/interfaces/flag-poll-response.interface.ts b/src/feature-flags/interfaces/flag-poll-response.interface.ts index 3db6832ea..762f53ed4 100644 --- a/src/feature-flags/interfaces/flag-poll-response.interface.ts +++ b/src/feature-flags/interfaces/flag-poll-response.interface.ts @@ -3,6 +3,12 @@ export interface FlagTarget { enabled: boolean; } +export interface FlagCustomTarget { + type: string; + id: string; + enabled: boolean; +} + export interface FlagPollEntry { slug: string; enabled: boolean; @@ -10,6 +16,8 @@ export interface FlagPollEntry { targets: { users: FlagTarget[]; organizations: FlagTarget[]; + /** Absent until the API's custom-targets rollout flag is enabled. */ + custom_targets?: FlagCustomTarget[]; }; } diff --git a/src/feature-flags/runtime-client.spec.ts b/src/feature-flags/runtime-client.spec.ts index 2ecd7f1af..249112565 100644 --- a/src/feature-flags/runtime-client.spec.ts +++ b/src/feature-flags/runtime-client.spec.ts @@ -147,6 +147,9 @@ describe('FeatureFlagsRuntimeClient', () => { expect(client.isEnabled('flag-a')).toBe(true); expect(client.isEnabled('flag-b')).toBe(false); expect(client.isEnabled('flag-b', { userId: 'user_123' })).toBe(true); + expect(client.isEnabled('flag-b', { user: { id: 'user_123' } })).toBe( + true, + ); expect(client.isEnabled('unknown')).toBe(false); expect(client.isEnabled('unknown', {}, true)).toBe(true); @@ -271,6 +274,41 @@ describe('FeatureFlagsRuntimeClient', () => { client.close(); }); + it('emits change when only custom targets change', async () => { + const client = createClientAndWait(); + await jest.advanceTimersByTimeAsync(0); + await client.waitUntilReady(); + + const changes: unknown[] = []; + client.on('change', (change) => changes.push(change)); + + const updatedResponse: FlagPollResponse = { + 'flag-a': pollResponse['flag-a'], + 'flag-b': { + ...pollResponse['flag-b'], + targets: { + ...pollResponse['flag-b'].targets, + custom_targets: [ + { type: 'workspace', id: 'ws_123', enabled: true }, + ], + }, + }, + }; + + fetchOnce(updatedResponse); + await jest.advanceTimersByTimeAsync(35_000); + + expect(changes).toEqual([ + { + key: 'flag-b', + previous: pollResponse['flag-b'], + current: updatedResponse['flag-b'], + }, + ]); + + client.close(); + }); + it('emits change when a flag is removed', async () => { const client = createClientAndWait(); await jest.advanceTimersByTimeAsync(0); diff --git a/src/feature-flags/runtime-client.ts b/src/feature-flags/runtime-client.ts index 170950d6f..e13841597 100644 --- a/src/feature-flags/runtime-client.ts +++ b/src/feature-flags/runtime-client.ts @@ -6,6 +6,7 @@ import { Evaluator } from './evaluator'; import { EvaluationContext, FlagChange, + FlagCustomTarget, FlagPollEntry, FlagPollResponse, FlagTarget, @@ -70,7 +71,7 @@ export class FeatureFlagsRuntimeClient extends EventEmitter this.logger = options.logger; this.store = new InMemoryStore(); - this.evaluator = new Evaluator(this.store); + this.evaluator = new Evaluator(this.store, this.logger); this.readyPromise = new Promise((resolve, reject) => { this.readyResolve = resolve; @@ -305,9 +306,24 @@ export class FeatureFlagsRuntimeClient extends EventEmitter return xs.some((t) => map.get(t.id) !== t.enabled); }; + // Type slugs cannot contain ':', so the first ':' unambiguously ends the + // type in this composite key even though target IDs may contain ':'. + const customTargetsChanged = ( + xs: FlagCustomTarget[], + ys: FlagCustomTarget[], + ): boolean => { + if (xs.length !== ys.length) return true; + const map = new Map(ys.map((t) => [`${t.type}:${t.id}`, t.enabled])); + return xs.some((t) => map.get(`${t.type}:${t.id}`) !== t.enabled); + }; + return ( targetsChanged(a.targets.users, b.targets.users) || - targetsChanged(a.targets.organizations, b.targets.organizations) + targetsChanged(a.targets.organizations, b.targets.organizations) || + customTargetsChanged( + a.targets.custom_targets ?? [], + b.targets.custom_targets ?? [], + ) ); } }