fix: ⚠️ correct execution order for the same level hooks (#458)

* fix: correct execution order for the same level hooks

Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>

* Update openfeature/client.go

Co-authored-by: Sahid Velji <sahidvelji@gmail.com>
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>

---------

Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Co-authored-by: Sahid Velji <sahidvelji@gmail.com>

Author: erka

openfeature/client.go

@@ -692,9 +692,8 @@ func (c *Client) evaluate(
 	// ensure that the same provider & hooks are used across this transaction to avoid unexpected behaviour
 	provider, globalHooks, globalEvalCtx := c.api.ForEvaluation(c.metadata.domain)
 
-	evalCtx = mergeContexts(evalCtx, c.evaluationContext, TransactionContext(ctx), globalEvalCtx)            // API (global) -> transaction -> client -> invocation
-	apiClientInvocationProviderHooks := slices.Concat(globalHooks, c.hooks, options.hooks, provider.Hooks()) // API, Client, Invocation, Provider
-	providerInvocationClientAPIHooks := slices.Concat(provider.Hooks(), options.hooks, c.hooks, globalHooks) // Provider, Invocation, Client, API
+	evalCtx = mergeContexts(evalCtx, c.evaluationContext, TransactionContext(ctx), globalEvalCtx) // API (global) -> transaction -> client -> invocation
+	hooks := slices.Concat(globalHooks, c.hooks, options.hooks, provider.Hooks())                 // API, Client, Invocation, Provider
 
 	var err error
 	hookCtx := HookContext{
@@ -707,29 +706,29 @@ func (c *Client) evaluate(
 	}
 
 	defer func() {
-		c.finallyHooks(ctx, hookCtx, providerInvocationClientAPIHooks, evalDetails, options)
+		c.finallyHooks(ctx, hookCtx, hooks, evalDetails, options)
 	}()
 
 	// bypass short-circuit logic for the Noop provider; it is essentially stateless and a "special case"
 	if _, ok := provider.(NoopProvider); !ok {
 		// short circuit if provider is in NOT READY state
 		if c.State() == NotReadyState {
-			c.errorHooks(ctx, hookCtx, providerInvocationClientAPIHooks, ProviderNotReadyError, options)
+			c.errorHooks(ctx, hookCtx, hooks, ProviderNotReadyError, options)
 			return evalDetails, ProviderNotReadyError
 		}
 
 		// short circuit if provider is in FATAL state
 		if c.State() == FatalState {
-			c.errorHooks(ctx, hookCtx, providerInvocationClientAPIHooks, ProviderFatalError, options)
+			c.errorHooks(ctx, hookCtx, hooks, ProviderFatalError, options)
 			return evalDetails, ProviderFatalError
 		}
 	}
 
-	evalCtx, err = c.beforeHooks(ctx, hookCtx, apiClientInvocationProviderHooks, evalCtx, options)
+	evalCtx, err = c.beforeHooks(ctx, hookCtx, hooks, evalCtx, options)
 	hookCtx.evaluationContext = evalCtx
 	if err != nil {
 		err = fmt.Errorf("before hook: %w", err)
-		c.errorHooks(ctx, hookCtx, providerInvocationClientAPIHooks, err, options)
+		c.errorHooks(ctx, hookCtx, hooks, err, options)
 		return evalDetails, err
 	}
 
@@ -763,17 +762,17 @@ func (c *Client) evaluate(
 	err = resolution.Error()
 	if err != nil {
 		err = fmt.Errorf("error code: %w", err)
-		c.errorHooks(ctx, hookCtx, providerInvocationClientAPIHooks, err, options)
+		c.errorHooks(ctx, hookCtx, hooks, err, options)
 		evalDetails.ResolutionDetail = resolution.ResolutionDetail()
 		evalDetails.Reason = ErrorReason
 		return evalDetails, err
 	}
 	evalDetails.Value = resolution.Value
 	evalDetails.ResolutionDetail = resolution.ResolutionDetail()
 
-	if err := c.afterHooks(ctx, hookCtx, providerInvocationClientAPIHooks, evalDetails, options); err != nil {
+	if err := c.afterHooks(ctx, hookCtx, hooks, evalDetails, options); err != nil {
 		err = fmt.Errorf("after hook: %w", err)
-		c.errorHooks(ctx, hookCtx, providerInvocationClientAPIHooks, err, options)
+		c.errorHooks(ctx, hookCtx, hooks, err, options)
 		return evalDetails, err
 	}
 
@@ -791,6 +790,8 @@ func flattenContext(evalCtx EvaluationContext) FlattenedContext {
 	return flatCtx
 }
 
+// beforeHooks executes the Before hook for each hook in the collection.
+// Hooks are executed in forward order: API → Client → Invocation → Provider.
 func (c *Client) beforeHooks(
 	ctx context.Context, hookCtx HookContext, hooks []Hook, evalCtx EvaluationContext, options EvaluationOptions,
 ) (EvaluationContext, error) {
@@ -807,10 +808,13 @@ func (c *Client) beforeHooks(
 	return mergeContexts(hookCtx.evaluationContext, evalCtx), nil
 }
 
+// afterHooks executes the After hook for each hook in the collection.
+// Hooks are executed in reverse order: Provider → Invocation → Client → API.
 func (c *Client) afterHooks(
 	ctx context.Context, hookCtx HookContext, hooks []Hook, evalDetails InterfaceEvaluationDetails, options EvaluationOptions,
 ) error {
-	for _, hook := range hooks {
+	// reverse order
+	for _, hook := range slices.Backward(hooks) {
 		if err := hook.After(ctx, hookCtx, evalDetails, options.hookHints); err != nil {
 			return err
 		}
@@ -819,14 +823,20 @@ func (c *Client) afterHooks(
 	return nil
 }
 
+// errorHooks executes the Error hook for each hook in the collection.
+// Hooks are executed in reverse order: Provider → Invocation → Client → API.
 func (c *Client) errorHooks(ctx context.Context, hookCtx HookContext, hooks []Hook, err error, options EvaluationOptions) {
-	for _, hook := range hooks {
+	// reverse order
+	for _, hook := range slices.Backward(hooks) {
 		hook.Error(ctx, hookCtx, err, options.hookHints)
 	}
 }
 
+// finallyHooks executes the Finally hook for each hook in the collection.
+// Hooks are executed in reverse order: Provider → Invocation → Client → API.
 func (c *Client) finallyHooks(ctx context.Context, hookCtx HookContext, hooks []Hook, evalDetails InterfaceEvaluationDetails, options EvaluationOptions) {
-	for _, hook := range hooks {
+	// reverse order
+	for _, hook := range slices.Backward(hooks) {
 		hook.Finally(ctx, hookCtx, evalDetails, options.hookHints)
 	}
 }

openfeature/hooks_test.go

@@ -652,7 +652,7 @@ func TestRequirement_4_4_2(t *testing.T) {
 			t.Errorf("error setting up provider %v", err)
 		}
 
-		mockProvider.EXPECT().Hooks().Return([]Hook{mockProviderHook}).Times(2)
+		mockProvider.EXPECT().Hooks().Return([]Hook{mockProviderHook}).Times(1)
 
 		client := NewClient(t.Name())
 		client.AddHooks(mockClientHook)
@@ -704,7 +704,7 @@ func TestRequirement_4_4_2(t *testing.T) {
 		client := NewClient(t.Name())
 		client.AddHooks(mockClientHook)
 
-		mockProvider.EXPECT().Hooks().Return([]Hook{mockProviderHook}).Times(2)
+		mockProvider.EXPECT().Hooks().Return([]Hook{mockProviderHook}).Times(1)
 
 		mockAPIHook.EXPECT().Before(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, errors.New("forced"))
 
@@ -808,8 +808,8 @@ func TestRequirement_4_4_6(t *testing.T) {
 
 			mockHook1.EXPECT().Before(gomock.Any(), gomock.Any(), gomock.Any())
 			mockHook2.EXPECT().Before(gomock.Any(), gomock.Any(), gomock.Any())
-			mockHook1.EXPECT().After(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(errors.New("forced"))
-			// the lack of mockHook2.EXPECT().After() asserts that remaining hooks aren't invoked after an error
+			mockHook2.EXPECT().After(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(errors.New("forced"))
+			// the lack of mockHook1.EXPECT().After() asserts that remaining hooks aren't invoked after an error
 			mockHook1.EXPECT().Error(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
 			mockHook1.EXPECT().Finally(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
 			mockHook2.EXPECT().Error(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())