From efbdf08f791de941674fa5521415450a0cc8a35f Mon Sep 17 00:00:00 2001 From: Mark Story Date: Wed, 7 Aug 2024 16:00:27 -0400 Subject: [PATCH 1/8] Hackity hack --- src/flagpole/__init__.py | 37 ++++++++-------- src/flagpole/conditions.py | 91 +++++++++++++++++++------------------- 2 files changed, 65 insertions(+), 63 deletions(-) diff --git a/src/flagpole/__init__.py b/src/flagpole/__init__.py index 98cf99c018b02..7480f1c1d1d64 100644 --- a/src/flagpole/__init__.py +++ b/src/flagpole/__init__.py @@ -61,12 +61,12 @@ """ from __future__ import annotations +import dataclasses from datetime import datetime from typing import Any import orjson import yaml -from pydantic import BaseModel, Field, ValidationError, constr from flagpole.conditions import ConditionBase, Segment from flagpole.evaluation_context import ContextBuilder, EvaluationContext @@ -76,26 +76,21 @@ class InvalidFeatureFlagConfiguration(Exception): pass -class Feature(BaseModel): - name: constr(min_length=1, to_lower=True) = Field( # type:ignore[valid-type] - description="The feature name." - ) +@dataclasses.dataclass(frozen=True) +class Feature: + name: str "The feature name." - owner: constr(min_length=1) = Field( # type:ignore[valid-type] - description="The owner of this feature. Either an email address or team name, preferably." - ) + owner: str "The owner of this feature. Either an email address or team name, preferably." - segments: list[Segment] = Field( - description="The list of segments to evaluate for the feature. An empty list will always evaluate to False." - ) - "The list of segments to evaluate for the feature. An empty list will always evaluate to False." - - enabled: bool = Field(default=True, description="Whether or not the feature is enabled.") + enabled: bool = dataclasses.field(default=False) "Whether or not the feature is enabled." - created_at: datetime = Field(description="The datetime when this feature was created.") + segments: list[Segment] = dataclasses.field(default_factory=list) + "The list of segments to evaluate for the feature. An empty list will always evaluate to False." + + created_at: str | None = None "The datetime when this feature was created." def match(self, context: EvaluationContext) -> bool: @@ -111,13 +106,19 @@ def match(self, context: EvaluationContext) -> bool: @classmethod def dump_schema_to_file(cls, file_path: str) -> None: - with open(file_path, "w") as file: - file.write(cls.schema_json(indent=2)) + raise NotImplementedError("nope") @classmethod def from_feature_dictionary(cls, name: str, config_dict: dict[str, Any]) -> Feature: try: - feature = cls(name=name, **config_dict) + segments = [Segment.from_dict(segment) for segment in config_dict.get("segments", [])] + feature = cls( + name=name, + owner=str(config_dict.get("owner", "")), + enabled=bool(config_dict.get("enabled", False)), + created_at=str(config_dict.get("created_at")), + segments=segments, + ) except ValidationError as exc: raise InvalidFeatureFlagConfiguration("Provided JSON is not a valid feature") from exc diff --git a/src/flagpole/conditions.py b/src/flagpole/conditions.py index f93139a6dbf6a..9e86ce0db7fab 100644 --- a/src/flagpole/conditions.py +++ b/src/flagpole/conditions.py @@ -1,6 +1,8 @@ +import dataclasses from abc import abstractmethod +from collections.abc import Mapping from enum import Enum -from typing import Annotated, Any, Literal, TypeVar +from typing import Annotated, Any, Literal, Self, Type, TypeVar from pydantic import BaseModel, Field, StrictBool, StrictFloat, StrictInt, StrictStr, constr @@ -48,20 +50,17 @@ def create_case_insensitive_set_from_list(values: list[T]) -> set[T]: return case_insensitive_set -class ConditionBase(BaseModel): - property: str = Field(description="The evaluation context property to match against.") +@dataclasses.dataclass(frozen=True) +class ConditionBase: + property: str """The evaluation context property to match against.""" - operator: ConditionOperatorKind = Field( - description="The operator to use when comparing the evaluation context property to the condition's value." - ) - """The operator to use when comparing the evaluation context property to the condition's value.""" - - value: Any = Field( - description="The value to compare against the condition's evaluation context property." - ) + value: Any """The value to compare against the condition's evaluation context property.""" + operator: ConditionOperatorKind | None = None + """The operator to use when comparing the evaluation context property to the condition's value.""" + def match(self, context: EvaluationContext, segment_name: str) -> bool: return self._operator_match( condition_property=context.get(self.property), segment_name=segment_name @@ -121,7 +120,7 @@ def _evaluate_equals( return condition_property == self.value -InOperatorValueTypes = list[StrictInt] | list[StrictFloat] | list[StrictStr] +InOperatorValueTypes = list[int] | list[float] | list[str] class InCondition(ConditionBase): @@ -142,7 +141,7 @@ def _operator_match(self, condition_property: Any, segment_name: str): ) -ContainsOperatorValueTypes = StrictInt | StrictStr | StrictFloat +ContainsOperatorValueTypes = int | str | float class ContainsCondition(ConditionBase): @@ -165,24 +164,13 @@ def _operator_match(self, condition_property: Any, segment_name: str): ) -EqualsOperatorValueTypes = ( - StrictInt - | StrictFloat - | StrictStr - | StrictBool - | list[StrictInt] - | list[StrictFloat] - | list[StrictStr] -) +EqualsOperatorValueTypes = int | float | str | bool | list[int] | list[float] | list[str] class EqualsCondition(ConditionBase): operator: Literal[ConditionOperatorKind.EQUALS] = ConditionOperatorKind.EQUALS value: EqualsOperatorValueTypes - strict_validation: bool = Field( - description="Whether the condition should enable strict validation, raising an exception if the evaluation context property is missing", - default=False, - ) + strict_validation: bool = dataclasses.field(default=False) """Whether the condition should enable strict validation, raising an exception if the evaluation context property is missing""" def _operator_match(self, condition_property: Any, segment_name: str): @@ -196,10 +184,7 @@ def _operator_match(self, condition_property: Any, segment_name: str): class NotEqualsCondition(ConditionBase): operator: Literal[ConditionOperatorKind.NOT_EQUALS] = ConditionOperatorKind.NOT_EQUALS value: EqualsOperatorValueTypes - strict_validation: bool = Field( - description="Whether the condition should enable strict validation, raising an exception if the evaluation context property is missing", - default=False, - ) + strict_validation: bool = dataclasses.field(default=False) """Whether the condition should enable strict validation, raising an exception if the evaluation context property is missing""" def _operator_match(self, condition_property: Any, segment_name: str): @@ -224,26 +209,33 @@ def _operator_match(self, condition_property: Any, segment_name: str): ] -class Segment(BaseModel): - name: constr(min_length=1) = Field( # type:ignore[valid-type] - description="A brief description or identifier for the segment" +OPERATOR_LOOKUP: Mapping[ConditionOperatorKind, type[ConditionBase]] = { + ConditionOperatorKind.IN: InCondition, + ConditionOperatorKind.NOT_IN: NotInCondition, + ConditionOperatorKind.CONTAINS: ContainsCondition, + ConditionOperatorKind.NOT_CONTAINS: NotContainsCondition, + ConditionOperatorKind.EQUALS: EqualsCondition, + ConditionOperatorKind.NOT_EQUALS: NotEqualsCondition, +} + + +def _condition_from_dict(data: Mapping[str, Any]) -> ConditionBase: + operator_kind = ConditionOperatorKind(data.get("operator", "invalid")) + condition_cls = OPERATOR_LOOKUP[operator_kind] + return condition_cls( + property=str(data.get("property")), operator=operator_kind, value=data.get("value") ) + + +@dataclasses.dataclass +class Segment: + name: str "A brief description or identifier for the segment" - conditions: list[AvailableConditions] = Field( - description="The list of conditions that the segment must be matched in order for this segment to be active" - ) + conditions: list[ConditionBase] = dataclasses.field(default_factory=list) "The list of conditions that the segment must be matched in order for this segment to be active" - rollout: int | None = Field( - default=0, - description=""" - Rollout rate controls how many buckets will be granted a feature when this segment matches. - - Rollout rates range from 0 (off) to 100 (all users). Rollout rates use `context.id` - to determine bucket membership consistently over time. - """, - ) + rollout: int | None = dataclasses.field(default=0) """ Rollout rate controls how many buckets will be granted a feature when this segment matches. @@ -251,6 +243,15 @@ class Segment(BaseModel): to determine bucket membership consistently over time. """ + @classmethod + def from_dict(cls, data: Mapping[str, Any]) -> Self: + conditions = [_condition_from_dict(condition) for condition in data.get("conditions", [])] + return cls( + name=str(data.get("name", "")), + rollout=int(data.get("rollout", 0)), + conditions=conditions, + ) + def match(self, context: EvaluationContext) -> bool: for condition in self.conditions: match_condition = condition.match(context, segment_name=self.name) From 67d43da2641472359eb9fa759bb2a2069414ffe4 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Wed, 7 Aug 2024 17:30:26 -0400 Subject: [PATCH 2/8] Get tests passing with dataclass based logic Early timing measurements are looking promising. --- src/flagpole/__init__.py | 25 +++--- src/flagpole/conditions.py | 47 ++-------- tests/flagpole/test_conditions.py | 138 ++++++------------------------ tests/flagpole/test_feature.py | 14 ++- 4 files changed, 52 insertions(+), 172 deletions(-) diff --git a/src/flagpole/__init__.py b/src/flagpole/__init__.py index 7480f1c1d1d64..353ac6e2fbd9e 100644 --- a/src/flagpole/__init__.py +++ b/src/flagpole/__init__.py @@ -62,7 +62,6 @@ from __future__ import annotations import dataclasses -from datetime import datetime from typing import Any import orjson @@ -84,7 +83,7 @@ class Feature: owner: str "The owner of this feature. Either an email address or team name, preferably." - enabled: bool = dataclasses.field(default=False) + enabled: bool = dataclasses.field(default=True) "Whether or not the feature is enabled." segments: list[Segment] = dataclasses.field(default_factory=list) @@ -104,22 +103,21 @@ def match(self, context: EvaluationContext) -> bool: return False - @classmethod - def dump_schema_to_file(cls, file_path: str) -> None: - raise NotImplementedError("nope") - @classmethod def from_feature_dictionary(cls, name: str, config_dict: dict[str, Any]) -> Feature: + segment_data = config_dict.get("segments") + if not isinstance(segment_data, list): + raise InvalidFeatureFlagConfiguration("Feature has no segments defined") try: - segments = [Segment.from_dict(segment) for segment in config_dict.get("segments", [])] + segments = [Segment.from_dict(segment) for segment in segment_data] feature = cls( name=name, owner=str(config_dict.get("owner", "")), - enabled=bool(config_dict.get("enabled", False)), + enabled=bool(config_dict.get("enabled", True)), created_at=str(config_dict.get("created_at")), segments=segments, ) - except ValidationError as exc: + except Exception as exc: raise InvalidFeatureFlagConfiguration("Provided JSON is not a valid feature") from exc return feature @@ -136,6 +134,9 @@ def from_feature_config_json( if not isinstance(config_data_dict, dict): raise InvalidFeatureFlagConfiguration("Feature JSON is not a valid feature") + if not name: + raise InvalidFeatureFlagConfiguration("Feature name is required") + return cls.from_feature_dictionary(name=name, config_dict=config_data_dict) @classmethod @@ -158,9 +159,9 @@ def from_bulk_yaml(cls, yaml_str) -> list[Feature]: return features def to_dict(self) -> dict[str, Any]: - json_dict = dict(orjson.loads(self.json())) - json_dict.pop("name") - return {self.name: json_dict} + dict_data = dataclasses.asdict(self) + dict_data.pop("name") + return {self.name: dict_data} def to_yaml_str(self) -> str: return yaml.dump(self.to_dict()) diff --git a/src/flagpole/conditions.py b/src/flagpole/conditions.py index 9e86ce0db7fab..5d356a23e70e3 100644 --- a/src/flagpole/conditions.py +++ b/src/flagpole/conditions.py @@ -2,9 +2,7 @@ from abc import abstractmethod from collections.abc import Mapping from enum import Enum -from typing import Annotated, Any, Literal, Self, Type, TypeVar - -from pydantic import BaseModel, Field, StrictBool, StrictFloat, StrictInt, StrictStr, constr +from typing import Any, Self, TypeVar from flagpole.evaluation_context import EvaluationContext @@ -58,8 +56,11 @@ class ConditionBase: value: Any """The value to compare against the condition's evaluation context property.""" - operator: ConditionOperatorKind | None = None - """The operator to use when comparing the evaluation context property to the condition's value.""" + operator: str | None = None + """ + The operator to use when comparing the evaluation context property to the condition's value. + Values must be a valid ConditionOperatorKind. + """ def match(self, context: EvaluationContext, segment_name: str) -> bool: return self._operator_match( @@ -98,12 +99,8 @@ def _evaluate_contains(self, condition_property: Any, segment_name: str) -> bool return value in create_case_insensitive_set_from_list(condition_property) - def _evaluate_equals( - self, condition_property: Any, segment_name: str, strict_validation: bool = False - ) -> bool: - # Strict validation enforces that a property exists when used in an - # equals condition - if condition_property is None and not strict_validation: + def _evaluate_equals(self, condition_property: Any, segment_name: str) -> bool: + if condition_property is None: return False if not isinstance(condition_property, type(self.value)): @@ -124,7 +121,6 @@ def _evaluate_equals( class InCondition(ConditionBase): - operator: Literal[ConditionOperatorKind.IN] = ConditionOperatorKind.IN value: InOperatorValueTypes def _operator_match(self, condition_property: Any, segment_name: str): @@ -132,7 +128,6 @@ def _operator_match(self, condition_property: Any, segment_name: str): class NotInCondition(ConditionBase): - operator: Literal[ConditionOperatorKind.NOT_IN] = ConditionOperatorKind.NOT_IN value: InOperatorValueTypes def _operator_match(self, condition_property: Any, segment_name: str): @@ -145,7 +140,6 @@ def _operator_match(self, condition_property: Any, segment_name: str): class ContainsCondition(ConditionBase): - operator: Literal[ConditionOperatorKind.CONTAINS] = ConditionOperatorKind.CONTAINS value: ContainsOperatorValueTypes def _operator_match(self, condition_property: Any, segment_name: str): @@ -155,7 +149,6 @@ def _operator_match(self, condition_property: Any, segment_name: str): class NotContainsCondition(ConditionBase): - operator: Literal[ConditionOperatorKind.NOT_CONTAINS] = ConditionOperatorKind.NOT_CONTAINS value: ContainsOperatorValueTypes def _operator_match(self, condition_property: Any, segment_name: str): @@ -168,47 +161,25 @@ def _operator_match(self, condition_property: Any, segment_name: str): class EqualsCondition(ConditionBase): - operator: Literal[ConditionOperatorKind.EQUALS] = ConditionOperatorKind.EQUALS value: EqualsOperatorValueTypes - strict_validation: bool = dataclasses.field(default=False) - """Whether the condition should enable strict validation, raising an exception if the evaluation context property is missing""" def _operator_match(self, condition_property: Any, segment_name: str): return self._evaluate_equals( condition_property=condition_property, segment_name=segment_name, - strict_validation=self.strict_validation, ) class NotEqualsCondition(ConditionBase): - operator: Literal[ConditionOperatorKind.NOT_EQUALS] = ConditionOperatorKind.NOT_EQUALS value: EqualsOperatorValueTypes - strict_validation: bool = dataclasses.field(default=False) - """Whether the condition should enable strict validation, raising an exception if the evaluation context property is missing""" def _operator_match(self, condition_property: Any, segment_name: str): return not self._evaluate_equals( condition_property=condition_property, segment_name=segment_name, - strict_validation=self.strict_validation, ) -# We have to group and annotate all the different subclasses of Operator -# in order for Pydantic to be able to discern between the different types -# when parsing a dict or JSON. -AvailableConditions = Annotated[ - InCondition - | NotInCondition - | ContainsCondition - | NotContainsCondition - | EqualsCondition - | NotEqualsCondition, - Field(discriminator="operator"), -] - - OPERATOR_LOOKUP: Mapping[ConditionOperatorKind, type[ConditionBase]] = { ConditionOperatorKind.IN: InCondition, ConditionOperatorKind.NOT_IN: NotInCondition, @@ -223,7 +194,7 @@ def _condition_from_dict(data: Mapping[str, Any]) -> ConditionBase: operator_kind = ConditionOperatorKind(data.get("operator", "invalid")) condition_cls = OPERATOR_LOOKUP[operator_kind] return condition_cls( - property=str(data.get("property")), operator=operator_kind, value=data.get("value") + property=str(data.get("property")), operator=operator_kind.value, value=data.get("value") ) diff --git a/tests/flagpole/test_conditions.py b/tests/flagpole/test_conditions.py index 5d7b1023cede5..bb5b3f326b683 100644 --- a/tests/flagpole/test_conditions.py +++ b/tests/flagpole/test_conditions.py @@ -1,12 +1,7 @@ -from typing import Any - -import orjson import pytest -from pydantic import ValidationError from flagpole import EvaluationContext from flagpole.conditions import ( - ConditionBase, ConditionTypeMismatchException, ContainsCondition, EqualsCondition, @@ -32,41 +27,7 @@ def test_returns_lowercase_string_set(self): assert create_case_insensitive_set_from_list(["AbC", "DEF"]) == {"abc", "def"} -def assert_valid_types(condition: type[ConditionBase], expected_types: list[Any]): - for value in expected_types: - condition_dict = dict(property="test", value=value) - json_condition = orjson.dumps(condition_dict) - try: - parsed_condition = condition.parse_raw(json_condition) - except ValidationError as exc: - raise AssertionError( - f"Expected value `{value}` to be a valid value for condition '{condition}'" - ) from exc - assert parsed_condition.value == value - - -def assert_invalid_types(condition: type[ConditionBase], invalid_types: list[Any]): - for value in invalid_types: - json_dict = dict(value=value) - condition_json = orjson.dumps(json_dict) - try: - condition.parse_raw(condition_json) - except ValidationError: - continue - - raise AssertionError( - f"Expected validation error for value: `{value}` for condition `{condition}`" - ) - - class TestInConditions: - def test_invalid_values(self): - with pytest.raises(ValidationError): - InCondition(property="foo", value="bar") - - with pytest.raises(ValidationError): - InCondition(property="foo", value=1234) - def test_is_in(self): values = ["bar", "baz"] condition = InCondition(property="foo", value=values) @@ -113,22 +74,17 @@ def test_invalid_property_value(self): values = ["bar", "baz"] condition = InCondition(property="foo", value=values) - with pytest.raises(ConditionTypeMismatchException): - condition.match(context=EvaluationContext({"foo": []}), segment_name="test") + bad_context = ([1], {"k": "v"}) + for attr_val in bad_context: + with pytest.raises(ConditionTypeMismatchException): + condition.match(context=EvaluationContext({"foo": attr_val}), segment_name="test") not_condition = NotInCondition(property="foo", value=values) - with pytest.raises(ConditionTypeMismatchException): - not_condition.match(context=EvaluationContext({"foo": []}), segment_name="test") - - def test_valid_json_and_reparse(self): - values = [["foo", "bar"], [1, 2], [1.1, 2.2], []] - assert_valid_types(condition=InCondition, expected_types=values) - assert_valid_types(condition=NotInCondition, expected_types=values) - - def test_invalid_value_type_parsing(self): - values = ["abc", 1, 2.2, True, None, ["a", 1], [True], [[]], [1, 2.2], [1.1, "2.2"]] - assert_invalid_types(condition=InCondition, invalid_types=values) - assert_invalid_types(condition=NotInCondition, invalid_types=values) + for attr_val in bad_context: + with pytest.raises(ConditionTypeMismatchException): + not_condition.match( + context=EvaluationContext({"foo": attr_val}), segment_name="test" + ) def test_missing_context_property(self): values = ["bar", "baz"] @@ -173,33 +129,21 @@ def test_does_not_contain(self): def test_invalid_property_provided(self): values = "baz" - - with pytest.raises(ConditionTypeMismatchException): - condition = ContainsCondition(property="foo", value=values) - assert not condition.match( - context=EvaluationContext({"foo": "oops"}), segment_name="test" - ) - - with pytest.raises(ConditionTypeMismatchException): - not_condition = NotContainsCondition(property="foo", value=values) - assert not_condition.match( - context=EvaluationContext({"foo": "oops"}), segment_name="test" - ) - - def test_valid_json_parsing_with_types(self): - values = [1, 2.2, "abc"] - assert_valid_types(condition=ContainsCondition, expected_types=values) - assert_valid_types(condition=NotContainsCondition, expected_types=values) - - def test_invalid_value_type_parsing(self): - values: list[Any] = [ - None, - [], - dict(foo="bar"), - [[]], - ] - assert_invalid_types(condition=ContainsCondition, invalid_types=values) - assert_invalid_types(condition=NotContainsCondition, invalid_types=values) + bad_context = ("oops", "1", 1, 3.14, None, False, True) + + for attr_val in bad_context: + with pytest.raises(ConditionTypeMismatchException): + condition = ContainsCondition(property="foo", value=values) + assert not condition.match( + context=EvaluationContext({"foo": attr_val}), segment_name="test" + ) + + for attr_val in bad_context: + with pytest.raises(ConditionTypeMismatchException): + not_condition = NotContainsCondition(property="foo", value=values) + assert not_condition.match( + context=EvaluationContext({"foo": attr_val}), segment_name="test" + ) def test_missing_context_property(self): condition = ContainsCondition(property="foo", value="bar") @@ -254,37 +198,3 @@ def test_equality_type_mismatch_strings(self): not_condition = NotEqualsCondition(property="foo", value=values) with pytest.raises(ConditionTypeMismatchException): not_condition.match(context=EvaluationContext({"foo": "foo"}), segment_name="test") - - def test_valid_json_parsing_with_types(self): - values = [1, 2.2, "abc", True, False, [], ["foo"], [1], [1.1]] - assert_valid_types(condition=EqualsCondition, expected_types=values) - assert_valid_types(condition=NotEqualsCondition, expected_types=values) - - def test_invalid_value_type_parsing(self): - values = [None, dict(foo="bar")] - assert_invalid_types(condition=EqualsCondition, invalid_types=values) - assert_invalid_types(condition=NotEqualsCondition, invalid_types=values) - - def test_with_missing_context_property(self): - value = "foo" - condition = EqualsCondition(property="foo", value=value, strict_validation=True) - - with pytest.raises(ConditionTypeMismatchException): - condition.match(context=EvaluationContext({"bar": value}), segment_name="test") - - not_condition = NotEqualsCondition(property="foo", value=value, strict_validation=True) - - with pytest.raises(ConditionTypeMismatchException): - not_condition.match(context=EvaluationContext({"bar": value}), segment_name="test") - - # Test non-strict validation for both conditions - condition = EqualsCondition(property="foo", value=value) - assert ( - condition.match(context=EvaluationContext({"bar": value}), segment_name="test") is False - ) - - not_condition = NotEqualsCondition(property="foo", value=value) - assert ( - not_condition.match(context=EvaluationContext({"bar": value}), segment_name="test") - is True - ) diff --git a/tests/flagpole/test_feature.py b/tests/flagpole/test_feature.py index cb0ff84685888..5b1a93ab1ca3a 100644 --- a/tests/flagpole/test_feature.py +++ b/tests/flagpole/test_feature.py @@ -1,5 +1,4 @@ from dataclasses import dataclass -from datetime import datetime, timezone import orjson import pytest @@ -31,7 +30,7 @@ def test_feature_with_empty_segments(self): ) assert feature.name == "foobar" - assert feature.created_at == datetime(2023, 10, 12, tzinfo=timezone.utc) + assert feature.created_at == "2023-10-12T00:00:00.000Z" assert feature.owner == "test-owner" assert feature.segments == [] @@ -152,12 +151,12 @@ def test_invalid_json(self): def test_empty_string_name(self): with pytest.raises(InvalidFeatureFlagConfiguration) as exception: Feature.from_feature_config_json("", '{"segments":[]}') - assert "Provided JSON is not a valid feature" in str(exception) + assert "Feature name is required" in str(exception) def test_missing_segments(self): with pytest.raises(InvalidFeatureFlagConfiguration) as exception: Feature.from_feature_config_json("foo", "{}") - assert "Provided JSON is not a valid feature" in str(exception) + assert "Feature has no segments defined" in str(exception) def test_enabled_feature(self): feature = Feature.from_feature_config_json( @@ -229,12 +228,11 @@ def test_dump_yaml(self): """, ) - parsed_json = orjson.loads(feature.json()) + parsed_json = orjson.loads(feature.to_json_str()) parsed_yaml = dict(yaml.safe_load(feature.to_yaml_str())) - assert "foo" in parsed_yaml - parsed_json.pop("name") - assert parsed_yaml["foo"] == parsed_json + assert "foo" in parsed_yaml + assert parsed_yaml == parsed_json features_from_yaml = Feature.from_bulk_yaml(feature.to_yaml_str()) assert features_from_yaml == [feature] From 3b44321de357d3d7c7447727d7c5e988e07d7798 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Thu, 8 Aug 2024 11:04:18 -0400 Subject: [PATCH 3/8] Add flagpole schema This schema was generated with pydantic and will be used in CI to validate that schema and dataclasses are kept compatible, and could be part of a validation flow for flagpole. --- src/flagpole/flagpole-schema.json | 388 ++++++++++++++++++++++++++++++ 1 file changed, 388 insertions(+) create mode 100644 src/flagpole/flagpole-schema.json diff --git a/src/flagpole/flagpole-schema.json b/src/flagpole/flagpole-schema.json new file mode 100644 index 0000000000000..b0f9512dd7b3d --- /dev/null +++ b/src/flagpole/flagpole-schema.json @@ -0,0 +1,388 @@ +{ + "title": "Feature", + "type": "object", + "properties": { + "name": { + "title": "Name", + "description": "The feature name.", + "minLength": 1, + "type": "string" + }, + "owner": { + "title": "Owner", + "description": "The owner of this feature. Either an email address or team name, preferably.", + "minLength": 1, + "type": "string" + }, + "segments": { + "title": "Segments", + "description": "The list of segments to evaluate for the feature. An empty list will always evaluate to False.", + "type": "array", + "items": { + "$ref": "#/definitions/Segment" + } + }, + "enabled": { + "title": "Enabled", + "description": "Whether or not the feature is enabled.", + "default": true, + "type": "boolean" + }, + "created_at": { + "title": "Created At", + "description": "The datetime when this feature was created.", + "type": "string", + "format": "date-time" + } + }, + "required": [ + "name", + "owner", + "segments", + "created_at" + ], + "definitions": { + "InCondition": { + "title": "InCondition", + "type": "object", + "properties": { + "property": { + "title": "Property", + "description": "The evaluation context property to match against.", + "type": "string" + }, + "operator": { + "title": "Operator", + "default": "in", + "enum": [ + "in" + ], + "type": "string" + }, + "value": { + "title": "Value", + "anyOf": [ + { + "type": "array", + "items": { + "type": "integer" + } + }, + { + "type": "array", + "items": { + "type": "number" + } + }, + { + "type": "array", + "items": { + "type": "string" + } + } + ] + } + }, + "required": [ + "property", + "value" + ] + }, + "NotInCondition": { + "title": "NotInCondition", + "type": "object", + "properties": { + "property": { + "title": "Property", + "description": "The evaluation context property to match against.", + "type": "string" + }, + "operator": { + "title": "Operator", + "default": "not_in", + "enum": [ + "not_in" + ], + "type": "string" + }, + "value": { + "title": "Value", + "anyOf": [ + { + "type": "array", + "items": { + "type": "integer" + } + }, + { + "type": "array", + "items": { + "type": "number" + } + }, + { + "type": "array", + "items": { + "type": "string" + } + } + ] + } + }, + "required": [ + "property", + "value" + ] + }, + "ContainsCondition": { + "title": "ContainsCondition", + "type": "object", + "properties": { + "property": { + "title": "Property", + "description": "The evaluation context property to match against.", + "type": "string" + }, + "operator": { + "title": "Operator", + "default": "contains", + "enum": [ + "contains" + ], + "type": "string" + }, + "value": { + "title": "Value", + "anyOf": [ + { + "type": "integer" + }, + { + "type": "string" + }, + { + "type": "number" + } + ] + } + }, + "required": [ + "property", + "value" + ] + }, + "NotContainsCondition": { + "title": "NotContainsCondition", + "type": "object", + "properties": { + "property": { + "title": "Property", + "description": "The evaluation context property to match against.", + "type": "string" + }, + "operator": { + "title": "Operator", + "default": "not_contains", + "enum": [ + "not_contains" + ], + "type": "string" + }, + "value": { + "title": "Value", + "anyOf": [ + { + "type": "integer" + }, + { + "type": "string" + }, + { + "type": "number" + } + ] + } + }, + "required": [ + "property", + "value" + ] + }, + "EqualsCondition": { + "title": "EqualsCondition", + "type": "object", + "properties": { + "property": { + "title": "Property", + "description": "The evaluation context property to match against.", + "type": "string" + }, + "operator": { + "title": "Operator", + "default": "equals", + "enum": [ + "equals" + ], + "type": "string" + }, + "value": { + "title": "Value", + "anyOf": [ + { + "type": "integer" + }, + { + "type": "number" + }, + { + "type": "string" + }, + { + "type": "boolean" + }, + { + "type": "array", + "items": { + "type": "integer" + } + }, + { + "type": "array", + "items": { + "type": "number" + } + }, + { + "type": "array", + "items": { + "type": "string" + } + } + ] + } + }, + "required": [ + "property", + "value" + ] + }, + "NotEqualsCondition": { + "title": "NotEqualsCondition", + "type": "object", + "properties": { + "property": { + "title": "Property", + "description": "The evaluation context property to match against.", + "type": "string" + }, + "operator": { + "title": "Operator", + "default": "not_equals", + "enum": [ + "not_equals" + ], + "type": "string" + }, + "value": { + "title": "Value", + "anyOf": [ + { + "type": "integer" + }, + { + "type": "number" + }, + { + "type": "string" + }, + { + "type": "boolean" + }, + { + "type": "array", + "items": { + "type": "integer" + } + }, + { + "type": "array", + "items": { + "type": "number" + } + }, + { + "type": "array", + "items": { + "type": "string" + } + } + ] + } + }, + "required": [ + "property", + "value" + ] + }, + "Segment": { + "title": "Segment", + "type": "object", + "properties": { + "name": { + "title": "Name", + "description": "A brief description or identifier for the segment", + "minLength": 1, + "type": "string" + }, + "conditions": { + "title": "Conditions", + "description": "The list of conditions that the segment must be matched in order for this segment to be active", + "type": "array", + "items": { + "discriminator": { + "propertyName": "operator", + "mapping": { + "in": "#/definitions/InCondition", + "not_in": "#/definitions/NotInCondition", + "contains": "#/definitions/ContainsCondition", + "not_contains": "#/definitions/NotContainsCondition", + "equals": "#/definitions/EqualsCondition", + "not_equals": "#/definitions/NotEqualsCondition" + } + }, + "oneOf": [ + { + "$ref": "#/definitions/InCondition" + }, + { + "$ref": "#/definitions/NotInCondition" + }, + { + "$ref": "#/definitions/ContainsCondition" + }, + { + "$ref": "#/definitions/NotContainsCondition" + }, + { + "$ref": "#/definitions/EqualsCondition" + }, + { + "$ref": "#/definitions/NotEqualsCondition" + } + ] + } + }, + "rollout": { + "title": "Rollout", + "description": "\n Rollout rate controls how many buckets will be granted a feature when this segment matches.\n\n Rollout rates range from 0 (off) to 100 (all users). Rollout rates use `context.id`\n to determine bucket membership consistently over time.\n ", + "default": 0, + "type": "integer" + } + }, + "required": [ + "name", + "conditions" + ] + } + } +} From bbf5dca4daf9796aff8de7f04063e15d7b176063 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Thu, 8 Aug 2024 11:41:07 -0400 Subject: [PATCH 4/8] Add schema and validate() to Feature --- src/flagpole/__init__.py | 28 +++++++++++++--- tests/flagpole/test_feature.py | 58 ++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 4 deletions(-) diff --git a/src/flagpole/__init__.py b/src/flagpole/__init__.py index 353ac6e2fbd9e..b7a6f35543982 100644 --- a/src/flagpole/__init__.py +++ b/src/flagpole/__init__.py @@ -62,8 +62,11 @@ from __future__ import annotations import dataclasses +import functools +import os from typing import Any +import jsonschema import orjson import yaml @@ -75,6 +78,14 @@ class InvalidFeatureFlagConfiguration(Exception): pass +@functools.cache +def load_json_schema() -> dict[str, Any]: + path = os.path.join(os.path.dirname(__file__), "flagpole-schema.json") + with open(path, "rb") as json_file: + data = orjson.loads(json_file.read()) + return data + + @dataclasses.dataclass(frozen=True) class Feature: name: str @@ -103,6 +114,17 @@ def match(self, context: EvaluationContext) -> bool: return False + def validate(self) -> bool: + """ + Validate a feature against the JSON schema. + Will raise if the the current dict form a feature does not match the schema. + """ + dict_data = dataclasses.asdict(self) + spec = load_json_schema() + jsonschema.validate(dict_data, spec) + + return True + @classmethod def from_feature_dictionary(cls, name: str, config_dict: dict[str, Any]) -> Feature: segment_data = config_dict.get("segments") @@ -123,9 +145,7 @@ def from_feature_dictionary(cls, name: str, config_dict: dict[str, Any]) -> Feat return feature @classmethod - def from_feature_config_json( - cls, name: str, config_json: str, context_builder: ContextBuilder | None = None - ) -> Feature: + def from_feature_config_json(cls, name: str, config_json: str) -> Feature: try: config_data_dict = orjson.loads(config_json) except orjson.JSONDecodeError as decode_error: @@ -150,7 +170,7 @@ def from_bulk_json(cls, json: str) -> list[Feature]: return features @classmethod - def from_bulk_yaml(cls, yaml_str) -> list[Feature]: + def from_bulk_yaml(cls, yaml_str: str) -> list[Feature]: features: list[Feature] = [] parsed_yaml = yaml.safe_load(yaml_str) for feature, yaml_dict in parsed_yaml.items(): diff --git a/tests/flagpole/test_feature.py b/tests/flagpole/test_feature.py index 5b1a93ab1ca3a..b5c6762091e94 100644 --- a/tests/flagpole/test_feature.py +++ b/tests/flagpole/test_feature.py @@ -1,5 +1,6 @@ from dataclasses import dataclass +import jsonschema import orjson import pytest import yaml @@ -148,6 +149,63 @@ def test_invalid_json(self): with pytest.raises(InvalidFeatureFlagConfiguration): Feature.from_feature_config_json("foobar", "{") + def test_validate_invalid_schema(self): + config = """ + { + "owner": "sentry", + "segments": [ + { + "name": "", + "rollout": 1, + "conditions": [] + } + ] + } + """ + feature = Feature.from_feature_config_json("trash", config) + with pytest.raises(jsonschema.ValidationError) as err: + feature.validate() + assert "is too short" in str(err) + + config = """ + { + "owner": "sentry", + "segments": [ + { + "name": "allowed orgs", + "rollout": 1, + "conditions": [ + { + "property": "organization_slug", + "operator": "contains", + "value": ["derp"] + } + ] + } + ] + } + """ + feature = Feature.from_feature_config_json("trash", config) + with pytest.raises(jsonschema.ValidationError) as err: + feature.validate() + assert "'contains'} is not valid" in str(err) + + def test_validate_valid(self): + config = """ + { + "owner": "sentry", + "segments": [ + { + "name": "ga", + "rollout": 100, + "conditions": [] + } + ] + } + """ + feature = Feature.from_feature_config_json("redpaint", config) + assert feature.validate() + def test_empty_string_name(self): with pytest.raises(InvalidFeatureFlagConfiguration) as exception: Feature.from_feature_config_json("", '{"segments":[]}') From 45431c956061cef1e6df18f6ad054264c6e482a0 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Thu, 8 Aug 2024 15:00:44 -0400 Subject: [PATCH 5/8] Fix failing tests and createflag command --- src/flagpole/conditions.py | 4 ++-- src/sentry/runner/commands/createflag.py | 14 +++++++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/flagpole/conditions.py b/src/flagpole/conditions.py index 5d356a23e70e3..1b9c2c8aaf7ca 100644 --- a/src/flagpole/conditions.py +++ b/src/flagpole/conditions.py @@ -56,9 +56,9 @@ class ConditionBase: value: Any """The value to compare against the condition's evaluation context property.""" - operator: str | None = None + operator: str """ - The operator to use when comparing the evaluation context property to the condition's value. + The name of the operator to use when comparing the evaluation context property to the condition's value. Values must be a valid ConditionOperatorKind. """ diff --git a/src/sentry/runner/commands/createflag.py b/src/sentry/runner/commands/createflag.py index c5fa2a92e6771..4cfc4512fbbec 100644 --- a/src/sentry/runner/commands/createflag.py +++ b/src/sentry/runner/commands/createflag.py @@ -42,17 +42,17 @@ def condition_wizard(display_sample_condition_properties: bool = False) -> Condi operator_kind = click.prompt("Operator type", type=condition_type_choices, show_choices=True) if operator_kind == ConditionOperatorKind.IN: - return InCondition(value=[], property=property_name) + return InCondition(value=[], operator="in", property=property_name) elif operator_kind == ConditionOperatorKind.NOT_IN: - return NotInCondition(value=[], property=property_name) + return NotInCondition(value=[], operator="not_in", property=property_name) elif operator_kind == ConditionOperatorKind.EQUALS: - return EqualsCondition(value="", property=property_name) + return EqualsCondition(value="", operator="equals", property=property_name) elif operator_kind == ConditionOperatorKind.NOT_EQUALS: - return NotEqualsCondition(value="", property=property_name) + return NotEqualsCondition(value="", operator="not_equals", property=property_name) elif operator_kind == ConditionOperatorKind.CONTAINS: - return ContainsCondition(value="", property=property_name) + return ContainsCondition(value="", operator="contains", property=property_name) elif operator_kind == ConditionOperatorKind.NOT_CONTAINS: - return NotContainsCondition(value="", property=property_name) + return NotContainsCondition(value="", operator="not_contains", property=property_name) raise Exception("An unknown condition operator was provided") @@ -143,7 +143,7 @@ def createflag( name=f"feature.{scope}:{name}", owner=owner, segments=segments, - created_at=datetime.now(), + created_at=datetime.now().isoformat(), ) except Exception as err: raise click.ClickException(f"{err}") From d5455095531a3f2bc98f1d78545913c1225f4dd4 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Thu, 8 Aug 2024 16:39:39 -0400 Subject: [PATCH 6/8] Fix mistake --- tests/flagpole/test_conditions.py | 60 ++++++++++++++++--------------- 1 file changed, 31 insertions(+), 29 deletions(-) diff --git a/tests/flagpole/test_conditions.py b/tests/flagpole/test_conditions.py index bb5b3f326b683..4573d6a30b644 100644 --- a/tests/flagpole/test_conditions.py +++ b/tests/flagpole/test_conditions.py @@ -30,16 +30,16 @@ def test_returns_lowercase_string_set(self): class TestInConditions: def test_is_in(self): values = ["bar", "baz"] - condition = InCondition(property="foo", value=values) + condition = InCondition(property="foo", value=values, operator="in") assert condition.match(context=EvaluationContext({"foo": "bar"}), segment_name="test") - not_condition = NotInCondition(property="foo", value=values) + not_condition = NotInCondition(property="foo", value=values, operator="not_in") assert not not_condition.match( context=EvaluationContext({"foo": "bar"}), segment_name="test" ) int_values = [1, 2] - condition = InCondition(property="foo", value=int_values) + condition = InCondition(property="foo", value=int_values, operator="in") # Validation check to ensure no type coercion occurs assert condition.value == int_values assert condition.match(context=EvaluationContext({"foo": 2}), segment_name="test") @@ -47,39 +47,39 @@ def test_is_in(self): def test_is_in_numeric_string(self): values = ["123", "456"] - condition = InCondition(property="foo", value=values) + condition = InCondition(property="foo", value=values, operator="in") assert condition.value == values assert not condition.match(context=EvaluationContext({"foo": 123}), segment_name="test") assert condition.match(context=EvaluationContext({"foo": "123"}), segment_name="test") def test_is_not_in(self): values = ["bar", "baz"] - condition = InCondition(property="foo", value=values) + condition = InCondition(property="foo", value=values, operator="in") assert not condition.match(context=EvaluationContext({"foo": "foo"}), segment_name="test") - not_condition = NotInCondition(property="foo", value=values) + not_condition = NotInCondition(property="foo", value=values, operator="not_in") assert not_condition.match(context=EvaluationContext({"foo": "foo"}), segment_name="test") def test_is_in_case_insensitivity(self): values = ["bAr", "baz"] - condition = InCondition(property="foo", value=values) + condition = InCondition(property="foo", value=values, operator="in") assert condition.match(context=EvaluationContext({"foo": "BaR"}), segment_name="test") - not_condition = NotInCondition(property="foo", value=values) + not_condition = NotInCondition(property="foo", value=values, operator="not_in") assert not not_condition.match( context=EvaluationContext({"foo": "BaR"}), segment_name="test" ) def test_invalid_property_value(self): values = ["bar", "baz"] - condition = InCondition(property="foo", value=values) + condition = InCondition(property="foo", value=values, operator="in") bad_context = ([1], {"k": "v"}) for attr_val in bad_context: with pytest.raises(ConditionTypeMismatchException): condition.match(context=EvaluationContext({"foo": attr_val}), segment_name="test") - not_condition = NotInCondition(property="foo", value=values) + not_condition = NotInCondition(property="foo", value=values, operator="not_in") for attr_val in bad_context: with pytest.raises(ConditionTypeMismatchException): not_condition.match( @@ -88,12 +88,12 @@ def test_invalid_property_value(self): def test_missing_context_property(self): values = ["bar", "baz"] - in_condition = InCondition(property="foo", value=values) + in_condition = InCondition(property="foo", value=values, operator="in") assert not in_condition.match( context=EvaluationContext({"bar": "bar"}), segment_name="test" ) - not_on_condition = NotInCondition(property="foo", value=values) + not_on_condition = NotInCondition(property="foo", value=values, operator="not_in") assert not_on_condition.match( context=EvaluationContext({"bar": "bar"}), segment_name="test" ) @@ -101,28 +101,28 @@ def test_missing_context_property(self): class TestContainsConditions: def test_does_contain(self): - condition = ContainsCondition(property="foo", value="bar") + condition = ContainsCondition(property="foo", value="bar", operator="contains") assert condition.match( context=EvaluationContext({"foo": ["foo", "bar"]}), segment_name="test" ) - not_condition = NotContainsCondition(property="foo", value="bar") + not_condition = NotContainsCondition(property="foo", value="bar", operator="not_contains") assert not not_condition.match( context=EvaluationContext({"foo": ["foo", "bar"]}), segment_name="test" ) - condition = ContainsCondition(property="foo", value=1) + condition = ContainsCondition(property="foo", value=1, operator="contains") assert condition.match(context=EvaluationContext({"foo": [1, 2]}), segment_name="test") assert not condition.match(context=EvaluationContext({"foo": [3, 4]}), segment_name="test") def test_does_not_contain(self): values = "baz" - condition = ContainsCondition(property="foo", value=values) + condition = ContainsCondition(property="foo", value=values, operator="contains") assert not condition.match( context=EvaluationContext({"foo": ["foo", "bar"]}), segment_name="test" ) - not_condition = NotContainsCondition(property="foo", value=values) + not_condition = NotContainsCondition(property="foo", value=values, operator="not_contains") assert not_condition.match( context=EvaluationContext({"foo": ["foo", "bar"]}), segment_name="test" ) @@ -133,25 +133,27 @@ def test_invalid_property_provided(self): for attr_val in bad_context: with pytest.raises(ConditionTypeMismatchException): - condition = ContainsCondition(property="foo", value=values) + condition = ContainsCondition(property="foo", value=values, operator="contains") assert not condition.match( context=EvaluationContext({"foo": attr_val}), segment_name="test" ) for attr_val in bad_context: with pytest.raises(ConditionTypeMismatchException): - not_condition = NotContainsCondition(property="foo", value=values) + not_condition = NotContainsCondition( + property="foo", value=values, operator="not_contains" + ) assert not_condition.match( context=EvaluationContext({"foo": attr_val}), segment_name="test" ) def test_missing_context_property(self): - condition = ContainsCondition(property="foo", value="bar") + condition = ContainsCondition(property="foo", value="bar", operator="contains") with pytest.raises(ConditionTypeMismatchException): condition.match(context=EvaluationContext({"bar": ["foo", "bar"]}), segment_name="test") - not_condition = NotContainsCondition(property="foo", value="bar") + not_condition = NotContainsCondition(property="foo", value="bar", operator="not_contains") with pytest.raises(ConditionTypeMismatchException): not_condition.match( @@ -162,39 +164,39 @@ def test_missing_context_property(self): class TestEqualsConditions: def test_is_equal_string(self): value = "foo" - condition = EqualsCondition(property="foo", value=value) + condition = EqualsCondition(property="foo", value=value, operator="equals") assert condition.match(context=EvaluationContext({"foo": "foo"}), segment_name="test") - not_condition = NotEqualsCondition(property="foo", value=value) + not_condition = NotEqualsCondition(property="foo", value=value, operator="not_equals") assert not not_condition.match( context=EvaluationContext({"foo": "foo"}), segment_name="test" ) def test_is_not_equals(self): values = "bar" - condition = EqualsCondition(property="foo", value=values) + condition = EqualsCondition(property="foo", value=values, operator="equals") assert not condition.match(context=EvaluationContext({"foo": "foo"}), segment_name="test") - not_condition = NotEqualsCondition(property="foo", value=values) + not_condition = NotEqualsCondition(property="foo", value=values, operator="not_equals") assert not_condition.match(context=EvaluationContext({"foo": "foo"}), segment_name="test") def test_is_equal_case_insensitive(self): values = "bAr" - condition = EqualsCondition(property="foo", value=values) + condition = EqualsCondition(property="foo", value=values, operator="equals") assert condition.match(context=EvaluationContext({"foo": "BaR"}), segment_name="test") - not_condition = NotEqualsCondition(property="foo", value=values) + not_condition = NotEqualsCondition(property="foo", value=values, operator="not_equals") assert not not_condition.match( context=EvaluationContext({"foo": "BaR"}), segment_name="test" ) def test_equality_type_mismatch_strings(self): values = ["foo", "bar"] - condition = EqualsCondition(property="foo", value=values) + condition = EqualsCondition(property="foo", value=values, operator="equals") with pytest.raises(ConditionTypeMismatchException): condition.match(context=EvaluationContext({"foo": "foo"}), segment_name="test") - not_condition = NotEqualsCondition(property="foo", value=values) + not_condition = NotEqualsCondition(property="foo", value=values, operator="not_equals") with pytest.raises(ConditionTypeMismatchException): not_condition.match(context=EvaluationContext({"foo": "foo"}), segment_name="test") From a08a7f77c8a09423dde0cd864f3fe6e8ae169e7e Mon Sep 17 00:00:00 2001 From: Mark Story Date: Thu, 8 Aug 2024 16:58:31 -0400 Subject: [PATCH 7/8] Consolidate switches for matching operators to classes --- src/flagpole/conditions.py | 12 +++-- src/sentry/runner/commands/createflag.py | 34 ++++-------- tests/flagpole/test_conditions.py | 54 +++++++++---------- .../sentry/runner/commands/test_createflag.py | 2 +- 4 files changed, 46 insertions(+), 56 deletions(-) diff --git a/src/flagpole/conditions.py b/src/flagpole/conditions.py index 1b9c2c8aaf7ca..620f70fee0a3c 100644 --- a/src/flagpole/conditions.py +++ b/src/flagpole/conditions.py @@ -56,7 +56,7 @@ class ConditionBase: value: Any """The value to compare against the condition's evaluation context property.""" - operator: str + operator: str = dataclasses.field(default="") """ The name of the operator to use when comparing the evaluation context property to the condition's value. Values must be a valid ConditionOperatorKind. @@ -122,6 +122,7 @@ def _evaluate_equals(self, condition_property: Any, segment_name: str) -> bool: class InCondition(ConditionBase): value: InOperatorValueTypes + operator: str = dataclasses.field(default="in") def _operator_match(self, condition_property: Any, segment_name: str): return self._evaluate_in(condition_property=condition_property, segment_name=segment_name) @@ -129,6 +130,7 @@ def _operator_match(self, condition_property: Any, segment_name: str): class NotInCondition(ConditionBase): value: InOperatorValueTypes + operator: str = dataclasses.field(default="not_in") def _operator_match(self, condition_property: Any, segment_name: str): return not self._evaluate_in( @@ -141,6 +143,7 @@ def _operator_match(self, condition_property: Any, segment_name: str): class ContainsCondition(ConditionBase): value: ContainsOperatorValueTypes + operator: str = dataclasses.field(default="contains") def _operator_match(self, condition_property: Any, segment_name: str): return self._evaluate_contains( @@ -150,6 +153,7 @@ def _operator_match(self, condition_property: Any, segment_name: str): class NotContainsCondition(ConditionBase): value: ContainsOperatorValueTypes + operator: str = dataclasses.field(default="not_contains") def _operator_match(self, condition_property: Any, segment_name: str): return not self._evaluate_contains( @@ -162,6 +166,7 @@ def _operator_match(self, condition_property: Any, segment_name: str): class EqualsCondition(ConditionBase): value: EqualsOperatorValueTypes + operator: str = dataclasses.field(default="equals") def _operator_match(self, condition_property: Any, segment_name: str): return self._evaluate_equals( @@ -172,6 +177,7 @@ def _operator_match(self, condition_property: Any, segment_name: str): class NotEqualsCondition(ConditionBase): value: EqualsOperatorValueTypes + operator: str = dataclasses.field(default="not_equals") def _operator_match(self, condition_property: Any, segment_name: str): return not self._evaluate_equals( @@ -190,7 +196,7 @@ def _operator_match(self, condition_property: Any, segment_name: str): } -def _condition_from_dict(data: Mapping[str, Any]) -> ConditionBase: +def condition_from_dict(data: Mapping[str, Any]) -> ConditionBase: operator_kind = ConditionOperatorKind(data.get("operator", "invalid")) condition_cls = OPERATOR_LOOKUP[operator_kind] return condition_cls( @@ -216,7 +222,7 @@ class Segment: @classmethod def from_dict(cls, data: Mapping[str, Any]) -> Self: - conditions = [_condition_from_dict(condition) for condition in data.get("conditions", [])] + conditions = [condition_from_dict(condition) for condition in data.get("conditions", [])] return cls( name=str(data.get("name", "")), rollout=int(data.get("rollout", 0)), diff --git a/src/sentry/runner/commands/createflag.py b/src/sentry/runner/commands/createflag.py index 4cfc4512fbbec..fe7757c47b071 100644 --- a/src/sentry/runner/commands/createflag.py +++ b/src/sentry/runner/commands/createflag.py @@ -3,16 +3,7 @@ import click from flagpole import Feature, Segment -from flagpole.conditions import ( - ConditionBase, - ConditionOperatorKind, - ContainsCondition, - EqualsCondition, - InCondition, - NotContainsCondition, - NotEqualsCondition, - NotInCondition, -) +from flagpole.conditions import ConditionBase, ConditionOperatorKind, condition_from_dict from sentry.runner.decorators import configuration valid_scopes = ["organizations", "projects"] @@ -41,20 +32,15 @@ def condition_wizard(display_sample_condition_properties: bool = False) -> Condi property_name = click.prompt("Context property name", type=str) operator_kind = click.prompt("Operator type", type=condition_type_choices, show_choices=True) - if operator_kind == ConditionOperatorKind.IN: - return InCondition(value=[], operator="in", property=property_name) - elif operator_kind == ConditionOperatorKind.NOT_IN: - return NotInCondition(value=[], operator="not_in", property=property_name) - elif operator_kind == ConditionOperatorKind.EQUALS: - return EqualsCondition(value="", operator="equals", property=property_name) - elif operator_kind == ConditionOperatorKind.NOT_EQUALS: - return NotEqualsCondition(value="", operator="not_equals", property=property_name) - elif operator_kind == ConditionOperatorKind.CONTAINS: - return ContainsCondition(value="", operator="contains", property=property_name) - elif operator_kind == ConditionOperatorKind.NOT_CONTAINS: - return NotContainsCondition(value="", operator="not_contains", property=property_name) - - raise Exception("An unknown condition operator was provided") + value: str | list[str] = "" + if operator_kind in {ConditionOperatorKind.IN, ConditionOperatorKind.NOT_IN}: + value = [] + condition = { + "property": property_name, + "operator": operator_kind, + "value": value, + } + return condition_from_dict(condition) def segment_wizard() -> list[Segment]: diff --git a/tests/flagpole/test_conditions.py b/tests/flagpole/test_conditions.py index 4573d6a30b644..2ca326371021a 100644 --- a/tests/flagpole/test_conditions.py +++ b/tests/flagpole/test_conditions.py @@ -30,16 +30,16 @@ def test_returns_lowercase_string_set(self): class TestInConditions: def test_is_in(self): values = ["bar", "baz"] - condition = InCondition(property="foo", value=values, operator="in") + condition = InCondition(property="foo", value=values) assert condition.match(context=EvaluationContext({"foo": "bar"}), segment_name="test") - not_condition = NotInCondition(property="foo", value=values, operator="not_in") + not_condition = NotInCondition(property="foo", value=values) assert not not_condition.match( context=EvaluationContext({"foo": "bar"}), segment_name="test" ) int_values = [1, 2] - condition = InCondition(property="foo", value=int_values, operator="in") + condition = InCondition(property="foo", value=int_values) # Validation check to ensure no type coercion occurs assert condition.value == int_values assert condition.match(context=EvaluationContext({"foo": 2}), segment_name="test") @@ -54,32 +54,32 @@ def test_is_in_numeric_string(self): def test_is_not_in(self): values = ["bar", "baz"] - condition = InCondition(property="foo", value=values, operator="in") + condition = InCondition(property="foo", value=values) assert not condition.match(context=EvaluationContext({"foo": "foo"}), segment_name="test") - not_condition = NotInCondition(property="foo", value=values, operator="not_in") + not_condition = NotInCondition(property="foo", value=values) assert not_condition.match(context=EvaluationContext({"foo": "foo"}), segment_name="test") def test_is_in_case_insensitivity(self): values = ["bAr", "baz"] - condition = InCondition(property="foo", value=values, operator="in") + condition = InCondition(property="foo", value=values) assert condition.match(context=EvaluationContext({"foo": "BaR"}), segment_name="test") - not_condition = NotInCondition(property="foo", value=values, operator="not_in") + not_condition = NotInCondition(property="foo", value=values) assert not not_condition.match( context=EvaluationContext({"foo": "BaR"}), segment_name="test" ) def test_invalid_property_value(self): values = ["bar", "baz"] - condition = InCondition(property="foo", value=values, operator="in") + condition = InCondition(property="foo", value=values) bad_context = ([1], {"k": "v"}) for attr_val in bad_context: with pytest.raises(ConditionTypeMismatchException): condition.match(context=EvaluationContext({"foo": attr_val}), segment_name="test") - not_condition = NotInCondition(property="foo", value=values, operator="not_in") + not_condition = NotInCondition(property="foo", value=values) for attr_val in bad_context: with pytest.raises(ConditionTypeMismatchException): not_condition.match( @@ -88,12 +88,12 @@ def test_invalid_property_value(self): def test_missing_context_property(self): values = ["bar", "baz"] - in_condition = InCondition(property="foo", value=values, operator="in") + in_condition = InCondition(property="foo", value=values) assert not in_condition.match( context=EvaluationContext({"bar": "bar"}), segment_name="test" ) - not_on_condition = NotInCondition(property="foo", value=values, operator="not_in") + not_on_condition = NotInCondition(property="foo", value=values) assert not_on_condition.match( context=EvaluationContext({"bar": "bar"}), segment_name="test" ) @@ -101,17 +101,17 @@ def test_missing_context_property(self): class TestContainsConditions: def test_does_contain(self): - condition = ContainsCondition(property="foo", value="bar", operator="contains") + condition = ContainsCondition(property="foo", value="bar") assert condition.match( context=EvaluationContext({"foo": ["foo", "bar"]}), segment_name="test" ) - not_condition = NotContainsCondition(property="foo", value="bar", operator="not_contains") + not_condition = NotContainsCondition(property="foo", value="bar") assert not not_condition.match( context=EvaluationContext({"foo": ["foo", "bar"]}), segment_name="test" ) - condition = ContainsCondition(property="foo", value=1, operator="contains") + condition = ContainsCondition(property="foo", value=1) assert condition.match(context=EvaluationContext({"foo": [1, 2]}), segment_name="test") assert not condition.match(context=EvaluationContext({"foo": [3, 4]}), segment_name="test") @@ -122,7 +122,7 @@ def test_does_not_contain(self): context=EvaluationContext({"foo": ["foo", "bar"]}), segment_name="test" ) - not_condition = NotContainsCondition(property="foo", value=values, operator="not_contains") + not_condition = NotContainsCondition(property="foo", value=values) assert not_condition.match( context=EvaluationContext({"foo": ["foo", "bar"]}), segment_name="test" ) @@ -133,27 +133,25 @@ def test_invalid_property_provided(self): for attr_val in bad_context: with pytest.raises(ConditionTypeMismatchException): - condition = ContainsCondition(property="foo", value=values, operator="contains") + condition = ContainsCondition(property="foo", value=values) assert not condition.match( context=EvaluationContext({"foo": attr_val}), segment_name="test" ) for attr_val in bad_context: with pytest.raises(ConditionTypeMismatchException): - not_condition = NotContainsCondition( - property="foo", value=values, operator="not_contains" - ) + not_condition = NotContainsCondition(property="foo", value=values) assert not_condition.match( context=EvaluationContext({"foo": attr_val}), segment_name="test" ) def test_missing_context_property(self): - condition = ContainsCondition(property="foo", value="bar", operator="contains") + condition = ContainsCondition(property="foo", value="bar") with pytest.raises(ConditionTypeMismatchException): condition.match(context=EvaluationContext({"bar": ["foo", "bar"]}), segment_name="test") - not_condition = NotContainsCondition(property="foo", value="bar", operator="not_contains") + not_condition = NotContainsCondition(property="foo", value="bar") with pytest.raises(ConditionTypeMismatchException): not_condition.match( @@ -164,28 +162,28 @@ def test_missing_context_property(self): class TestEqualsConditions: def test_is_equal_string(self): value = "foo" - condition = EqualsCondition(property="foo", value=value, operator="equals") + condition = EqualsCondition(property="foo", value=value) assert condition.match(context=EvaluationContext({"foo": "foo"}), segment_name="test") - not_condition = NotEqualsCondition(property="foo", value=value, operator="not_equals") + not_condition = NotEqualsCondition(property="foo", value=value) assert not not_condition.match( context=EvaluationContext({"foo": "foo"}), segment_name="test" ) def test_is_not_equals(self): values = "bar" - condition = EqualsCondition(property="foo", value=values, operator="equals") + condition = EqualsCondition(property="foo", value=values) assert not condition.match(context=EvaluationContext({"foo": "foo"}), segment_name="test") - not_condition = NotEqualsCondition(property="foo", value=values, operator="not_equals") + not_condition = NotEqualsCondition(property="foo", value=values) assert not_condition.match(context=EvaluationContext({"foo": "foo"}), segment_name="test") def test_is_equal_case_insensitive(self): values = "bAr" - condition = EqualsCondition(property="foo", value=values, operator="equals") + condition = EqualsCondition(property="foo", value=values) assert condition.match(context=EvaluationContext({"foo": "BaR"}), segment_name="test") - not_condition = NotEqualsCondition(property="foo", value=values, operator="not_equals") + not_condition = NotEqualsCondition(property="foo", value=values) assert not not_condition.match( context=EvaluationContext({"foo": "BaR"}), segment_name="test" ) @@ -197,6 +195,6 @@ def test_equality_type_mismatch_strings(self): with pytest.raises(ConditionTypeMismatchException): condition.match(context=EvaluationContext({"foo": "foo"}), segment_name="test") - not_condition = NotEqualsCondition(property="foo", value=values, operator="not_equals") + not_condition = NotEqualsCondition(property="foo", value=values) with pytest.raises(ConditionTypeMismatchException): not_condition.match(context=EvaluationContext({"foo": "foo"}), segment_name="test") diff --git a/tests/sentry/runner/commands/test_createflag.py b/tests/sentry/runner/commands/test_createflag.py index 5ab7c16d9d95c..57e8f27d73e31 100644 --- a/tests/sentry/runner/commands/test_createflag.py +++ b/tests/sentry/runner/commands/test_createflag.py @@ -70,7 +70,7 @@ def test_all_condition_types(self): "--owner=Test Owner", input="\n".join(cli_input), ) - assert rv.exit_code == 0 + assert rv.exit_code == 0, rv.output parsed_feature = self.convert_output_to_feature(rv.output) assert parsed_feature.name == "feature.organizations:new-flag" assert parsed_feature.owner == "Test Owner" From 8a10b73babeefd1428b3c5856e2402349ed3686f Mon Sep 17 00:00:00 2001 From: Mark Story Date: Wed, 14 Aug 2024 17:55:33 -0400 Subject: [PATCH 8/8] Improve error handling for invalid operators --- src/flagpole/__init__.py | 4 +++- src/flagpole/conditions.py | 4 ++++ tests/flagpole/test_feature.py | 18 ++++++++++++++++++ 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/flagpole/__init__.py b/src/flagpole/__init__.py index b7a6f35543982..7018eaf175db7 100644 --- a/src/flagpole/__init__.py +++ b/src/flagpole/__init__.py @@ -140,7 +140,9 @@ def from_feature_dictionary(cls, name: str, config_dict: dict[str, Any]) -> Feat segments=segments, ) except Exception as exc: - raise InvalidFeatureFlagConfiguration("Provided JSON is not a valid feature") from exc + raise InvalidFeatureFlagConfiguration( + "Provided config_dict is not a valid feature" + ) from exc return feature diff --git a/src/flagpole/conditions.py b/src/flagpole/conditions.py index 620f70fee0a3c..62d7ab6363d21 100644 --- a/src/flagpole/conditions.py +++ b/src/flagpole/conditions.py @@ -198,6 +198,10 @@ def _operator_match(self, condition_property: Any, segment_name: str): def condition_from_dict(data: Mapping[str, Any]) -> ConditionBase: operator_kind = ConditionOperatorKind(data.get("operator", "invalid")) + if operator_kind not in OPERATOR_LOOKUP: + valid = ", ".join(OPERATOR_LOOKUP.keys()) + raise ValueError(f"The {operator_kind} is not a known operator. Choose from {valid}") + condition_cls = OPERATOR_LOOKUP[operator_kind] return condition_cls( property=str(data.get("property")), operator=operator_kind.value, value=data.get("value") diff --git a/tests/flagpole/test_feature.py b/tests/flagpole/test_feature.py index b5c6762091e94..df73fc1cb0131 100644 --- a/tests/flagpole/test_feature.py +++ b/tests/flagpole/test_feature.py @@ -216,6 +216,24 @@ def test_missing_segments(self): Feature.from_feature_config_json("foo", "{}") assert "Feature has no segments defined" in str(exception) + def test_invalid_operator_condition(self): + config = """ + { + "owner": "sentry", + "segments": [ + { + "name": "derp", + "conditions": [ + {"property": "user_email", "operator": "trash", "value": 1} + ] + } + ] + } + """ + with pytest.raises(InvalidFeatureFlagConfiguration) as exception: + Feature.from_feature_config_json("foo", config) + assert "Provided config_dict is not a valid feature" in str(exception) + def test_enabled_feature(self): feature = Feature.from_feature_config_json( "foo",