Repository: flipt-io/flipt-client-sdks (language: python)

Issue:
[FLI-1269] Ensure consistency between SDKs
I think we should audit our existing client SDKs in the various languages and ensure they have a similar API before we make any of them 1.0.

Heres a few areas I think we could check for consistency/improve upon:

1. Remove the `Result` types as public facing return types. Similar to how we now do in the new dart SDK ([https://github.com/flipt-io/flipt-client-sdks/pull/351#pullrequestreview-2287892390](https://github.com/flipt-io/flipt-client-sdks/pull/351#pullrequestreview-2287892390)), just return the underlying types instead of the wrapped `Result` types
2. Ensure consistent naming of Request/Response types. ie: `EvaluationRequest`, `EvaluationResponse`, `BooleanEvaluationResponse`, `VariantEvaluationResponse`, etc.
3. Ensure the methods are named consistently across SDKs (ie: `evaluateVariant` / `evaluate_variant` depending on language idioms, `evaluateBoolean`, `evaluateBatch`, `listFlags`)
4. Ensure all methods are supported in all SDKs. I think we may be missing `listFlags` in a few
5. Ensure clients are named similarly, ex: `flipt-client` instead of `flipt-client-{language}` for their published names. As each ecosystem will have its own package manager, so the `-{language}` bit is unnecessary for the public facing package name (where applicable, ie we still need `flipt-client` and `flipt-client-browser` as they are both NPM packages)
6. Ensure types/classes named consistently, ie: `FliptEvaluationClient` instead of `FliptClient` or maybe vice-versa?
7. Dont expose anything publicly in the API that references `Engine`, ie `EngineOpts/Options`. The fact that we call the Rust evaluator an engine under the hood should not leak externally
8. Use `optional` types for languages that support it. ie: `variantKey` and `variantAttachment` should be optional in Variant evaluation responses

Anything else? Feel free to add

<sub>From [SyncLinear.com](https://synclinear.com) | [FLI-1269](https://linear.app/flipt/issue/FLI-1269/ensure-consistency-between-sdks)</sub>

## SDKS to Refactor

- [x] Node
- [x] Browser
- [x] Go
- [x] Dart
- [x] Ruby
- [x] Python
- [x] Java

## Published

- [x] Node
- [x] Browser
- [ ] Go
- [x] Dart
- [ ] Ruby
- [ ] Python
- [x] Java


Discussion / hints:

Fix patch:
diff --git a/flipt-client-java/src/main/java/io/flipt/client/EvaluationException.java b/flipt-client-java/src/main/java/io/flipt/client/EvaluationException.java
new file mode 100644
index 00000000..8eae3e2b
--- /dev/null
+++ b/flipt-client-java/src/main/java/io/flipt/client/EvaluationException.java
@@ -0,0 +1,37 @@
+package io.flipt.client;
+
+public class EvaluationException extends Exception {
+
+  /** Constructs a new EvaluationException with {@code null} as its detail message. */
+  public EvaluationException() {
+    super();
+  }
+
+  /**
+   * Constructs a new EvaluationException with the specified detail message.
+   *
+   * @param message the detail message.
+   */
+  public EvaluationException(String message) {
+    super(message);
+  }
+
+  /**
+   * Constructs a new EvaluationException with the specified detail message and cause.
+   *
+   * @param message the detail message.
+   * @param cause the cause of the exception (a {@code null} value is permitted).
+   */
+  public EvaluationException(String message, Throwable cause) {
+    super(message, cause);
+  }
+
+  /**
+   * Constructs a new EvaluationException with the specified cause.
+   *
+   * @param cause the cause of the exception (a {@code null} value is permitted).
+   */
+  public EvaluationException(Throwable cause) {
+    super(cause);
+  }
+}
diff --git a/flipt-client-java/src/main/java/io/flipt/client/FliptEvaluationClient.java b/flipt-client-java/src/main/java/io/flipt/client/FliptEvaluationClient.java
index 1648770d..2e41b304 100644
--- a/flipt-client-java/src/main/java/io/flipt/client/FliptEvaluationClient.java
+++ b/flipt-client-java/src/main/java/io/flipt/client/FliptEvaluationClient.java
@@ -15,6 +15,7 @@
 import java.util.Optional;
 
 public class FliptEvaluationClient {
+  private static final String STATUS_SUCCESS = "success";
   private final Pointer engine;
 
   private final ObjectMapper objectMapper;
@@ -38,12 +39,17 @@ public interface CLibrary extends Library {
   }
 
   private FliptEvaluationClient(String namespace, EngineOpts engineOpts)
-      throws JsonProcessingException {
+      throws EvaluationException {
 
     ObjectMapper objectMapper = new ObjectMapper();
     objectMapper.registerModule(new Jdk8Module());
 
-    String engineOptions = objectMapper.writeValueAsString(engineOpts);
+    String engineOptions;
+    try {
+      engineOptions = objectMapper.writeValueAsString(engineOpts);
+    } catch (JsonProcessingException e) {
+      throw new EvaluationException(e);
+    }
 
     Pointer engine = CLibrary.INSTANCE.initialize_engine(namespace, engineOptions);
 
@@ -89,7 +95,7 @@ public FliptEvaluationClientBuilder reference(String reference) {
       return this;
     }
 
-    public FliptEvaluationClient build() throws JsonProcessingException {
+    public FliptEvaluationClient build() throws EvaluationException {
       return new FliptEvaluationClient(
           namespace,
           new EngineOpts(
@@ -130,35 +136,55 @@ public Map<String, String> getContext() {
     }
   }
 
-  public Result<VariantEvaluationResponse> evaluateVariant(
-      String flagKey, String entityId, Map<String, String> context) throws JsonProcessingException {
+  public VariantEvaluationResponse evaluateVariant(
+      String flagKey, String entityId, Map<String, String> context) throws EvaluationException {
     InternalEvaluationRequest evaluationRequest =
         new InternalEvaluationRequest(flagKey, entityId, context);
 
-    String evaluationRequestSerialized = this.objectMapper.writeValueAsString(evaluationRequest);
+    String evaluationRequestSerialized;
+    try {
+      evaluationRequestSerialized = this.objectMapper.writeValueAsString(evaluationRequest);
+    } catch (JsonProcessingException e) {
+      throw new EvaluationException(e);
+    }
+
     Pointer value = CLibrary.INSTANCE.evaluate_variant(this.engine, evaluationRequestSerialized);
 
     TypeReference<Result<VariantEvaluationResponse>> typeRef =
         new TypeReference<Result<VariantEvaluationResponse>>() {};
 
-    return this.readValue(value, typeRef);
+    Result<VariantEvaluationResponse> resp = this.readValue(value, typeRef);
+    if (!FliptEvaluationClient.STATUS_SUCCESS.equals(resp.getStatus())) {
+      throw new EvaluationException(resp.getErrorMessage().get());
+    }
+    return resp.getResult().get();
   }
 
-  public Result<BooleanEvaluationResponse> evaluateBoolean(
-      String flagKey, String entityId, Map<String, String> context) throws JsonProcessingException {
+  public BooleanEvaluationResponse evaluateBoolean(
+      String flagKey, String entityId, Map<String, String> context) throws EvaluationException {
     InternalEvaluationRequest evaluationRequest =
         new InternalEvaluationRequest(flagKey, entityId, context);
 
-    String evaluationRequestSerialized = this.objectMapper.writeValueAsString(evaluationRequest);
+    String evaluationRequestSerialized;
+    try {
+      evaluationRequestSerialized = this.objectMapper.writeValueAsString(evaluationRequest);
+    } catch (JsonProcessingException e) {
+      throw new EvaluationException(e);
+    }
     Pointer value = CLibrary.INSTANCE.evaluate_boolean(this.engine, evaluationRequestSerialized);
 
     TypeReference<Result<BooleanEvaluationResponse>> typeRef =
         new TypeReference<Result<BooleanEvaluationResponse>>() {};
-    return this.readValue(value, typeRef);
+    Result<BooleanEvaluationResponse> resp = this.readValue(value, typeRef);
+
+    if (!FliptEvaluationClient.STATUS_SUCCESS.equals(resp.getStatus())) {
+      throw new EvaluationException(resp.getErrorMessage().get());
+    }
+    return resp.getResult().get();
   }
 
-  public Result<BatchEvaluationResponse> evaluateBatch(EvaluationRequest[] batchEvaluationRequests)
-      throws JsonProcessingException {
+  public BatchEvaluationResponse evaluateBatch(EvaluationRequest[] batchEvaluationRequests)
+      throws EvaluationException {
     ArrayList<InternalEvaluationRequest> evaluationRequests =
         new ArrayList<>(batchEvaluationRequests.length);
 
@@ -171,33 +197,48 @@ public Result<BatchEvaluationResponse> evaluateBatch(EvaluationRequest[] batchEv
               batchEvaluationRequests[i].getContext()));
     }
 
-    String batchEvaluationRequestSerialized =
-        this.objectMapper.writeValueAsString(evaluationRequests.toArray());
+    String batchEvaluationRequestSerialized;
+    try {
+      batchEvaluationRequestSerialized =
+          this.objectMapper.writeValueAsString(evaluationRequests.toArray());
+    } catch (JsonProcessingException e) {
+      throw new EvaluationException(e);
+    }
     Pointer value = CLibrary.INSTANCE.evaluate_batch(this.engine, batchEvaluationRequestSerialized);
 
     TypeReference<Result<BatchEvaluationResponse>> typeRef =
         new TypeReference<Result<BatchEvaluationResponse>>() {};
 
-    return this.readValue(value, typeRef);
+    Result<BatchEvaluationResponse> resp = this.readValue(value, typeRef);
+    if (!FliptEvaluationClient.STATUS_SUCCESS.equals(resp.getStatus())) {
+      throw new EvaluationException(resp.getErrorMessage().get());
+    }
+    return resp.getResult().get();
   }
 
-  public Result<ArrayList<Flag>> listFlags() throws JsonProcessingException {
+  public ArrayList<Flag> listFlags() throws EvaluationException {
     Pointer value = CLibrary.INSTANCE.list_flags(this.engine);
 
     TypeReference<Result<ArrayList<Flag>>> typeRef =
         new TypeReference<Result<ArrayList<Flag>>>() {};
 
-    return this.readValue(value, typeRef);
+    Result<ArrayList<Flag>> resp = this.readValue(value, typeRef);
+    if (!FliptEvaluationClient.STATUS_SUCCESS.equals(resp.getStatus())) {
+      throw new EvaluationException(resp.getErrorMessage().get());
+    }
+    return resp.getResult().get();
   }
 
   public void close() {
     CLibrary.INSTANCE.destroy_engine(this.engine);
   }
 
-  private <T> T readValue(Pointer ptr, TypeReference<T> typeRef) throws JsonProcessingException {
+  private <T> T readValue(Pointer ptr, TypeReference<T> typeRef) throws EvaluationException {
     try {
       String value = ptr.getString(0, "UTF-8");
       return this.objectMapper.readValue(value, typeRef);
+    } catch (JsonProcessingException e) {
+      throw new EvaluationException(e);
     } finally {
       CLibrary.INSTANCE.destroy_string(ptr);
     }


Test patch:
diff --git a/flipt-client-java/src/test/java/TestFliptEvaluationClient.java b/flipt-client-java/src/test/java/TestFliptEvaluationClient.java
index f6dd1424..de92c0a5 100644
--- a/flipt-client-java/src/test/java/TestFliptEvaluationClient.java
+++ b/flipt-client-java/src/test/java/TestFliptEvaluationClient.java
@@ -30,14 +30,7 @@ void testEvaluateVariant() throws Exception {
     Map<String, String> context = new HashMap<>();
     context.put("fizz", "buzz");
 
-    Result<VariantEvaluationResponse> result =
-        fliptClient.evaluateVariant("flag1", "entity", context);
-
-    Assertions.assertEquals("success", result.getStatus());
-    Assertions.assertFalse(result.getErrorMessage().isPresent());
-    Assertions.assertTrue(result.getResult().isPresent());
-
-    VariantEvaluationResponse response = result.getResult().get();
+    VariantEvaluationResponse response = fliptClient.evaluateVariant("flag1", "entity", context);
 
     Assertions.assertEquals("flag1", response.getFlagKey());
     Assertions.assertTrue(response.isMatch());
@@ -51,14 +44,9 @@ void testEvaluateBoolean() throws Exception {
     Map<String, String> context = new HashMap<>();
     context.put("fizz", "buzz");
 
-    Result<BooleanEvaluationResponse> result =
+    BooleanEvaluationResponse response =
         fliptClient.evaluateBoolean("flag_boolean", "entity", context);
 
-    Assertions.assertEquals("success", result.getStatus());
-    Assertions.assertFalse(result.getErrorMessage().isPresent());
-    Assertions.assertTrue(result.getResult().isPresent());
-
-    BooleanEvaluationResponse response = result.getResult().get();
     Assertions.assertEquals("flag_boolean", response.getFlagKey());
     Assertions.assertTrue(response.isEnabled());
     Assertions.assertEquals("MATCH_EVALUATION_REASON", response.getReason());
@@ -75,12 +63,7 @@ void testEvaluateBatch() throws Exception {
       new EvaluationRequest("notfound", "entity", context)
     };
 
-    Result<BatchEvaluationResponse> result = fliptClient.evaluateBatch(evalRequests);
-
-    Assertions.assertEquals("success", result.getStatus());
-    Assertions.assertFalse(result.getErrorMessage().isPresent());
-    Assertions.assertTrue(result.getResult().isPresent());
-    BatchEvaluationResponse response = result.getResult().get();
+    BatchEvaluationResponse response = fliptClient.evaluateBatch(evalRequests);
 
     Assertions.assertEquals(3, response.getResponses().length);
     Response[] responses = response.getResponses();
@@ -108,13 +91,7 @@ void testEvaluateBatch() throws Exception {
 
   @Test
   void testListFlags() throws Exception {
-    Result<ArrayList<Flag>> result = fliptClient.listFlags();
-
-    Assertions.assertEquals("success", result.getStatus());
-    Assertions.assertFalse(result.getErrorMessage().isPresent());
-    Assertions.assertTrue(result.getResult().isPresent());
-
-    ArrayList<Flag> flags = result.getResult().get();
+    ArrayList<Flag> flags = fliptClient.listFlags();
     Assertions.assertEquals(2, flags.size());
   }