feat: error resolution flow control without exceptions (#1095)

* Allowing flowcontrol with out exceptions So far we used exception to handle our flowcontrol, but Exceptions are costly. In the end we enriched our evaluation Details with errorCode and errorMessage. This can be also handled by the providers if desired, to reduce the execution footprint in errornous cases, which do not have to be exceptions. Eg FlagNotFound - it might be the case, but in performance critical environments, an exception rather than a normal return, can cause overhead and can be already too costly. Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com> * fix: adding reason, and removing stacktraces from errors Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com> * Update src/main/java/dev/openfeature/sdk/exceptions/TypeMismatchError.java Co-authored-by: Justin Abrahms <justin@abrah.ms> Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com> --------- Signed-off-by: Simon Schrottner <simon.schrottner@dynatrace.com> Co-authored-by: Justin Abrahms <justin@abrah.ms>
open-feature · Sep 17, 2024 · 6fc0b90 · 6fc0b90
1 parent 29901b8
commit 6fc0b90
Show file tree

Hide file tree

Showing 7 changed files with 110 additions and 16 deletions.
diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java
@@ -21,7 +21,7 @@
  * You should not instantiate this or reference this class.
  * Use the dev.openfeature.sdk.Client interface instead.
  * @see Client
- * 
+ *
  * @deprecated // TODO: eventually we will make this non-public. See issue #872
  */
 @Slf4j
@@ -132,7 +132,11 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(FlagValueType type, String key
 
             details = FlagEvaluationDetails.from(providerEval, key);
             if (details.getErrorCode() != null) {
-                throw ExceptionUtils.instantiateErrorByErrorCode(details.getErrorCode(), details.getErrorMessage());
+                OpenFeatureError error = ExceptionUtils.instantiateErrorByErrorCode(
+                        details.getErrorCode(),
+                        details.getErrorMessage());
+                enrichDetailsWithErrorDefaults(defaultValue, details);
+                hookSupport.errorHooks(type, afterHookContext, error, mergedHooks, hints);
             } else {
                 hookSupport.afterHooks(type, afterHookContext, details, mergedHooks, hints);
             }
@@ -146,8 +150,7 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(FlagValueType type, String key
                 details.setErrorCode(ErrorCode.GENERAL);
             }
             details.setErrorMessage(e.getMessage());
-            details.setValue(defaultValue);
-            details.setReason(Reason.ERROR.toString());
+            enrichDetailsWithErrorDefaults(defaultValue, details);
             hookSupport.errorHooks(type, afterHookContext, e, mergedHooks, hints);
         } finally {
             hookSupport.afterAllHooks(type, afterHookContext, mergedHooks, hints);
@@ -156,6 +159,11 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(FlagValueType type, String key
         return details;
     }
 
+    private static <T> void enrichDetailsWithErrorDefaults(T defaultValue, FlagEvaluationDetails<T> details) {
+        details.setValue(defaultValue);
+        details.setReason(Reason.ERROR.toString());
+    }
+
     /**
      * Merge invocation contexts with API, transaction and client contexts.
      * Does not merge before context.

diff --git a/src/main/java/dev/openfeature/sdk/exceptions/FlagNotFoundError.java b/src/main/java/dev/openfeature/sdk/exceptions/FlagNotFoundError.java
@@ -4,14 +4,11 @@
 import lombok.Getter;
 import lombok.experimental.StandardException;
 
-@SuppressWarnings("checkstyle:MissingJavadocType")
+@SuppressWarnings({"checkstyle:MissingJavadocType", "squid:S110"})
 @StandardException
-public class FlagNotFoundError extends OpenFeatureError {
+public class FlagNotFoundError extends OpenFeatureErrorWithoutStacktrace {
     private static final long serialVersionUID = 1L;
-    @Getter private final ErrorCode errorCode = ErrorCode.FLAG_NOT_FOUND;
+    @Getter
+    private final ErrorCode errorCode = ErrorCode.FLAG_NOT_FOUND;
 
-    @Override
-    public synchronized Throwable fillInStackTrace() {
-        return this;
-    }
 }
diff --git a/src/main/java/dev/openfeature/sdk/exceptions/OpenFeatureErrorWithoutStacktrace.java b/src/main/java/dev/openfeature/sdk/exceptions/OpenFeatureErrorWithoutStacktrace.java
@@ -0,0 +1,14 @@
+package dev.openfeature.sdk.exceptions;
+
+import lombok.experimental.StandardException;
+
+@SuppressWarnings("checkstyle:MissingJavadocType")
+@StandardException
+public abstract class OpenFeatureErrorWithoutStacktrace extends OpenFeatureError {
+    private static final long serialVersionUID = 1L;
+
+    @Override
+    public synchronized Throwable fillInStackTrace() {
+        return this;
+    }
+}
diff --git a/src/main/java/dev/openfeature/sdk/exceptions/ProviderNotReadyError.java b/src/main/java/dev/openfeature/sdk/exceptions/ProviderNotReadyError.java
@@ -4,9 +4,9 @@
 import lombok.Getter;
 import lombok.experimental.StandardException;
 
-@SuppressWarnings("checkstyle:MissingJavadocType")
+@SuppressWarnings({"checkstyle:MissingJavadocType", "squid:S110"})
 @StandardException
-public class ProviderNotReadyError extends OpenFeatureError {
+public class ProviderNotReadyError extends OpenFeatureErrorWithoutStacktrace {
     private static final long serialVersionUID = 1L;
     @Getter private final ErrorCode errorCode = ErrorCode.PROVIDER_NOT_READY;
 }
diff --git a/src/main/java/dev/openfeature/sdk/exceptions/TypeMismatchError.java b/src/main/java/dev/openfeature/sdk/exceptions/TypeMismatchError.java
@@ -7,6 +7,7 @@
 /**
  * The type of the flag value does not match the expected type.
  */
+@SuppressWarnings({"checkstyle:MissingJavadocType", "squid:S110"})
 @StandardException
 public class TypeMismatchError extends OpenFeatureError {
     private static final long serialVersionUID = 1L;

diff --git a/src/test/java/dev/openfeature/sdk/AlwaysBrokenWithDetailsProvider.java b/src/test/java/dev/openfeature/sdk/AlwaysBrokenWithDetailsProvider.java
@@ -0,0 +1,53 @@
+package dev.openfeature.sdk;
+
+import dev.openfeature.sdk.exceptions.FlagNotFoundError;
+
+public class AlwaysBrokenWithDetailsProvider implements FeatureProvider {
+
+    @Override
+    public Metadata getMetadata() {
+        return () -> {
+            throw new FlagNotFoundError(TestConstants.BROKEN_MESSAGE);
+        };
+    }
+
+    @Override
+    public ProviderEvaluation<Boolean> getBooleanEvaluation(String key, Boolean defaultValue, EvaluationContext ctx) {
+        return ProviderEvaluation.<Boolean>builder()
+                .errorMessage(TestConstants.BROKEN_MESSAGE)
+                .errorCode(ErrorCode.FLAG_NOT_FOUND)
+                .build();
+    }
+
+    @Override
+    public ProviderEvaluation<String> getStringEvaluation(String key, String defaultValue, EvaluationContext ctx) {
+        return ProviderEvaluation.<String>builder()
+                .errorMessage(TestConstants.BROKEN_MESSAGE)
+                .errorCode(ErrorCode.FLAG_NOT_FOUND)
+                .build();
+    }
+
+    @Override
+    public ProviderEvaluation<Integer> getIntegerEvaluation(String key, Integer defaultValue, EvaluationContext ctx) {
+        return ProviderEvaluation.<Integer>builder()
+                .errorMessage(TestConstants.BROKEN_MESSAGE)
+                .errorCode(ErrorCode.FLAG_NOT_FOUND)
+                .build();
+    }
+
+    @Override
+    public ProviderEvaluation<Double> getDoubleEvaluation(String key, Double defaultValue, EvaluationContext ctx) {
+        return ProviderEvaluation.<Double>builder()
+                .errorMessage(TestConstants.BROKEN_MESSAGE)
+                .errorCode(ErrorCode.FLAG_NOT_FOUND)
+                .build();
+    }
+
+    @Override
+    public ProviderEvaluation<Value> getObjectEvaluation(String key, Value defaultValue, EvaluationContext invocationContext) {
+        return ProviderEvaluation.<Value>builder()
+                .errorMessage(TestConstants.BROKEN_MESSAGE)
+                .errorCode(ErrorCode.FLAG_NOT_FOUND)
+                .build();
+    }
+}
diff --git a/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java b/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java
@@ -113,7 +113,9 @@ public void initialize(EvaluationContext evaluationContext) throws Exception {
         OpenFeatureAPI.getInstance().setProvider(providerName, provider);
         assertThat(api.getProvider(providerName).getState()).isEqualTo(ProviderState.NOT_READY);
         Client client = OpenFeatureAPI.getInstance().getClient(providerName);
-        assertEquals(ErrorCode.PROVIDER_NOT_READY, client.getBooleanDetails("return_error_when_not_initialized", false).getErrorCode());
+        FlagEvaluationDetails<Boolean> details = client.getBooleanDetails("return_error_when_not_initialized", false);
+        assertEquals(ErrorCode.PROVIDER_NOT_READY, details.getErrorCode());
+        assertEquals(Reason.ERROR.toString(), details.getReason());
     }
 
     @Specification(number="1.1.5", text="The API MUST provide a function for retrieving the metadata field of the configured provider.")
@@ -259,10 +261,29 @@ public void initialize(EvaluationContext evaluationContext) throws Exception {
     @Test void broken_provider() {
         FeatureProviderTestUtils.setFeatureProvider(new AlwaysBrokenProvider());
         Client c = api.getClient();
-        assertFalse(c.getBooleanValue("key", false));
-        FlagEvaluationDetails<Boolean> details = c.getBooleanDetails("key", false);
+        boolean defaultValue = false;
+        assertFalse(c.getBooleanValue("key", defaultValue));
+        FlagEvaluationDetails<Boolean> details = c.getBooleanDetails("key", defaultValue);
         assertEquals(ErrorCode.FLAG_NOT_FOUND, details.getErrorCode());
         assertEquals(TestConstants.BROKEN_MESSAGE, details.getErrorMessage());
+        assertEquals(Reason.ERROR.toString(), details.getReason());
+        assertEquals(defaultValue, details.getValue());
+    }
+
+    @Specification(number="1.4.8", text="In cases of abnormal execution, the `evaluation details` structure's `error code` field **MUST** contain an `error code`.")
+    @Specification(number="1.4.9", text="In cases of abnormal execution (network failure, unhandled error, etc) the `reason` field in the `evaluation details` SHOULD indicate an error.")
+    @Specification(number="1.4.10", text="Methods, functions, or operations on the client MUST NOT throw exceptions, or otherwise abnormally terminate. Flag evaluation calls must always return the `default value` in the event of abnormal execution. Exceptions include functions or methods for the purposes for configuration or setup.")
+    @Specification(number="1.4.13", text="In cases of abnormal execution, the `evaluation details` structure's `error message` field **MAY** contain a string containing additional details about the nature of the error.")
+    @Test void broken_provider_withDetails() {
+        FeatureProviderTestUtils.setFeatureProvider(new AlwaysBrokenWithDetailsProvider());
+        Client c = api.getClient();
+        boolean defaultValue = false;
+        assertFalse(c.getBooleanValue("key", defaultValue));
+        FlagEvaluationDetails<Boolean> details = c.getBooleanDetails("key", defaultValue);
+        assertEquals(ErrorCode.FLAG_NOT_FOUND, details.getErrorCode());
+        assertEquals(TestConstants.BROKEN_MESSAGE, details.getErrorMessage());
+        assertEquals(Reason.ERROR.toString(), details.getReason());
+        assertEquals(defaultValue, details.getValue());
     }
 
     @Specification(number="1.4.11", text="Methods, functions, or operations on the client SHOULD NOT write log messages.")