From 4fb8e6bc19e2bc3d7d220a71cfacdcf5bccddd21 Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Sat, 7 Sep 2024 17:18:40 +0200 Subject: [PATCH 1/3] 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 --- .../openfeature/sdk/OpenFeatureClient.java | 8 ++- .../sdk/AlwaysBrokenWithDetailsProvider.java | 53 +++++++++++++++++++ .../sdk/FlagEvaluationSpecTest.java | 21 +++++++- 3 files changed, 78 insertions(+), 4 deletions(-) create mode 100644 src/test/java/dev/openfeature/sdk/AlwaysBrokenWithDetailsProvider.java diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java index d8004e5de..3e3dc70fa 100644 --- 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 FlagEvaluationDetails 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()); + details.setValue(defaultValue); + hookSupport.errorHooks(type, afterHookContext, error, mergedHooks, hints); } else { hookSupport.afterHooks(type, afterHookContext, details, mergedHooks, hints); } diff --git a/src/test/java/dev/openfeature/sdk/AlwaysBrokenWithDetailsProvider.java b/src/test/java/dev/openfeature/sdk/AlwaysBrokenWithDetailsProvider.java new file mode 100644 index 000000000..b3ead41bd --- /dev/null +++ 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 getBooleanEvaluation(String key, Boolean defaultValue, EvaluationContext ctx) { + return ProviderEvaluation.builder() + .errorMessage(TestConstants.BROKEN_MESSAGE) + .errorCode(ErrorCode.FLAG_NOT_FOUND) + .build(); + } + + @Override + public ProviderEvaluation getStringEvaluation(String key, String defaultValue, EvaluationContext ctx) { + return ProviderEvaluation.builder() + .errorMessage(TestConstants.BROKEN_MESSAGE) + .errorCode(ErrorCode.FLAG_NOT_FOUND) + .build(); + } + + @Override + public ProviderEvaluation getIntegerEvaluation(String key, Integer defaultValue, EvaluationContext ctx) { + return ProviderEvaluation.builder() + .errorMessage(TestConstants.BROKEN_MESSAGE) + .errorCode(ErrorCode.FLAG_NOT_FOUND) + .build(); + } + + @Override + public ProviderEvaluation getDoubleEvaluation(String key, Double defaultValue, EvaluationContext ctx) { + return ProviderEvaluation.builder() + .errorMessage(TestConstants.BROKEN_MESSAGE) + .errorCode(ErrorCode.FLAG_NOT_FOUND) + .build(); + } + + @Override + public ProviderEvaluation getObjectEvaluation(String key, Value defaultValue, EvaluationContext invocationContext) { + return ProviderEvaluation.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 index b4978cb4b..f52b4b392 100644 --- a/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java +++ b/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java @@ -259,10 +259,27 @@ 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 details = c.getBooleanDetails("key", false); + boolean defaultValue = false; + assertFalse(c.getBooleanValue("key", defaultValue)); + FlagEvaluationDetails details = c.getBooleanDetails("key", defaultValue); assertEquals(ErrorCode.FLAG_NOT_FOUND, details.getErrorCode()); assertEquals(TestConstants.BROKEN_MESSAGE, details.getErrorMessage()); + 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 details = c.getBooleanDetails("key", defaultValue); + assertEquals(ErrorCode.FLAG_NOT_FOUND, details.getErrorCode()); + assertEquals(TestConstants.BROKEN_MESSAGE, details.getErrorMessage()); + assertEquals(defaultValue, details.getValue()); } @Specification(number="1.4.11", text="Methods, functions, or operations on the client SHOULD NOT write log messages.") From 3d4b992089c637a5f21eb9157002c5aff2e10206 Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Tue, 10 Sep 2024 20:28:32 +0200 Subject: [PATCH 2/3] fix: adding reason, and removing stacktraces from errors Signed-off-by: Simon Schrottner --- .../dev/openfeature/sdk/OpenFeatureClient.java | 10 +++++++--- .../sdk/exceptions/FlagNotFoundError.java | 11 ++++------- .../OpenFeatureErrorWithoutStacktrace.java | 14 ++++++++++++++ .../sdk/exceptions/ProviderNotReadyError.java | 4 ++-- .../sdk/exceptions/TypeMismatchError.java | 3 ++- .../openfeature/sdk/FlagEvaluationSpecTest.java | 6 +++++- 6 files changed, 34 insertions(+), 14 deletions(-) create mode 100644 src/main/java/dev/openfeature/sdk/exceptions/OpenFeatureErrorWithoutStacktrace.java diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java index 3e3dc70fa..79982a2b6 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java @@ -135,7 +135,7 @@ private FlagEvaluationDetails evaluateFlag(FlagValueType type, String key OpenFeatureError error = ExceptionUtils.instantiateErrorByErrorCode( details.getErrorCode(), details.getErrorMessage()); - details.setValue(defaultValue); + enrichDetailsWithErrorDefaults(defaultValue, details); hookSupport.errorHooks(type, afterHookContext, error, mergedHooks, hints); } else { hookSupport.afterHooks(type, afterHookContext, details, mergedHooks, hints); @@ -150,8 +150,7 @@ private FlagEvaluationDetails 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); @@ -160,6 +159,11 @@ private FlagEvaluationDetails evaluateFlag(FlagValueType type, String key return details; } + private static void enrichDetailsWithErrorDefaults(T defaultValue, FlagEvaluationDetails 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 index 6685f96d5..7c88ebb44 100644 --- 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 new file mode 100644 index 000000000..2931e6bbb --- /dev/null +++ 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 index 218073441..0416eae26 100644 --- 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 index d27c6209f..eb7bbf362 100644 --- a/src/main/java/dev/openfeature/sdk/exceptions/TypeMismatchError.java +++ b/src/main/java/dev/openfeature/sdk/exceptions/TypeMismatchError.java @@ -7,8 +7,9 @@ /** * The type of the flag value does not match the expected type. */ +@SuppressWarnings({"checkstyle:MissingJavadocType", "squid:S110"}) @StandardException -public class TypeMismatchError extends OpenFeatureError { +public class TypeMismatchError extends OpenFeatureErrorWithoutStacktrace { private static final long serialVersionUID = 1L; @Getter private final ErrorCode errorCode = ErrorCode.TYPE_MISMATCH; diff --git a/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java b/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java index f52b4b392..c4a6fd6c5 100644 --- 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 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.") @@ -264,6 +266,7 @@ public void initialize(EvaluationContext evaluationContext) throws Exception { FlagEvaluationDetails 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()); } @@ -279,6 +282,7 @@ public void initialize(EvaluationContext evaluationContext) throws Exception { FlagEvaluationDetails 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()); } From c3be65b94b51ae9e9d2fc53daac5d1904a5a9bee Mon Sep 17 00:00:00 2001 From: Simon Schrottner Date: Fri, 13 Sep 2024 10:56:21 +0200 Subject: [PATCH 3/3] Update src/main/java/dev/openfeature/sdk/exceptions/TypeMismatchError.java Co-authored-by: Justin Abrahms Signed-off-by: Simon Schrottner --- .../java/dev/openfeature/sdk/exceptions/TypeMismatchError.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/dev/openfeature/sdk/exceptions/TypeMismatchError.java b/src/main/java/dev/openfeature/sdk/exceptions/TypeMismatchError.java index eb7bbf362..9d88922c7 100644 --- a/src/main/java/dev/openfeature/sdk/exceptions/TypeMismatchError.java +++ b/src/main/java/dev/openfeature/sdk/exceptions/TypeMismatchError.java @@ -9,7 +9,7 @@ */ @SuppressWarnings({"checkstyle:MissingJavadocType", "squid:S110"}) @StandardException -public class TypeMismatchError extends OpenFeatureErrorWithoutStacktrace { +public class TypeMismatchError extends OpenFeatureError { private static final long serialVersionUID = 1L; @Getter private final ErrorCode errorCode = ErrorCode.TYPE_MISMATCH;