diff --git a/.gitmodules b/.gitmodules index f9b9019..93de146 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,4 +1,4 @@ [submodule "tests/engine/engine-tests/engine-test-data"] path = tests/engine/engine-tests/engine-test-data url = git@github.com:Flagsmith/engine-test-data.git - branch = v2.5.0 + branch = v3.1.0 diff --git a/flagsmith-engine/evaluation/evaluationContext/evaluationContext.types.ts b/flagsmith-engine/evaluation/evaluationContext/evaluationContext.types.ts index d1ec209..8b6e0c3 100644 --- a/flagsmith-engine/evaluation/evaluationContext/evaluationContext.types.ts +++ b/flagsmith-engine/evaluation/evaluationContext/evaluationContext.types.ts @@ -6,7 +6,7 @@ */ /** - * An environment's unique identifier. + * Unique environment key. May be used for selecting a value for a multivariate feature, or for % split segmentation. */ export type Key = string; /** @@ -14,7 +14,7 @@ export type Key = string; */ export type Name = string; /** - * A unique identifier for an identity, used for segment and multivariate feature flag targeting, and displayed in the Flagsmith UI. + * A unique identifier for an identity as displayed in the Flagsmith UI. */ export type Identifier = string; /** @@ -22,7 +22,7 @@ export type Identifier = string; */ export type Key1 = string; /** - * Key used for % split segmentation. + * Unique segment key used for % split segmentation. */ export type Key2 = string; /** @@ -85,13 +85,9 @@ export type SubRules = SegmentRule[]; */ export type Rules = SegmentRule[]; /** - * Key used when selecting a value for a multivariate feature. Set to an internal identifier or a UUID, depending on Flagsmith implementation. + * Unique feature key used when selecting a variant if the feature is multivariate. Set to an internal identifier or a UUID, depending on Flagsmith implementation. */ export type Key3 = string; -/** - * Unique feature identifier. - */ -export type FeatureKey = string; /** * Feature name. */ @@ -155,7 +151,7 @@ export interface EnvironmentContext { */ export interface IdentityContext { identifier: Identifier; - key: Key1; + key?: Key1; traits?: Traits; [k: string]: unknown; } @@ -214,7 +210,6 @@ export interface InSegmentCondition { */ export interface FeatureContext { key: Key3; - feature_key: FeatureKey; name: Name2; enabled: Enabled; value: Value2; diff --git a/flagsmith-engine/evaluation/evaluationContext/mappers.ts b/flagsmith-engine/evaluation/evaluationContext/mappers.ts index dede412..4593f60 100644 --- a/flagsmith-engine/evaluation/evaluationContext/mappers.ts +++ b/flagsmith-engine/evaluation/evaluationContext/mappers.ts @@ -53,7 +53,6 @@ function mapEnvironmentModelToEvaluationContext( features[fs.feature.name] = { key: fs.djangoID?.toString() || fs.featurestateUUID, - feature_key: fs.feature.id.toString(), name: fs.feature.name, enabled: fs.enabled, value: fs.getValue(), @@ -75,16 +74,18 @@ function mapEnvironmentModelToEvaluationContext( segment.featureStates.length > 0 ? segment.featureStates.map(fs => ({ key: fs.djangoID?.toString() || fs.featurestateUUID, - feature_key: fs.feature.id.toString(), name: fs.feature.name, enabled: fs.enabled, value: fs.getValue(), - priority: fs.featureSegment?.priority + priority: fs.featureSegment?.priority, + metadata: { + flagsmithId: fs.feature.id + } })) : [], metadata: { source: SegmentSource.API, - flagsmith_id: segment.id + flagsmithId: segment.id } }; } @@ -115,11 +116,16 @@ function mapIdentityModelToIdentityContext( traitsContext[trait.traitKey] = trait.traitValue; } - return { + const identityContext: IdentityContext = { identifier: identity.identifier, - key: identity.djangoID?.toString() || identity.compositeKey, traits: traitsContext }; + + if (identity.djangoID !== undefined) { + identityContext.key = identity.djangoID.toString(); + } + + return identityContext; } function mapSegmentRuleModelToRule(rule: any): any { @@ -147,7 +153,6 @@ function mapIdentityOverridesToSegments(identityOverrides: IdentityModel[]): Seg a.feature.name.localeCompare(b.feature.name) ); const overridesKey = sortedFeatures.map(fs => ({ - feature_key: fs.feature.id.toString(), name: fs.feature.name, enabled: fs.enabled, value: fs.getValue(), diff --git a/flagsmith-engine/evaluation/evaluationResult/evaluationResult.types.ts b/flagsmith-engine/evaluation/evaluationResult/evaluationResult.types.ts index 390146a..5f3920b 100644 --- a/flagsmith-engine/evaluation/evaluationResult/evaluationResult.types.ts +++ b/flagsmith-engine/evaluation/evaluationResult/evaluationResult.types.ts @@ -5,10 +5,6 @@ * and run json-schema-to-typescript to regenerate this file. */ -/** - * Unique feature identifier. - */ -export type FeatureKey = string; /** * Feature name. */ @@ -25,10 +21,6 @@ export type Value = string | number | boolean | null; * Reason for the feature flag evaluation. */ export type Reason = string; -/** - * Unique segment identifier. - */ -export type Key = string; /** * Segment name. */ @@ -53,7 +45,6 @@ export interface Flags { [k: string]: FlagResult; } export interface FlagResult { - feature_key: FeatureKey; name: Name; enabled: Enabled; value: Value; @@ -68,7 +59,6 @@ export interface FeatureMetadata { [k: string]: unknown; } export interface SegmentResult { - key: Key; name: Name1; metadata?: SegmentMetadata; [k: string]: unknown; diff --git a/flagsmith-engine/evaluation/models.ts b/flagsmith-engine/evaluation/models.ts index 71ea56b..9b8cc06 100644 --- a/flagsmith-engine/evaluation/models.ts +++ b/flagsmith-engine/evaluation/models.ts @@ -5,18 +5,27 @@ import type { EvaluationResult as EvaluationContextResult, FlagResult, - FeatureMetadata + FeatureMetadata, + SegmentMetadata } from './evaluationResult/evaluationResult.types.js'; import type { FeatureContext, EnvironmentContext, IdentityContext, - Segments + SegmentContext } from './evaluationContext/evaluationContext.types.js'; +export * from './evaluationContext/evaluationContext.types.js'; + +export enum SegmentSource { + API = 'api', + IDENTITY_OVERRIDE = 'identity_override' +} + +// Feature types export interface CustomFeatureMetadata extends FeatureMetadata { - flagsmithId?: number; + flagsmithId: number; } export interface FeatureContextWithMetadata @@ -29,14 +38,6 @@ export type FeaturesWithMetadata = [k: string]: FeatureContextWithMetadata; }; -export interface GenericEvaluationContext { - environment: EnvironmentContext; - identity?: IdentityContext | null; - segments?: Segments; - features?: FeaturesWithMetadata; - [k: string]: unknown; -} - export type FlagResultWithMetadata = FlagResult & { metadata: T; }; @@ -46,19 +47,50 @@ export type EvaluationResultFlags = FlagResultWithMetadata >; +// Segment types +export interface CustomSegmentMetadata extends SegmentMetadata { + flagsmithId: number; + source?: SegmentSource; +} + +export interface SegmentContextWithMetadata + extends SegmentContext { + metadata: T; + overrides?: FeatureContextWithMetadata[]; +} + +export type SegmentsWithMetadata = { + [k: string]: SegmentContextWithMetadata; +}; + +export interface SegmentResultWithMetadata { + name: string; + metadata: CustomSegmentMetadata; +} + export type EvaluationResultSegments = EvaluationContextResult['segments']; +// Evaluation context types +export interface GenericEvaluationContext< + T extends FeatureMetadata = FeatureMetadata, + S extends SegmentMetadata = SegmentMetadata +> { + environment: EnvironmentContext; + identity?: IdentityContext | null; + segments?: SegmentsWithMetadata; + features?: FeaturesWithMetadata; + [k: string]: unknown; +} + +export type EvaluationContextWithMetadata = GenericEvaluationContext< + CustomFeatureMetadata, + CustomSegmentMetadata +>; + +// Evaluation result types export type EvaluationResult = { flags: EvaluationResultFlags; segments: EvaluationResultSegments; }; export type EvaluationResultWithMetadata = EvaluationResult; -export type EvaluationContextWithMetadata = GenericEvaluationContext; - -export enum SegmentSource { - API = 'api', - IDENTITY_OVERRIDE = 'identity_override' -} - -export * from './evaluationContext/evaluationContext.types.js'; diff --git a/flagsmith-engine/index.ts b/flagsmith-engine/index.ts index 534b9e1..c57abd0 100644 --- a/flagsmith-engine/index.ts +++ b/flagsmith-engine/index.ts @@ -6,7 +6,7 @@ import { CustomFeatureMetadata, FlagResultWithMetadata } from './evaluation/models.js'; -import { getIdentitySegments } from './segments/evaluators.js'; +import { getIdentitySegments, getIdentityKey } from './segments/evaluators.js'; import { EvaluationResultFlags } from './evaluation/models.js'; import { TARGETING_REASONS } from './features/types.js'; import { getHashedPercentageForObjIds } from './utils/hashing/index.js'; @@ -62,7 +62,6 @@ export function evaluateSegments(context: EvaluationContextWithMetadata): { const identitySegments = getIdentitySegments(context); const segments = identitySegments.map(segment => ({ - key: segment.key, name: segment.name, ...(segment.metadata ? { @@ -96,7 +95,7 @@ export function processSegmentOverrides(identitySegments: any[]): Record = {}; for (const feature of Object.values(context.features || {})) { - const segmentOverride = segmentOverrides[feature.feature_key]; + const segmentOverride = segmentOverrides[feature.name]; const finalFeature = segmentOverride ? segmentOverride.feature : feature; const hasOverride = !!segmentOverride; const { value: evaluatedValue, reason: evaluatedReason } = hasOverride ? { value: finalFeature.value, reason: undefined } - : evaluateFeatureValue(finalFeature, context.identity?.key); + : evaluateFeatureValue(finalFeature, getIdentityKey(context)); flags[finalFeature.name] = { - feature_key: finalFeature.feature_key, name: finalFeature.name, enabled: finalFeature.enabled, value: evaluatedValue, @@ -198,7 +196,7 @@ export function shouldApplyOverride( override: any, existingOverrides: Record ): boolean { - const currentOverride = existingOverrides[override.feature_key]; + const currentOverride = existingOverrides[override.name]; return ( !currentOverride || isHigherPriority(override.priority, currentOverride.feature.priority) ); diff --git a/flagsmith-engine/segments/evaluators.ts b/flagsmith-engine/segments/evaluators.ts index 0b08526..aecae7f 100644 --- a/flagsmith-engine/segments/evaluators.ts +++ b/flagsmith-engine/segments/evaluators.ts @@ -49,7 +49,7 @@ export function traitsMatchSegmentCondition( ): boolean { if (condition.operator === PERCENTAGE_SPLIT) { const contextValueKey = - getContextValue(condition.property, context) || context?.identity?.key; + getContextValue(condition.property, context) || getIdentityKey(context); const hashedPercentage = getHashedPercentageForObjIds([segmentKey, contextValueKey]); return hashedPercentage <= parseFloat(String(condition.value)); } @@ -173,3 +173,9 @@ export function getContextValue(jsonPath: string, context?: GenericEvaluationCon return undefined; } } + +export function getIdentityKey(context?: GenericEvaluationContext): string | undefined { + if (!context?.identity) return undefined; + + return context.identity.key || `${context.environment.key}_${context.identity.identifier}`; +} diff --git a/flagsmith-engine/segments/models.ts b/flagsmith-engine/segments/models.ts index b912867..8f3fb4b 100644 --- a/flagsmith-engine/segments/models.ts +++ b/flagsmith-engine/segments/models.ts @@ -144,6 +144,9 @@ export class SegmentConditionModel { return parsedTraitValue % divisor === remainder; }, evaluateIn: (traitValue: string[] | string) => { + if (!traitValue || typeof traitValue === 'boolean') { + return false; + } if (Array.isArray(this.value)) { return this.value.includes(traitValue.toString()); } @@ -224,9 +227,13 @@ export class SegmentModel { if (segmentResult.metadata?.source === SegmentSource.IDENTITY_OVERRIDE) { continue; } - const segmentContext = evaluationContext.segments[segmentResult.key]; + const flagsmithId = segmentResult.metadata?.flagsmithId; + if (!flagsmithId) { + continue; + } + const segmentContext = evaluationContext.segments[flagsmithId.toString()]; if (segmentContext) { - const segment = new SegmentModel(parseInt(segmentContext.key), segmentContext.name); + const segment = new SegmentModel(flagsmithId, segmentContext.name); segment.rules = segmentContext.rules.map(rule => new SegmentRuleModel(rule.type)); segment.featureStates = SegmentModel.createFeatureStatesFromOverrides( segmentContext.overrides || [] @@ -240,33 +247,39 @@ export class SegmentModel { private static createFeatureStatesFromOverrides(overrides: Overrides): FeatureStateModel[] { if (!overrides) return []; - return overrides.map(override => { - const feature = new FeatureModel( - parseInt(override.feature_key), - override.name, - override.variants?.length && override.variants.length > 0 - ? CONSTANTS.MULTIVARIATE - : CONSTANTS.STANDARD - ); - - const featureState = new FeatureStateModel( - feature, - override.enabled, - override.priority || 0 - ); - - if (override.value !== undefined) { - featureState.setValue(override.value); - } + return overrides + .filter(override => { + const flagsmithId = override?.metadata?.flagsmithId; + return typeof flagsmithId === 'number'; + }) + .map(override => { + const flagsmithId = override.metadata!.flagsmithId as number; + const feature = new FeatureModel( + flagsmithId, + override.name, + override.variants?.length && override.variants.length > 0 + ? CONSTANTS.MULTIVARIATE + : CONSTANTS.STANDARD + ); - if (override.variants && override.variants.length > 0) { - featureState.multivariateFeatureStateValues = this.createMultivariateValues( - override.variants + const featureState = new FeatureStateModel( + feature, + override.enabled, + override.priority || 0 ); - } - return featureState; - }); + if (override.value !== undefined) { + featureState.setValue(override.value); + } + + if (override.variants && override.variants.length > 0) { + featureState.multivariateFeatureStateValues = this.createMultivariateValues( + override.variants + ); + } + + return featureState; + }); } private static createMultivariateValues(variants: any[]): MultivariateFeatureStateValueModel[] { diff --git a/sdk/models.ts b/sdk/models.ts index f6c48a1..17b0b22 100644 --- a/sdk/models.ts +++ b/sdk/models.ts @@ -120,7 +120,10 @@ export class Flags { for (const flag of Object.values(evaluationResult.flags)) { const flagsmithId = flag.metadata?.flagsmithId; if (!flagsmithId) { - continue; + throw new Error( + `FlagResult metadata.flagsmithId is missing for feature "${flag.name}". ` + + `This indicates a bug in the SDK, please report it.` + ); } flags[flag.name] = new Flag({ diff --git a/tests/engine/engine-tests/engine-test-data b/tests/engine/engine-tests/engine-test-data index 41c2021..6ab57ec 160000 --- a/tests/engine/engine-tests/engine-test-data +++ b/tests/engine/engine-tests/engine-test-data @@ -1 +1 @@ -Subproject commit 41c202145e375c712600e318c439456de5b221d7 +Subproject commit 6ab57ec67bc84659e8b5aa41534b04fe45cc4cbe diff --git a/tests/engine/unit/engine.test.ts b/tests/engine/unit/engine.test.ts index 8eb7d5d..c1a60a3 100644 --- a/tests/engine/unit/engine.test.ts +++ b/tests/engine/unit/engine.test.ts @@ -30,7 +30,6 @@ test('test_get_evaluation_result_without_any_override', () => { const flag = Object.values(result.flags).find(f => f.name === feature1().name); expect(flag).toBeDefined(); expect(flag?.name).toBe(feature1().name); - expect(flag?.feature_key).toBe(feature1().id.toString()); expect(flag?.reason).toBe(TARGETING_REASONS.DEFAULT); }); @@ -112,22 +111,18 @@ test('shouldApplyOverride with priority conflicts', () => { feature1: { feature: { key: 'key', - feature_key: 'feature1', - name: 'name', + name: 'feature1', enabled: true, value: 'value', - priority: 5 + priority: 5, + metadata: { flagsmithId: 1 } }, segmentName: 'segment1' } }; - expect(shouldApplyOverride({ feature_key: 'feature1', priority: 2 }, existingOverrides)).toBe( - true - ); - expect(shouldApplyOverride({ feature_key: 'feature1', priority: 10 }, existingOverrides)).toBe( - false - ); + expect(shouldApplyOverride({ name: 'feature1', priority: 2 }, existingOverrides)).toBe(true); + expect(shouldApplyOverride({ name: 'feature1', priority: 10 }, existingOverrides)).toBe(false); }); test('evaluateSegments handles segments with identity identifier matching', () => { @@ -176,7 +171,6 @@ test('evaluateSegments handles segments with identity identifier matching', () = overrides: [ { key: 'override1', - feature_key: 'feature1', name: 'feature1', enabled: true, value: 'overridden_value', @@ -188,21 +182,21 @@ test('evaluateSegments handles segments with identity identifier matching', () = features: { feature1: { key: 'fs1', - feature_key: 'feature1', name: 'feature1', enabled: false, - value: 'default_value' + value: 'default_value', + metadata: { flagsmithId: 1 } } } }; - const result = evaluateSegments(context); + const result = evaluateSegments(context as any); expect(result.segments).toHaveLength(2); expect(result.segments).toEqual( expect.arrayContaining([ - { key: '1', name: 'segment_with_no_overrides' }, - { key: '2', name: 'segment_with_overrides' } + { name: 'segment_with_no_overrides' }, + { name: 'segment_with_overrides' } ]) ); @@ -239,7 +233,6 @@ test('evaluateSegments handles priority conflicts correctly', () => { overrides: [ { key: 'override1', - feature_key: 'feature1', name: 'feature1', enabled: true, value: 'low_priority_value', @@ -265,7 +258,6 @@ test('evaluateSegments handles priority conflicts correctly', () => { overrides: [ { key: 'override2', - feature_key: 'feature1', name: 'feature1', enabled: false, value: 'high_priority_value', @@ -277,15 +269,15 @@ test('evaluateSegments handles priority conflicts correctly', () => { features: { feature1: { key: 'fs1', - feature_key: 'feature1', name: 'feature1', enabled: false, - value: 'default_value' + value: 'default_value', + metadata: { flagsmithId: 1 } } } }; - const result = evaluateSegments(context); + const result = evaluateSegments(context as any); expect(result.segments).toHaveLength(2); @@ -323,7 +315,6 @@ test('evaluateSegments with non-matching identity returns empty', () => { overrides: [ { key: 'override1', - feature_key: 'feature1', name: 'feature1', enabled: true, value: 'overridden_value' @@ -334,7 +325,7 @@ test('evaluateSegments with non-matching identity returns empty', () => { features: {} }; - const result = evaluateSegments(context); + const result = evaluateSegments(context as any); expect(result.segments).toEqual([]); expect(result.segmentOverrides).toEqual({}); @@ -345,14 +336,14 @@ test('evaluateFeatures with multivariate evaluation', () => { features: { mv_feature: { key: 'mv', - feature_key: 'mv_feature', name: 'Multivariate Feature', enabled: true, value: 'default', variants: [ - { value: 'variant_a', weight: 0 }, - { value: 'variant_b', weight: 100 } - ] + { value: 'variant_a', weight: 0, priority: 1 }, + { value: 'variant_b', weight: 100, priority: 2 } + ], + metadata: { flagsmithId: 1 } } }, identity: { key: 'test_user', identifier: 'test_user' }, @@ -362,6 +353,6 @@ test('evaluateFeatures with multivariate evaluation', () => { } }; - const flags = evaluateFeatures(context, {}); + const flags = evaluateFeatures(context as any, {}); expect(flags['Multivariate Feature'].value).toBe('variant_b'); }); diff --git a/tests/engine/unit/segments/segments_model.test.ts b/tests/engine/unit/segments/segments_model.test.ts index 5607f03..0c84448 100644 --- a/tests/engine/unit/segments/segments_model.test.ts +++ b/tests/engine/unit/segments/segments_model.test.ts @@ -1,3 +1,4 @@ +import { SegmentSource } from '../../../../flagsmith-engine/evaluation/models'; import { EvaluationContext } from '../../../../flagsmith-engine/evaluationContext/evaluationContext.types'; import { CONSTANTS } from '../../../../flagsmith-engine/features/constants'; import { @@ -140,7 +141,7 @@ test('test_segment_rule_matching_function', () => { }); test('test_fromSegmentResult_with_multiple_variants', () => { - const segmentResults = [{ key: '1', name: 'test_segment' }]; + const segmentResults = [{ name: 'test_segment', metadata: { flagsmithId: '1' } }]; const evaluationContext: EvaluationContext = { identity: { @@ -156,6 +157,10 @@ test('test_fromSegmentResult_with_multiple_variants', () => { '1': { key: '1', name: 'test_segment', + metadata: { + source: SegmentSource.API, + flagsmithId: '1' + }, rules: [ { type: 'ALL', @@ -171,11 +176,13 @@ test('test_fromSegmentResult_with_multiple_variants', () => { overrides: [ { key: 'override', - feature_key: '1', name: 'multivariate_feature', enabled: true, value: 'default_value', priority: 1, + metadata: { + flagsmithId: 1 + }, variants: [ { id: 1, value: 'variant_a', weight: 30 }, { id: 2, value: 'variant_b', weight: 70 } diff --git a/tests/sdk/flagsmith.test.ts b/tests/sdk/flagsmith.test.ts index 41c1d8c..caa80f6 100644 --- a/tests/sdk/flagsmith.test.ts +++ b/tests/sdk/flagsmith.test.ts @@ -519,3 +519,23 @@ test('get_user_agent_extracts_version_from_package_json', async () => { expect(userAgent).toBe(`flagsmith-nodejs-sdk/${packageJson.version}`); }); + +test('Flags.fromEvaluationResult throws error when metadata.flagsmithId is missing', () => { + const evaluationResult = { + flags: { + test_feature: { + enabled: true, + name: 'test_feature', + value: 'test_value', + reason: 'DEFAULT', + metadata: {} + } + }, + segments: [] + }; + + expect(() => Flags.fromEvaluationResult(evaluationResult as any)).toThrow( + 'FlagResult metadata.flagsmithId is missing for feature "test_feature". ' + + 'This indicates a bug in the SDK, please report it.' + ); +});