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

* feat!: errorCode as enum, reason as string

- makes errorCode an enum
- makes reason a string
- adds errorMessage to resolution/evaluation details

.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

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)

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>

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();
 }

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
 }

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.

src/main/java/dev/openfeature/javasdk/NoOpProvider.java

@@ -25,7 +25,7 @@ public class NoOpProvider implements FeatureProvider {
         return ProviderEvaluation.<Boolean>builder()
                 .value(defaultValue)
                 .variant(PASSED_IN_DEFAULT)
-                .reason(Reason.DEFAULT)
+                .reason(Reason.DEFAULT.toString())
                 .build();
     }
 
@@ -34,7 +34,7 @@ public class NoOpProvider implements FeatureProvider {
         return ProviderEvaluation.<String>builder()
                 .value(defaultValue)
                 .variant(PASSED_IN_DEFAULT)
-                .reason(Reason.DEFAULT)
+                .reason(Reason.DEFAULT.toString())
                 .build();
     }
 
@@ -43,7 +43,7 @@ public class NoOpProvider implements FeatureProvider {
         return ProviderEvaluation.<Integer>builder()
                 .value(defaultValue)
                 .variant(PASSED_IN_DEFAULT)
-                .reason(Reason.DEFAULT)
+                .reason(Reason.DEFAULT.toString())
                 .build();
     }
 
@@ -52,7 +52,7 @@ public class NoOpProvider implements FeatureProvider {
         return ProviderEvaluation.<Double>builder()
                 .value(defaultValue)
                 .variant(PASSED_IN_DEFAULT)
-                .reason(Reason.DEFAULT)
+                .reason(Reason.DEFAULT.toString())
                 .build();
     }
 
@@ -62,7 +62,7 @@ public class NoOpProvider implements FeatureProvider {
         return ProviderEvaluation.<Value>builder()
                 .value(defaultValue)
                 .variant(PASSED_IN_DEFAULT)
-                .reason(Reason.DEFAULT)
+                .reason(Reason.DEFAULT.toString())
                 .build();
     }
 }

src/main/java/dev/openfeature/javasdk/OpenFeatureClient.java

@@ -7,6 +7,7 @@ import java.util.List;
 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 @@ public class OpenFeatureClient implements Client {
             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);

src/main/java/dev/openfeature/javasdk/ProviderEvaluation.java

@@ -9,6 +9,7 @@ import javax.annotation.Nullable;
 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;
 }

src/main/java/dev/openfeature/javasdk/exceptions/FlagNotFoundError.java

@@ -7,5 +7,5 @@ import lombok.experimental.StandardException;
 @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;
 }

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;
+
+}

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;
+
+}

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);
     }
 }

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());
     }
 }

src/test/java/dev/openfeature/javasdk/FlagEvaluationSpecTest.java

@@ -183,14 +183,16 @@ class FlagEvaluationSpecTest implements HookFixtures {
     }
 
     @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 @@ class FlagEvaluationSpecTest implements HookFixtures {
         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 @@ class FlagEvaluationSpecTest implements HookFixtures {
         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.")

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 " +

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.";
+}

src/test/java/dev/openfeature/javasdk/integration/StepDefinitions.java

@@ -2,11 +2,9 @@ package dev.openfeature.javasdk.integration;
 
 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 class StepDefinitions {
             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 class StepDefinitions {
             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 class StepDefinitions {
             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 class StepDefinitions {
             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 class StepDefinitions {
     @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 class StepDefinitions {
 
     @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 class StepDefinitions {
 
     @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.
     }
 
 }