Skip to content

Commit

Permalink
feat: add evaluation details to finally hook (#1087)
Browse files Browse the repository at this point in the history 
## This PR

- adds evaluation details to the `finally` stage in hooks.

### Notes

This breaks the signature of the `finally` stages based on [this spec
enhancement](open-feature/spec#280). It is
**not** considered a breaking change to the SDK because hooks are marked
as experimental in the spec, and the change has no impact on known
hooks.

The noteworthy change to the interface is:

```diff
- finally?(hookContext: Readonly<HookContext<T>>, hookHints?: HookHints): HooksReturn;
+ finally?(hookContext: Readonly<HookContext<T>>, evaluationDetails: EvaluationDetails<T>, hookHints?: HookHints): HooksReturn;
```

### Follow-up Tasks

- Update the JS contribs repo

---------

Signed-off-by: Michael Beemer <[email protected]>
  • Loading branch information
@beeme1mr
beeme1mr authored Dec 12, 2024
1 parent 5e5b160 commit 2135254
Show file tree
Hide file tree
Showing 5 changed files with 106 additions and 51 deletions.
42 changes: 26 additions & 16 deletions packages/server/src/client/internal/open-feature-client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ import type {
Logger,
TrackingEventDetails,
OpenFeatureError,
ResolutionDetails} from '@openfeature/core';
FlagMetadata,
ResolutionDetails,
} from '@openfeature/core';
import {
ErrorCode,
ProviderFatalError,
Expand All @@ -24,7 +26,7 @@ import type { FlagEvaluationOptions } from '../../evaluation';
import type { ProviderEvents } from '../../events';
import type { InternalEventEmitter } from '../../events/internal/internal-event-emitter';
import type { Hook } from '../../hooks';
import type { Provider} from '../../provider';
import type { Provider } from '../../provider';
import { ProviderStatus } from '../../provider';
import type { Client } from './../client';

Expand Down Expand Up @@ -279,6 +281,8 @@ export class OpenFeatureClient implements Client {
logger: this._logger,
};

let evaluationDetails: EvaluationDetails<T>;

try {
const frozenContext = await this.beforeHooks(allHooks, hookContext, options);

Expand All @@ -287,27 +291,27 @@ export class OpenFeatureClient implements Client {
// run the referenced resolver, binding the provider.
const resolution = await resolver.call(this._provider, flagKey, defaultValue, frozenContext, this._logger);

const evaluationDetails = {
const resolutionDetails = {
...resolution,
flagMetadata: Object.freeze(resolution.flagMetadata ?? {}),
flagKey,
};

if (evaluationDetails.errorCode) {
const err = instantiateErrorByErrorCode(evaluationDetails.errorCode);
if (resolutionDetails.errorCode) {
const err = instantiateErrorByErrorCode(resolutionDetails.errorCode);
await this.errorHooks(allHooksReversed, hookContext, err, options);
return this.getErrorEvaluationDetails(flagKey, defaultValue, err);
evaluationDetails = this.getErrorEvaluationDetails(flagKey, defaultValue, err, resolutionDetails.flagMetadata);
} else {
await this.afterHooks(allHooksReversed, hookContext, resolutionDetails, options);
evaluationDetails = resolutionDetails;
}

await this.afterHooks(allHooksReversed, hookContext, evaluationDetails, options);

return evaluationDetails;
} catch (err: unknown) {
await this.errorHooks(allHooksReversed, hookContext, err, options);
return this.getErrorEvaluationDetails(flagKey, defaultValue, err);
} finally {
await this.finallyHooks(allHooksReversed, hookContext, options);
evaluationDetails = this.getErrorEvaluationDetails(flagKey, defaultValue, err);
}

await this.finallyHooks(allHooksReversed, hookContext, evaluationDetails, options);
return evaluationDetails;
}

private async beforeHooks(hooks: Hook[], hookContext: HookContext, options: FlagEvaluationOptions) {
Expand Down Expand Up @@ -353,11 +357,16 @@ export class OpenFeatureClient implements Client {
}
}

private async finallyHooks(hooks: Hook[], hookContext: HookContext, options: FlagEvaluationOptions) {
private async finallyHooks(
hooks: Hook[],
hookContext: HookContext,
evaluationDetails: EvaluationDetails<FlagValue>,
options: FlagEvaluationOptions,
) {
// run "finally" hooks sequentially
for (const hook of hooks) {
try {
await hook?.finally?.(hookContext, options.hookHints);
await hook?.finally?.(hookContext, evaluationDetails, options.hookHints);
} catch (err) {
this._logger.error(`Unhandled error during 'finally' hook: ${err}`);
if (err instanceof Error) {
Expand Down Expand Up @@ -403,6 +412,7 @@ export class OpenFeatureClient implements Client {
flagKey: string,
defaultValue: T,
err: unknown,
flagMetadata: FlagMetadata = {},
): EvaluationDetails<T> {
const errorMessage: string = (err as Error)?.message;
const errorCode: ErrorCode = (err as OpenFeatureError)?.code || ErrorCode.GENERAL;
Expand All @@ -412,7 +422,7 @@ export class OpenFeatureClient implements Client {
errorMessage,
value: defaultValue,
reason: StandardResolutionReasons.ERROR,
flagMetadata: Object.freeze({}),
flagMetadata: Object.freeze(flagMetadata),
flagKey,
};
}
Expand Down
28 changes: 26 additions & 2 deletions packages/server/test/hooks.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,30 @@ describe('Hooks', () => {
});
});
});

describe('Requirement 4.3.8', () => {
it('"evaluation details" passed to the "finally" stage matches the evaluation details returned to the application author', async () => {
OpenFeature.setProvider(MOCK_PROVIDER);
let evaluationDetailsHooks;

const evaluationDetails = await client.getBooleanDetails(
FLAG_KEY,
false,
{},
{
hooks: [
{
finally: (_, details) => {
evaluationDetailsHooks = details;
},
},
],
},
);

expect(evaluationDetailsHooks).toEqual(evaluationDetails);
});
});
});

describe('Requirement 4.4.2', () => {
Expand Down Expand Up @@ -922,14 +946,14 @@ describe('Hooks', () => {
done(err);
}
},
after: (_hookContext, _evaluationDetils, hookHints) => {
after: (_hookContext, _evaluationDetails, hookHints) => {
try {
expect(hookHints?.hint).toBeTruthy();
} catch (err) {
done(err);
}
},
finally: (_, hookHints) => {
finally: (_, _evaluationDetails, hookHints) => {
try {
expect(hookHints?.hint).toBeTruthy();
done();
Expand Down
8 changes: 6 additions & 2 deletions packages/shared/src/hooks/hook.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,13 @@ export interface BaseHook<T extends FlagValue = FlagValue, BeforeHookReturn = un

/**
* Runs after all other hook stages, regardless of success or error.
* Errors thrown here are unhandled by the client and will surface in application code.
* @param hookContext
* @param evaluationDetails
* @param hookHints
*/
finally?(hookContext: Readonly<HookContext<T>>, hookHints?: HookHints): HooksReturn;
finally?(
hookContext: Readonly<HookContext<T>>,
evaluationDetails: EvaluationDetails<T>,
hookHints?: HookHints,
): HooksReturn;
}
41 changes: 25 additions & 16 deletions packages/web/src/client/internal/open-feature-client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ import type {
Logger,
TrackingEventDetails,
OpenFeatureError,
ResolutionDetails } from '@openfeature/core';
FlagMetadata,
ResolutionDetails,
} from '@openfeature/core';
import {
ErrorCode,
ProviderFatalError,
Expand All @@ -24,7 +26,7 @@ import type { FlagEvaluationOptions } from '../../evaluation';
import type { ProviderEvents } from '../../events';
import type { InternalEventEmitter } from '../../events/internal/internal-event-emitter';
import type { Hook } from '../../hooks';
import type { Provider} from '../../provider';
import type { Provider } from '../../provider';
import { ProviderStatus } from '../../provider';
import type { Client } from './../client';

Expand Down Expand Up @@ -234,6 +236,8 @@ export class OpenFeatureClient implements Client {
logger: this._logger,
};

let evaluationDetails: EvaluationDetails<T>;

try {
this.beforeHooks(allHooks, hookContext, options);

Expand All @@ -242,27 +246,26 @@ export class OpenFeatureClient implements Client {
// run the referenced resolver, binding the provider.
const resolution = resolver.call(this._provider, flagKey, defaultValue, context, this._logger);

const evaluationDetails = {
const resolutionDetails = {
...resolution,
flagMetadata: Object.freeze(resolution.flagMetadata ?? {}),
flagKey,
};

if (evaluationDetails.errorCode) {
const err = instantiateErrorByErrorCode(evaluationDetails.errorCode);
if (resolutionDetails.errorCode) {
const err = instantiateErrorByErrorCode(resolutionDetails.errorCode);
this.errorHooks(allHooksReversed, hookContext, err, options);
return this.getErrorEvaluationDetails(flagKey, defaultValue, err);
evaluationDetails = this.getErrorEvaluationDetails(flagKey, defaultValue, err, resolutionDetails.flagMetadata);
} else {
this.afterHooks(allHooksReversed, hookContext, resolutionDetails, options);
evaluationDetails = resolutionDetails;
}

this.afterHooks(allHooksReversed, hookContext, evaluationDetails, options);

return evaluationDetails;
} catch (err: unknown) {
this.errorHooks(allHooksReversed, hookContext, err, options);
return this.getErrorEvaluationDetails(flagKey, defaultValue, err);
} finally {
this.finallyHooks(allHooksReversed, hookContext, options);
evaluationDetails = this.getErrorEvaluationDetails(flagKey, defaultValue, err);
}
this.finallyHooks(allHooksReversed, hookContext, evaluationDetails, options);
return evaluationDetails;
}

private beforeHooks(hooks: Hook[], hookContext: HookContext, options: FlagEvaluationOptions) {
Expand Down Expand Up @@ -301,11 +304,16 @@ export class OpenFeatureClient implements Client {
}
}

private finallyHooks(hooks: Hook[], hookContext: HookContext, options: FlagEvaluationOptions) {
private finallyHooks(
hooks: Hook[],
hookContext: HookContext,
evaluationDetails: EvaluationDetails<FlagValue>,
options: FlagEvaluationOptions,
) {
// run "finally" hooks sequentially
for (const hook of hooks) {
try {
hook?.finally?.(hookContext, options.hookHints);
hook?.finally?.(hookContext, evaluationDetails, options.hookHints);
} catch (err) {
this._logger.error(`Unhandled error during 'finally' hook: ${err}`);
if (err instanceof Error) {
Expand Down Expand Up @@ -337,6 +345,7 @@ export class OpenFeatureClient implements Client {
flagKey: string,
defaultValue: T,
err: unknown,
flagMetadata: FlagMetadata = {},
): EvaluationDetails<T> {
const errorMessage: string = (err as Error)?.message;
const errorCode: ErrorCode = (err as OpenFeatureError)?.code || ErrorCode.GENERAL;
Expand All @@ -346,7 +355,7 @@ export class OpenFeatureClient implements Client {
errorMessage,
value: defaultValue,
reason: StandardResolutionReasons.ERROR,
flagMetadata: Object.freeze({}),
flagMetadata: Object.freeze(flagMetadata),
flagKey,
};
}
Expand Down
38 changes: 23 additions & 15 deletions packages/web/test/hooks.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -1,16 +1,5 @@
import type {
Provider,
ResolutionDetails,
Client,
FlagValueType,
EvaluationContext,
Hook} from '../src';
import {
GeneralError,
OpenFeature,
StandardResolutionReasons,
ErrorCode,
} from '../src';
import type { Provider, ResolutionDetails, Client, FlagValueType, EvaluationContext, Hook } from '../src';
import { GeneralError, OpenFeature, StandardResolutionReasons, ErrorCode } from '../src';

const BOOLEAN_VALUE = true;

Expand Down Expand Up @@ -282,6 +271,25 @@ describe('Hooks', () => {
});
});
});

describe('Requirement 4.3.8', () => {
it('"evaluation details" passed to the "finally" stage matches the evaluation details returned to the application author', () => {
OpenFeature.setProvider(MOCK_PROVIDER);
let evaluationDetailsHooks;

const evaluationDetails = client.getBooleanDetails(FLAG_KEY, false, {
hooks: [
{
finally: (_, details) => {
evaluationDetailsHooks = details;
},
},
],
});

expect(evaluationDetailsHooks).toEqual(evaluationDetails);
});
});
});

describe('Requirement 4.4.2', () => {
Expand Down Expand Up @@ -759,14 +767,14 @@ describe('Hooks', () => {
done(err);
}
},
after: (_hookContext, _evaluationDetils, hookHints) => {
after: (_hookContext, _evaluationDetails, hookHints) => {
try {
expect(hookHints?.hint).toBeTruthy();
} catch (err) {
done(err);
}
},
finally: (_, hookHints) => {
finally: (_, _evaluationDetails, hookHints) => {
try {
expect(hookHints?.hint).toBeTruthy();
done();
Expand Down

0 comments on commit 2135254

Please sign in to comment.