diff --git a/src/flagpole/__init__.py b/src/flagpole/__init__.py index 98cf99c018b028..7018eaf175db7e 100644 --- a/src/flagpole/__init__.py +++ b/src/flagpole/__init__.py @@ -61,12 +61,14 @@ """ from __future__ import annotations -from datetime import datetime +import dataclasses +import functools +import os from typing import Any +import jsonschema 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 +78,29 @@ class InvalidFeatureFlagConfiguration(Exception): pass -class Feature(BaseModel): - name: constr(min_length=1, to_lower=True) = Field( # type:ignore[valid-type] - description="The feature name." - ) +@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 "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=True) "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: @@ -109,24 +114,40 @@ def match(self, context: EvaluationContext) -> bool: return False - @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)) + 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") + if not isinstance(segment_data, list): + raise InvalidFeatureFlagConfiguration("Feature has no segments defined") try: - feature = cls(name=name, **config_dict) - except ValidationError as exc: - raise InvalidFeatureFlagConfiguration("Provided JSON is not a valid feature") from exc + 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", True)), + created_at=str(config_dict.get("created_at")), + segments=segments, + ) + except Exception as exc: + raise InvalidFeatureFlagConfiguration( + "Provided config_dict is not a valid feature" + ) from exc 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: @@ -135,6 +156,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 @@ -148,7 +172,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(): @@ -157,9 +181,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 f93139a6dbf6ae..62d7ab6363d215 100644 --- a/src/flagpole/conditions.py +++ b/src/flagpole/conditions.py @@ -1,8 +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 pydantic import BaseModel, Field, StrictBool, StrictFloat, StrictInt, StrictStr, constr +from typing import Any, Self, TypeVar from flagpole.evaluation_context import EvaluationContext @@ -48,20 +48,20 @@ 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: 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. + """ + def match(self, context: EvaluationContext, segment_name: str) -> bool: return self._operator_match( condition_property=context.get(self.property), segment_name=segment_name @@ -99,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)): @@ -121,20 +117,20 @@ 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): - operator: Literal[ConditionOperatorKind.IN] = ConditionOperatorKind.IN 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) class NotInCondition(ConditionBase): - operator: Literal[ConditionOperatorKind.NOT_IN] = ConditionOperatorKind.NOT_IN value: InOperatorValueTypes + operator: str = dataclasses.field(default="not_in") def _operator_match(self, condition_property: Any, segment_name: str): return not self._evaluate_in( @@ -142,12 +138,12 @@ def _operator_match(self, condition_property: Any, segment_name: str): ) -ContainsOperatorValueTypes = StrictInt | StrictStr | StrictFloat +ContainsOperatorValueTypes = int | str | float class ContainsCondition(ConditionBase): - operator: Literal[ConditionOperatorKind.CONTAINS] = ConditionOperatorKind.CONTAINS value: ContainsOperatorValueTypes + operator: str = dataclasses.field(default="contains") def _operator_match(self, condition_property: Any, segment_name: str): return self._evaluate_contains( @@ -156,8 +152,8 @@ def _operator_match(self, condition_property: Any, segment_name: str): class NotContainsCondition(ConditionBase): - operator: Literal[ConditionOperatorKind.NOT_CONTAINS] = ConditionOperatorKind.NOT_CONTAINS value: ContainsOperatorValueTypes + operator: str = dataclasses.field(default="not_contains") def _operator_match(self, condition_property: Any, segment_name: str): return not self._evaluate_contains( @@ -165,85 +161,62 @@ 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, - ) - """Whether the condition should enable strict validation, raising an exception if the evaluation context property is missing""" + operator: str = dataclasses.field(default="equals") 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 = Field( - description="Whether the condition should enable strict validation, raising an exception if the evaluation context property is missing", - default=False, - ) - """Whether the condition should enable strict validation, raising an exception if the evaluation context property is missing""" + operator: str = dataclasses.field(default="not_equals") 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, + ConditionOperatorKind.CONTAINS: ContainsCondition, + ConditionOperatorKind.NOT_CONTAINS: NotContainsCondition, + ConditionOperatorKind.EQUALS: EqualsCondition, + ConditionOperatorKind.NOT_EQUALS: NotEqualsCondition, +} -class Segment(BaseModel): - name: constr(min_length=1) = Field( # type:ignore[valid-type] - description="A brief description or identifier for the segment" +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") ) + + +@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 +224,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) diff --git a/src/flagpole/flagpole-schema.json b/src/flagpole/flagpole-schema.json new file mode 100644 index 00000000000000..b0f9512dd7b3dd --- /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" + ] + } + } +} diff --git a/src/sentry/runner/commands/createflag.py b/src/sentry/runner/commands/createflag.py index c5fa2a92e67714..fe7757c47b0719 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=[], property=property_name) - elif operator_kind == ConditionOperatorKind.NOT_IN: - return NotInCondition(value=[], property=property_name) - elif operator_kind == ConditionOperatorKind.EQUALS: - return EqualsCondition(value="", property=property_name) - elif operator_kind == ConditionOperatorKind.NOT_EQUALS: - return NotEqualsCondition(value="", property=property_name) - elif operator_kind == ConditionOperatorKind.CONTAINS: - return ContainsCondition(value="", property=property_name) - elif operator_kind == ConditionOperatorKind.NOT_CONTAINS: - return NotContainsCondition(value="", 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]: @@ -143,7 +129,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}") diff --git a/tests/flagpole/test_conditions.py b/tests/flagpole/test_conditions.py index 5d7b1023cede50..2ca326371021ad 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) @@ -86,7 +47,7 @@ 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") @@ -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"] @@ -161,7 +117,7 @@ def test_does_contain(self): 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" ) @@ -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") @@ -246,7 +190,7 @@ def test_is_equal_case_insensitive(self): 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") @@ -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 cb0ff84685888a..df73fc1cb01317 100644 --- a/tests/flagpole/test_feature.py +++ b/tests/flagpole/test_feature.py @@ -1,6 +1,6 @@ from dataclasses import dataclass -from datetime import datetime, timezone +import jsonschema import orjson import pytest import yaml @@ -31,7 +31,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 == [] @@ -149,15 +149,90 @@ 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":[]}') - 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_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( @@ -229,12 +304,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] diff --git a/tests/sentry/runner/commands/test_createflag.py b/tests/sentry/runner/commands/test_createflag.py index 5ab7c16d9d95c2..57e8f27d73e31d 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"