fix(flagd): adjust flagd to updated error codes (#285)

Fixing the inconsistencies regarding error codes for edge cases, as described in #1679


Signed-off-by: Konvalinka <[email protected]>
Co-authored-by: Simon Schrottner <[email protected]>

Author: leakonvalinka

.gitmodules

@@ -8,4 +8,4 @@
 [submodule "providers/openfeature-provider-flagd/openfeature/test-harness"]
 	path = providers/openfeature-provider-flagd/openfeature/test-harness
 	url = https://github.com/open-feature/flagd-testbed.git
-	branch = v2.8.0
+	branch = v2.10.2

providers/openfeature-provider-flagd/openfeature/test-harness

@@ -5,13 +5,13 @@
 )
 from openfeature.evaluation_context import EvaluationContext
 from openfeature.event import ProviderEventDetails
-from openfeature.exception import FlagNotFoundError, ParseError
+from openfeature.exception import ErrorCode, FlagNotFoundError, GeneralError, ParseError
 from openfeature.flag_evaluation import FlagResolutionDetails, Reason
 
 from ..config import Config
 from .process.connector import FlagStateConnector
 from .process.connector.grpc_watcher import GrpcWatcher
-from .process.flags import FlagStore
+from .process.flags import Flag, FlagStore
 from .process.targeting import targeting
 
 T = typing.TypeVar("T")
@@ -128,30 +128,55 @@ def _resolve(
             )
 
         if not flag.targeting:
-            variant, value = flag.default
-            return FlagResolutionDetails(
-                value, variant=variant, flag_metadata=metadata, reason=Reason.STATIC
-            )
+            return _default_resolve(flag, metadata, Reason.STATIC)
 
-        variant = targeting(flag.key, flag.targeting, evaluation_context)
+        try:
+            variant = targeting(flag.key, flag.targeting, evaluation_context)
+            if variant is None:
+                return _default_resolve(flag, metadata, Reason.DEFAULT)
 
-        if variant is None:
-            variant, value = flag.default
-            return FlagResolutionDetails(
-                value, variant=variant, flag_metadata=metadata, reason=Reason.DEFAULT
-            )
-        if not isinstance(variant, (str, bool)):
-            raise ParseError(
-                "Parsed JSONLogic targeting did not return a string or bool"
-            )
+            # convert to string to support shorthand (boolean in python is with capital T hence the special case)
+            if isinstance(variant, bool):
+                variant = str(variant).lower()
+            elif not isinstance(variant, str):
+                variant = str(variant)
+
+            if variant not in flag.variants:
+                raise GeneralError(
+                    f"Resolved variant {variant} not in variants config."
+                )
+
+        except ReferenceError:
+            raise ParseError(f"Invalid targeting {targeting}") from ReferenceError
 
         variant, value = flag.get_variant(variant)
         if value is None:
-            raise ParseError(f"Resolved variant {variant} not in variants config.")
+            raise GeneralError(f"Resolved variant {variant} not in variants config.")
 
         return FlagResolutionDetails(
             value,
             variant=variant,
             reason=Reason.TARGETING_MATCH,
             flag_metadata=metadata,
         )
+
+
+def _default_resolve(
+    flag: Flag,
+    metadata: typing.Mapping[str, typing.Union[float, int, str, bool]],
+    reason: Reason,
+) -> FlagResolutionDetails:
+    variant, value = flag.default
+    if variant is None:
+        return FlagResolutionDetails(
+            value,
+            variant=variant,
+            reason=Reason.ERROR,
+            error_code=ErrorCode.FLAG_NOT_FOUND,
+            flag_metadata=metadata,
+        )
+    if variant not in flag.variants:
+        raise GeneralError(f"Resolved variant {variant} not in variants config.")
+    return FlagResolutionDetails(
+        value, variant=variant, flag_metadata=metadata, reason=reason
+    )

providers/openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/resolvers/in_process.py

@@ -72,30 +72,22 @@ class Flag:
     key: str
     state: str
     variants: typing.Mapping[str, typing.Any]
-    default_variant: typing.Union[bool, str]
+    default_variant: typing.Optional[typing.Union[bool, str]] = None
     targeting: typing.Optional[dict] = None
     metadata: typing.Optional[
         typing.Mapping[str, typing.Union[float, int, str, bool]]
     ] = None
 
     def __post_init__(self) -> None:
-        if not self.state or not isinstance(self.state, str):
+        if not self.state or not (self.state == "ENABLED" or self.state == "DISABLED"):
             raise ParseError("Incorrect 'state' value provided in flag config")
 
         if not self.variants or not isinstance(self.variants, dict):
             raise ParseError("Incorrect 'variants' value provided in flag config")
 
-        if not self.default_variant or not isinstance(
-            self.default_variant, (str, bool)
-        ):
+        if self.default_variant and not isinstance(self.default_variant, (str, bool)):
             raise ParseError("Incorrect 'defaultVariant' value provided in flag config")
 
-        if self.targeting and not isinstance(self.targeting, dict):
-            raise ParseError("Incorrect 'targeting' value provided in flag config")
-
-        if self.default_variant not in self.variants:
-            raise ParseError("Default variant does not match set of variants")
-
         if self.metadata:
             if not isinstance(self.metadata, dict):
                 raise ParseError("Flag metadata is not a valid json object")
@@ -106,6 +98,8 @@ def __post_init__(self) -> None:
     def from_dict(cls, key: str, data: dict) -> "Flag":
         if "defaultVariant" in data:
             data["default_variant"] = data["defaultVariant"]
+            if data["default_variant"] == "":
+                data["default_variant"] = None
             del data["defaultVariant"]
 
         data.pop("source", None)
@@ -119,13 +113,16 @@ def from_dict(cls, key: str, data: dict) -> "Flag":
             raise ParseError from err
 
     @property
-    def default(self) -> tuple[str, typing.Any]:
+    def default(self) -> tuple[typing.Optional[str], typing.Any]:
         return self.get_variant(self.default_variant)
 
     def get_variant(
-        self, variant_key: typing.Union[str, bool]
-    ) -> tuple[str, typing.Any]:
+        self, variant_key: typing.Union[str, bool, None]
+    ) -> tuple[typing.Optional[str], typing.Any]:
         if isinstance(variant_key, bool):
             variant_key = str(variant_key).lower()
 
+        if not variant_key:
+            return None, None
+
         return variant_key, self.variants.get(variant_key)

providers/openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/resolvers/process/flags.py

@@ -5,6 +5,7 @@
 from json_logic.types import JsonValue
 
 from openfeature.evaluation_context import EvaluationContext
+from openfeature.exception import ParseError
 
 from .custom_ops import (
     ends_with,
@@ -27,6 +28,9 @@ def targeting(
     targeting: dict,
     evaluation_context: typing.Optional[EvaluationContext] = None,
 ) -> JsonValue:
+    if not isinstance(targeting, dict):
+        raise ParseError(f"Invalid 'targeting' value in flag: {targeting}")
+
     json_logic_context = evaluation_context.attributes if evaluation_context else {}
     json_logic_context["$flagd"] = {"flagKey": key, "timestamp": int(time.time())}
     json_logic_context["targetingKey"] = (

providers/openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/resolvers/process/targeting.py

@@ -48,7 +48,7 @@ def start(self) -> "FlagdContainer":
         return self
 
     @wait_container_is_ready(ConnectionError)
-    def _checker(self, host: str, port: str) -> None:
+    def _checker(self, host: str, port: int) -> None:
         # First we wait for Flagd to say it's listening
         wait_for_logs(
             self,
@@ -58,7 +58,7 @@ def _checker(self, host: str, port: str) -> None:
 
         time.sleep(1)
         # Second we use the GRPC health check endpoint
-        with grpc.insecure_channel(host + ":" + port) as channel:
+        with grpc.insecure_channel(host + ":" + str(port)) as channel:
             health_stub = health_pb2_grpc.HealthStub(channel)
 
             def health_check_call(stub: health_pb2_grpc.HealthStub):

providers/openfeature-provider-flagd/tests/e2e/flagd_container.py

@@ -36,6 +36,18 @@ def update_context(
     evaluation_context.attributes[key] = type_cast[type_info](value)
 
 
+@given(
+    parsers.cfparse(
+        'a context containing a key "{key}", with type "{type_info}" and with value ""'
+    ),
+)
+def update_context_without_value(
+    evaluation_context: EvaluationContext, key: str, type_info: str
+):
+    """a context containing a key and value."""
+    update_context(evaluation_context, key, type_info, "")
+
+
 @when(
     parsers.cfparse(
         'context contains keys {fields:s} with values "{svalue}", "{svalue2}", {ivalue:d}, "{bvalue:bool}"',

providers/openfeature-provider-flagd/tests/e2e/step/context_steps.py

@@ -1,5 +1,5 @@
 import requests
-from asserts import assert_equal
+from asserts import assert_equal, assert_is_none
 from pytest_bdd import given, parsers, then, when
 
 from openfeature.client import OpenFeatureClient
@@ -96,6 +96,25 @@ def resolve_details_reason(
     assert_equal(details.reason, Reason(reason))
 
 
+@then(
+    parsers.cfparse('the error-code should be "{error_code}"'),
+)
+def resolve_details_error_code(
+    details: FlagEvaluationDetails[JsonPrimitive],
+    error_code: str,
+):
+    assert_equal(details.error_code, error_code)
+
+
+@then(
+    parsers.cfparse('the error-code should be ""'),
+)
+def resolve_details_empty_error_code(
+    details: FlagEvaluationDetails[JsonPrimitive],
+):
+    assert_is_none(details.error_code)
+
+
 @then(parsers.cfparse("the resolved metadata should contain"))
 def metadata_contains(details: FlagEvaluationDetails[JsonPrimitive], datatable):
     assert_equal(len(details.flag_metadata), len(datatable) - 1)  # skip table header

providers/openfeature-provider-flagd/tests/e2e/step/flag_step.py

@@ -0,0 +1,12 @@
+{
+  "flags": {
+    "basic-flag": {
+      "state": "ENABLED",
+      "variants": {
+        "true": true,
+        "false": false
+      },
+      "targeting": {}
+    }
+  }
+}

providers/openfeature-provider-flagd/tests/flags/basic-flag-without-default.json

@@ -23,11 +23,9 @@ def create_client(provider: FlagdProvider):
         "not-a-flag.json",
         "basic-flag-wrong-structure.json",
         "basic-flag-invalid.not-json",
-        "basic-flag-wrong-variant.json",
         "basic-flag-broken-state.json",
         "basic-flag-broken-variants.json",
         "basic-flag-broken-default.json",
-        "basic-flag-broken-targeting.json",
     ],
 )
 def test_file_load_errors(file_name: str):
@@ -46,6 +44,38 @@ def test_file_load_errors(file_name: str):
     assert res.error_code == ErrorCode.FLAG_NOT_FOUND
 
 
+def test_non_existent_variant():
+    path = os.path.abspath(os.path.join(os.path.dirname(__file__), "./flags/"))
+    client = create_client(
+        FlagdProvider(
+            resolver_type=ResolverType.IN_PROCESS,
+            offline_flag_source_path=f"{path}/basic-flag-wrong-variant.json",
+        )
+    )
+
+    res = client.get_boolean_details("basic-flag", False)
+
+    assert res.value is False
+    assert res.reason == Reason.ERROR
+    assert res.error_code == ErrorCode.GENERAL
+
+
+def test_broken_targeting():
+    path = os.path.abspath(os.path.join(os.path.dirname(__file__), "./flags/"))
+    client = create_client(
+        FlagdProvider(
+            resolver_type=ResolverType.IN_PROCESS,
+            offline_flag_source_path=f"{path}/basic-flag-broken-targeting.json",
+        )
+    )
+
+    res = client.get_boolean_details("basic-flag", False)
+
+    assert res.value is False
+    assert res.reason == Reason.ERROR
+    assert res.error_code == ErrorCode.PARSE_ERROR
+
+
 @pytest.mark.parametrize(
     "file_name",
     [