Additional hook tests, incl. functionality for error hooks & api hooks.

lib/src/main/java/javasdk/FlagEvaluationOptions.java

@@ -5,8 +5,10 @@ import lombok.Data;
 import lombok.Singular;
 
 import java.util.List;
+import java.util.Map;
 
 @Data @Builder
 public class FlagEvaluationOptions {
     @Singular private List<Hook> hooks;
+    private Map<String, Object> hookHints;
 }

lib/src/main/java/javasdk/Hook.java

@@ -5,5 +5,5 @@ public abstract class Hook<T> {
     void before(HookContext<T> ctx) {}
     void after(HookContext<T> ctx, FlagEvaluationDetails<T> details) {}
     void error(HookContext<T> ctx, Exception error) {}
-    void afterAll(HookContext<T> ctx) {}
+    void finallyAfter(HookContext<T> ctx) {}
 }

lib/src/main/java/javasdk/OpenFeatureAPI.java

@@ -4,10 +4,14 @@ import lombok.Getter;
 import lombok.Setter;
 
 import javax.annotation.Nullable;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
 
 public class OpenFeatureAPI {
     @Getter @Setter private FeatureProvider provider;
     private static OpenFeatureAPI api;
+    @Getter private List<Hook> apiHooks;
 
     public static OpenFeatureAPI getInstance() {
         synchronized (OpenFeatureAPI.class) {
@@ -18,6 +22,10 @@ public class OpenFeatureAPI {
         return api;
     }
 
+    public OpenFeatureAPI() {
+        this.apiHooks = new ArrayList<>();
+    }
+
     public Client getClient() {
         return getClient(null, null);
     }
@@ -30,4 +38,11 @@ public class OpenFeatureAPI {
         return new OpenFeatureClient(this, name, version);
     }
 
+    public void registerHooks(Hook... hooks) {
+        this.apiHooks.addAll(Arrays.asList(hooks));
+    }
+
+    public void clearHooks() {
+        this.apiHooks.clear();
+    }
 }

lib/src/main/java/javasdk/OpenFeatureClient.java

@@ -1,6 +1,7 @@
 package javasdk;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Lists;
 import javasdk.exceptions.GeneralError;
 import javasdk.exceptions.OpenFeatureError;
 import lombok.Getter;
@@ -44,6 +45,7 @@ public class OpenFeatureClient implements Client {
             mergedHooks = ImmutableList.<Hook>builder()
                     .addAll(options.getHooks())
                     .addAll(clientHooks)
+                    .addAll(openfeatureApi.getApiHooks())
                     .build();
         } else {
             mergedHooks = clientHooks;
@@ -75,6 +77,7 @@ public class OpenFeatureClient implements Client {
             } else {
                 details.errorCode = ErrorCode.GENERAL;
             }
+            this.errorHooks(hookCtx, e, mergedHooks);
         } finally {
             this.afterAllHooks(hookCtx, mergedHooks);
         }
@@ -82,9 +85,15 @@ public class OpenFeatureClient implements Client {
         return details;
     }
 
+    private void errorHooks(HookContext hookCtx, Exception e, List<Hook> hooks) {
+        for (Hook hook : hooks) {
+            hook.error(hookCtx, e);
+        }
+    }
+
     private void afterAllHooks(HookContext hookCtx, List<Hook> hooks) {
         for (Hook hook : hooks) {
-            hook.afterAll(hookCtx);
+            hook.finallyAfter(hookCtx);
         }
     }
 
@@ -95,7 +104,8 @@ public class OpenFeatureClient implements Client {
     }
 
     private HookContext beforeHooks(HookContext hookCtx, List<Hook> hooks) {
-        for (Hook hook : hooks) {
+        // These traverse backwards from normal.
+        for (Hook hook : Lists.reverse(hooks)) {
             hook.before(hookCtx);
             // TODO: Merge returned context w/ hook context object
         }

lib/src/test/java/javasdk/DeveloperExperienceTest.java

@@ -29,7 +29,7 @@ class DeveloperExperienceTest {
         Client client = api.getClient();
         client.registerHooks(exampleHook);
         Boolean retval = client.getBooleanValue(flagKey, false);
-        verify(exampleHook, times(1)).afterAll(any());
+        verify(exampleHook, times(1)).finallyAfter(any());
         assertFalse(retval);
     }
 
@@ -43,8 +43,8 @@ class DeveloperExperienceTest {
         client.registerHooks(clientHook);
         Boolean retval = client.getBooleanValue(flagKey, false, new EvaluationContext(),
                 FlagEvaluationOptions.builder().hook(evalHook).build());
-        verify(clientHook, times(1)).afterAll(any());
+        verify(clientHook, times(1)).finallyAfter(any());
-        verify(evalHook, times(1)).afterAll(any());
+        verify(evalHook, times(1)).finallyAfter(any());
         assertFalse(retval);
     }
 

lib/src/test/java/javasdk/FlagEvaluationSpecTests.java

@@ -75,16 +75,16 @@ public class FlagEvaluationSpecTests {
         String key = "key";
         assertFalse(c.getBooleanValue(key, false));
         assertFalse(c.getBooleanValue(key, false, new EvaluationContext()));
-        assertFalse(c.getBooleanValue(key, false, new EvaluationContext(), new FlagEvaluationOptions(null)));
+        assertFalse(c.getBooleanValue(key, false, new EvaluationContext(), FlagEvaluationOptions.builder().build()));
 
 
         assertEquals("my-string", c.getStringValue(key, "my-string"));
         assertEquals("my-string", c.getStringValue(key, "my-string", new EvaluationContext()));
-        assertEquals("my-string", c.getStringValue(key, "my-string", new EvaluationContext(), new FlagEvaluationOptions(null)));
+        assertEquals("my-string", c.getStringValue(key, "my-string", new EvaluationContext(), FlagEvaluationOptions.builder().build()));
 
         assertEquals(4, c.getIntegerValue(key, 4));
         assertEquals(4, c.getIntegerValue(key, 4, new EvaluationContext()));
-        assertEquals(4, c.getIntegerValue(key, 4, new EvaluationContext(), new FlagEvaluationOptions(null)));
+        assertEquals(4, c.getIntegerValue(key, 4, new EvaluationContext(), FlagEvaluationOptions.builder().build()));
 
     }
 

lib/src/test/java/javasdk/HookSpecTests.java

@@ -1,10 +1,25 @@
 package javasdk;
 
+import lombok.SneakyThrows;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 
-import static org.junit.jupiter.api.Assertions.fail;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+import static org.junit.jupiter.api.Assertions.*;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.*;
 
 public class HookSpecTests {
+    @AfterEach
+    void emptyApiHooks() {
+        // it's a singleton. Don't pollute each test.
+        OpenFeatureAPI.getInstance().clearHooks();
+    }
+
     @Specification(spec="hooks", number="1.3", text="flag key, flag type, default value properties MUST be immutable. If the language does not support immutability, the hook MUST NOT modify these properties.")
     @Test void immutableValues() {
         try {
@@ -122,31 +137,170 @@ public class HookSpecTests {
                 .build();
     }
 
-    @Specification(spec="hooks", number="1.4", text="The evaluation context MUST be mutable only within the before hook.")
-    @Specification(spec="hooks", number="2.1", text="HookHints MUST be a map of objects.")
-    @Specification(spec="hooks", number="2.2", text="Condition: HookHints MUST be immutable.")
-    @Specification(spec="hooks", number="3.1", text="Hooks MUST specify at least one stage.")
     @Specification(spec="hooks", number="3.2", text="The before stage MUST run before flag evaluation occurs. It accepts a hook context (required) and HookHints (optional) as parameters and returns either a HookContext or nothing.")
-    @Specification(spec="hooks", number="3.3", text="The after stage MUST run after flag evaluation occurs. It accepts a hook context (required), flag evaluation details (required) and HookHints (optional). It has no return value.")
+    @Test void before_runs_ahead_of_evaluation() {
+        OpenFeatureAPI api = OpenFeatureAPI.getInstance();
+        api.setProvider(new AlwaysBrokenProvider());
+        Client client = api.getClient();
+        Hook<Boolean> evalHook = (Hook<Boolean>) mock(Hook.class);
+
+        client.getBooleanValue("key", false, new EvaluationContext(),
+                FlagEvaluationOptions.builder().hook(evalHook).build());
+
+        verify(evalHook, times(1)).before(any());
+    }
+
+    @Specification(spec="hooks", number="5.1", text="Flag evalution options MUST contain a list of hooks to evaluate.")
+    @Test void feo_has_hook_list() {
+        FlagEvaluationOptions feo = FlagEvaluationOptions.builder()
+                .build();
+        assertNotNull(feo.getHooks());
+    }
+
+    @Specification(spec="hooks", number="4.3", text="If an error occurs in the finally hook, it MUST NOT trigger the error hook.")
+    @Test void errors_in_finally() {
+        Hook<Boolean> h = mock(Hook.class);
+        doThrow(RuntimeException.class).when(h).finallyAfter(any());
+
+        OpenFeatureAPI api = OpenFeatureAPI.getInstance();
+        api.setProvider(new NoOpProvider<>());
+        Client c= api.getClient();
+
+        assertThrows(RuntimeException.class, () -> c.getBooleanValue("key", false, null, FlagEvaluationOptions.builder().hook(h).build()));
+
+        verify(h, times(1)).finallyAfter(any());
+        verify(h, times(0)).error(any(), any());
+    }
+
     @Specification(spec="hooks", number="3.4", text="The error hook MUST run when errors are encountered in the before stage, the after stage or during flag evaluation. It accepts hook context (required), exception for what went wrong (required), and HookHints (optional). It has no return value.")
-    @Specification(spec="hooks", number="3.5", text="The finally hook MUST run after the before, after, and error stages. It accepts a hook context (required) and HookHints (optional). There is no return value.")
+    @Test void error_hook_run_during_non_finally_stage() {
-    @Specification(spec="hooks", number="3.6", text="Condition: If finally is a reserved word in the language, finallyAfter SHOULD be used.")
+        final boolean[] error_called = {false};
-    @Specification(spec="hooks", number="4.1", text="The API, Client and invocation MUST have a method for registering hooks which accepts flag evaluation options")
+        Hook h = mock(Hook.class);
+        doThrow(RuntimeException.class).when(h).finallyAfter(any());
+
+        verify(h, times(0)).error(any(), any());
+    }
     @Specification(spec="hooks", number="4.2", text="Hooks MUST be evaluated in the following order:" +
             "before: API, Client, Invocation" +
             "after: Invocation, Client, API" +
             "error (if applicable): Invocation, Client, API" +
             "finally: Invocation, Client, API")
-    @Specification(spec="hooks", number="4.3", text=" If an error occurs in the finally hook, it MUST NOT trigger the error hook.")
+    @Test void eval_order() {
+        List<String> evalOrder = new ArrayList<String>();
+        OpenFeatureAPI api = OpenFeatureAPI.getInstance();
+        api.setProvider(new NoOpProvider());
+        api.registerHooks(new Hook() {
+            @Override
+            void before(HookContext ctx) {
+                evalOrder.add("api before");
+            }
+
+            @Override
+            void after(HookContext ctx, FlagEvaluationDetails details) {
+                evalOrder.add("api after");
+                throw new RuntimeException(); // trigger error flows.
+            }
+
+            @Override
+            void error(HookContext ctx, Exception error) {
+                evalOrder.add("api error");
+            }
+
+            @Override
+            void finallyAfter(HookContext ctx) {
+                evalOrder.add("api finally");
+            }
+        });
+
+        Client c = api.getClient();
+        c.registerHooks(new Hook() {
+            @Override
+            void before(HookContext ctx) {
+                evalOrder.add("client before");
+            }
+
+            @Override
+            void after(HookContext ctx, FlagEvaluationDetails details) {
+                evalOrder.add("client after");
+            }
+
+            @Override
+            void error(HookContext ctx, Exception error) {
+                evalOrder.add("client error");
+            }
+
+            @Override
+            void finallyAfter(HookContext ctx) {
+                evalOrder.add("client finally");
+            }
+        });
+
+        c.getBooleanValue("key", false, null, FlagEvaluationOptions.builder()
+                        .hook(new Hook() {
+                            @Override
+                            void before(HookContext ctx) {
+                                evalOrder.add("invocation before");
+                            }
+
+                            @Override
+                            void after(HookContext ctx, FlagEvaluationDetails details) {
+                                evalOrder.add("invocation after");
+                            }
+
+                            @Override
+                            void error(HookContext ctx, Exception error) {
+                                evalOrder.add("invocation error");
+                            }
+
+                            @Override
+                            void finallyAfter(HookContext ctx) {
+                                evalOrder.add("invocation finally");
+                            }
+                        })
+                .build());
+
+        ArrayList<String> expectedOrder = new ArrayList<String>();
+        expectedOrder.addAll(Arrays.asList(
+                "api before", "client before", "invocation before",
+                "invocation after", "client after", "api after",
+                "invocation error", "client error", "api error",
+                "invocation finally", "client finally", "api finally"));
+        assertEquals(expectedOrder, evalOrder);
+    }
+
+    @Specification(spec="hooks", number="1.4", text="The evaluation context MUST be mutable only within the before hook.")
+    @Specification(spec="hooks", number="2.1", text="HookHints MUST be a map of objects.")
+    @Specification(spec="hooks", number="2.2", text="Condition: HookHints MUST be immutable.")
+    @Specification(spec="hooks", number="3.1", text="Hooks MUST specify at least one stage.")
+    @Specification(spec="hooks", number="3.3", text="The after stage MUST run after flag evaluation occurs. It accepts a hook context (required), flag evaluation details (required) and HookHints (optional). It has no return value.")
+    @Specification(spec="hooks", number="3.5", text="The finally hook MUST run after the before, after, and error stages. It accepts a hook context (required) and HookHints (optional). There is no return value.")
+    @Specification(spec="hooks", number="4.1", text="The API, Client and invocation MUST have a method for registering hooks which accepts flag evaluation options")
     @Specification(spec="hooks", number="4.4", text="If an error occurs in the before or after hooks, the error hooks MUST be invoked.")
     @Specification(spec="hooks", number="4.5", text="If an error occurs during the evaluation of before or after hooks, any remaining hooks in the before or after stages MUST NOT be invoked.")
     @Specification(spec="hooks", number="4.6", text="If an error is encountered in the error stage, it MUST NOT be returned to the user.")
-    @Specification(spec="hooks", number="5.1", text="Flag evalution options MUST contain a list of hooks to evaluate.")
     @Specification(spec="hooks", number="5.2", text="Flag evaluation options MAY contain HookHints, a map of data to be provided to hook invocations.")
     @Specification(spec="hooks", number="5.3", text="HookHints MUST be passed to each hook through a parameter. It is merged into the object in the precedence order API -> Client -> Invocation (last wins).")
     @Specification(spec="hooks", number="5.4", text="The hook MUST NOT alter the HookHints object.")
     @Specification(spec="hooks", number="6.1", text="HookHints MUST passed between each hook.")
     void todo() {}
 
+    @SneakyThrows
+    @Specification(spec="hooks", number="3.6", text="Condition: If finally is a reserved word in the language, finallyAfter SHOULD be used.")
+    @Disabled
+    @Test void doesnt_use_finally() {
+//        Class [] carr = new Class[1];
+//        carr[0] = HookContext.class;
+//
+//        try {
+//            Hook.class.getMethod("finally", carr);
+//            fail("Not possible. Finally is a reserved word.");
+//        } catch (NoSuchMethodException e) {
+//            // expected
+//        }
+
+        Hook.class.getMethod("finallyAfter", HookContext.class);
+
+    }
+
 
 }