Skip to content

Commit

Permalink
feat: add targeting validation/warning (#878)
Browse files Browse the repository at this point in the history
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
  • Loading branch information
toddbaert authored Apr 16, 2024
1 parent 1830a11 commit 2a4dbcf
Show file tree
Hide file tree
Showing 7 changed files with 153 additions and 85 deletions.
2 changes: 1 addition & 1 deletion libs/shared/flagd-core/flagd-schemas
Submodule flagd-schemas updated 36 files
+9 −5 .github/schema-store-issue-template.md
+3 −3 .github/workflows/pr-checks.yml
+3 −3 .github/workflows/release-please.yaml
+2 −2 .release-please-manifest.json
+1 −1 CONTRIBUTING.md
+9 −2 Makefile
+1 −1 go.mod
+52 −0 json/CHANGELOG.md
+240 −1 json/examples/full.json
+0 −101 json/flagd-definitions.yaml
+5 −2 json/flagd_definitions.go
+14 −4 json/flagd_definitions_test.go
+28 −13 json/flags.json
+118 −0 json/flags.yaml
+582 −0 json/targeting.json
+427 −0 json/targeting.yaml
+1 −0 json/test/negative/empty-variants.json
+26 −0 json/test/negative/fractional-invalid-bucketing.json
+25 −0 json/test/negative/fractional-invalid-weighting.json
+23 −0 json/test/negative/invalid-ends-with-param.json
+23 −0 json/test/negative/invalid-flagd-props.json
+23 −0 json/test/negative/invalid-starts-with-param.json
+12 −11 json/test/negative/malformed-flag.json
+8 −6 json/test/negative/missing-variants.json
+1 −0 json/test/negative/mixed-variant-types.ffconfig.json
+9 −8 json/test/negative/no-default-variant.json
+23 −0 json/test/negative/sem-ver-invalid-range-specifier.json
+23 −0 json/test/negative/sem-ver-invalid-ver-expression.json
+11 −10 json/test/negative/state-set-incorrectly.json
+73 −0 json/test/positive/basic-json-ops.json
+161 −0 json/test/positive/custom-ops.json
+24 −31 json/test/positive/example.flagd.json
+71 −0 json/test/positive/if-shorthand.json
+25 −0 protobuf/CHANGELOG.md
+1 −1 protobuf/buf.gen.ts-connect.yaml
+12 −0 protobuf/flagd/sync/v1/sync.proto
22 changes: 20 additions & 2 deletions libs/shared/flagd-core/src/lib/feature-flag.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { FlagValue } from '@openfeature/core';
import { FlagValue, ParseError } from '@openfeature/core';
import { sha1 } from 'object-hash';

/**
Expand All @@ -15,7 +15,7 @@ export interface Flag {
* Flagd flag configuration structure for internal reference.
*/
export class FeatureFlag {
private readonly _state: string;
private readonly _state: 'ENABLED' | 'DISABLED';
private readonly _defaultVariant: string;
private readonly _variants: Map<string, FlagValue>;
private readonly _targeting: unknown;
Expand All @@ -27,6 +27,7 @@ export class FeatureFlag {
this._variants = new Map<string, FlagValue>(Object.entries(flag['variants']));
this._targeting = flag['targeting'];
this._hash = sha1(flag);
this.validateStructure();
}

get hash(): string {
Expand All @@ -48,4 +49,21 @@ export class FeatureFlag {
get variants(): Map<string, FlagValue> {
return this._variants;
}

validateStructure() {
// basic validation, ideally this sort of thing is caught by IDEs and other schema validation before we get here
// consistent with Java/Go and other implementations, we only warn for schema validation, but we fail for this sort of basic structural errors
if (this._state !== 'ENABLED' && this._state !== 'DISABLED') {
throw new ParseError(`Invalid flag state: ${JSON.stringify(this._state, undefined, 2)}`);
}
if (this._defaultVariant === undefined) {
// this can be falsy, and int, etc...
throw new ParseError(`Invalid flag defaultVariant: ${JSON.stringify(this._defaultVariant, undefined, 2)}`);
}
if (!this._variants.has(this._defaultVariant)) {
throw new ParseError(
`Default variant ${this._defaultVariant} missing from variants ${JSON.stringify(this._variants, undefined, 2)}`,
);
}
}
}
30 changes: 16 additions & 14 deletions libs/shared/flagd-core/src/lib/flagd-core.spec.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { FlagNotFoundError, ParseError, StandardResolutionReasons, TypeMismatchError } from '@openfeature/core';
import { FlagdCore } from './flagd-core';
import { FlagNotFoundError, GeneralError, StandardResolutionReasons, TypeMismatchError } from '@openfeature/core';

describe('flagd-core resolving', () => {
describe('truthy variant values', () => {
Expand Down Expand Up @@ -132,18 +132,14 @@ describe('flagd-core targeting evaluations', () => {
describe('flagd-core validations', () => {
// flags of disabled, invalid variants and missing variant
const mixFlags =
'{"flags":{"myBoolFlag":{"state":"DISABLED","variants":{"on":true,"off":false},"defaultVariant":"on"},"myStringFlag":{"state":"ENABLED","variants":{"on":true,"off":false},"defaultVariant":"on"},"myIntFlag":{"state":"ENABLED","variants":{"two":2},"defaultVariant":"one"}}}';
'{"flags":{"myBoolFlag":{"state":"DISABLED","variants":{"on":true,"off":false},"defaultVariant":"on"},"myStringFlag":{"state":"ENABLED","variants":{"on":true,"off":false},"defaultVariant":"on"}}}';
let core: FlagdCore;

beforeAll(() => {
core = new FlagdCore();
core.setConfigurations(mixFlags);
});

it('should validate flag type - eval int as boolean', () => {
expect(() => core.resolveBooleanEvaluation('myIntFlag', true, {}, console)).toThrow(GeneralError);
});

it('should throw because the flag does not exist', () => {
const flagKey = 'nonexistentFlagKey';
expect(() => core.resolveBooleanEvaluation(flagKey, false, {}, console)).toThrow(
Expand All @@ -161,10 +157,6 @@ describe('flagd-core validations', () => {
it('should validate variant', () => {
expect(() => core.resolveStringEvaluation('myStringFlag', 'hello', {}, console)).toThrow(TypeMismatchError);
});

it('should validate variant existence', () => {
expect(() => core.resolveNumberEvaluation('myIntFlag', 100, {}, console)).toThrow(GeneralError);
});
});

describe('flagd-core common flag definitions', () => {
Expand Down Expand Up @@ -226,11 +218,21 @@ describe('flagd-core common flag definitions', () => {
expect(resolved.variant).toBe('false');
});

it('should throw with invalid targeting rules', () => {
it('should throw ParseError with invalid invalid flag state', () => {
const core = new FlagdCore();
const flagCfg = `{"flags":{"isEnabled":{"state":"ENABLED","variants":{"true":true,"false":false},"defaultVariant":"false","targeting":{"invalid": ["this is not valid targeting"]}}}}`;
core.setConfigurations(flagCfg);
const flagCfg = `{"flags":{"isEnabled":{"state":"INVALID","variants":{"true":true,"false":false},"defaultVariant":"false"}}}`;
expect(() => core.setConfigurations(flagCfg)).toThrow(ParseError);
});

expect(() => core.resolveBooleanEvaluation('isEnabled', false, {}, console)).toThrow();
it('should throw ParseError with invalid invalid flag variants', () => {
const core = new FlagdCore();
const flagCfg = `{"flags":{"isEnabled":{"state":"ENABLED","variants":"invalid","defaultVariant":"false"}}}`;
expect(() => core.setConfigurations(flagCfg)).toThrow(ParseError);
});

it('should throw ParseError with invalid invalid flag defaultVariant', () => {
const core = new FlagdCore();
const flagCfg = `{"flags":{"isEnabled":{"state":"ENABLED","variants":{"true":true,"false":false},"defaultVariant":{}}}}`;
expect(() => core.setConfigurations(flagCfg)).toThrow(ParseError);
});
});
2 changes: 1 addition & 1 deletion libs/shared/flagd-core/src/lib/flagd-core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export class FlagdCore implements Storage {
private _targeting: Targeting;

constructor(storage?: Storage, logger?: Logger) {
this._storage = storage ? storage : new MemoryStorage();
this._storage = storage ? storage : new MemoryStorage(logger);
this._logger = logger ? new SafeLogger(logger) : new DefaultLogger();
this._targeting = new Targeting(this._logger);
}
Expand Down
160 changes: 101 additions & 59 deletions libs/shared/flagd-core/src/lib/parser.spec.ts
Original file line number Diff line number Diff line change
@@ -1,75 +1,117 @@
import { parse } from './parser';
import { ParseError } from '@openfeature/core';
import { parse } from './parser';

describe('Flag configurations', () => {
it('should parse valid configurations - single', () => {
const simpleFlag =
'{\n' +
' "flags": {\n' +
' "myBoolFlag": {\n' +
' "state": "ENABLED",\n' +
' "variants": {\n' +
' "on": true,\n' +
' "off": false\n' +
' },\n' +
' "defaultVariant": "on"\n' +
' }\n' +
' }\n' +
'}';

const flags = parse(simpleFlag);
expect(flags).toBeTruthy();
expect(flags.get('myBoolFlag')).toBeTruthy();
});
describe('throwIfSchemaInvalid=false', () => {
it('should parse valid configurations - single', () => {
const simpleFlag =
'{\n' +
' "flags": {\n' +
' "myBoolFlag": {\n' +
' "state": "ENABLED",\n' +
' "variants": {\n' +
' "on": true,\n' +
' "off": false\n' +
' },\n' +
' "defaultVariant": "on"\n' +
' }\n' +
' }\n' +
'}';

it('should parse valid configurations - long', () => {
const longFlag =
'{"flags":{"myBoolFlag":{"state":"ENABLED","variants":{"on":true,"off":false},"defaultVariant":"on"},"myStringFlag":{"state":"ENABLED","variants":{"key1":"val1","key2":"val2"},"defaultVariant":"key1"},"myFloatFlag":{"state":"ENABLED","variants":{"one":1.23,"two":2.34},"defaultVariant":"one"},"myIntFlag":{"state":"ENABLED","variants":{"one":1,"two":2},"defaultVariant":"one"},"myObjectFlag":{"state":"ENABLED","variants":{"object1":{"key":"val"},"object2":{"key":true}},"defaultVariant":"object1"},"fibAlgo":{"variants":{"recursive":"recursive","memo":"memo","loop":"loop","binet":"binet"},"defaultVariant":"recursive","state":"ENABLED","targeting":{"if":[{"$ref":"emailWithFaas"},"binet",null]}},"targetedFlag":{"variants":{"first":"AAA","second":"BBB","third":"CCC"},"defaultVariant":"first","state":"ENABLED","targeting":{"if":[{"in":["@openfeature.dev",{"var":"email"}]},"second",{"in":["Chrome",{"var":"userAgent"}]},"third",null]}}},"$evaluators":{"emailWithFaas":{"in":["@faas.com",{"var":["email"]}]}}}';
const flags = parse(simpleFlag, false);
expect(flags).toBeTruthy();
expect(flags.get('myBoolFlag')).toBeTruthy();
});

const flags = parse(longFlag);
expect(flags).toBeTruthy();
expect(flags.get('myBoolFlag')).toBeTruthy();
expect(flags.get('myStringFlag')).toBeTruthy();
});
it('should parse valid configurations - long', () => {
const longFlag =
'{"flags":{"myBoolFlag":{"state":"ENABLED","variants":{"on":true,"off":false},"defaultVariant":"on"},"myStringFlag":{"state":"ENABLED","variants":{"key1":"val1","key2":"val2"},"defaultVariant":"key1"},"myFloatFlag":{"state":"ENABLED","variants":{"one":1.23,"two":2.34},"defaultVariant":"one"},"myIntFlag":{"state":"ENABLED","variants":{"one":1,"two":2},"defaultVariant":"one"},"myObjectFlag":{"state":"ENABLED","variants":{"object1":{"key":"val"},"object2":{"key":true}},"defaultVariant":"object1"},"fibAlgo":{"variants":{"recursive":"recursive","memo":"memo","loop":"loop","binet":"binet"},"defaultVariant":"recursive","state":"ENABLED","targeting":{"if":[{"$ref":"emailWithFaas"},"binet",null]}},"targetedFlag":{"variants":{"first":"AAA","second":"BBB","third":"CCC"},"defaultVariant":"first","state":"ENABLED","targeting":{"if":[{"in":["@openfeature.dev",{"var":"email"}]},"second",{"in":["Chrome",{"var":"userAgent"}]},"third",null]}}},"$evaluators":{"emailWithFaas":{"in":["@faas.com",{"var":["email"]}]}}}';

it('should fail if invalid - missing default value', () => {
const invalidFlag =
'{\n' +
' "flags": {\n' +
' "myBoolFlag": {\n' +
' "state": "ENABLED",\n' +
' "variants": {\n' +
' "on": true,\n' +
' "off": false\n' +
' }\n' +
' }\n' +
' }\n' +
'}';

expect(() => parse(invalidFlag)).toThrowError();
});
const flags = parse(longFlag, false);
expect(flags).toBeTruthy();
expect(flags.get('myBoolFlag')).toBeTruthy();
expect(flags.get('myStringFlag')).toBeTruthy();
});

it('should parse flag configurations with references', () => {
const flagWithRef =
'{"flags":{"fibAlgo":{"variants":{"recursive":"recursive","binet":"binet"},"defaultVariant":"recursive","state":"ENABLED","targeting":{"if":[{"$ref":"emailSuffixFaas"},"binet",null]}}},"$evaluators":{"emailSuffixFaas":{"in":["@faas.com",{"var":["email"]}]}}}';
it('should fail if invalid - missing default value', () => {
const invalidFlag =
'{\n' +
' "flags": {\n' +
' "myBoolFlag": {\n' +
' "state": "ENABLED",\n' +
' "variants": {\n' +
' "on": true,\n' +
' "off": false\n' +
' }\n' +
' }\n' +
' }\n' +
'}';

const flags = parse(flagWithRef);
expect(flags).toBeTruthy();
expect(() => parse(invalidFlag, false)).toThrowError();
});

const fibAlgo = flags.get('fibAlgo');
expect(fibAlgo).toBeTruthy();
expect(fibAlgo?.targeting).toStrictEqual({ if: [{ in: ['@faas.com', { var: ['email'] }] }, 'binet', null] });
});
it('should parse flag configurations with references', () => {
const flagWithRef =
'{"flags":{"fibAlgo":{"variants":{"recursive":"recursive","binet":"binet"},"defaultVariant":"recursive","state":"ENABLED","targeting":{"if":[{"$ref":"emailSuffixFaas"},"binet",null]}}},"$evaluators":{"emailSuffixFaas":{"in":["@faas.com",{"var":["email"]}]}}}';

const flags = parse(flagWithRef, false);
expect(flags).toBeTruthy();

const fibAlgo = flags.get('fibAlgo');
expect(fibAlgo).toBeTruthy();
expect(fibAlgo?.targeting).toStrictEqual({ if: [{ in: ['@faas.com', { var: ['email'] }] }, 'binet', null] });
});

it('should throw a parsing error due to invalid JSON', () => {
const invalidJson = '{';

expect(() => parse(invalidJson, false)).toThrow(ParseError);
});

it('should throw a parsing error due to invalid flagd configuration', () => {
const invalidFlagdConfig = '{"flags":{"fibAlgo":{}}}';

expect(() => parse(invalidFlagdConfig, false)).toThrow(ParseError);
});

it('should throw a parsing error due to invalid JSON', () => {
const invalidJson = '{';
it('should not throw if targeting invalid', () => {
const invalidFlag =
'{\n' +
' "flags": {\n' +
' "myBoolFlag": {\n' +
' "state": "ENABLED",\n' +
' "variants": {\n' +
' "on": true,\n' +
' "off": false\n' +
' },\n' +
' "defaultVariant": "on",\n' +
' "targeting": "invalid"\n' +
' }\n' +
' }\n' +
'}';

expect(() => parse(invalidJson)).toThrow(ParseError);
expect(() => parse(invalidFlag, false)).not.toThrow(ParseError);
});
});

it('should throw a parsing error due to invalid flagd configuration', () => {
const invalidFlagdConfig = '{"flags":{"fibAlgo":{}}}';
describe('throwIfSchemaInvalid=true', () => {
it('should not throw if targeting invalid', () => {
const invalidFlag =
'{\n' +
' "flags": {\n' +
' "myBoolFlag": {\n' +
' "state": "ENABLED",\n' +
' "variants": {\n' +
' "on": true,\n' +
' "off": false\n' +
' },\n' +
' "defaultVariant": "on",\n' +
' "targeting": "invalid"\n' +
' }\n' +
' }\n' +
'}';

expect(() => parse(invalidFlagdConfig)).toThrow(ParseError);
expect(() => parse(invalidFlag, true)).toThrow(ParseError);
});
});
});
17 changes: 11 additions & 6 deletions libs/shared/flagd-core/src/lib/parser.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { Logger, ParseError } from '@openfeature/core';
import Ajv from 'ajv';
import flagsSchema from '../../flagd-schemas/json/flags.json';
import targetingSchema from '../../flagd-schemas/json/targeting.json';
import { FeatureFlag, Flag } from './feature-flag';
import flagDefinitionSchema from '../../flagd-schemas/json/flagd-definitions.json';
import { ParseError } from '@openfeature/core';

const ajv = new Ajv();
const validate = ajv.compile(flagDefinitionSchema);
const ajv = new Ajv({ strict: false });
const validate = ajv.addSchema(targetingSchema).compile(flagsSchema);

const evaluatorKey = '$evaluators';
const bracketReplacer = new RegExp('^[^{]*\\{|}[^}]*$', 'g');
Expand All @@ -14,13 +15,17 @@ const errorMessages = 'invalid flagd flag configuration';
/**
* Validate and parse flag configurations.
*/
export function parse(flagCfg: string): Map<string, FeatureFlag> {
export function parse(flagCfg: string, throwIfSchemaInvalid: boolean, logger?: Logger): Map<string, FeatureFlag> {
try {
const transformed = transform(flagCfg);
const flags: { flags: { [key: string]: Flag } } = JSON.parse(transformed);
const isValid = validate(flags);
if (!isValid) {
throw new ParseError(errorMessages);
const message = `${errorMessages}: ${JSON.stringify(validate.errors, undefined, 2)}`;
logger?.warn(message);
if (throwIfSchemaInvalid) {
throw new ParseError(message);
}
}
const flagMap = new Map<string, FeatureFlag>();

Expand Down
5 changes: 3 additions & 2 deletions libs/shared/flagd-core/src/lib/storage.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { Logger } from '@openfeature/core';
import { FeatureFlag } from './feature-flag';
import { parse } from './parser';

Expand Down Expand Up @@ -33,7 +34,7 @@ export interface Storage {
export class MemoryStorage implements Storage {
private _flags: Map<string, FeatureFlag>;

constructor() {
constructor(private logger?: Logger) {
this._flags = new Map<string, FeatureFlag>();
}

Expand All @@ -46,7 +47,7 @@ export class MemoryStorage implements Storage {
}

setConfigurations(cfg: string): string[] {
const newFlags = parse(cfg);
const newFlags = parse(cfg, false, this.logger);
const oldFlags = this._flags;
const added: string[] = [];
const removed: string[] = [];
Expand Down

0 comments on commit 2a4dbcf

Please sign in to comment.