feat!: errorCode as enum, reason as string (open-feature#80)

* feat!: errorCode as enum, reason as string - makes errorCode an enum - makes reason a string - adds errorMessage to resolution/evaluation details Signed-off-by: Bhandari, Pramesh(AWF) <[email protected]>
pbhandari9541 · Nov 3, 2022 · 82fe2e5 · 82fe2e5
1 parent 37e7670
commit 82fe2e5
Show file tree

Hide file tree

Showing 18 changed files with 98 additions and 47 deletions.
diff --git a/.github/workflows/pullrequest.yml b/.github/workflows/pullrequest.yml
@@ -34,7 +34,7 @@ jobs:
             ${{ runner.os }}-maven-
 
       - name: Build with Maven
-        run: mvn --batch-mode --update-snapshots verify -P integration-test
+        run: mvn --batch-mode --update-snapshots verify # -P integration-test - add this back once we have a compatible flagd
 
       - name: Upload coverage to Codecov
         uses: codecov/codecov-action@v2

diff --git a/README.md b/README.md
@@ -3,7 +3,7 @@
 [![Maven Central](https://maven-badges.herokuapp.com/maven-central/dev.openfeature/javasdk/badge.svg)](https://maven-badges.herokuapp.com/maven-central/dev.openfeature/javasdk)
 [![javadoc](https://javadoc.io/badge2/dev.openfeature/javasdk/javadoc.svg)](https://javadoc.io/doc/dev.openfeature/javasdk) 
 [![Project Status: WIP – Initial development is in progress, but there has not yet been a stable, usable release suitable for the public.](https://www.repostatus.org/badges/latest/wip.svg)](https://www.repostatus.org/#wip)
-[![Specification](https://img.shields.io/static/v1?label=Specification&message=v0.4.0&color=yellow)](https://github.com/open-feature/spec/tree/v0.4.0)
+[![Specification](https://img.shields.io/static/v1?label=Specification&message=v0.5.0&color=yellow)](https://github.com/open-feature/spec/tree/v0.5.0)
 [![Known Vulnerabilities](https://snyk.io/test/github/open-feature/java-sdk/badge.svg)](https://snyk.io/test/github/open-feature/java-sdk)
 [![on-merge](https://github.com/open-feature/java-sdk/actions/workflows/merge.yml/badge.svg)](https://github.com/open-feature/java-sdk/actions/workflows/merge.yml)
 [![codecov](https://codecov.io/gh/open-feature/java-sdk/branch/main/graph/badge.svg?token=XMS9L7PBY1)](https://codecov.io/gh/open-feature/java-sdk)

diff --git a/pom.xml b/pom.xml
@@ -141,6 +141,7 @@
     <dependency>
       <groupId>dev.openfeature.contrib.providers</groupId>
       <artifactId>flagd</artifactId>
+      <!-- TODO: update this version -->
       <version>0.3.2</version>
       <scope>test</scope>
     </dependency>

diff --git a/src/main/java/dev/openfeature/javasdk/BaseEvaluation.java b/src/main/java/dev/openfeature/javasdk/BaseEvaluation.java
@@ -21,11 +21,18 @@ public interface BaseEvaluation<T> {
      * Describes how we came to the value that we're returning.
      * @return {Reason}
      */
-    Reason getReason();
+    String getReason();
 
     /**
      * The error code, if applicable. Should only be set when the Reason is ERROR.
      * @return {ErrorCode}
      */
-    String getErrorCode();
+    ErrorCode getErrorCode();
+
+    /**
+     * The error message (usually from exception.getMessage()), if applicable.
+     * Should only be set when the Reason is ERROR.
+     * @return {String}
+     */
+    String getErrorMessage();
 }
diff --git a/src/main/java/dev/openfeature/javasdk/ErrorCode.java b/src/main/java/dev/openfeature/javasdk/ErrorCode.java
@@ -1,5 +1,5 @@
 package dev.openfeature.javasdk;
 
 public enum ErrorCode {
-    PROVIDER_NOT_READY, FLAG_NOT_FOUND, PARSE_ERROR, TYPE_MISMATCH, GENERAL
+    PROVIDER_NOT_READY, FLAG_NOT_FOUND, PARSE_ERROR, TYPE_MISMATCH, TARGETING_KEY_MISSING, INVALID_CONTEXT, GENERAL
 }
diff --git a/src/main/java/dev/openfeature/javasdk/FlagEvaluationDetails.java b/src/main/java/dev/openfeature/javasdk/FlagEvaluationDetails.java
@@ -14,8 +14,9 @@ public class FlagEvaluationDetails<T> implements BaseEvaluation<T> {
     private String flagKey;
     private T value;
     @Nullable private String variant;
-    private Reason reason;
-    @Nullable private String errorCode;
+    @Nullable private String reason;
+    private ErrorCode errorCode;
+    @Nullable private String errorMessage;
 
     /**
      * Generate detail payload from the provider response.

diff --git a/src/main/java/dev/openfeature/javasdk/NoOpProvider.java b/src/main/java/dev/openfeature/javasdk/NoOpProvider.java
@@ -25,7 +25,7 @@ public ProviderEvaluation<Boolean> getBooleanEvaluation(String key, Boolean defa
         return ProviderEvaluation.<Boolean>builder()
                 .value(defaultValue)
                 .variant(PASSED_IN_DEFAULT)
-                .reason(Reason.DEFAULT)
+                .reason(Reason.DEFAULT.toString())
                 .build();
     }
 
@@ -34,7 +34,7 @@ public ProviderEvaluation<String> getStringEvaluation(String key, String default
         return ProviderEvaluation.<String>builder()
                 .value(defaultValue)
                 .variant(PASSED_IN_DEFAULT)
-                .reason(Reason.DEFAULT)
+                .reason(Reason.DEFAULT.toString())
                 .build();
     }
 
@@ -43,7 +43,7 @@ public ProviderEvaluation<Integer> getIntegerEvaluation(String key, Integer defa
         return ProviderEvaluation.<Integer>builder()
                 .value(defaultValue)
                 .variant(PASSED_IN_DEFAULT)
-                .reason(Reason.DEFAULT)
+                .reason(Reason.DEFAULT.toString())
                 .build();
     }
 
@@ -52,7 +52,7 @@ public ProviderEvaluation<Double> getDoubleEvaluation(String key, Double default
         return ProviderEvaluation.<Double>builder()
                 .value(defaultValue)
                 .variant(PASSED_IN_DEFAULT)
-                .reason(Reason.DEFAULT)
+                .reason(Reason.DEFAULT.toString())
                 .build();
     }
 
@@ -62,7 +62,7 @@ public ProviderEvaluation<Value> getObjectEvaluation(String key, Value defaultVa
         return ProviderEvaluation.<Value>builder()
                 .value(defaultValue)
                 .variant(PASSED_IN_DEFAULT)
-                .reason(Reason.DEFAULT)
+                .reason(Reason.DEFAULT.toString())
                 .build();
     }
 }
diff --git a/src/main/java/dev/openfeature/javasdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/javasdk/OpenFeatureClient.java
@@ -7,6 +7,7 @@
 import java.util.Map;
 
 import dev.openfeature.javasdk.exceptions.GeneralError;
+import dev.openfeature.javasdk.exceptions.OpenFeatureError;
 import dev.openfeature.javasdk.internal.ObjectUtils;
 import lombok.Getter;
 import lombok.Setter;
@@ -94,9 +95,14 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(FlagValueType type, String key
             if (details == null) {
                 details = FlagEvaluationDetails.<T>builder().build();
             }
+            if (e instanceof OpenFeatureError) {
+                details.setErrorCode(((OpenFeatureError)e).getErrorCode());
+            } else {
+                details.setErrorCode(ErrorCode.GENERAL);
+            }
+            details.setErrorMessage(e.getMessage());
             details.setValue(defaultValue);
-            details.setReason(Reason.ERROR);
-            details.setErrorCode(e.getMessage());
+            details.setReason(Reason.ERROR.toString());
             hookSupport.errorHooks(type, hookCtx, e, mergedHooks, hints);
         } finally {
             hookSupport.afterAllHooks(type, hookCtx, mergedHooks, hints);

diff --git a/src/main/java/dev/openfeature/javasdk/ProviderEvaluation.java b/src/main/java/dev/openfeature/javasdk/ProviderEvaluation.java
@@ -9,6 +9,7 @@
 public class ProviderEvaluation<T> implements BaseEvaluation<T> {
     T value;
     @Nullable String variant;
-    Reason reason;
-    @Nullable String errorCode;
+    @Nullable private String reason;
+    ErrorCode errorCode;
+    @Nullable private String errorMessage;
 }
diff --git a/src/main/java/dev/openfeature/javasdk/exceptions/FlagNotFoundError.java b/src/main/java/dev/openfeature/javasdk/exceptions/FlagNotFoundError.java
@@ -7,5 +7,5 @@
 @StandardException
 public class FlagNotFoundError extends OpenFeatureError {
     private static final long serialVersionUID = 1L;
-    @Getter private final ErrorCode errorCode = ErrorCode.GENERAL;
+    @Getter private final ErrorCode errorCode = ErrorCode.FLAG_NOT_FOUND;
 }
diff --git a/src/main/java/dev/openfeature/javasdk/exceptions/InvalidContextError.java b/src/main/java/dev/openfeature/javasdk/exceptions/InvalidContextError.java
@@ -0,0 +1,13 @@
+package dev.openfeature.javasdk.exceptions;
+
+import dev.openfeature.javasdk.ErrorCode;
+import lombok.Getter;
+import lombok.experimental.StandardException;
+
+@StandardException
+public class InvalidContextError extends OpenFeatureError {
+    private static final long serialVersionUID = 1L;
+
+    @Getter private final ErrorCode errorCode = ErrorCode.INVALID_CONTEXT;
+
+}
diff --git a/src/main/java/dev/openfeature/javasdk/exceptions/TargetingKeyMissingError.java b/src/main/java/dev/openfeature/javasdk/exceptions/TargetingKeyMissingError.java
@@ -0,0 +1,13 @@
+package dev.openfeature.javasdk.exceptions;
+
+import dev.openfeature.javasdk.ErrorCode;
+import lombok.Getter;
+import lombok.experimental.StandardException;
+
+@StandardException
+public class TargetingKeyMissingError extends OpenFeatureError {
+    private static final long serialVersionUID = 1L;
+
+    @Getter private final ErrorCode errorCode = ErrorCode.TARGETING_KEY_MISSING;
+
+}
diff --git a/src/test/java/dev/openfeature/javasdk/AlwaysBrokenProvider.java b/src/test/java/dev/openfeature/javasdk/AlwaysBrokenProvider.java
@@ -1,39 +1,41 @@
 package dev.openfeature.javasdk;
 
+import dev.openfeature.javasdk.exceptions.FlagNotFoundError;
 
 public class AlwaysBrokenProvider implements FeatureProvider {
+
     @Override
     public Metadata getMetadata() {
         return new Metadata() {
             @Override
             public String getName() {
-                throw new NotImplementedException("BORK");
+                throw new FlagNotFoundError(TestConstants.BROKEN_MESSAGE);
             }
         };
     }
 
     @Override
     public ProviderEvaluation<Boolean> getBooleanEvaluation(String key, Boolean defaultValue, EvaluationContext ctx) {
-        throw new NotImplementedException("BORK");
+        throw new FlagNotFoundError(TestConstants.BROKEN_MESSAGE);
     }
 
     @Override
     public ProviderEvaluation<String> getStringEvaluation(String key, String defaultValue, EvaluationContext ctx) {
-        throw new NotImplementedException("BORK");
+        throw new FlagNotFoundError(TestConstants.BROKEN_MESSAGE);
     }
 
     @Override
     public ProviderEvaluation<Integer> getIntegerEvaluation(String key, Integer defaultValue, EvaluationContext ctx) {
-        throw new NotImplementedException("BORK");
+        throw new FlagNotFoundError(TestConstants.BROKEN_MESSAGE);
     }
 
     @Override
     public ProviderEvaluation<Double> getDoubleEvaluation(String key, Double defaultValue, EvaluationContext ctx) {
-        throw new NotImplementedException("BORK");
+        throw new FlagNotFoundError(TestConstants.BROKEN_MESSAGE);
     }
 
     @Override
     public ProviderEvaluation<Value> getObjectEvaluation(String key, Value defaultValue, EvaluationContext invocationContext) {
-        throw new NotImplementedException("BORK");
+        throw new FlagNotFoundError(TestConstants.BROKEN_MESSAGE);
     }
 }
diff --git a/src/test/java/dev/openfeature/javasdk/DeveloperExperienceTest.java b/src/test/java/dev/openfeature/javasdk/DeveloperExperienceTest.java
@@ -85,8 +85,9 @@ class DeveloperExperienceTest implements HookFixtures {
         api.setProvider(new AlwaysBrokenProvider());
         Client client = api.getClient();
         FlagEvaluationDetails<Boolean> retval = client.getBooleanDetails(flagKey, false);
-        assertEquals("BORK", retval.getErrorCode());
-        assertEquals(Reason.ERROR, retval.getReason());
+        assertEquals(ErrorCode.FLAG_NOT_FOUND, retval.getErrorCode());
+        assertEquals(TestConstants.BROKEN_MESSAGE, retval.getErrorMessage());
+        assertEquals(Reason.ERROR.toString(), retval.getReason());
         assertFalse(retval.getValue());
     }
 }
diff --git a/src/test/java/dev/openfeature/javasdk/FlagEvaluationSpecTest.java b/src/test/java/dev/openfeature/javasdk/FlagEvaluationSpecTest.java
@@ -183,14 +183,16 @@ private Client _client() {
     }
 
     @Specification(number="1.4.9", 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.7", text="In cases of abnormal execution, the evaluation details structure's error code field MUST contain a string identifying an error occurred during flag evaluation and the nature of the error.")
+    @Specification(number="1.4.7", text="In cases of abnormal execution, the `evaluation details` structure's `error code` field **MUST** contain an `error code`.")
+    @Specification(number="1.4.12", 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() {
         OpenFeatureAPI api = OpenFeatureAPI.getInstance();
         api.setProvider(new AlwaysBrokenProvider());
         Client c = api.getClient();
         assertFalse(c.getBooleanValue("key", false));
         FlagEvaluationDetails<Boolean> details = c.getBooleanDetails("key", false);
-        assertEquals("BORK", details.getErrorCode());
+        assertEquals(ErrorCode.FLAG_NOT_FOUND, details.getErrorCode());
+        assertEquals(TestConstants.BROKEN_MESSAGE, details.getErrorMessage());
     }
 
     @Specification(number="1.4.10", text="In the case of abnormal execution, the client SHOULD log an informative error message.")
@@ -200,8 +202,8 @@ private Client _client() {
         api.setProvider(new AlwaysBrokenProvider());
         Client c = api.getClient();
         FlagEvaluationDetails<Boolean> result = c.getBooleanDetails("test", false);
-        assertEquals(Reason.ERROR, result.getReason());
-        assertThat(TEST_LOGGER.getLoggingEvents()).contains(LoggingEvent.error("Unable to correctly evaluate flag with key {} due to exception {}", "test", "BORK"));
+        assertEquals(Reason.ERROR.toString(), result.getReason());
+        assertThat(TEST_LOGGER.getLoggingEvents()).contains(LoggingEvent.error("Unable to correctly evaluate flag with key {} due to exception {}", "test", TestConstants.BROKEN_MESSAGE));
     }
 
     @Specification(number="1.2.2", text="The client interface MUST define a metadata member or accessor, containing an immutable name field or accessor of type string, which corresponds to the name value supplied during client creation.")
@@ -221,7 +223,7 @@ private Client _client() {
         api.setProvider(new AlwaysBrokenProvider());
         Client c = api.getClient();
         FlagEvaluationDetails<Boolean> result = c.getBooleanDetails("test", false);
-        assertEquals(Reason.ERROR, result.getReason());
+        assertEquals(Reason.ERROR.toString(), result.getReason());
     }
 
     @Specification(number="3.2.1", text="The API, Client and invocation MUST have a method for supplying evaluation context.")

diff --git a/src/test/java/dev/openfeature/javasdk/ProviderSpecTest.java b/src/test/java/dev/openfeature/javasdk/ProviderSpecTest.java
@@ -41,11 +41,10 @@ public class ProviderSpecTest {
 
     }
 
-    @Specification(number="2.6", text="The provider SHOULD populate the flag resolution structure's " +
-            "reason field with a string indicating the semantic reason for the returned flag value.")
+    @Specification(number="2.6", text="The `provider` SHOULD populate the `flag resolution` structure's `reason` field with `\"DEFAULT\",` `\"TARGETING_MATCH\"`, `\"SPLIT\"`, `\"DISABLED\"`, `\"UNKNOWN\"`, `\"ERROR\"` or some other string indicating the semantic reason for the returned flag value.")
     @Test void has_reason() {
         ProviderEvaluation<Boolean> result = p.getBooleanEvaluation("key", false, new EvaluationContext());
-        assertEquals(Reason.DEFAULT, result.getReason());
+        assertEquals(Reason.DEFAULT.toString(), result.getReason());
     }
 
     @Specification(number="2.7", text="In cases of normal execution, the provider MUST NOT populate " +
@@ -55,9 +54,9 @@ public class ProviderSpecTest {
         assertNull(result.getErrorCode());
     }
 
-    @Specification(number="2.8", text="In cases of abnormal execution, the provider MUST indicate an " +
-    "error using the idioms of the implementation language, with an associated error code having possible " +
-    "values PROVIDER_NOT_READY, FLAG_NOT_FOUND, PARSE_ERROR, TYPE_MISMATCH, or GENERAL.")
+    @Specification(number="2.8", text="In cases of abnormal execution, the `provider` **MUST** indicate an error using the idioms of the implementation language, with an associated `error code` and optional associated `error message`.")
+    @Specification(number="2.11", text="In cases of normal execution, the `provider` **MUST NOT** populate the `flag resolution` structure's `error message` field, or otherwise must populate it with a null or falsy value.")
+    @Specification(number="2.12", text="In cases of abnormal execution, the `evaluation details` structure's `error message` field **MAY** contain a string containing additional detail about the nature of the error.")
     @Test void up_to_provider_implementation() {}
 
     @Specification(number="2.5", text="In cases of normal execution, the provider SHOULD populate the " +

diff --git a/src/test/java/dev/openfeature/javasdk/TestConstants.java b/src/test/java/dev/openfeature/javasdk/TestConstants.java
@@ -0,0 +1,5 @@
+package dev.openfeature.javasdk;
+
+public class TestConstants {
+    public static final String BROKEN_MESSAGE = "This is borked.";
+}
diff --git a/src/test/java/dev/openfeature/javasdk/integration/StepDefinitions.java b/src/test/java/dev/openfeature/javasdk/integration/StepDefinitions.java
@@ -2,11 +2,9 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
-import static org.junit.jupiter.api.Assumptions.assumeFalse;
 
 import dev.openfeature.contrib.providers.flagd.FlagdProvider;
 import dev.openfeature.javasdk.Client;
-import dev.openfeature.javasdk.ErrorCode;
 import dev.openfeature.javasdk.EvaluationContext;
 import dev.openfeature.javasdk.FlagEvaluationDetails;
 import dev.openfeature.javasdk.OpenFeatureAPI;
@@ -132,7 +130,7 @@ public void the_resolved_boolean_value_should_be_the_variant_should_be_and_the_r
             String expectedVariant, String expectedReason) {
         assertEquals(Boolean.valueOf(expectedValue), booleanFlagDetails.getValue());
         assertEquals(expectedVariant, booleanFlagDetails.getVariant());
-        assertEquals(Reason.valueOf(expectedReason), booleanFlagDetails.getReason());
+        assertEquals(expectedReason, booleanFlagDetails.getReason());
     }
 
     // string details
@@ -147,7 +145,7 @@ public void the_resolved_string_value_should_be_the_variant_should_be_and_the_re
             String expectedVariant, String expectedReason) {
         assertEquals(expectedValue, this.stringFlagDetails.getValue());
         assertEquals(expectedVariant, this.stringFlagDetails.getVariant());
-        assertEquals(Reason.valueOf(expectedReason), this.stringFlagDetails.getReason());
+        assertEquals(expectedReason, this.stringFlagDetails.getReason());
     }
 
     // integer details
@@ -161,7 +159,7 @@ public void the_resolved_integer_value_should_be_the_variant_should_be_and_the_r
             String expectedVariant, String expectedReason) {
         assertEquals(expectedValue, this.intFlagDetails.getValue());
         assertEquals(expectedVariant, this.intFlagDetails.getVariant());
-        assertEquals(Reason.valueOf(expectedReason), this.intFlagDetails.getReason());
+        assertEquals(expectedReason, this.intFlagDetails.getReason());
     }
 
     // float/double details
@@ -175,7 +173,7 @@ public void the_resolved_float_value_should_be_the_variant_should_be_and_the_rea
             String expectedVariant, String expectedReason) {
         assertEquals(expectedValue, this.doubleFlagDetails.getValue());
         assertEquals(expectedVariant, this.doubleFlagDetails.getVariant());
-        assertEquals(Reason.valueOf(expectedReason), this.doubleFlagDetails.getReason());
+        assertEquals(expectedReason, this.doubleFlagDetails.getReason());
     }
 
     // object details
@@ -198,7 +196,7 @@ public void the_resolved_object_value_should_be_contain_fields_and_with_values_a
     @Then("the variant should be {string}, and the reason should be {string}")
     public void the_variant_should_be_and_the_reason_should_be(String expectedVariant, String expectedReason) {
         assertEquals(expectedVariant, this.objectFlagDetails.getVariant());
-        assertEquals(Reason.valueOf(expectedReason), this.objectFlagDetails.getReason());
+        assertEquals(expectedReason, this.objectFlagDetails.getReason());
     }
 
     /*
@@ -255,8 +253,9 @@ public void then_the_default_string_value_should_be_returned() {
 
     @Then("the reason should indicate an error and the error code should indicate a missing flag with {string}")
     public void the_reason_should_indicate_an_error_and_the_error_code_should_be_flag_not_found(String errorCode) {
-        assertEquals(Reason.ERROR, notFoundDetails.getReason());
-        assertTrue(notFoundDetails.getErrorCode().contains(errorCode));
+        assertEquals(Reason.ERROR.toString(), notFoundDetails.getReason());
+        assertTrue(notFoundDetails.getErrorMessage().contains(errorCode));
+        // TODO: add errorCode assertion once flagd provider is updated.
     }
 
     // type mismatch
@@ -275,8 +274,9 @@ public void then_the_default_integer_value_should_be_returned() {
 
     @Then("the reason should indicate an error and the error code should indicate a type mismatch with {string}")
     public void the_reason_should_indicate_an_error_and_the_error_code_should_be_type_mismatch(String errorCode) {
-        assertEquals(Reason.ERROR, typeErrorDetails.getReason());
-        assertTrue(typeErrorDetails.getErrorCode().contains(errorCode));
+        assertEquals(Reason.ERROR.toString(), typeErrorDetails.getReason());
+        assertTrue(typeErrorDetails.getErrorMessage().contains(errorCode));
+        // TODO: add errorCode assertion once flagd provider is updated.
     }
 
 }