Skip to content

Commit 6fb07fc

Browse files
lukas-reininglukas-reining
committed
feat: lock provider binding to a single API instance
Signed-off-by: Lukas Reining <lukas.reining@codecentric.de>
1 parent e79c06a commit 6fb07fc

5 files changed

Lines changed: 259 additions & 2 deletions

File tree

‎packages/server/src/open-feature.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ export class OpenFeatureAPI
3939

4040
private _transactionContextPropagator: TransactionContextPropagator = NOOP_TRANSACTION_CONTEXT_PROPAGATOR;
4141

42-
private constructor() {
42+
protected constructor() {
4343
super('server');
4444
}
4545

‎packages/server/test/open-feature.spec.ts‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,19 @@
11
import type { Paradigm } from '@openfeature/core';
2+
import { BOUND_API_KEY } from '@openfeature/core';
23
import type { Provider, ProviderStatus } from '../src';
34
import { OpenFeature, OpenFeatureAPI } from '../src';
45
import { OpenFeatureClient } from '../src/client/internal/open-feature-client';
56

7+
class TestOpenFeatureAPI extends OpenFeatureAPI {
8+
constructor() {
9+
super();
10+
}
11+
}
12+
13+
function getBoundApi(provider: object): unknown {
14+
return BOUND_API_KEY in provider ? provider[BOUND_API_KEY] : undefined;
15+
}
16+
617
const mockProvider = (config?: { initialStatus?: ProviderStatus; runsOn?: Paradigm }) => {
718
return {
819
metadata: {
@@ -230,4 +241,103 @@ describe('OpenFeature', () => {
230241
expect(provider3.onClose).toHaveBeenCalled();
231242
});
232243
});
244+
245+
describe('Requirement 1.x.x', () => {
246+
it('should throw if a provider instance is already bound to a different API instance', () => {
247+
const otherApi = new TestOpenFeatureAPI();
248+
const provider = mockProvider();
249+
250+
otherApi.setProvider(provider);
251+
252+
expect(() => OpenFeature.setProvider(provider)).toThrow(/already bound to a different OpenFeature API instance/);
253+
});
254+
255+
it('should throw if a provider instance bound to another API is set via setProviderAndWait', async () => {
256+
const otherApi = new TestOpenFeatureAPI();
257+
const provider = mockProvider();
258+
259+
await otherApi.setProviderAndWait(provider);
260+
261+
await expect(OpenFeature.setProviderAndWait(provider)).rejects.toThrow(
262+
/already bound to a different OpenFeature API instance/,
263+
);
264+
});
265+
266+
it('should not throw if the provider is already bound to the same API instance', () => {
267+
const provider = mockProvider();
268+
OpenFeature.setProvider(provider);
269+
270+
expect(() => OpenFeature.setProvider('another-domain', provider)).not.toThrow();
271+
});
272+
273+
it('should unbind a provider when it is replaced and no longer used by any domain', () => {
274+
const provider = mockProvider();
275+
OpenFeature.setProvider(provider);
276+
277+
expect(getBoundApi(provider)).toBe(OpenFeature);
278+
279+
const newProvider = mockProvider();
280+
OpenFeature.setProvider(newProvider);
281+
282+
expect(getBoundApi(provider)).toBeUndefined();
283+
expect(getBoundApi(newProvider)).toBe(OpenFeature);
284+
});
285+
286+
it('should not unbind a provider that is still used by another domain', async () => {
287+
const provider = { ...mockProvider(), onClose: jest.fn() };
288+
289+
await OpenFeature.setProviderAndWait('domain1', provider);
290+
await OpenFeature.setProviderAndWait('domain2', provider);
291+
292+
const newProvider = mockProvider();
293+
await OpenFeature.setProviderAndWait('domain1', newProvider);
294+
295+
expect(getBoundApi(provider)).toBe(OpenFeature);
296+
expect(provider.onClose).not.toHaveBeenCalled();
297+
});
298+
299+
it('should unbind all providers on clearProviders', async () => {
300+
const provider1 = mockProvider();
301+
const provider2 = mockProvider();
302+
303+
OpenFeature.setProvider(provider1);
304+
OpenFeature.setProvider('domain1', provider2);
305+
306+
expect(getBoundApi(provider1)).toBe(OpenFeature);
307+
expect(getBoundApi(provider2)).toBe(OpenFeature);
308+
309+
await OpenFeature.clearProviders();
310+
311+
expect(getBoundApi(provider1)).toBeUndefined();
312+
expect(getBoundApi(provider2)).toBeUndefined();
313+
});
314+
315+
it('should unbind all providers on close', async () => {
316+
const provider1 = mockProvider();
317+
const provider2 = mockProvider();
318+
319+
OpenFeature.setProvider(provider1);
320+
OpenFeature.setProvider('domain1', provider2);
321+
322+
expect(getBoundApi(provider1)).toBe(OpenFeature);
323+
expect(getBoundApi(provider2)).toBe(OpenFeature);
324+
325+
await OpenFeature.close();
326+
327+
expect(getBoundApi(provider1)).toBeUndefined();
328+
expect(getBoundApi(provider2)).toBeUndefined();
329+
});
330+
331+
it('should allow re-registering a provider after it has been unbound', async () => {
332+
const provider = mockProvider();
333+
OpenFeature.setProvider(provider);
334+
expect(getBoundApi(provider)).toBe(OpenFeature);
335+
336+
await OpenFeature.clearProviders();
337+
expect(getBoundApi(provider)).toBeUndefined();
338+
339+
expect(() => OpenFeature.setProvider(provider)).not.toThrow();
340+
expect(getBoundApi(provider)).toBe(OpenFeature);
341+
});
342+
});
233343
});

‎packages/shared/src/open-feature.ts‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,13 @@ import type { Paradigm } from './types';
2121

2222
type AnyProviderStatus = ClientProviderStatus | ServerProviderStatus;
2323

24+
/**
25+
* Well-known Symbol used to track which {@link OpenFeatureCommonAPI} instance a provider is currently bound to.
26+
* The symbol is shared across module boundaries and globally unique in the runtime.
27+
* @internal
28+
*/
29+
export const BOUND_API_KEY = Symbol.for('@openfeature/bound-api');
30+
2431
/**
2532
* A provider and its current status.
2633
* For internal use only.
@@ -217,6 +224,21 @@ export abstract class OpenFeatureCommonAPI<
217224
contextOrUndefined?: EvaluationContext,
218225
): this;
219226

227+
private bindProvider(provider: P): void {
228+
Object.defineProperty(provider, BOUND_API_KEY, {
229+
value: this,
230+
configurable: true,
231+
enumerable: false,
232+
writable: false,
233+
});
234+
}
235+
236+
private unbindProvider(provider: P): void {
237+
if (BOUND_API_KEY in provider) {
238+
delete provider[BOUND_API_KEY];
239+
}
240+
}
241+
220242
protected setAwaitableProvider(domainOrProvider?: string | P, providerOrUndefined?: P): Promise<void> | void {
221243
const domain = stringOrUndefined(domainOrProvider);
222244
const provider = objectOrUndefined<P>(domainOrProvider) ?? objectOrUndefined<P>(providerOrUndefined);
@@ -241,6 +263,12 @@ export abstract class OpenFeatureCommonAPI<
241263
throw new GeneralError(`Provider '${provider.metadata.name}' is intended for use on the ${provider.runsOn}.`);
242264
}
243265

266+
if (BOUND_API_KEY in provider && provider[BOUND_API_KEY] !== this) {
267+
throw new GeneralError(
268+
`Provider '${provider.metadata.name}' is already bound to a different OpenFeature API instance. A provider instance must not be registered with multiple API instances simultaneously.`,
269+
);
270+
}
271+
244272
const emitters = this.getAssociatedEventEmitters(domain);
245273

246274
let initializationPromise: Promise<void> | void = undefined;
@@ -300,10 +328,13 @@ export abstract class OpenFeatureCommonAPI<
300328
this._defaultProvider = wrappedProvider;
301329
}
302330

331+
this.bindProvider(provider);
332+
303333
this.transferListeners(oldProvider, provider, domain, emitters);
304334

305335
// Do not close a provider that is bound to any client
306336
if (!this.allProviders.includes(oldProvider)) {
337+
this.unbindProvider(oldProvider);
307338
oldProvider?.onClose?.()?.catch((err: Error | undefined) => {
308339
this._logger.error(`error closing provider: ${err?.message}, ${err?.stack}`);
309340
});
@@ -399,6 +430,8 @@ export abstract class OpenFeatureCommonAPI<
399430
await this?._defaultProvider.provider?.onClose?.();
400431
} catch (err) {
401432
this.handleShutdownError(this._defaultProvider.provider, err);
433+
} finally {
434+
this.unbindProvider(this._defaultProvider.provider);
402435
}
403436

404437
const wrappers = Array.from(this._domainScopedProviders);
@@ -409,6 +442,8 @@ export abstract class OpenFeatureCommonAPI<
409442
await wrapper?.provider.onClose?.();
410443
} catch (err) {
411444
this.handleShutdownError(wrapper?.provider, err);
445+
} finally {
446+
this.unbindProvider(wrapper?.provider);
412447
}
413448
}),
414449
);

‎packages/web/src/open-feature.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ export class OpenFeatureAPI
3434
protected _domainScopedProviders: Map<string, ProviderWrapper<Provider, ClientProviderStatus>> = new Map();
3535
protected _createEventEmitter = () => new OpenFeatureEventEmitter();
3636

37-
private constructor() {
37+
protected constructor() {
3838
super('client');
3939
}
4040

‎packages/web/test/open-feature.spec.ts‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,19 @@
11
import type { Paradigm } from '@openfeature/core';
2+
import { BOUND_API_KEY } from '@openfeature/core';
23
import type { Provider } from '../src';
34
import { OpenFeature, OpenFeatureAPI, ProviderStatus } from '../src';
45
import { OpenFeatureClient } from '../src/client/internal/open-feature-client';
56

7+
class TestOpenFeatureAPI extends OpenFeatureAPI {
8+
constructor() {
9+
super();
10+
}
11+
}
12+
13+
function getBoundApi(provider: object): unknown {
14+
return BOUND_API_KEY in provider ? provider[BOUND_API_KEY] : undefined;
15+
}
16+
617
const mockProvider = (config?: { initialStatus?: ProviderStatus; runsOn?: Paradigm }) => {
718
return {
819
metadata: {
@@ -256,4 +267,105 @@ describe('OpenFeature', () => {
256267
});
257268
});
258269
});
270+
271+
describe('Requirement 1.x.x', () => {
272+
it('should throw if a provider instance is already bound to a different API instance', () => {
273+
const otherApi = new TestOpenFeatureAPI();
274+
const provider = mockProvider();
275+
276+
otherApi.setProvider(provider);
277+
278+
expect(() => OpenFeature.setProvider(provider)).toThrow(/already bound to a different OpenFeature API instance/);
279+
});
280+
281+
it('should throw if a provider instance bound to another API is set via setProviderAndWait', async () => {
282+
const otherApi = new TestOpenFeatureAPI();
283+
const provider = mockProvider();
284+
285+
await otherApi.setProviderAndWait(provider);
286+
287+
await expect(OpenFeature.setProviderAndWait(provider)).rejects.toThrow(
288+
/already bound to a different OpenFeature API instance/,
289+
);
290+
});
291+
292+
it('should not throw if the provider is already bound to the same API instance', () => {
293+
const provider = mockProvider();
294+
OpenFeature.setProvider(provider);
295+
296+
expect(() => OpenFeature.setProvider('another-domain', provider)).not.toThrow();
297+
});
298+
299+
it('should unbind a provider when it is replaced and no longer used by any domain', () => {
300+
const provider = mockProvider();
301+
OpenFeature.setProvider(provider);
302+
303+
expect(getBoundApi(provider)).toBe(OpenFeature);
304+
305+
// Replace the provider with a new one.
306+
const newProvider = mockProvider();
307+
OpenFeature.setProvider(newProvider);
308+
309+
// The old provider should be unbound.
310+
expect(getBoundApi(provider)).toBeUndefined();
311+
expect(getBoundApi(newProvider)).toBe(OpenFeature);
312+
});
313+
314+
it('should not unbind a provider that is still used by another domain', async () => {
315+
const provider = { ...mockProvider(), onClose: jest.fn() };
316+
317+
await OpenFeature.setProviderAndWait('domain1', provider);
318+
await OpenFeature.setProviderAndWait('domain2', provider);
319+
320+
const newProvider = mockProvider();
321+
await OpenFeature.setProviderAndWait('domain1', newProvider);
322+
323+
expect(getBoundApi(provider)).toBe(OpenFeature);
324+
expect(provider.onClose).not.toHaveBeenCalled();
325+
});
326+
327+
it('should unbind all providers on clearProviders', async () => {
328+
const provider1 = mockProvider();
329+
const provider2 = mockProvider();
330+
331+
OpenFeature.setProvider(provider1);
332+
OpenFeature.setProvider('domain1', provider2);
333+
334+
expect(getBoundApi(provider1)).toBe(OpenFeature);
335+
expect(getBoundApi(provider2)).toBe(OpenFeature);
336+
337+
await OpenFeature.clearProviders();
338+
339+
expect(getBoundApi(provider1)).toBeUndefined();
340+
expect(getBoundApi(provider2)).toBeUndefined();
341+
});
342+
343+
it('should unbind all providers on close', async () => {
344+
const provider1 = mockProvider();
345+
const provider2 = mockProvider();
346+
347+
OpenFeature.setProvider(provider1);
348+
OpenFeature.setProvider('domain1', provider2);
349+
350+
expect(getBoundApi(provider1)).toBe(OpenFeature);
351+
expect(getBoundApi(provider2)).toBe(OpenFeature);
352+
353+
await OpenFeature.close();
354+
355+
expect(getBoundApi(provider1)).toBeUndefined();
356+
expect(getBoundApi(provider2)).toBeUndefined();
357+
});
358+
359+
it('should allow re-registering a provider after it has been unbound', async () => {
360+
const provider = mockProvider();
361+
OpenFeature.setProvider(provider);
362+
expect(getBoundApi(provider)).toBe(OpenFeature);
363+
364+
await OpenFeature.clearProviders();
365+
expect(getBoundApi(provider)).toBeUndefined();
366+
367+
expect(() => OpenFeature.setProvider(provider)).not.toThrow();
368+
expect(getBoundApi(provider)).toBe(OpenFeature);
369+
});
370+
});
259371
});

0 commit comments

Comments
 (0)