refactor: Update finally hook signature to be compliant to Requirement 4.3.8 of OpenFeature specification

MaximFischuk · MaximFischuk · commit 031f9c517b23 · 2024-12-19T13:28:25.000+02:00
diff --git a/examples/hooks.rs b/examples/hooks.rs
@@ -114,7 +114,12 @@ impl Hook for DummyLoggingHook {
         );
     }
 
-    async fn finally<'a>(&self, context: &HookContext<'a>, _hints: Option<&'a HookHints>) {
+    async fn finally<'a>(
+        &self,
+        context: &HookContext<'a>,
+        _: &EvaluationDetails<Value>,
+        _hints: Option<&'a HookHints>,
+    ) {
         log::info!(
             "Finally({}) evaluating flag {} of type {}",
             self.0,
diff --git a/src/api/client.rs b/src/api/client.rs
@@ -338,11 +338,21 @@ impl Client {
             .await;
         hook_context.evaluation_context = &context;
 
+        // INFO: Result of the resolution or error reson with default value
+        // This bind is defined here to minimize cloning of the `Value`
+        let evaluation_details;
+
         if let Err(error) = result {
             self.error_hooks(after_hooks.clone(), &hook_context, &error, hints)
                 .await;
-            self.finally_hooks(after_hooks.into_iter(), &hook_context, hints)
-                .await;
+            evaluation_details = EvaluationDetails::error_reason(flag_key, T::default());
+            self.finally_hooks(
+                after_hooks.into_iter(),
+                &hook_context,
+                &evaluation_details,
+                hints,
+            )
+            .await;
 
             return Err(error);
         }
@@ -360,18 +370,27 @@ impl Client {
                     .after_hooks(after_hooks.clone(), &hook_context, &details, hints)
                     .await
                 {
+                    evaluation_details = EvaluationDetails::error_reason(flag_key, T::default());
                     self.error_hooks(after_hooks.clone(), &hook_context, &error, hints)
                         .await;
+                } else {
+                    evaluation_details = details;
                 }
             }
             Err(ref error) => {
+                evaluation_details = EvaluationDetails::error_reason(flag_key, T::default());
                 self.error_hooks(after_hooks.clone(), &hook_context, error, hints)
                     .await;
             }
         }
 
-        self.finally_hooks(after_hooks.into_iter(), &hook_context, hints)
-            .await;
+        self.finally_hooks(
+            after_hooks.into_iter(),
+            &hook_context,
+            &evaluation_details,
+            hints,
+        )
+        .await;
 
         result
     }
@@ -441,12 +460,13 @@ impl Client {
         &self,
         hooks: I,
         hook_context: &HookContext<'_>,
+        evaluation_details: &EvaluationDetails<Value>,
         hints: Option<&HookHints>,
     ) where
         I: Iterator<Item = &'a HookWrapper>,
     {
         for hook in hooks {
-            hook.finally(hook_context, hints).await;
+            hook.finally(hook_context, evaluation_details, hints).await;
         }
     }
 }
diff --git a/src/evaluation/details.rs b/src/evaluation/details.rs
@@ -33,6 +33,19 @@ pub struct EvaluationDetails<T> {
     pub flag_metadata: FlagMetadata,
 }
 
+impl EvaluationDetails<Value> {
+    /// Creates a new `EvaluationDetails` instance with an error reason.
+    pub fn error_reason(flag_key: impl Into<String>, value: impl Into<Value>) -> Self {
+        Self {
+            value: value.into(),
+            flag_key: flag_key.into(),
+            reason: Some(EvaluationReason::Error),
+            variant: None,
+            flag_metadata: FlagMetadata::default(),
+        }
+    }
+}
+
 impl<T> EvaluationDetails<T>
 where
     T: Into<Value>,
diff --git a/src/hooks/logging.rs b/src/hooks/logging.rs
@@ -34,7 +34,12 @@ impl Hook for LoggingHook {
     ) {
         log::error!("Error hook for flag {}: {:?}", context.flag_key, error);
     }
-    async fn finally<'a>(&self, context: &HookContext<'a>, _: Option<&'a HookHints>) {
+    async fn finally<'a>(
+        &self,
+        context: &HookContext<'a>,
+        _: &EvaluationDetails<Value>,
+        _: Option<&'a HookHints>,
+    ) {
         log::trace!("Finally hook for flag {}", context.flag_key);
     }
 }
diff --git a/src/hooks/mod.rs b/src/hooks/mod.rs
@@ -44,7 +44,12 @@ pub trait Hook: Send + Sync + 'static {
     );
 
     /// This method is called after the flag evaluation, regardless of the result.
-    async fn finally<'a>(&self, context: &HookContext<'a>, hints: Option<&'a HookHints>);
+    async fn finally<'a>(
+        &self,
+        context: &HookContext<'a>,
+        evaluation_details: &EvaluationDetails<Value>,
+        hints: Option<&'a HookHints>,
+    );
 }
 
 // ============================================================
@@ -102,7 +107,7 @@ mod tests {
 
     use crate::{
         provider::{MockFeatureProvider, ResolutionDetails},
-        EvaluationErrorCode, EvaluationOptions, OpenFeature, StructValue,
+        EvaluationErrorCode, EvaluationOptions, EvaluationReason, OpenFeature, StructValue,
     };
 
     use super::*;
@@ -437,7 +442,22 @@ mod tests {
                 .in_sequence(&mut seq)
                 .return_const(());
 
-            mock_hook.expect_finally().return_const(());
+            mock_hook
+                .expect_finally()
+                .withf(|ctx, details, _| {
+                    assert_eq!(ctx.flag_key, "flag");
+                    assert_eq!(ctx.flag_type, Type::Bool);
+                    assert_eq!(
+                        ctx.evaluation_context,
+                        &EvaluationContext::default().with_custom_field("is", "a test")
+                    );
+                    assert_eq!(ctx.default_value, Some(Value::Bool(false)));
+                    assert_eq!(details.flag_key, "flag");
+                    assert_eq!(details.value, Value::Bool(false));
+                    assert_eq!(details.reason, Some(EvaluationReason::Error));
+                    true
+                })
+                .return_const(());
 
             // evaluation
             client = client.with_hook(mock_hook);
@@ -531,6 +551,18 @@ mod tests {
             .expect_finally()
             .once()
             .in_sequence(&mut seq)
+            .withf(|ctx, details, _| {
+                assert_eq!(ctx.flag_key, "flag");
+                assert_eq!(ctx.flag_type, Type::Bool);
+                assert_eq!(
+                    ctx.evaluation_context,
+                    &EvaluationContext::default().with_custom_field("is", "a test")
+                );
+                assert_eq!(ctx.default_value, Some(Value::Bool(false)));
+                assert_eq!(details.flag_key, "flag");
+                assert_eq!(details.value, Value::Bool(true));
+                true
+            })
             .return_const(());
 
         // evaluation
@@ -621,22 +653,22 @@ mod tests {
             .expect_finally()
             .once()
             .in_sequence(&mut seq)
-            .returning(|_, _| {});
+            .returning(|_, _, _| {});
         mock_invocation_hook
             .expect_finally()
             .once()
             .in_sequence(&mut seq)
-            .returning(|_, _| {});
+            .returning(|_, _, _| {});
         mock_client_hook
             .expect_finally()
             .once()
             .in_sequence(&mut seq)
-            .returning(|_, _| {});
+            .returning(|_, _, _| {});
         mock_api_hook
             .expect_finally()
             .once()
             .in_sequence(&mut seq)
-            .returning(|_, _| {});
+            .returning(|_, _, _| {});
 
         provider
             .expect_hooks()
@@ -779,22 +811,22 @@ mod tests {
             .expect_finally()
             .once()
             .in_sequence(&mut seq)
-            .returning(|_, _| {});
+            .returning(|_, _, _| {});
         mock_invocation_hook
             .expect_finally()
             .once()
             .in_sequence(&mut seq)
-            .returning(|_, _| {});
+            .returning(|_, _, _| {});
         mock_client_hook
             .expect_finally()
             .once()
             .in_sequence(&mut seq)
-            .returning(|_, _| {});
+            .returning(|_, _, _| {});
         mock_api_hook
             .expect_finally()
             .once()
             .in_sequence(&mut seq)
-            .returning(|_, _| {});
+            .returning(|_, _, _| {});
 
         provider
             .expect_hooks()

Original file line number	Diff line number	Diff line change
`@@ -34,7 +34,12 @@ impl Hook for LoggingHook {`
`34`	`34`	`) {`
`35`	`35`	`log::error!("Error hook for flag {}: {:?}", context.flag_key, error);`
`36`	`36`	`}`
`37`		`- async fn finally<'a>(&self, context: &HookContext<'a>, _: Option<&'a HookHints>) {`
	`37`	`+ async fn finally<'a>(`
	`38`	`+ &self,`
	`39`	`+ context: &HookContext<'a>,`
	`40`	`+ _: &EvaluationDetails<Value>,`
	`41`	`+ _: Option<&'a HookHints>,`
	`42`	`+ ) {`
`38`	`43`	`log::trace!("Finally hook for flag {}", context.flag_key);`
`39`	`44`	`}`
`40`	`45`	`}`