From 407732267aa8714e30456e23fffba5e4cf5a6897 Mon Sep 17 00:00:00 2001 From: Lukas Reining Date: Fri, 21 Jul 2023 17:00:05 +0200 Subject: [PATCH 1/2] fix: call onContextChange for all providers when changing context Signed-off-by: Lukas Reining --- packages/client/src/open-feature.ts | 4 +- .../client/test/evaluation-context.spec.ts | 70 +++++++++++++++++++ 2 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 packages/client/test/evaluation-context.spec.ts diff --git a/packages/client/src/open-feature.ts b/packages/client/src/open-feature.ts index a6cb4b315..130384c83 100644 --- a/packages/client/src/open-feature.ts +++ b/packages/client/src/open-feature.ts @@ -37,7 +37,9 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI implements Ma async setContext(context: EvaluationContext): Promise { const oldContext = this._context; this._context = context; - await this._defaultProvider?.onContextChange?.(oldContext, context); + + const allProviders = [this._defaultProvider, ...this._clientProviders.values()]; + await Promise.all(allProviders.map((provider) => provider.onContextChange?.(oldContext, context))); } getContext(): EvaluationContext { diff --git a/packages/client/test/evaluation-context.spec.ts b/packages/client/test/evaluation-context.spec.ts new file mode 100644 index 000000000..1b89a7c05 --- /dev/null +++ b/packages/client/test/evaluation-context.spec.ts @@ -0,0 +1,70 @@ +import { EvaluationContext, JsonValue, OpenFeature, Provider, ProviderMetadata, ResolutionDetails } from '../src'; + +class MockProvider implements Provider { + readonly metadata: ProviderMetadata; + + constructor(options?: { name?: string }) { + this.metadata = { name: options?.name ?? 'mock-provider' }; + } + + // eslint-disable-next-line @typescript-eslint/no-unused-vars + onContextChange(oldContext: EvaluationContext, newContext: EvaluationContext): Promise { + return Promise.resolve(); + } + + resolveBooleanEvaluation(): ResolutionDetails { + throw new Error('Not implemented'); + } + + resolveNumberEvaluation(): ResolutionDetails { + throw new Error('Not implemented'); + } + + resolveObjectEvaluation(): ResolutionDetails { + throw new Error('Not implemented'); + } + + resolveStringEvaluation(): ResolutionDetails { + throw new Error('Not implemented'); + } +} + +describe('Evaluation Context', () => { + describe('Requirement 3.2.2', () => { + it('the API MUST have a method for setting the global evaluation context', () => { + const context: EvaluationContext = { property1: false }; + OpenFeature.setContext(context); + expect(OpenFeature.getContext()).toEqual(context); + }); + }); + + describe('Requirement 3.2.4', () => { + describe('when the global evaluation context is set, the on context changed handler MUST run', () => { + it('on all registered providers', async () => { + // Set initial context + const context: EvaluationContext = { property1: false }; + await OpenFeature.setContext(context); + + // Set some providers + const defaultProvider = new MockProvider(); + const provider1 = new MockProvider(); + const provider2 = new MockProvider(); + + OpenFeature.setProvider(defaultProvider); + OpenFeature.setProvider('client1', provider1); + OpenFeature.setProvider('client2', provider2); + + // Spy on context changed handlers of all providers + const contextChangedSpys = [defaultProvider, provider1, provider2].map((provider) => + jest.spyOn(provider, 'onContextChange') + ); + + // Change context + const newContext: EvaluationContext = { property1: true, property2: 'prop2' }; + await OpenFeature.setContext(newContext); + + contextChangedSpys.forEach((spy) => expect(spy).toHaveBeenCalledWith(context, newContext)); + }); + }); + }); +}); From 461ecf4e19310d71abcf02283d279069effb8e4c Mon Sep 17 00:00:00 2001 From: Lukas Reining Date: Sun, 23 Jul 2023 14:19:12 +0200 Subject: [PATCH 2/2] fix: let all onContextChanged handlers run, even if one fails Signed-off-by: Lukas Reining --- packages/client/src/open-feature.ts | 10 ++++++- .../client/test/evaluation-context.spec.ts | 29 +++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/packages/client/src/open-feature.ts b/packages/client/src/open-feature.ts index 130384c83..aefb0c82c 100644 --- a/packages/client/src/open-feature.ts +++ b/packages/client/src/open-feature.ts @@ -39,7 +39,15 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI implements Ma this._context = context; const allProviders = [this._defaultProvider, ...this._clientProviders.values()]; - await Promise.all(allProviders.map((provider) => provider.onContextChange?.(oldContext, context))); + await Promise.all( + allProviders.map(async (provider) => { + try { + return await provider.onContextChange?.(oldContext, context); + } catch (err) { + this._logger?.error(`Error running context change handler of provider ${provider.metadata.name}:`, err); + } + }) + ); } getContext(): EvaluationContext { diff --git a/packages/client/test/evaluation-context.spec.ts b/packages/client/test/evaluation-context.spec.ts index 1b89a7c05..79e91198e 100644 --- a/packages/client/test/evaluation-context.spec.ts +++ b/packages/client/test/evaluation-context.spec.ts @@ -65,6 +65,35 @@ describe('Evaluation Context', () => { contextChangedSpys.forEach((spy) => expect(spy).toHaveBeenCalledWith(context, newContext)); }); + + it('on all registered providers even if one fails', async () => { + // Set initial context + const context: EvaluationContext = { property1: false }; + await OpenFeature.setContext(context); + + // Set some providers + const defaultProvider = new MockProvider(); + const provider1 = new MockProvider(); + const provider2 = new MockProvider(); + + OpenFeature.setProvider(defaultProvider); + OpenFeature.setProvider('client1', provider1); + OpenFeature.setProvider('client2', provider2); + + // Spy on context changed handlers of all providers + const contextChangedSpys = [defaultProvider, provider1, provider2].map((provider) => + jest.spyOn(provider, 'onContextChange') + ); + + // Let first handler fail + contextChangedSpys[0].mockImplementation(() => Promise.reject(new Error('Error'))); + + // Change context + const newContext: EvaluationContext = { property1: true, property2: 'prop2' }; + await OpenFeature.setContext(newContext); + + contextChangedSpys.forEach((spy) => expect(spy).toHaveBeenCalledWith(context, newContext)); + }); }); }); });