Skip to content

fix: init in-process error, throw on invalid rules #767

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions libs/providers/flagd/src/e2e/constants.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export const FLAGD_NAME = 'flagd Provider';
export const E2E_CLIENT_NAME = 'e2e';
export const UNSTABLE_CLIENT_NAME = 'unstable';
export const UNAVAILABLE_CLIENT_NAME = 'unavailable';
31 changes: 24 additions & 7 deletions libs/providers/flagd/src/e2e/setup-in-process-provider.ts
Original file line number Diff line number Diff line change
@@ -1,27 +1,44 @@
import assert from 'assert';
import { OpenFeature } from '@openfeature/server-sdk';
import { FlagdProvider } from '../lib/flagd-provider';

const FLAGD_NAME = 'flagd Provider';
const E2E_CLIENT_NAME = 'e2e';
const UNSTABLE_CLIENT_NAME = 'unstable';
import { E2E_CLIENT_NAME, FLAGD_NAME, UNSTABLE_CLIENT_NAME, UNAVAILABLE_CLIENT_NAME } from './constants';

// register the flagd provider before the tests.
console.log('Setting flagd provider...');
OpenFeature.setProvider(
E2E_CLIENT_NAME,
new FlagdProvider({ cache: 'disabled', resolverType: 'in-process', host: 'localhost', port: 9090 }),
new FlagdProvider({ resolverType: 'in-process', host: 'localhost', port: 9090 }),
);
OpenFeature.setProvider(
UNSTABLE_CLIENT_NAME,
new FlagdProvider({ resolverType: 'in-process', host: 'localhost', port: 9091 }),
);
OpenFeature.setProvider(
UNAVAILABLE_CLIENT_NAME,
new FlagdProvider({ resolverType: 'in-process', host: 'localhost', port: 9092 }),
);
assert(
OpenFeature.getProviderMetadata(E2E_CLIENT_NAME).name === FLAGD_NAME,
new Error(`Expected ${FLAGD_NAME} provider to be configured, instead got: ${OpenFeature.providerMetadata.name}`),
new Error(
`Expected ${FLAGD_NAME} provider to be configured, instead got: ${
OpenFeature.getProviderMetadata(E2E_CLIENT_NAME).name
}`,
),
);
assert(
OpenFeature.getProviderMetadata(UNSTABLE_CLIENT_NAME).name === FLAGD_NAME,
new Error(`Expected ${FLAGD_NAME} provider to be configured, instead got: ${OpenFeature.providerMetadata.name}`),
new Error(
`Expected ${FLAGD_NAME} provider to be configured, instead got: ${
OpenFeature.getProviderMetadata(UNSTABLE_CLIENT_NAME).name
}`,
),
);
assert(
OpenFeature.getProviderMetadata(UNAVAILABLE_CLIENT_NAME).name === FLAGD_NAME,
new Error(
`Expected ${FLAGD_NAME} provider to be configured, instead got: ${
OpenFeature.getProviderMetadata(UNAVAILABLE_CLIENT_NAME).name
}`,
),
);
console.log('flagd provider configured!');
26 changes: 20 additions & 6 deletions libs/providers/flagd/src/e2e/setup-rpc-provider.ts
Original file line number Diff line number Diff line change
@@ -1,21 +1,35 @@
import assert from 'assert';
import { OpenFeature } from '@openfeature/server-sdk';
import { FlagdProvider } from '../lib/flagd-provider';

const FLAGD_NAME = 'flagd Provider';
const E2E_CLIENT_NAME = 'e2e';
const UNSTABLE_CLIENT_NAME = 'unstable';
import { E2E_CLIENT_NAME, FLAGD_NAME, UNSTABLE_CLIENT_NAME, UNAVAILABLE_CLIENT_NAME } from './constants';

// register the flagd provider before the tests.
console.log('Setting flagd provider...');
OpenFeature.setProvider(E2E_CLIENT_NAME, new FlagdProvider({ cache: 'disabled' }));
OpenFeature.setProvider(UNSTABLE_CLIENT_NAME, new FlagdProvider({ cache: 'disabled', port: 8014 }));
OpenFeature.setProvider(UNAVAILABLE_CLIENT_NAME, new FlagdProvider({ cache: 'disabled', port: 8015 }));
assert(
OpenFeature.getProviderMetadata(E2E_CLIENT_NAME).name === FLAGD_NAME,
new Error(`Expected ${FLAGD_NAME} provider to be configured, instead got: ${OpenFeature.providerMetadata.name}`),
new Error(
`Expected ${FLAGD_NAME} provider to be configured, instead got: ${
OpenFeature.getProviderMetadata(E2E_CLIENT_NAME).name
}`,
),
);
assert(
OpenFeature.getProviderMetadata(UNSTABLE_CLIENT_NAME).name === FLAGD_NAME,
new Error(`Expected ${FLAGD_NAME} provider to be configured, instead got: ${OpenFeature.providerMetadata.name}`),
new Error(
`Expected ${FLAGD_NAME} provider to be configured, instead got: ${
OpenFeature.getProviderMetadata(UNSTABLE_CLIENT_NAME).name
}`,
),
);
assert(
OpenFeature.getProviderMetadata(UNAVAILABLE_CLIENT_NAME).name === FLAGD_NAME,
new Error(
`Expected ${FLAGD_NAME} provider to be configured, instead got: ${
OpenFeature.getProviderMetadata(UNAVAILABLE_CLIENT_NAME).name
}`,
),
);
console.log('flagd provider configured!');
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,13 @@ import {
StandardResolutionReasons,
} from '@openfeature/server-sdk';
import { defineFeature, loadFeature } from 'jest-cucumber';
import { E2E_CLIENT_NAME } from '../constants';

// load the feature file.
const feature = loadFeature('features/evaluation.feature');

// get a client (flagd provider registered in setup)
const client = OpenFeature.getClient('e2e');
const client = OpenFeature.getClient(E2E_CLIENT_NAME);

const givenAnOpenfeatureClientIsRegistered = (
given: (stepMatcher: string, stepDefinitionCallback: () => void) => void,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import { EvaluationContext, OpenFeature, ProviderEvents } from '@openfeature/server-sdk';
import { EvaluationContext, EvaluationDetails, OpenFeature, ProviderEvents } from '@openfeature/server-sdk';
import { defineFeature, loadFeature } from 'jest-cucumber';
import { StepsDefinitionCallbackFunction } from 'jest-cucumber/dist/src/feature-definition-creation';
import { E2E_CLIENT_NAME } from '../constants';

// load the feature file.
const feature = loadFeature('features/flagd-json-evaluator.feature');

// get a client (flagd provider registered in setup)
const client = OpenFeature.getClient('e2e');
const client = OpenFeature.getClient(E2E_CLIENT_NAME);

const aFlagProviderIsSet = (given: (stepMatcher: string, stepDefinitionCallback: () => void) => void) => {
given('a flagd provider is set', () => undefined);
Expand Down Expand Up @@ -56,10 +57,12 @@ defineFeature(feature, (test) => {
defaultValue = defaultVal;
});
and(
/^a context containing a nested property with outer key "(.*)" and inner key "(.*)", with value "(.*)"$/,
/^a context containing a nested property with outer key "(.*)" and inner key "(.*)", with value (.*)$/,
(outerKey: string, innerKey: string, value: string) => {
// we have to support string and non-string params in this test (we test invalid context value 3)
const valueNoQuotes = value.replaceAll('"', '');
evaluationContext[outerKey] = {
[innerKey]: value,
[innerKey]: parseInt(valueNoQuotes) || valueNoQuotes,
};
},
);
Expand All @@ -71,7 +74,70 @@ defineFeature(feature, (test) => {

test('Substring operators', evaluateStringFlagWithContext);

test('Semantic version operator numeric comparision', evaluateStringFlagWithContext);
test('Semantic version operator numeric comparison', evaluateStringFlagWithContext);

test('Semantic version operator semantic comparision', evaluateStringFlagWithContext);
test('Semantic version operator semantic comparison', evaluateStringFlagWithContext);

test('Time-based operations', ({ given, when, and, then }) => {
let flagKey: string;
let defaultValue: number;
const evaluationContext: EvaluationContext = {};

aFlagProviderIsSet(given);

when(/^an integer flag with key "(.*)" is evaluated with default value (\d+)$/, (key, defaultVal) => {
flagKey = key;
defaultValue = defaultVal;
});

and(/^a context containing a key "(.*)", with value (.*)$/, (key, value) => {
evaluationContext[key] = value;
});
then(/^the returned value should be (.*)$/, async (expectedValue) => {
const value = await client.getNumberValue(flagKey, defaultValue, evaluationContext);
expect(value).toEqual(parseInt(expectedValue));
});
});

test('Targeting by targeting key', ({ given, when, and, then }) => {
let flagKey: string;
let defaultValue: string;
let details: EvaluationDetails<string>;

aFlagProviderIsSet(given);

when(/^a string flag with key "(.*)" is evaluated with default value "(.*)"$/, (key, defaultVal) => {
flagKey = key;
defaultValue = defaultVal;
});

and(/^a context containing a targeting key with value "(.*)"$/, async (targetingKeyValue) => {
details = await client.getStringDetails(flagKey, defaultValue, { targetingKey: targetingKeyValue });
});

then(/^the returned value should be "(.*)"$/, (expectedValue) => {
expect(details.value).toEqual(expectedValue);
});

then(/^the returned reason should be "(.*)"$/, (expectedReason) => {
expect(details.reason).toEqual(expectedReason);
});
});

test('Errors and edge cases', ({ given, when, then }) => {
let flagKey: string;
let defaultValue: number;

aFlagProviderIsSet(given);

when(/^an integer flag with key "(.*)" is evaluated with default value (.*)$/, (key, defaultVal) => {
flagKey = key;
defaultValue = parseInt(defaultVal);
});

then(/^the returned value should be (.*)$/, async (expectedValue) => {
const value = await client.getNumberValue(flagKey, defaultValue);
expect(value).toEqual(parseInt(expectedValue));
});
});
});
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import { OpenFeature, ProviderEvents } from '@openfeature/server-sdk';
import { defineFeature, loadFeature } from 'jest-cucumber';
import { UNAVAILABLE_CLIENT_NAME, UNSTABLE_CLIENT_NAME } from '../constants';

jest.setTimeout(30000);

// load the feature file.
const feature = loadFeature('features/flagd-reconnect.feature');

// get a client (flagd provider registered in setup)
const client = OpenFeature.getClient('unstable');
const client = OpenFeature.getClient(UNSTABLE_CLIENT_NAME);

defineFeature(feature, (test) => {
let readyRunCount = 0;
Expand Down Expand Up @@ -46,4 +47,22 @@ defineFeature(feature, (test) => {
expect(readyRunCount).toBeGreaterThan(1);
});
});

test('Provider unavailable', ({ given, when, then }) => {
let errorHandlerRun = 0;

given('flagd is unavailable', async () => {
// handled in setup
});

when('a flagd provider is set and initialization is awaited', () => {
OpenFeature.getClient(UNAVAILABLE_CLIENT_NAME).addHandler(ProviderEvents.Error, () => {
errorHandlerRun++;
});
});

then('an error should be indicated within the configured deadline', () => {
expect(errorHandlerRun).toBeGreaterThan(0);
});
});
});
5 changes: 3 additions & 2 deletions libs/providers/flagd/src/e2e/step-definitions/flagd.spec.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import { OpenFeature, ProviderEvents, EventDetails } from '@openfeature/server-sdk';
import { OpenFeature, ProviderEvents } from '@openfeature/server-sdk';
import { defineFeature, loadFeature } from 'jest-cucumber';
import { E2E_CLIENT_NAME } from '../constants';

// load the feature file.
const feature = loadFeature('features/flagd.feature');

// get a client (flagd provider registered in setup)
const client = OpenFeature.getClient('e2e');
const client = OpenFeature.getClient(E2E_CLIENT_NAME);

const aFlagProviderIsSet = (given: (stepMatcher: string, stepDefinitionCallback: () => void) => void) => {
given('a flagd provider is set', () => undefined);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,39 +70,67 @@ export class GrpcFetch implements DataFetch {
) {
this._logger?.debug('Starting gRPC sync connection');
closeStreamIfDefined(this._syncStream);
this._syncStream = this._syncClient.syncFlags(this._request);
Copy link
Member Author

@toddbaert toddbaert Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-test change 2: This can throw before it ever connects, which resulted in an unhandled exception... when that happens the .on('error') handler never takes effect. This is fixed by making the catch and .on('error') both share a local method (handleError).

As you can see, all reconnect e2e tests still work with this change.

try {
this._syncStream = this._syncClient.syncFlags(this._request);
this._syncStream.on('data', (data: SyncFlagsResponse) => {
this._logger?.debug(`Received sync payload`);

this._syncStream.on('data', (data: SyncFlagsResponse) => {
this._logger?.debug(`Received sync payload`);
try {
const changes = dataCallback(data.flagConfiguration);
if (this._initialized && changes.length > 0) {
changedCallback(changes);
}
} catch (err) {
this._logger?.debug('Error processing sync payload: ', (err as Error)?.message ?? 'unknown error');
}

try {
const changes = dataCallback(data.flagConfiguration);
if (this._initialized && changes.length > 0) {
changedCallback(changes);
if (resolveConnect) {
resolveConnect();
} else if (!this._isConnected) {
// Not the first connection and there's no active connection.
this._logger?.debug('Reconnected to gRPC sync');
reconnectCallback();
}
} catch (err) {
this._logger?.debug('Error processing sync payload: ', (err as Error)?.message ?? 'unknown error');
}
this._isConnected = true;
});

if (resolveConnect) {
resolveConnect();
} else if (!this._isConnected) {
// Not the first connection and there's no active connection.
this._logger?.debug('Reconnected to gRPC sync');
reconnectCallback();
}
this._isConnected = true;
});
this._syncStream.on('error', (err: ServiceError | undefined) => {
this.handleError(
err as Error,
dataCallback,
reconnectCallback,
changedCallback,
disconnectCallback,
rejectConnect,
);
});
} catch (err) {
this.handleError(
err as Error,
dataCallback,
reconnectCallback,
changedCallback,
disconnectCallback,
rejectConnect,
);
}
}

this._syncStream.on('error', (err: ServiceError | undefined) => {
this._logger?.error('Connection error, attempting to reconnect');
this._logger?.debug(err);
this._isConnected = false;
const errorMessage = err?.message ?? 'Failed to connect to syncFlags stream';
disconnectCallback(errorMessage);
rejectConnect?.(new GeneralError(errorMessage));
this.reconnect(dataCallback, reconnectCallback, changedCallback, disconnectCallback);
});
private handleError(
err: Error,
dataCallback: (flags: string) => string[],
reconnectCallback: () => void,
changedCallback: (flagsChanged: string[]) => void,
disconnectCallback: (message: string) => void,
rejectConnect?: (reason: Error) => void,
) {
this._logger?.error('Connection error, attempting to reconnect');
this._logger?.debug(err);
this._isConnected = false;
const errorMessage = err?.message ?? 'Failed to connect to syncFlags stream';
disconnectCallback(errorMessage);
rejectConnect?.(new GeneralError(errorMessage));
this.reconnect(dataCallback, reconnectCallback, changedCallback, disconnectCallback);
}

private reconnect(
Expand Down
8 changes: 8 additions & 0 deletions libs/shared/flagd-core/src/lib/flagd-core.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -225,4 +225,12 @@ describe('flagd-core common flag definitions', () => {
expect(resolved.reason).toBe(StandardResolutionReasons.STATIC);
expect(resolved.variant).toBe('false');
});

it('should throw with invalid targeting rules', () => {
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);

expect(() => core.resolveBooleanEvaluation('isEnabled', false, {}, console)).toThrow();
Comment on lines +229 to +234
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New unit test for e2e test I mentioned.

});
});
Loading