Skip to content
Draft
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
2 changes: 1 addition & 1 deletion packages/server/src/open-feature.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export class OpenFeatureAPI

private _transactionContextPropagator: TransactionContextPropagator = NOOP_TRANSACTION_CONTEXT_PROPAGATOR;

private constructor() {
protected constructor() {
super('server');
}

Expand Down
110 changes: 110 additions & 0 deletions packages/server/test/open-feature.spec.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,19 @@
import type { Paradigm } from '@openfeature/core';
import { BOUND_API_KEY } from '@openfeature/core';
import type { Provider, ProviderStatus } from '../src';
import { OpenFeature, OpenFeatureAPI } from '../src';
import { OpenFeatureClient } from '../src/client/internal/open-feature-client';

class TestOpenFeatureAPI extends OpenFeatureAPI {
constructor() {
super();
}
}

function getBoundApi(provider: object): unknown {
return BOUND_API_KEY in provider ? provider[BOUND_API_KEY] : undefined;
}

const mockProvider = (config?: { initialStatus?: ProviderStatus; runsOn?: Paradigm }) => {
return {
metadata: {
Expand Down Expand Up @@ -230,4 +241,103 @@ describe('OpenFeature', () => {
expect(provider3.onClose).toHaveBeenCalled();
});
});

describe('Requirement 1.x.x', () => {
it('should throw if a provider instance is already bound to a different API instance', () => {
const otherApi = new TestOpenFeatureAPI();
const provider = mockProvider();

otherApi.setProvider(provider);

expect(() => OpenFeature.setProvider(provider)).toThrow(/already bound to a different OpenFeature API instance/);
});

it('should throw if a provider instance bound to another API is set via setProviderAndWait', async () => {
const otherApi = new TestOpenFeatureAPI();
const provider = mockProvider();

await otherApi.setProviderAndWait(provider);

await expect(OpenFeature.setProviderAndWait(provider)).rejects.toThrow(
/already bound to a different OpenFeature API instance/,
);
});

it('should not throw if the provider is already bound to the same API instance', () => {
const provider = mockProvider();
OpenFeature.setProvider(provider);

expect(() => OpenFeature.setProvider('another-domain', provider)).not.toThrow();
});

it('should unbind a provider when it is replaced and no longer used by any domain', () => {
const provider = mockProvider();
OpenFeature.setProvider(provider);

expect(getBoundApi(provider)).toBe(OpenFeature);

const newProvider = mockProvider();
OpenFeature.setProvider(newProvider);

expect(getBoundApi(provider)).toBeUndefined();
expect(getBoundApi(newProvider)).toBe(OpenFeature);
});

it('should not unbind a provider that is still used by another domain', async () => {
const provider = { ...mockProvider(), onClose: jest.fn() };

await OpenFeature.setProviderAndWait('domain1', provider);
await OpenFeature.setProviderAndWait('domain2', provider);

const newProvider = mockProvider();
await OpenFeature.setProviderAndWait('domain1', newProvider);

expect(getBoundApi(provider)).toBe(OpenFeature);
expect(provider.onClose).not.toHaveBeenCalled();
});

it('should unbind all providers on clearProviders', async () => {
const provider1 = mockProvider();
const provider2 = mockProvider();

OpenFeature.setProvider(provider1);
OpenFeature.setProvider('domain1', provider2);

expect(getBoundApi(provider1)).toBe(OpenFeature);
expect(getBoundApi(provider2)).toBe(OpenFeature);

await OpenFeature.clearProviders();

expect(getBoundApi(provider1)).toBeUndefined();
expect(getBoundApi(provider2)).toBeUndefined();
});

it('should unbind all providers on close', async () => {
const provider1 = mockProvider();
const provider2 = mockProvider();

OpenFeature.setProvider(provider1);
OpenFeature.setProvider('domain1', provider2);

expect(getBoundApi(provider1)).toBe(OpenFeature);
expect(getBoundApi(provider2)).toBe(OpenFeature);

await OpenFeature.close();

expect(getBoundApi(provider1)).toBeUndefined();
expect(getBoundApi(provider2)).toBeUndefined();
});

it('should allow re-registering a provider after it has been unbound', async () => {
const provider = mockProvider();
OpenFeature.setProvider(provider);
expect(getBoundApi(provider)).toBe(OpenFeature);

await OpenFeature.clearProviders();
expect(getBoundApi(provider)).toBeUndefined();

expect(() => OpenFeature.setProvider(provider)).not.toThrow();
expect(getBoundApi(provider)).toBe(OpenFeature);
});
});
});
35 changes: 35 additions & 0 deletions packages/shared/src/open-feature.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@ import type { Paradigm } from './types';

type AnyProviderStatus = ClientProviderStatus | ServerProviderStatus;

/**
* Well-known Symbol used to track which {@link OpenFeatureCommonAPI} instance a provider is currently bound to.
* The symbol is shared across module boundaries and globally unique in the runtime.
* @internal
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Do we have to move this to an extra internal file as we do with OpenFeatureClient implementations?
This is only exported for the tests.

*/
export const BOUND_API_KEY = Symbol.for('@openfeature/js-sdk/provider/bound-api');

/**
* A provider and its current status.
* For internal use only.
Expand Down Expand Up @@ -217,6 +224,21 @@ export abstract class OpenFeatureCommonAPI<
contextOrUndefined?: EvaluationContext,
): this;

private bindProvider(provider: P): void {
Object.defineProperty(provider, BOUND_API_KEY, {
value: this,
configurable: true,
enumerable: false,
writable: false,
});
}
Comment on lines +227 to +234
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This way is a transparent implementation to the providers.
We can not directly add it to the provider interface "non-breaking" as it only is an interface.


private unbindProvider(provider: P): void {
if (BOUND_API_KEY in provider) {
delete provider[BOUND_API_KEY];
}
}

protected setAwaitableProvider(domainOrProvider?: string | P, providerOrUndefined?: P): Promise<void> | void {
const domain = stringOrUndefined(domainOrProvider);
const provider = objectOrUndefined<P>(domainOrProvider) ?? objectOrUndefined<P>(providerOrUndefined);
Expand All @@ -241,6 +263,12 @@ export abstract class OpenFeatureCommonAPI<
throw new GeneralError(`Provider '${provider.metadata.name}' is intended for use on the ${provider.runsOn}.`);
}

if (BOUND_API_KEY in provider && provider[BOUND_API_KEY] !== this) {
throw new GeneralError(
`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.`,
);
}

const emitters = this.getAssociatedEventEmitters(domain);

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

this.bindProvider(provider);

this.transferListeners(oldProvider, provider, domain, emitters);

// Do not close a provider that is bound to any client
if (!this.allProviders.includes(oldProvider)) {
this.unbindProvider(oldProvider);
oldProvider?.onClose?.()?.catch((err: Error | undefined) => {
this._logger.error(`error closing provider: ${err?.message}, ${err?.stack}`);
});
Expand Down Expand Up @@ -399,6 +430,8 @@ export abstract class OpenFeatureCommonAPI<
await this?._defaultProvider.provider?.onClose?.();
} catch (err) {
this.handleShutdownError(this._defaultProvider.provider, err);
} finally {
this.unbindProvider(this._defaultProvider.provider);
}

const wrappers = Array.from(this._domainScopedProviders);
Expand All @@ -409,6 +442,8 @@ export abstract class OpenFeatureCommonAPI<
await wrapper?.provider.onClose?.();
} catch (err) {
this.handleShutdownError(wrapper?.provider, err);
} finally {
this.unbindProvider(wrapper?.provider);
}
Comment on lines 442 to 447
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The optional chaining on wrapper seems unnecessary. wrapper is destructured from an array of _domainScopedProviders entries and should not be nullable. The provider property on ProviderWrapper is also not nullable. Using wrapper.provider directly would improve clarity and type safety.

Suggested change
await wrapper?.provider.onClose?.();
} catch (err) {
this.handleShutdownError(wrapper?.provider, err);
} finally {
this.unbindProvider(wrapper?.provider);
}
await wrapper.provider.onClose?.();
} catch (err) {
this.handleShutdownError(wrapper.provider, err);
} finally {
this.unbindProvider(wrapper.provider);
}

}),
);
Expand Down
2 changes: 1 addition & 1 deletion packages/web/src/open-feature.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export class OpenFeatureAPI
protected _domainScopedProviders: Map<string, ProviderWrapper<Provider, ClientProviderStatus>> = new Map();
protected _createEventEmitter = () => new OpenFeatureEventEmitter();

private constructor() {
protected constructor() {
super('client');
}

Expand Down
112 changes: 112 additions & 0 deletions packages/web/test/open-feature.spec.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,19 @@
import type { Paradigm } from '@openfeature/core';
import { BOUND_API_KEY } from '@openfeature/core';
import type { Provider } from '../src';
import { OpenFeature, OpenFeatureAPI, ProviderStatus } from '../src';
import { OpenFeatureClient } from '../src/client/internal/open-feature-client';

class TestOpenFeatureAPI extends OpenFeatureAPI {
constructor() {
super();
}
}

function getBoundApi(provider: object): unknown {
return BOUND_API_KEY in provider ? provider[BOUND_API_KEY] : undefined;
}

const mockProvider = (config?: { initialStatus?: ProviderStatus; runsOn?: Paradigm }) => {
return {
metadata: {
Expand Down Expand Up @@ -256,4 +267,105 @@ describe('OpenFeature', () => {
});
});
});

describe('Requirement 1.x.x', () => {
it('should throw if a provider instance is already bound to a different API instance', () => {
const otherApi = new TestOpenFeatureAPI();
const provider = mockProvider();

otherApi.setProvider(provider);

expect(() => OpenFeature.setProvider(provider)).toThrow(/already bound to a different OpenFeature API instance/);
});

it('should throw if a provider instance bound to another API is set via setProviderAndWait', async () => {
const otherApi = new TestOpenFeatureAPI();
const provider = mockProvider();

await otherApi.setProviderAndWait(provider);

await expect(OpenFeature.setProviderAndWait(provider)).rejects.toThrow(
/already bound to a different OpenFeature API instance/,
);
});

it('should not throw if the provider is already bound to the same API instance', () => {
const provider = mockProvider();
OpenFeature.setProvider(provider);

expect(() => OpenFeature.setProvider('another-domain', provider)).not.toThrow();
});

it('should unbind a provider when it is replaced and no longer used by any domain', () => {
const provider = mockProvider();
OpenFeature.setProvider(provider);

expect(getBoundApi(provider)).toBe(OpenFeature);

// Replace the provider with a new one.
const newProvider = mockProvider();
OpenFeature.setProvider(newProvider);

// The old provider should be unbound.
expect(getBoundApi(provider)).toBeUndefined();
expect(getBoundApi(newProvider)).toBe(OpenFeature);
});

it('should not unbind a provider that is still used by another domain', async () => {
const provider = { ...mockProvider(), onClose: jest.fn() };

await OpenFeature.setProviderAndWait('domain1', provider);
await OpenFeature.setProviderAndWait('domain2', provider);

const newProvider = mockProvider();
await OpenFeature.setProviderAndWait('domain1', newProvider);

expect(getBoundApi(provider)).toBe(OpenFeature);
expect(provider.onClose).not.toHaveBeenCalled();
});

it('should unbind all providers on clearProviders', async () => {
const provider1 = mockProvider();
const provider2 = mockProvider();

OpenFeature.setProvider(provider1);
OpenFeature.setProvider('domain1', provider2);

expect(getBoundApi(provider1)).toBe(OpenFeature);
expect(getBoundApi(provider2)).toBe(OpenFeature);

await OpenFeature.clearProviders();

expect(getBoundApi(provider1)).toBeUndefined();
expect(getBoundApi(provider2)).toBeUndefined();
});

it('should unbind all providers on close', async () => {
const provider1 = mockProvider();
const provider2 = mockProvider();

OpenFeature.setProvider(provider1);
OpenFeature.setProvider('domain1', provider2);

expect(getBoundApi(provider1)).toBe(OpenFeature);
expect(getBoundApi(provider2)).toBe(OpenFeature);

await OpenFeature.close();

expect(getBoundApi(provider1)).toBeUndefined();
expect(getBoundApi(provider2)).toBeUndefined();
});

it('should allow re-registering a provider after it has been unbound', async () => {
const provider = mockProvider();
OpenFeature.setProvider(provider);
expect(getBoundApi(provider)).toBe(OpenFeature);

await OpenFeature.clearProviders();
expect(getBoundApi(provider)).toBeUndefined();

expect(() => OpenFeature.setProvider(provider)).not.toThrow();
expect(getBoundApi(provider)).toBe(OpenFeature);
});
});
});