address review comments

Signed-off-by: Adam Wootton <[email protected]>

Author: ajwootto

libs/providers/multi-provider/src/lib/errors.ts

@@ -1,4 +1,6 @@
 import { ErrorCode, GeneralError, OpenFeatureError } from '@openfeature/server-sdk';
+import { PromiseAllSettledResult } from '@opentelemetry/sdk-metrics/build/esnext/utils';
+import { RegisteredProvider } from './types';
 
 export class ErrorWithCode extends OpenFeatureError {
   constructor(
@@ -31,3 +33,21 @@ export const constructAggregateError = (providerErrors: { error: unknown; provid
     errorsWithSource,
   );
 };
+
+export const throwAggregateErrorFromPromiseResults = (
+  result: PromiseAllSettledResult<unknown>[],
+  providerEntries: RegisteredProvider[],
+) => {
+  const errors = result
+    .map((r, i) => {
+      if (r.status === 'rejected') {
+        return { error: r.reason, providerName: providerEntries[i].name };
+      }
+      return null;
+    })
+    .filter((val): val is { error: unknown; providerName: string } => Boolean(val));
+
+  if (errors.length) {
+    throw constructAggregateError(errors);
+  }
+};

libs/providers/multi-provider/src/lib/multi-provider.ts

@@ -10,25 +10,15 @@ import {
   Logger,
   OpenFeatureEventEmitter,
   Provider,
+  ProviderMetadata,
+  BeforeHookContext,
   ResolutionDetails,
 } from '@openfeature/server-sdk';
-import { BeforeHookContext, ProviderMetadata } from '@openfeature/core';
 import { HookExecutor } from './hook-executor';
-import { constructAggregateError } from './errors';
+import { constructAggregateError, throwAggregateErrorFromPromiseResults } from './errors';
 import { BaseEvaluationStrategy, ProviderResolutionResult, FirstMatchStrategy } from './strategies';
 import { StatusTracker } from './status-tracker';
-
-// Represents an entry in the constructor's provider array which may or may not have a name set
-export type ProviderEntryInput = {
-  provider: Provider;
-  name?: string;
-};
-
-// Represents a processed and "registered" provider entry where a name has been chosen
-export type RegisteredProvider = {
-  name: string;
-  provider: Provider;
-};
+import { ProviderEntryInput, RegisteredProvider } from './types';
 
 export class MultiProvider implements Provider {
   readonly runsOn = 'server';
@@ -76,7 +66,7 @@ export class MultiProvider implements Provider {
         throw new Error('Provider names must be unique');
       }
 
-      providersByName[candidateName] ||= [];
+      providersByName[candidateName] ??= [];
       providersByName[candidateName].push(constructorProvider.provider);
     }
 
@@ -96,11 +86,15 @@ export class MultiProvider implements Provider {
   }
 
   async initialize(context?: EvaluationContext): Promise<void> {
-    await Promise.all(this.providerEntries.map((provider) => provider.provider.initialize?.(context)));
+    const result = await Promise.allSettled(
+      this.providerEntries.map((provider) => provider.provider.initialize?.(context)),
+    );
+    throwAggregateErrorFromPromiseResults(result, this.providerEntries);
   }
 
   async onClose(): Promise<void> {
-    await Promise.all(this.providerEntries.map((provider) => provider.provider.onClose?.()));
+    const result = await Promise.allSettled(this.providerEntries.map((provider) => provider.provider.onClose?.()));
+    throwAggregateErrorFromPromiseResults(result, this.providerEntries);
   }
 
   resolveBooleanEvaluation(
@@ -145,11 +139,11 @@ export class MultiProvider implements Provider {
     const hookHints = this.hookHints.get(context);
 
     if (!hookContext || !hookHints) {
-      throw new GeneralError(`Hook context not available for evaluation`);
+      throw new GeneralError('Hook context not available for evaluation');
     }
 
-    let tasks: Promise<boolean>[] = [];
-    let resolutions: ProviderResolutionResult<T>[] = [];
+    const tasks: Promise<boolean>[] = [];
+    const resolutions: ProviderResolutionResult<T>[] = [];
 
     for (const providerEntry of this.providerEntries) {
       const task = this.evaluateProviderEntry(
@@ -232,11 +226,12 @@ export class MultiProvider implements Provider {
     }
 
     if (this.evaluationStrategy.runMode === 'sequential') {
-      if (
-        !this.evaluationStrategy.shouldEvaluateNextProvider(strategyContext, context, evaluationResult, thrownError)
-      ) {
-        return false;
-      }
+      return this.evaluationStrategy.shouldEvaluateNextProvider(
+        strategyContext,
+        context,
+        evaluationResult,
+        thrownError,
+      );
     }
     return true;
   }

libs/providers/multi-provider/src/lib/status-tracker.ts

@@ -1,5 +1,5 @@
 import { EventDetails, OpenFeatureEventEmitter, ProviderEvents, ProviderStatus } from '@openfeature/server-sdk';
-import { RegisteredProvider } from '@openfeature/multi-provider';
+import { RegisteredProvider } from './types';
 
 /**
  * Tracks each individual provider's status by listening to emitted events

libs/providers/multi-provider/src/lib/strategies/BaseEvaluationStrategy.ts

@@ -6,7 +6,7 @@ import {
   ProviderStatus,
   ResolutionDetails,
 } from '@openfeature/server-sdk';
-import { ErrorWithCode } from '@openfeature/multi-provider';
+import { ErrorWithCode } from '../errors';
 
 export type StrategyEvaluationContext = {
   flagKey: string;
@@ -18,12 +18,23 @@ export type StrategyPerProviderContext = StrategyEvaluationContext & {
   providerStatus: ProviderStatus;
 };
 
-export type ProviderResolutionResult<T extends FlagValue> = {
+type ProviderResolutionResultBase = {
   provider: Provider;
   providerName: string;
-  thrownError?: unknown;
-  details?: ResolutionDetails<T>;
 };
+
+export type ProviderResolutionSuccessResult<T extends FlagValue> = ProviderResolutionResultBase & {
+  details: ResolutionDetails<T>;
+};
+
+export type ProviderResolutionErrorResult = ProviderResolutionResultBase & {
+  thrownError: unknown;
+};
+
+export type ProviderResolutionResult<T extends FlagValue> =
+  | ProviderResolutionSuccessResult<T>
+  | ProviderResolutionErrorResult;
+
 export type FinalResult<T extends FlagValue> = {
   details?: ResolutionDetails<T>;
   provider?: Provider;
@@ -67,16 +78,15 @@ export abstract class BaseEvaluationStrategy {
   ): FinalResult<T>;
 
   protected hasError(resolution: ProviderResolutionResult<FlagValue>): boolean {
-    return 'thrownError' in resolution || !!resolution.details?.errorCode;
+    return 'thrownError' in resolution || !!resolution.details.errorCode;
   }
 
   protected collectProviderErrors<T extends FlagValue>(resolutions: ProviderResolutionResult<T>[]): FinalResult<T> {
-    let errors: FinalResult<FlagValue>['errors'] = [];
+    const errors: FinalResult<FlagValue>['errors'] = [];
     for (const resolution of resolutions) {
       if ('thrownError' in resolution) {
         errors.push({ providerName: resolution.providerName, error: resolution.thrownError });
-      }
-      if (resolution.details?.errorCode) {
+      } else if (resolution.details.errorCode) {
         errors.push({
           providerName: resolution.providerName,
           error: new ErrorWithCode(resolution.details.errorCode, resolution.details.errorMessage ?? 'unknown error'),
@@ -86,7 +96,7 @@ export abstract class BaseEvaluationStrategy {
     return { errors };
   }
 
-  protected resolutionToFinalResult<T extends FlagValue>(resolution: ProviderResolutionResult<T>) {
+  protected resolutionToFinalResult<T extends FlagValue>(resolution: ProviderResolutionSuccessResult<T>) {
     return { details: resolution.details, provider: resolution.provider, providerName: resolution.providerName };
   }
 }

libs/providers/multi-provider/src/lib/strategies/ComparisonStrategy.ts

@@ -2,6 +2,7 @@ import {
   BaseEvaluationStrategy,
   FinalResult,
   ProviderResolutionResult,
+  ProviderResolutionSuccessResult,
   StrategyPerProviderContext,
 } from './BaseEvaluationStrategy';
 import { EvaluationContext, FlagValue, Provider } from '@openfeature/server-sdk';
@@ -25,29 +26,29 @@ export class ComparisonStrategy extends BaseEvaluationStrategy {
   override determineFinalResult<T extends FlagValue>(
     strategyContext: StrategyPerProviderContext,
     context: EvaluationContext,
-    resolutions: ProviderResolutionResult<T>[],
+    resolutions: ProviderResolutionSuccessResult<T>[],
   ): FinalResult<T> {
     let value: T | undefined;
-    let fallbackResolution: ProviderResolutionResult<T> | undefined;
+    let fallbackResolution: ProviderResolutionSuccessResult<T> | undefined;
     let mismatch = false;
     for (const resolution of resolutions) {
-      if (resolution.thrownError || resolution.details?.errorCode) {
+      if ('thrownError' in resolution || resolution.details.errorCode) {
         return this.collectProviderErrors(resolutions);
       }
       if (resolution.provider === this.fallbackProvider) {
         fallbackResolution = resolution;
       }
-      if (typeof value !== 'undefined' && value !== resolution.details!.value) {
+      if (typeof value !== 'undefined' && value !== resolution.details.value) {
         mismatch = true;
       } else {
-        value = resolution.details!.value;
+        value = resolution.details.value;
       }
     }
 
     if (mismatch) {
       this.onMismatch?.(resolutions);
       return {
-        details: fallbackResolution!.details!,
+        details: fallbackResolution!.details,
         provider: fallbackResolution!.provider,
       };
     }

libs/providers/multi-provider/src/lib/strategies/FirstMatchStrategy.ts

@@ -32,7 +32,7 @@ export class FirstMatchStrategy extends BaseEvaluationStrategy {
     resolutions: ProviderResolutionResult<T>[],
   ): FinalResult<T> {
     const finalResolution = resolutions[resolutions.length - 1];
-    if (this.hasError(finalResolution)) {
+    if ('thrownError' in finalResolution || !!finalResolution.details.errorCode) {
       return this.collectProviderErrors(resolutions);
     }
     return this.resolutionToFinalResult(finalResolution);

libs/providers/multi-provider/src/lib/strategies/FirstSuccessfulStrategy.ts

@@ -28,7 +28,7 @@ export class FirstSuccessfulStrategy extends BaseEvaluationStrategy {
     resolutions: ProviderResolutionResult<T>[],
   ): FinalResult<T> {
     const finalResolution = resolutions[resolutions.length - 1];
-    if (this.hasError(finalResolution)) {
+    if ('thrownError' in finalResolution || !!finalResolution.details.errorCode) {
       return this.collectProviderErrors(resolutions);
     }
     return this.resolutionToFinalResult(finalResolution);

libs/providers/multi-provider/src/lib/types.ts

@@ -0,0 +1,10 @@
+// Represents an entry in the constructor's provider array which may or may not have a name set
+import { Provider } from '@openfeature/server-sdk';
+
+export type ProviderEntryInput = {
+  provider: Provider;
+  name?: string;
+};
+
+// Represents a processed and "registered" provider entry where a name has been chosen
+export type RegisteredProvider = Required<ProviderEntryInput>;