refactor(flag-evaluator): align evaluation error terminology

.changeset/consistent-evaluation-errors.md

@@ -0,0 +1,7 @@
+---
+"@reflag/flag-evaluation": patch
+"@reflag/browser-sdk": patch
+"@reflag/node-sdk": patch
+---
+
+Use consistent evaluation error terminology internally and clarify the error emitted when flags are evaluated before initial flag state is available.

.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.

packages/browser-sdk/src/client.ts

@@ -406,10 +406,10 @@ const CLIENT_NOT_INITIALIZED_EVALUATION_ERROR = {
   code: "CLIENT_NOT_INITIALIZED",
   field: "",
   message:
-    "ReflagClient was not initialized before this flag was evaluated. Call initialize() before evaluating flags.",
+    "Flag was evaluated before the initial flag state was available. Await initialize() or wait for SDK loading to complete before evaluating flags.",
 } as const;
 
-function withClientInitializationDiagnostic(
+function withClientInitializationError(
   errors: CheckEvent["evaluationErrors"],
   evaluatedBeforeInitialization: boolean,
 ): CheckEvent["evaluationErrors"] {
@@ -1445,7 +1445,7 @@ export class ReflagClient {
             version: f?.targetingVersion,
             ruleEvaluationResults: f?.ruleEvaluationResults,
             missingContextFields: f?.missingContextFields,
-            evaluationErrors: withClientInitializationDiagnostic(
+            evaluationErrors: withClientInitializationError(
               f?.evaluationErrors,
               evaluatedBeforeInitialization,
             ),
@@ -1464,7 +1464,7 @@ export class ReflagClient {
             version: f?.config?.version,
             ruleEvaluationResults: f?.config?.ruleEvaluationResults,
             missingContextFields: f?.config?.missingContextFields,
-            evaluationErrors: withClientInitializationDiagnostic(
+            evaluationErrors: withClientInitializationError(
               f?.config?.evaluationErrors,
               evaluatedBeforeInitialization,
             ),

packages/browser-sdk/src/flag/flags.ts

@@ -92,7 +92,7 @@ export type RawFlag = {
   missingContextFields?: string[];
 
   /**
-   * Non-fatal diagnostics produced while evaluating targeting rules.
+   * Non-fatal errors produced while evaluating targeting rules.
    */
   evaluationErrors?: Array<{
     code: string;
@@ -142,7 +142,7 @@ export type RawFlag = {
     missingContextFields?: string[];
 
     /**
-     * Non-fatal diagnostics produced while evaluating targeting rules.
+     * Non-fatal errors produced while evaluating targeting rules.
      */
     evaluationErrors?: RawFlag["evaluationErrors"];
   };
@@ -265,7 +265,7 @@ export interface CheckEvent {
   missingContextFields?: string[];
 
   /**
-   * Non-fatal diagnostics produced while evaluating the flag.
+   * Non-fatal errors produced while evaluating the flag.
    */
   evaluationErrors?: RawFlag["evaluationErrors"];
 }

packages/browser-sdk/test/usage.test.ts

@@ -33,7 +33,7 @@ const clientNotInitializedError = {
   code: "CLIENT_NOT_INITIALIZED",
   field: "",
   message:
-    "ReflagClient was not initialized before this flag was evaluated. Call initialize() before evaluating flags.",
+    "Flag was evaluated before the initial flag state was available. Await initialize() or wait for SDK loading to complete before evaluating flags.",
 };
 
 vi.mock("../src/sse");
@@ -456,7 +456,7 @@ describe(`sends "check" events `, () => {
       });
     });
 
-    it("adds diagnostics when flags are evaluated before initialization", () => {
+    it("adds errors when flags are evaluated before initialization", () => {
       const sendCheckEventSpy = vi.spyOn(
         FlagsClient.prototype,
         "sendCheckEvent",
@@ -485,7 +485,7 @@ describe(`sends "check" events `, () => {
       );
     });
 
-    it("does not add initialization diagnostics to bootstrapped evaluations", () => {
+    it("does not add initialization errors to bootstrapped evaluations", () => {
       const sendCheckEventSpy = vi.spyOn(
         FlagsClient.prototype,
         "sendCheckEvent",
@@ -508,7 +508,7 @@ describe(`sends "check" events `, () => {
       );
     });
 
-    it(`does not send check events or add initialization diagnostics when offline`, () => {
+    it(`does not send check events or add initialization errors when offline`, () => {
       const sendCheckEventSpy = vi.spyOn(
         FlagsClient.prototype,
         "sendCheckEvent",

packages/flag-evaluation/src/index.ts

@@ -74,23 +74,26 @@ export type FilterTree<T extends FilterClass> =
  * - "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<string>(CONTEXT_FILTER_OPERATORS);
+
 const ARRAY_OPERATORS = new Set<ContextFilterOperator>([
   "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<string, EvaluationError>,
+  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<string, EvaluationError>,
+  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<string, EvaluationError>,
+  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<string, EvaluationError>,
+): 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,
@@ -638,7 +752,7 @@ export interface EvaluationParams<T extends RuleValue> {
  * @property {boolean[]} ruleEvaluationResults - Array indicating the success or failure of each rule evaluated.
  * @property {string} [reason] - Optional field providing additional explanation regarding the evaluation result.
  * @property {string[]} [missingContextFields] - Legacy array of context fields that were required but not provided during evaluation.
- * @property {EvaluationError[]} [errors] - Non-fatal diagnostics for rules that could not be evaluated.
+ * @property {EvaluationError[]} [errors] - Non-fatal errors for rules that could not be evaluated.
  */
 export interface EvaluationResult<T extends RuleValue> {
   flagKey: string;