fix(flags): preserve invalid rules errors

Author: btthomas

packages/core/src/flags/configuration/__tests__/rules.test.ts

@@ -66,7 +66,7 @@ describe('rules configuration', () => {
         ).toBe(true);
     });
 
-    it('omits a flag that uses an unsupported operator', () => {
+    it('preserves a flag with an unsupported operator and reports PARSE_ERROR', () => {
         const source = buildRulesConfiguration();
         const condition =
             source.flags['dynamic-flag'].allocations[0].rules?.[0]
@@ -83,10 +83,25 @@ describe('rules configuration', () => {
         if (prepared.status !== 'ready') {
             throw new Error(prepared.errorMessage);
         }
-        expect(prepared.configuration.flags).toEqual({});
+        expect(prepared.configuration.flags).toHaveProperty('dynamic-flag');
+        expect(
+            flaggingCoreRulesEngine.evaluate({
+                configuration: prepared.configuration,
+                type: 'boolean',
+                flagKey: 'dynamic-flag',
+                defaultValue: false,
+                context: { targetingKey: 'user-1' },
+                logger: getNoopRulesLogger()
+            })
+        ).toMatchObject({
+            value: false,
+            reason: 'ERROR',
+            errorCode: 'PARSE_ERROR',
+            errorMessage: expect.stringContaining('FUTURE_OPERATOR')
+        });
     });
 
-    it('omits a flag that contains an invalid regular expression', () => {
+    it('reports PARSE_ERROR for a flag with an invalid regular expression', () => {
         const source = buildRulesConfiguration();
         const conditions =
             source.flags['dynamic-flag'].allocations[0].rules?.[0].conditions;
@@ -105,10 +120,22 @@ describe('rules configuration', () => {
         if (prepared.status !== 'ready') {
             throw new Error(prepared.errorMessage);
         }
-        expect(prepared.configuration.flags).toEqual({});
+        expect(
+            flaggingCoreRulesEngine.evaluate({
+                configuration: prepared.configuration,
+                type: 'boolean',
+                flagKey: 'dynamic-flag',
+                defaultValue: false,
+                context: { targetingKey: 'user-1' },
+                logger: getNoopRulesLogger()
+            })
+        ).toMatchObject({
+            errorCode: 'PARSE_ERROR',
+            errorMessage: 'A regular expression condition is not valid.'
+        });
     });
 
-    it('omits a flag whose split points to an absent variation', () => {
+    it('reports PARSE_ERROR when a split points to an absent variation', () => {
         const source = buildRulesConfiguration();
         source.flags['dynamic-flag'].allocations[0].splits[0].variationKey =
             'absent';
@@ -119,10 +146,22 @@ describe('rules configuration', () => {
         if (prepared.status !== 'ready') {
             throw new Error(prepared.errorMessage);
         }
-        expect(prepared.configuration.flags).toEqual({});
+        expect(
+            flaggingCoreRulesEngine.evaluate({
+                configuration: prepared.configuration,
+                type: 'boolean',
+                flagKey: 'dynamic-flag',
+                defaultValue: false,
+                context: { targetingKey: 'user-1' },
+                logger: getNoopRulesLogger()
+            })
+        ).toMatchObject({
+            errorCode: 'PARSE_ERROR',
+            errorMessage: 'A split has an invalid variation key.'
+        });
     });
 
-    it('keeps valid flags when it omits an invalid flag', () => {
+    it('keeps valid flags usable when another flag has a parse error', () => {
         const source = buildRulesConfiguration();
         const validFlag = buildRulesConfiguration().flags['dynamic-flag'];
         validFlag.key = 'valid-flag';
@@ -143,8 +182,46 @@ describe('rules configuration', () => {
             throw new Error(prepared.errorMessage);
         }
         expect(Object.keys(prepared.configuration.flags)).toEqual([
+            'dynamic-flag',
             'valid-flag'
         ]);
+        expect(
+            flaggingCoreRulesEngine.evaluate({
+                configuration: prepared.configuration,
+                type: 'boolean',
+                flagKey: 'valid-flag',
+                defaultValue: false,
+                context: { targetingKey: 'user-1', country: 'US' },
+                logger: getNoopRulesLogger()
+            })
+        ).toMatchObject({ value: true, errorCode: undefined });
+    });
+
+    // TODO(FFL-2837): Replace this legacy JSON compatibility test with a
+    // generated protobuf fixture after a flagging-core release contains
+    // DataDog/openfeature-js-client#344 at or after `be0d886`.
+    it('keeps supported known data when an unknown field is present', () => {
+        const source = buildRulesConfiguration();
+        (source.flags['dynamic-flag'] as typeof source.flags['dynamic-flag'] & {
+            futureField: string;
+        }).futureField = 'ignored';
+
+        const prepared = prepareRulesConfiguration(source);
+
+        expect(prepared.status).toBe('ready');
+        if (prepared.status !== 'ready') {
+            throw new Error(prepared.errorMessage);
+        }
+        expect(
+            flaggingCoreRulesEngine.evaluate({
+                configuration: prepared.configuration,
+                type: 'boolean',
+                flagKey: 'dynamic-flag',
+                defaultValue: false,
+                context: { targetingKey: 'user-1', country: 'US' },
+                logger: getNoopRulesLogger()
+            })
+        ).toMatchObject({ value: true, errorCode: undefined });
     });
 
     it('normalizes a real flagging-core evaluation', () => {

packages/core/src/flags/configuration/rules.ts

@@ -15,7 +15,8 @@ import type { EvaluationContext, JsonValue, PrimitiveValue } from '../types';
 // TODO(FFL-2837): Replace this legacy UFC v1 alias with
 // `NonNullable<FlagsConfiguration['rules']>['response']` after a flagging-core
 // release contains DataDog/openfeature-js-client#344. Keep the
-// `FlagsConfiguration` type import on the flagging-core package root.
+// `FlagsConfiguration` type import on the flagging-core package root. PR #344
+// now preserves invalid flags and reports their stored errors during evaluation.
 type RulesConfigurationResponse = UniversalFlagConfigurationV1;
 
 export type RulesValueType = 'boolean' | 'string' | 'number' | 'object';
@@ -125,6 +126,15 @@ export const toRulesEvaluationContext = (
 const hasOwn = (value: object, key: PropertyKey): boolean =>
     Object.prototype.hasOwnProperty.call(value, key);
 
+// TODO(FFL-2837): Delete this compatibility error store after a flagging-core
+// release contains DataDog/openfeature-js-client#344 at or after `ba1dbaf`.
+// The generated protobuf parser uses the same per-configuration error model,
+// and its evaluator returns `PARSE_ERROR` with the stored validation message.
+const errorsByConfiguration = new WeakMap<
+    RulesConfigurationResponse,
+    ReadonlyMap<string, string>
+>();
+
 const isRecord = (value: unknown): value is Record<string, unknown> =>
     typeof value === 'object' && value !== null && !Array.isArray(value);
 
@@ -431,24 +441,31 @@ export const prepareRulesConfiguration = (
 
     // TODO(FFL-2837): Delete this legacy JSON clone and validator after a
     // flagging-core release contains upstream PR #344. That implementation
-    // decodes a generated Protobuf-ES response and omits unsupported or invalid
-    // flags. Do not adapt this validator to the generated response type.
+    // decodes a generated Protobuf-ES response, preserves invalid flags, and
+    // records per-flag errors for evaluation. Do not adapt this validator to
+    // the generated response type.
     const errorMessage = validateRulesConfigurationEnvelope(clone);
     if (errorMessage) {
         return { status: 'error', errorMessage };
     }
 
-    const flags = (clone as RulesConfigurationResponse).flags;
+    const configuration = clone as RulesConfigurationResponse;
+    const flags = configuration.flags;
+    const errors = new Map<string, string>();
     for (const [flagKey, flag] of Object.entries(flags)) {
-        if (validateFlag(flag)) {
-            delete flags[flagKey];
+        const flagError = validateFlag(flag);
+        if (flagError) {
+            errors.set(flagKey, flagError);
         }
     }
 
     freezeValue(clone);
+    if (errors.size > 0) {
+        errorsByConfiguration.set(configuration, errors);
+    }
     return {
         status: 'ready',
-        configuration: clone as RulesConfigurationResponse
+        configuration
     };
 };
 
@@ -514,6 +531,21 @@ export const flaggingCoreRulesEngine: RulesEngine = {
             };
         }
 
+        // TODO(FFL-2837): Delete this compatibility check with the local error
+        // store after the published PR #344 evaluator reports parser errors.
+        const configurationError = errorsByConfiguration
+            .get(request.configuration)
+            ?.get(request.flagKey);
+        if (configurationError) {
+            return {
+                value: request.defaultValue,
+                reason: 'ERROR',
+                errorCode: 'PARSE_ERROR',
+                errorMessage: configurationError,
+                metadata: {}
+            };
+        }
+
         const result = evaluateRules(
             request.configuration,
             request.type,

packages/core/src/flags/configuration/wire.ts

@@ -28,7 +28,8 @@ import type {
 // evaluator on the package root. Use `FlagsConfiguration.rules`. The distribution
 // layer must put one base64 encoding of the raw dd-source#34959 protobuf response
 // in the version 1 `rules.response` field. Do not add that service transport or
-// envelope construction here.
+// envelope construction here. PR #344 preserves invalid protobuf flags and
+// reports their validation errors when the flag is evaluated.
 type PendingRulesConfiguration = FlagsConfiguration & {
     rulesBased?: {
         response: UniversalFlagConfigurationV1;
@@ -77,7 +78,8 @@ export const configurationFromString = (source: string): FlagsConfiguration => {
     // pending types above. The upstream parser decodes `rules.response` as a
     // generated Protobuf-ES message. Do not adapt this shim to decode a raw
     // service response or to add a base64 layer. Do not copy the strict base64
-    // validator that PR #344 removed in favor of the Protobuf-ES decoder.
+    // validator that PR #344 removed in favor of the Protobuf-ES decoder. The
+    // published parser must also include PR #344's unknown-field tolerance.
     const pendingRules = readPendingRulesWire(source);
     if (pendingRules) {
         try {