diff --git a/.changeset/structured-evaluation-failures.md b/.changeset/structured-evaluation-failures.md new file mode 100644 index 000000000..bd6117126 --- /dev/null +++ b/.changeset/structured-evaluation-failures.md @@ -0,0 +1,5 @@ +--- +"@reflag/flag-evaluation": patch +--- + +Return structured diagnostics for invalid numeric and date operands and unknown targeting operators instead of writing evaluation failures directly to the console. diff --git a/packages/flag-evaluation/src/index.ts b/packages/flag-evaluation/src/index.ts index 33c69e891..e0c7d4745 100644 --- a/packages/flag-evaluation/src/index.ts +++ b/packages/flag-evaluation/src/index.ts @@ -74,23 +74,26 @@ export type FilterTree = * - "IS_TRUE": Checks if a boolean value is true. * - "IS_FALSE": Checks if a boolean value is false. */ -export type ContextFilterOperator = - | "IS" - | "IS_NOT" - | "ANY_OF" - | "NOT_ANY_OF" - | "CONTAINS" - | "NOT_CONTAINS" - | "GT" - | "LT" - | "AFTER" - | "BEFORE" - | "DATE_AFTER" - | "DATE_BEFORE" - | "SET" - | "NOT_SET" - | "IS_TRUE" - | "IS_FALSE"; +const CONTEXT_FILTER_OPERATORS = [ + "IS", + "IS_NOT", + "ANY_OF", + "NOT_ANY_OF", + "CONTAINS", + "NOT_CONTAINS", + "GT", + "LT", + "AFTER", + "BEFORE", + "DATE_AFTER", + "DATE_BEFORE", + "SET", + "NOT_SET", + "IS_TRUE", + "IS_FALSE", +] as const; + +export type ContextFilterOperator = (typeof CONTEXT_FILTER_OPERATORS)[number]; /** * Represents a filter configuration used to filter data based on specific context. @@ -211,6 +214,24 @@ export type EvaluationError = field: string; operator: ContextFilterOperator | "rolloutPercentage"; message: string; + } + | { + code: "INVALID_CONTEXT_VALUE"; + field: string; + operator: ContextFilterOperator; + message: string; + } + | { + code: "INVALID_TARGETING_VALUE"; + field: string; + operator: ContextFilterOperator; + message: string; + } + | { + code: "UNKNOWN_OPERATOR"; + field: string; + operator: string; + message: string; }; function normalizeArrayElement(value: unknown): string | undefined { @@ -373,6 +394,8 @@ export function hashInt(hashInput: string): number { return Math.floor((value / 0xfffff) * 100000); } +const CONTEXT_FILTER_OPERATOR_SET = new Set(CONTEXT_FILTER_OPERATORS); + const ARRAY_OPERATORS = new Set([ "IS", "IS_NOT", @@ -450,21 +473,8 @@ export function evaluate( !normalizedFieldValue.toLowerCase().includes(value.toLowerCase()) ); case "GT": - if (isNaN(Number(normalizedFieldValue)) || isNaN(Number(value))) { - // TODO: return error instead? used logger previously - console.error( - `GT operator requires numeric values: ${normalizedFieldValue}, ${value}`, - ); - return false; - } return Number(normalizedFieldValue) > Number(value); case "LT": - if (isNaN(Number(normalizedFieldValue)) || isNaN(Number(value))) { - console.error( - `LT operator requires numeric values: ${normalizedFieldValue}, ${value}`, - ); - return false; - } return Number(normalizedFieldValue) < Number(value); case "AFTER": case "BEFORE": { @@ -481,12 +491,6 @@ export function evaluate( case "DATE_BEFORE": { const fieldValueDate = new Date(normalizedFieldValue).getTime(); const valueDate = new Date(value).getTime(); - if (isNaN(fieldValueDate) || isNaN(valueDate)) { - console.error( - `${operator} operator requires valid date values: ${normalizedFieldValue}, ${value}`, - ); - return false; - } return operator === "DATE_AFTER" ? fieldValueDate >= valueDate : fieldValueDate <= valueDate; @@ -512,7 +516,6 @@ export function evaluate( case "IS_FALSE": return normalizedFieldValue == "false"; default: - console.error(`unknown operator: ${operator}`); return false; } } @@ -545,6 +548,104 @@ function addMissingContextFieldError( }); } +type ExpectedValue = "numeric" | "a valid date" | "a numeric day offset"; + +function isExpectedValue(value: string | undefined, expected: ExpectedValue) { + return expected === "a valid date" + ? !isNaN(new Date(value ?? "").getTime()) + : !isNaN(Number(value)); +} + +function addInvalidContextValueError( + errors: Map, + field: string, + operator: ContextFilterOperator, + expected: ExpectedValue, +): void { + errors.set(`invalid-context:${field}:${operator}`, { + code: "INVALID_CONTEXT_VALUE", + field, + operator, + message: `Context field "${field}" must be ${expected} for operator "${operator}".`, + }); +} + +function addInvalidTargetingValueError( + errors: Map, + field: string, + operator: ContextFilterOperator, + expected: ExpectedValue, +): void { + errors.set(`invalid-targeting:${field}:${operator}`, { + code: "INVALID_TARGETING_VALUE", + field, + operator, + message: `Targeting value for operator "${operator}" and context field "${field}" must be ${expected}.`, + }); +} + +function addUnknownOperatorError( + errors: Map, + field: string, + operator: string, +): void { + errors.set(`unknown-operator:${field}:${operator}`, { + code: "UNKNOWN_OPERATOR", + field, + operator, + message: `Unknown targeting operator "${operator}" for context field "${field}".`, + }); +} + +function hasValidOperatorValues( + filter: ContextFilter, + normalizedFieldValue: string, + errors: Map, +): boolean { + let contextExpected: ExpectedValue | undefined; + let targetingExpected: ExpectedValue | undefined; + + switch (filter.operator) { + case "GT": + case "LT": + contextExpected = targetingExpected = "numeric"; + break; + case "AFTER": + case "BEFORE": + contextExpected = "a valid date"; + targetingExpected = "a numeric day offset"; + break; + case "DATE_AFTER": + case "DATE_BEFORE": + contextExpected = targetingExpected = "a valid date"; + break; + default: + return true; + } + + const contextValid = isExpectedValue(normalizedFieldValue, contextExpected); + const targetingValid = isExpectedValue(filter.values?.[0], targetingExpected); + + if (!contextValid) { + addInvalidContextValueError( + errors, + filter.field, + filter.operator, + contextExpected, + ); + } + if (!targetingValid) { + addInvalidTargetingValueError( + errors, + filter.field, + filter.operator, + targetingExpected, + ); + } + + return contextValid && targetingValid; +} + function evaluateRecursively( filter: RuleFilter, context: FlattenedContext, @@ -554,6 +655,12 @@ function evaluateRecursively( case "constant": return filter.value; case "context": { + const operator = String(filter.operator); + if (!CONTEXT_FILTER_OPERATOR_SET.has(operator)) { + addUnknownOperatorError(errors, filter.field, operator); + return false; + } + if ( !(filter.field in context) && filter.operator !== "SET" && @@ -572,6 +679,13 @@ function evaluateRecursively( return false; } + if ( + !Array.isArray(normalizedFieldValue) && + !hasValidOperatorValues(filter, normalizedFieldValue, errors) + ) { + return false; + } + return evaluate( normalizedFieldValue, filter.operator, diff --git a/packages/flag-evaluation/test/index.test.ts b/packages/flag-evaluation/test/index.test.ts index 56fc48174..69a4a1aa3 100644 --- a/packages/flag-evaluation/test/index.test.ts +++ b/packages/flag-evaluation/test/index.test.ts @@ -1,6 +1,7 @@ import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; import { + ContextFilterOperator, evaluate, evaluateFlagRules, EvaluationParams, @@ -887,6 +888,191 @@ describe("evaluate flag targeting integration ", () => { }); }); + describe("invalid scalar operator values", () => { + it("returns diagnostics for invalid numeric context and targeting values", () => { + const rules: Rule[] = [ + { + value: true, + filter: { + type: "context", + field: "user.age", + operator: "GT", + values: ["not-numeric"], + }, + }, + ]; + const context = { user: { age: "also-not-numeric" } }; + + for (const result of [ + evaluateFlagRules({ flagKey: "numeric", rules, context }), + newEvaluator(rules)(context, "numeric"), + ]) { + expect(result.value).toBeUndefined(); + expect(result.ruleEvaluationResults).toEqual([false]); + expect(result.errors).toEqual([ + { + code: "INVALID_CONTEXT_VALUE", + field: "user.age", + operator: "GT", + message: + 'Context field "user.age" must be numeric for operator "GT".', + }, + { + code: "INVALID_TARGETING_VALUE", + field: "user.age", + operator: "GT", + message: + 'Targeting value for operator "GT" and context field "user.age" must be numeric.', + }, + ]); + } + }); + + it.each([ + ["Infinity", "GT", "1"], + ["1", "LT", "Infinity"], + ] as const)( + "preserves numeric evaluation semantics for %s %s %s", + (contextValue, operator, targetingValue) => { + const rules: Rule[] = [ + { + value: true, + filter: { + type: "context", + field: "value", + operator, + values: [targetingValue], + }, + }, + ]; + const context = { value: contextValue }; + + expect(evaluate(contextValue, operator, [targetingValue])).toBe(true); + for (const result of [ + evaluateFlagRules({ flagKey: "numeric", rules, context }), + newEvaluator(rules)(context, "numeric"), + ]) { + expect(result.value).toBe(true); + expect(result.errors).toBeUndefined(); + } + }, + ); + + it("returns diagnostics for invalid date context and targeting values", () => { + const rules: Rule[] = [ + { + value: true, + filter: { + type: "context", + field: "user.createdAt", + operator: "DATE_AFTER", + values: ["not-a-date"], + }, + }, + ]; + const context = { user: { createdAt: "also-not-a-date" } }; + + for (const result of [ + evaluateFlagRules({ flagKey: "date", rules, context }), + newEvaluator(rules)(context, "date"), + ]) { + expect(result.errors).toEqual([ + { + code: "INVALID_CONTEXT_VALUE", + field: "user.createdAt", + operator: "DATE_AFTER", + message: + 'Context field "user.createdAt" must be a valid date for operator "DATE_AFTER".', + }, + { + code: "INVALID_TARGETING_VALUE", + field: "user.createdAt", + operator: "DATE_AFTER", + message: + 'Targeting value for operator "DATE_AFTER" and context field "user.createdAt" must be a valid date.', + }, + ]); + } + }); + + it("returns a diagnostic for an invalid relative-date offset", () => { + const rules: Rule[] = [ + { + value: true, + filter: { + type: "context", + field: "user.createdAt", + operator: "AFTER", + values: ["not-a-day-offset"], + }, + }, + ]; + const result = evaluateFlagRules({ + flagKey: "relative-date", + rules, + context: { user: { createdAt: "2024-01-10" } }, + }); + + expect(result.errors).toEqual([ + { + code: "INVALID_TARGETING_VALUE", + field: "user.createdAt", + operator: "AFTER", + message: + 'Targeting value for operator "AFTER" and context field "user.createdAt" must be a numeric day offset.', + }, + ]); + }); + + it("returns a diagnostic for an unknown operator", () => { + const rules: Rule[] = [ + { + value: true, + filter: { + type: "context", + field: "user.role", + operator: "UNKNOWN" as ContextFilterOperator, + values: ["admin"], + }, + }, + ]; + const result = evaluateFlagRules({ + flagKey: "unknown-operator", + rules, + context: { user: { role: "admin" } }, + }); + + expect(result.errors).toEqual([ + { + code: "UNKNOWN_OPERATOR", + field: "user.role", + operator: "UNKNOWN", + message: + 'Unknown targeting operator "UNKNOWN" for context field "user.role".', + }, + ]); + }); + + it("does not write invalid evaluations directly to the console", () => { + const consoleError = vi + .spyOn(console, "error") + .mockImplementation(() => undefined); + + try { + expect(evaluate("not-numeric", "GT", ["also-not-numeric"])).toBe(false); + expect(evaluate("not-a-date", "DATE_AFTER", ["also-not-a-date"])).toBe( + false, + ); + expect( + evaluate("value", "UNKNOWN" as ContextFilterOperator, ["target"]), + ).toBe(false); + expect(consoleError).not.toHaveBeenCalled(); + } finally { + consoleError.mockRestore(); + } + }); + }); + describe("DATE_AFTER and DATE_BEFORE in flag rules", () => { it("should evaluate DATE_AFTER operator in flag rules", () => { const res = evaluateFlagRules({