Skip to content

Commit

Permalink
fix: some handlers fail to run (#753)
Browse files Browse the repository at this point in the history 
fixed an issue where some handlers would fail to run if the associated
provider did not have a onContextChange method

---------

Signed-off-by: Todd Baert <[email protected]>
  • Loading branch information
@toddbaert
toddbaert authored Jan 11, 2024
1 parent b6adbba commit f4597af
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 39 deletions.
52 changes: 35 additions & 17 deletions packages/client/src/open-feature.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ const GLOBAL_OPENFEATURE_API_KEY = Symbol.for('@openfeature/web-sdk/api');
type OpenFeatureGlobal = {
[GLOBAL_OPENFEATURE_API_KEY]?: OpenFeatureAPI;
};
type NameProviderRecord = {
name?: string;
provider: Provider;
}

const _globalThis = globalThis as OpenFeatureGlobal;

export class OpenFeatureAPI extends OpenFeatureCommonAPI<Provider, Hook> implements ManageContext<Promise<void>> {
Expand Down Expand Up @@ -81,16 +86,23 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI<Provider, Hook> impleme
const oldContext = this._context;
this._context = context;

const providersWithoutContextOverride = Array.from(this._clientProviders.entries())
// collect all providers that are using the default context (not mapped to a name)
const defaultContextNameProviders: NameProviderRecord[] = Array.from(this._clientProviders.entries())
.filter(([name]) => !this._namedProviderContext.has(name))
.reduce<Provider[]>((acc, [, provider]) => {
acc.push(provider);
.reduce<NameProviderRecord[]>((acc, [name, provider]) => {
acc.push({ name, provider });
return acc;
}, []);

const allProviders = [this._defaultProvider, ...providersWithoutContextOverride];
const allProviders: NameProviderRecord[] = [
// add in the default (no name)
{ name: undefined, provider: this._defaultProvider },
...defaultContextNameProviders,
];
await Promise.all(
allProviders.map((provider) => this.runProviderContextChangeHandler(undefined, provider, oldContext, context)),
allProviders.map((tuple) =>
this.runProviderContextChangeHandler(tuple.name, tuple.provider, oldContext, context),
),
);
}
}
Expand Down Expand Up @@ -196,19 +208,25 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI<Provider, Hook> impleme
oldContext: EvaluationContext,
newContext: EvaluationContext,
): Promise<void> {
const providerName = provider.metadata.name;
return provider.onContextChange?.(oldContext, newContext).then(() => {
this.getAssociatedEventEmitters(clientName).forEach((emitter) => {
emitter?.emit(ProviderEvents.ContextChanged, { clientName, providerName });
});
this._events?.emit(ProviderEvents.ContextChanged, { clientName, providerName });
}).catch((err) => {
this._logger?.error(`Error running ${provider.metadata.name}'s context change handler:`, err);
this.getAssociatedEventEmitters(clientName).forEach((emitter) => {
emitter?.emit(ProviderEvents.Error, { clientName, providerName, message: err?.message, });
});
this._events?.emit(ProviderEvents.Error, { clientName, providerName, message: err?.message, });
const providerName = provider.metadata.name;
try {
await provider.onContextChange?.(oldContext, newContext);

// only run the event handlers if the onContextChange method succeeded
this.getAssociatedEventEmitters(clientName).forEach((emitter) => {
emitter?.emit(ProviderEvents.ContextChanged, { clientName, providerName });
});
this._events?.emit(ProviderEvents.ContextChanged, { clientName, providerName });
} catch (err) {
// run error handlers instead
const error = err as Error | undefined;
const message = `Error running ${provider?.metadata?.name}'s context change handler: ${error?.message}`;
this._logger?.error(`${message}`, err);
this.getAssociatedEventEmitters(clientName).forEach((emitter) => {
emitter?.emit(ProviderEvents.Error, { clientName, providerName, message });
});
this._events?.emit(ProviderEvents.Error, { clientName, providerName, message });
}
}
}

Expand Down
82 changes: 60 additions & 22 deletions packages/client/test/events.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,15 @@ class MockProvider implements Provider {
private asyncDelay?: number;
private enableEvents: boolean;
status?: ProviderStatus = undefined;
onContextChange?: () => Promise<void>;

constructor(options?: {
hasInitialize?: boolean;
initialStatus?: ProviderStatus;
asyncDelay?: number;
enableEvents?: boolean;
failOnInit?: boolean;
noContextChanged?: boolean;
failOnContextChange?: boolean;
name?: string;
}) {
Expand All @@ -43,6 +45,9 @@ class MockProvider implements Provider {
this.enableEvents = options?.enableEvents ?? true;
this.failOnInit = options?.failOnInit ?? false;
this.failOnContextChange = options?.failOnContextChange ?? false;
if (!options?.noContextChanged) {
this.onContextChange = this.changeHandler;
}

if (this.enableEvents) {
this.events = new OpenFeatureEventEmitter();
Expand All @@ -62,16 +67,6 @@ class MockProvider implements Provider {

initialize: jest.Mock<Promise<void>, []> | undefined;

async onContextChange(): Promise<void> {
return new Promise((resolve, reject) => setTimeout(() => {
if (this.failOnContextChange) {
reject(new Error(ERR_MESSAGE));
} else {
resolve();
}
}, this.asyncDelay));
}

resolveBooleanEvaluation(): ResolutionDetails<boolean> {
throw new Error('Not implemented');
}
Expand All @@ -87,6 +82,18 @@ class MockProvider implements Provider {
resolveStringEvaluation(): ResolutionDetails<string> {
throw new Error('Not implemented');
}

private changeHandler() {
return new Promise<void>((resolve, reject) =>
setTimeout(() => {
if (this.failOnContextChange) {
reject(new Error(ERR_MESSAGE));
} else {
resolve();
}
}, this.asyncDelay),
);
}
}

describe('Events', () => {
Expand All @@ -96,6 +103,7 @@ describe('Events', () => {

afterEach(async () => {
await OpenFeature.clearProviders();
OpenFeature.clearHandlers();
jest.clearAllMocks();
clientId = uuid();
// hacky, but it's helpful to clear the handlers between tests
Expand Down Expand Up @@ -610,7 +618,6 @@ describe('Events', () => {
expect(details?.clientName).toEqual(clientId);
expect(details?.providerName).toEqual(provider.metadata.name);
done();
OpenFeature.removeHandler(ProviderEvents.ContextChanged, handler);
} catch (e) {
done(e);
}
Expand All @@ -630,7 +637,6 @@ describe('Events', () => {
expect(details?.clientName).toEqual(clientId);
expect(details?.providerName).toEqual(provider.metadata.name);
done();
OpenFeature.removeHandler(ProviderEvents.Error, handler);
} catch (e) {
done(e);
}
Expand All @@ -643,15 +649,25 @@ describe('Events', () => {
describe('context set for different client', () => {
it("If the provider's `on context changed` function terminates normally, associated `PROVIDER_CONTEXT_CHANGED` handlers MUST run.", (done) => {
const provider = new MockProvider({ initialStatus: ProviderStatus.READY });
let runCount = 0;

OpenFeature.setProvider(clientId, provider);

// expect 2 runs, since 2 providers are impacted by this context change (global)
const handler = (details?: EventDetails) => {
try {
expect(details?.clientName).toBeUndefined();
expect(details?.providerName).toEqual(provider.metadata.name);
OpenFeature.removeHandler(ProviderEvents.ContextChanged, handler);
done();
runCount++;
// one run should be global
if (details?.clientName === undefined) {
expect(details?.providerName).toEqual(OpenFeature.getProviderMetadata().name);
} else if (details?.clientName === clientId) {
// one run should be for client
expect(details?.clientName).toEqual(clientId);
expect(details?.providerName).toEqual(provider.metadata.name);
}
if (runCount == 2) {
done();
}
} catch (e) {
done(e);
}
Expand All @@ -669,10 +685,10 @@ describe('Events', () => {

const handler = (details?: EventDetails) => {
try {
expect(details?.clientName).toBeUndefined();
// expect only one error run, because only one provider throws
expect(details?.clientName).toEqual(clientId);
expect(details?.providerName).toEqual(provider.metadata.name);
expect(details?.message).toEqual(ERR_MESSAGE);
OpenFeature.removeHandler(ProviderEvents.Error, handler);
expect(details?.message).toBeTruthy();
done();
} catch (e) {
done(e);
Expand All @@ -696,7 +712,6 @@ describe('Events', () => {
try {
expect(details?.clientName).toEqual(clientId);
expect(details?.providerName).toEqual(provider.metadata.name);
OpenFeature.removeHandler(ProviderEvents.ContextChanged, handler);
done();
} catch (e) {
done(e);
Expand All @@ -717,8 +732,7 @@ describe('Events', () => {
try {
expect(details?.clientName).toEqual(clientId);
expect(details?.providerName).toEqual(provider.metadata.name);
expect(details?.message).toEqual(ERR_MESSAGE);
OpenFeature.removeHandler(ProviderEvents.Error, handler);
expect(details?.message).toBeTruthy();
done();
} catch (e) {
done(e);
Expand All @@ -730,5 +744,29 @@ describe('Events', () => {

});
});

describe('provider', () => {
describe('has no onContextChange handler', () => {
it('runs API ContextChanged event handler', (done) => {
const noChangeHandlerProvider = 'noChangeHandlerProvider';
const provider = new MockProvider({ initialStatus: ProviderStatus.READY, noContextChanged: true });

OpenFeature.setProvider(noChangeHandlerProvider, provider);
OpenFeature.setContext(noChangeHandlerProvider, {});

const handler = (details?: EventDetails) => {
try {
expect(details?.clientName).toEqual(noChangeHandlerProvider);
expect(details?.providerName).toEqual(provider.metadata.name);
done();
} catch (e) {
done(e);
}
};

OpenFeature.addHandler(ProviderEvents.ContextChanged, handler);
});
});
});
});
});
7 changes: 7 additions & 0 deletions packages/shared/src/open-feature.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,13 @@ export abstract class OpenFeatureCommonAPI<P extends CommonProvider = CommonProv
this._events.removeHandler(eventType, handler);
}

/**
* Removes all event handlers.
*/
clearHandlers(): void {
this._events.removeAllHandlers();
}

/**
* Gets the current handlers for the given provider event type.
* @param {AnyProviderEvent} eventType The provider event type to get the current handlers for
Expand Down

0 comments on commit f4597af

Please sign in to comment.