diff --git a/api/experimentation/services.py b/api/experimentation/services.py index ee8c1c2bff75..ed064b4556f2 100644 --- a/api/experimentation/services.py +++ b/api/experimentation/services.py @@ -16,7 +16,7 @@ from django.db import transaction from django.db.models import Q from django.utils import timezone -from flag_engine.segments.constants import PERCENTAGE_SPLIT +from flag_engine.segments.constants import ALL_RULE, PERCENTAGE_SPLIT from rest_framework.exceptions import ValidationError from audit.models import AuditLog @@ -81,6 +81,9 @@ from integrations.flagsmith.client import get_openfeature_client from segments.models import Condition, Segment, SegmentRule +# TODO: Delete alias as per https://github.com/Flagsmith/flagsmith/issues/7818 +from segments.types import SegmentRule as SegmentRuleType + _ROLLOUT_VALUE_TYPE: dict[str, "FeatureValueType"] = { INTEGER: "integer", STRING: "string", @@ -645,6 +648,23 @@ def transition_experiment_status( return experiment +def _rollout_segment_rules(rollout_percentage: float) -> list[SegmentRuleType]: + return [ + { + "type": ALL_RULE, + "conditions": [ + { + "property": "$.identity.key", + "operator": PERCENTAGE_SPLIT, + "value": str(rollout_percentage), + "description": None, + } + ], + "rules": [], + } + ] + + def _create_rollout_segment( experiment: Experiment, rollout_percentage: float ) -> Segment: @@ -652,7 +672,10 @@ def _create_rollout_segment( name=f"experiment-{experiment.id}-rollout", project=experiment.feature.project, is_system_segment=True, + rules_data=_rollout_segment_rules(rollout_percentage), ) + + # TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 rule = SegmentRule.objects.create(segment=segment, type=SegmentRule.ALL_RULE) Condition.objects.create( rule=rule, @@ -660,6 +683,7 @@ def _create_rollout_segment( property="$.identity.key", value=str(rollout_percentage), ) + return segment @@ -684,11 +708,16 @@ def validate_rollout_spec(experiment: Experiment, spec: RolloutSpec) -> None: def _sync_rollout_segment(experiment: Experiment, rollout_percentage: float) -> Segment: segment = experiment.rollout_segment if segment is not None: + segment.rules_data = _rollout_segment_rules(rollout_percentage) + segment.save(update_fields=["rules_data"]) + + # TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 condition = Condition.objects.get( rule__segment=segment, operator=PERCENTAGE_SPLIT ) condition.value = str(rollout_percentage) condition.save() + return segment segment = _create_rollout_segment(experiment, rollout_percentage) experiment.rollout_segment = segment diff --git a/api/integrations/launch_darkly/services.py b/api/integrations/launch_darkly/services.py index 9c33997bb588..800f1e8c9d4e 100644 --- a/api/integrations/launch_darkly/services.py +++ b/api/integrations/launch_darkly/services.py @@ -6,8 +6,10 @@ from django.conf import settings from django.core import signing +from django.db import transaction from django.utils import timezone from flag_engine.segments import constants +from flag_engine.segments.types import ConditionOperator from requests.exceptions import RequestException from environments.identities.models import Identity @@ -41,6 +43,10 @@ from projects.tags.models import Tag from segment_membership.services import enqueue_membership_refresh from segments.models import Condition, Segment, SegmentRule +from segments.types import SegmentCondition + +# TODO: Delete alias as per https://github.com/Flagsmith/flagsmith/issues/7818 +from segments.types import SegmentRule as SegmentRuleType from users.models import FFAdminUser from util.db import closing_stale_connections from util.util import iter_chunked_concat, truncate @@ -141,7 +147,7 @@ def _create_tags_from_ld( return tags_by_ld_tag -def _ld_operator_to_flagsmith_operator(ld_operator: str) -> Optional[str]: +def _ld_operator_to_flagsmith_operator(ld_operator: str) -> Optional[ConditionOperator]: """ Convert a Launch Darkly operator to its closest Flagsmith equivalent. If not convertible, return None. @@ -290,6 +296,97 @@ def _create_feature_segments_for_segment_match_clauses( return feature_states +def _add_clauses_to_segment_rule( + import_request: LaunchDarklyImportRequest, + segment_name: str, + clauses: list[Clause], + rule: SegmentRuleType, +) -> None: + """Add Launch Darkly clauses to a segment's "ALL" root rule as subrules.""" + subrules = rule["rules"] + negated_subrule_index: Optional[int] = None + + for clause in clauses: + _property = clause["attribute"] + operator = _ld_operator_to_flagsmith_operator(clause["op"]) + if operator is None: + _log_error( + import_request=import_request, + error_message=f"Can't map launch darkly operator: {clause['op']}" + f" skipping for segment: {segment_name}", + ) + continue + + conditions: list[SegmentCondition] = [] + for value in _convert_ld_values( + [str(value) for value in clause["values"]], clause["op"] + ): + if len(value) > settings.SEGMENT_CONDITION_VALUE_LIMIT: + _log_error( + import_request=import_request, + error_message=( + f"Segment condition value '{truncate(value)}' for property '{_property}' exceeds the limit of" + f" {settings.SEGMENT_CONDITION_VALUE_LIMIT} characters," + f" skipping for segment '{segment_name}'" + ), + ) + continue + conditions.append( + { + "property": _property, + "operator": operator, + "value": value, + "description": None, + } + ) + + if clause["negate"] is True: + if negated_subrule_index is None: + subrules.append({"type": constants.NONE_RULE, "conditions": []}) + negated_subrule_index = len(subrules) - 1 + subrules[negated_subrule_index]["conditions"] += conditions + else: + subrules.append({"type": constants.ANY_RULE, "conditions": conditions}) + + +def _add_users_to_segment_rule( + import_request: LaunchDarklyImportRequest, + segment_name: str, + users: list[str], + negate: bool, + rule: SegmentRuleType, +) -> None: + """Add Launch Darkly's targeted user lists to a segment's "ALL" root rule as subrules.""" + for identities_string in iter_chunked_concat( + values=users, + delimiter=",", + max_len=settings.SEGMENT_CONDITION_VALUE_LIMIT, + ): + if len(identities_string) > settings.SEGMENT_CONDITION_VALUE_LIMIT: + _log_error( + import_request=import_request, + error_message=( + f"Targeting key '{truncate(identities_string)}' exceeds the limit of" + f" {settings.SEGMENT_CONDITION_VALUE_LIMIT} characters, " + f"skipping for segment '{segment_name}'" + ), + ) + continue + rule["rules"].append( + { + "type": constants.NONE_RULE if negate else constants.ANY_RULE, + "conditions": [ + { + "property": "key", + "operator": constants.IN, + "value": identities_string, + "description": None, + } + ], + } + ) + + def _create_segment_rule_for_segment( import_request: LaunchDarklyImportRequest, segment: Segment, @@ -341,14 +438,6 @@ def _create_segment_rule_for_segment( # Create a condition for each value. Each condition is "OR"ed together. for value in values: if len(value) > settings.SEGMENT_CONDITION_VALUE_LIMIT: - _log_error( - import_request=import_request, - error_message=( - f"Segment condition value '{truncate(value)}' for property '{_property}' exceeds the limit of" - f" {settings.SEGMENT_CONDITION_VALUE_LIMIT} characters," - f" skipping for segment '{segment.name}'" - ), - ) continue Condition.objects.update_or_create( rule=target_rule, @@ -357,12 +446,6 @@ def _create_segment_rule_for_segment( operator=operator, created_with_segment=True, ) - else: - _log_error( - import_request=import_request, - error_message=f"Can't map launch darkly operator: {clause['op']}" - f" skipping for segment: {segment.name}", - ) return parent_rule @@ -409,7 +492,21 @@ def _create_feature_segment_from_clauses( name=rule_name, project=project, feature=feature ) + rules: list[SegmentRuleType] = ( + segment.rules_data # LaunchDarkly environments share the segment + or [{"type": constants.ALL_RULE, "conditions": [], "rules": []}] + ) + _add_clauses_to_segment_rule( + import_request=import_request, + segment_name=segment.name, + clauses=clauses, + rule=rules[0], + ) + segment.rules_data = rules + segment.save(update_fields=["rules_data"]) + # Create a targeting rule for the new feature-specific segment. + # TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 _create_segment_rule_for_segment( import_request=import_request, segment=segment, @@ -974,14 +1071,6 @@ def _include_users_to_segment( max_len=settings.SEGMENT_CONDITION_VALUE_LIMIT, ): if len(identities_string) > settings.SEGMENT_CONDITION_VALUE_LIMIT: - _log_error( - import_request=import_request, - error_message=( - f"Targeting key '{truncate(identities_string)}' exceeds the limit of" - f" {settings.SEGMENT_CONDITION_VALUE_LIMIT} characters, " - f"skipping for segment '{segment.name}'" - ), - ) continue included_rule = SegmentRule.objects.create( rule=parent_rule, @@ -1024,9 +1113,22 @@ def _create_segments_from_ld( # TODO: Tagging segments is not supported yet. https://github.com/Flagsmith/flagsmith/issues/3241 + root_rule: SegmentRuleType = { + "type": constants.ALL_RULE, + "conditions": [], + "rules": [], + } + # Create the segment rule for the segment. rules = ld_segment["rules"] for rule in rules: + _add_clauses_to_segment_rule( + import_request=import_request, + segment_name=segment.name, + clauses=rule["clauses"], + rule=root_rule, + ) + # TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 _create_segment_rule_for_segment( import_request=import_request, segment=segment, @@ -1048,6 +1150,22 @@ def _create_segments_from_ld( ] ) + _add_users_to_segment_rule( + import_request=import_request, + segment_name=segment.name, + users=ld_segment["included"], + negate=False, + rule=root_rule, + ) + _add_users_to_segment_rule( + import_request=import_request, + segment_name=segment.name, + users=ld_segment["excluded"], + negate=True, + rule=root_rule, + ) + + # TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 _include_users_to_segment( import_request=import_request, segment=segment, @@ -1072,8 +1190,12 @@ def _create_segments_from_ld( # Create an empty rule if there are no rules. This is required to create an "SegmentRule" object. # Otherwise, UI fails to display the segment. + # TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 SegmentRule.objects.get_or_create(segment=segment, type=SegmentRule.ALL_RULE) + segment.rules_data = [root_rule] + segment.save(update_fields=["rules_data"]) + return segments_by_ld_key @@ -1152,41 +1274,44 @@ def process_import_request( ) raise - # Create environments - environments_by_ld_environment_key = _create_environments_from_ld( - ld_environments=ld_environments, - project_id=import_request.project_id, - ) + with transaction.atomic(): + # Create environments + environments_by_ld_environment_key = _create_environments_from_ld( + ld_environments=ld_environments, + project_id=import_request.project_id, + ) - # Create segments using `ld_segment_tags` - # TODO populate with LD tags when https://github.com/Flagsmith/flagsmith/issues/3241 is done - segment_tags_by_ld_tag: dict[str, Tag] = {} - segments_by_ld_key = _create_segments_from_ld( - import_request=import_request, - ld_segments=ld_segments, - environments_by_ld_environment_key=environments_by_ld_environment_key, - tags_by_ld_tag=segment_tags_by_ld_tag, - project_id=import_request.project_id, - ) + # Create segments using `ld_segment_tags` + # TODO populate with LD tags when https://github.com/Flagsmith/flagsmith/issues/3241 is done + segment_tags_by_ld_tag: dict[str, Tag] = {} + segments_by_ld_key = _create_segments_from_ld( + import_request=import_request, + ld_segments=ld_segments, + environments_by_ld_environment_key=environments_by_ld_environment_key, + tags_by_ld_tag=segment_tags_by_ld_tag, + project_id=import_request.project_id, + ) - # Create flags - flag_tags_by_ld_tag = _create_tags_from_ld( - ld_tags=ld_flag_tags, - project_id=import_request.project_id, - ) - _create_features_from_ld( - import_request=import_request, - ld_flags=ld_flags, - environments_by_ld_environment_key=environments_by_ld_environment_key, - tags_by_ld_tag=flag_tags_by_ld_tag, - segments_by_ld_key=segments_by_ld_key, - project_id=import_request.project_id, - ) + # Create flags + flag_tags_by_ld_tag = _create_tags_from_ld( + ld_tags=ld_flag_tags, + project_id=import_request.project_id, + ) + _create_features_from_ld( + import_request=import_request, + ld_flags=ld_flags, + environments_by_ld_environment_key=environments_by_ld_environment_key, + tags_by_ld_tag=flag_tags_by_ld_tag, + segments_by_ld_key=segments_by_ld_key, + project_id=import_request.project_id, + ) - # Count deprecated flags for reporting - import_request.status["deprecated_flag_count"] = sum( - 1 for ld_flag in ld_flags if ld_flag["deprecated"] - ) + # Count deprecated flags for reporting + import_request.status["deprecated_flag_count"] = sum( + 1 for ld_flag in ld_flags if ld_flag["deprecated"] + ) - # Refresh membership counts for the segments the import just created. - enqueue_membership_refresh(import_request.project) + # Refresh membership counts for the segments the import just created. + transaction.on_commit( + lambda: enqueue_membership_refresh(import_request.project) + ) diff --git a/api/segments/migrations/0032_add_segment_rules_data.py b/api/segments/migrations/0032_add_segment_rules_data.py new file mode 100644 index 000000000000..77f78a82a9b6 --- /dev/null +++ b/api/segments/migrations/0032_add_segment_rules_data.py @@ -0,0 +1,107 @@ +# Generated by Django 5.2.16 on 2026-08-07 15:09 +import typing + +from django.apps.registry import Apps +from django.db import migrations, models +from django.db.backends.base.schema import BaseDatabaseSchemaEditor + +SegmentRule = dict[str, typing.Any] + +BATCH_SIZE = 500 + + +def backfill_segment_rules_data( + apps: Apps, _: BaseDatabaseSchemaEditor | None = None +) -> None: + Segment = apps.get_model("segments", "Segment") + SegmentRule = apps.get_model("segments", "SegmentRule") + Condition = apps.get_model("segments", "Condition") + + rules = SegmentRule.objects.filter(deleted_at__isnull=True).only( + "segment_id", "rule_id", "type" + ) + conditions = Condition.objects.filter(deleted_at__isnull=True).only( + "rule_id", "property", "operator", "value", "description" + ) + + segments = Segment.objects.filter( + id=models.F("version_of"), # Means "current version" + deleted_at__isnull=True, + ).only("id").prefetch_related( + models.Prefetch("rules", rules, to_attr="live_rules"), + models.Prefetch("live_rules__conditions", conditions, to_attr="live_conditions"), + models.Prefetch("live_rules__rules", rules, to_attr="live_rules"), + models.Prefetch("live_rules__live_rules__conditions", conditions, to_attr="live_conditions"), + models.Prefetch("live_rules__live_rules__rules", rules, to_attr="live_rules"), # Empty almost all cases + ).order_by("id") + + last_id = 0 # don't leroy jenkins local memory + while segments_chunk := list(segments.filter(id__gt=last_id)[:BATCH_SIZE]): + for segment in segments_chunk: + segment.rules_data = _rasterise_segment_rules(segment) + Segment.objects.bulk_update(segments_chunk, fields=["rules_data"]) + last_id = segments_chunk[-1].id + + +def nullify_segment_rules_data( + apps: Apps, _: BaseDatabaseSchemaEditor | None = None +) -> None: + Segment = apps.get_model("segments", "Segment") + Segment.objects.filter(rules_data__isnull=False).update(rules_data=None) + + +def _rasterise_segment_rules(obj: typing.Any) -> list[SegmentRule]: + return [ + _rasterise_segment_rule(rule) + for rule in ( + obj.live_rules + if hasattr(obj, "live_rules") # We only prefetch two levels deep + else obj.rules.filter(deleted_at__isnull=True) + ) + ] + + +def _rasterise_segment_rule(rule: typing.Any) -> SegmentRule: + rule_data: SegmentRule = { + "type": rule.type, + "conditions": [ + { + "property": condition.property, + "operator": condition.operator, + "value": condition.value, + "description": condition.description, + } + for condition in ( + rule.live_conditions + if hasattr(rule, "live_conditions") # We only prefetch two levels deep + else rule.conditions.filter(deleted_at__isnull=True) + ) + ], + } + if (subrules := _rasterise_segment_rules(rule)): + rule_data["rules"] = subrules + return rule_data + + +class Migration(migrations.Migration): + + dependencies = [ + ("segments", "0031_segment_managed_by"), + ] + + operations = [ + migrations.AddField( + model_name="historicalsegment", + name="rules_data", + field=models.JSONField(null=True), + ), + migrations.AddField( + model_name="segment", + name="rules_data", + field=models.JSONField(null=True), + ), + migrations.RunPython( + code=backfill_segment_rules_data, + reverse_code=nullify_segment_rules_data, + ), + ] diff --git a/api/segments/models.py b/api/segments/models.py index 12022af3991a..2fd49c38716d 100644 --- a/api/segments/models.py +++ b/api/segments/models.py @@ -30,6 +30,9 @@ from projects.models import Project from segments.services import copy_segment_rules_and_conditions +# TODO: Delete alias as per https://github.com/Flagsmith/flagsmith/issues/7818 +from segments.types import SegmentRule as SegmentRuleType + ModelT = typing.TypeVar("ModelT", bound=models.Model) logger = logging.getLogger(__name__) @@ -105,6 +108,10 @@ class Segment( Feature, on_delete=models.CASCADE, related_name="segments", null=True ) + rules_data: models.JSONField[ + list[SegmentRuleType], list[SegmentRuleType] | None + ] = models.JSONField(null=True) + version = models.IntegerField(default=1, null=True) version_of = models.ForeignKey( diff --git a/api/segments/serializers.py b/api/segments/serializers.py index eb0caee4ed92..9e9c36fd4bb6 100644 --- a/api/segments/serializers.py +++ b/api/segments/serializers.py @@ -1,8 +1,9 @@ -from typing import Any +from typing import Any, cast import structlog from django.conf import settings from django.db import transaction +from drf_spectacular.utils import extend_schema_field from drf_writable_nested.serializers import WritableNestedModelSerializer from rest_framework import serializers from rest_framework.exceptions import ValidationError @@ -13,12 +14,20 @@ from segment_membership.constants import MAX_SEGMENT_MEMBERS_PAGE_SIZE from segment_membership.models import SegmentMembershipCount from segment_membership.services import enqueue_membership_refresh -from segments.models import Condition, Segment, SegmentRule +from segments.models import Condition, Segment, SegmentRule, WhitelistedSegment +from segments.types import ( + LegacySegmentRule, +) + +# TODO: Delete alias as per https://github.com/Flagsmith/flagsmith/issues/7818 +from segments.types import SegmentRule as SegmentRuleType logger = structlog.get_logger(__name__) DictList = list[dict[str, Any]] +SEGMENT_RULES_MAX_DEPTH = 2 + class SegmentMembershipCountSerializer( serializers.ModelSerializer[SegmentMembershipCount] @@ -74,6 +83,8 @@ class Meta: ] +# TODO: Replace with list[types.SegmentRule] as per https://github.com/Flagsmith/flagsmith/issues/7818 +@extend_schema_field(list[LegacySegmentRule]) # type: ignore[arg-type] class SegmentRuleSerializer(_BaseSegmentRuleSerializer): rules = _NestedSegmentRuleSerializer( many=True, @@ -101,6 +112,8 @@ def __init__(self, *args: Any, **kwargs: Any) -> None: Because WritableNestedModelSerializer uses `initial_data` instead of `data` we need to override the `__init__` method to remove rules and conditions that are marked for deletion. + + TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 """ data = kwargs.get("data") if data and "rules" in data: @@ -128,7 +141,15 @@ class Meta: "membership_counts", "managed_by", ] - read_only_fields = ["membership_counts", "managed_by"] + read_only_fields = [ + "managed_by", + "membership_counts", + "project", + ] + + def to_internal_value(self, data: dict[str, Any]) -> Any: + self._validate_rules_depth(data.get("rules", [])) + return super().to_internal_value(data) def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: attrs = super().validate(attrs) @@ -140,12 +161,16 @@ def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: organisation = project.organisation self._validate_required_metadata(organisation, metadata, project) - self._validate_segment_rules_conditions_limit(attrs["rules"]) self._validate_project_segment_limit(project) + + if "rules" in attrs: + self._validate_rules_condition_count(attrs["rules"]) + return attrs def create(self, validated_data: dict[str, Any]): # type: ignore[no-untyped-def] metadata_data = validated_data.pop("metadata", []) + self._set_rules_data(validated_data) segment = super().create(validated_data) # type: ignore[no-untyped-call] self._update_metadata(segment, metadata_data) enqueue_membership_refresh(segment.project) @@ -153,6 +178,7 @@ def create(self, validated_data: dict[str, Any]): # type: ignore[no-untyped-def def update(self, segment: Segment, validated_data: dict[str, Any]): # type: ignore[no-untyped-def] metadata = validated_data.pop("metadata", []) + self._set_rules_data(validated_data) with transaction.atomic(): if not segment.change_request: segment_revision = segment.clone(is_revision=True) @@ -166,6 +192,92 @@ def update(self, segment: Segment, validated_data: dict[str, Any]): # type: ign enqueue_membership_refresh(segment.project) return segment + def _validate_rules_depth( + self, rules: list[LegacySegmentRule], _depth: int = 1 + ) -> None: + # The serializer just ignores rules nested too deep, so we raise for clarity. + if rules and _depth > SEGMENT_RULES_MAX_DEPTH: + raise ValidationError( + { + "segment": [ + f"Rules must not be nested more than " + f"{SEGMENT_RULES_MAX_DEPTH} levels deep." + ] + } + ) + for rule in rules: + self._validate_rules_depth( + cast(list[LegacySegmentRule], rule.get("rules", [])), _depth + 1 + ) + + def _validate_rules_condition_count(self, rules: list[LegacySegmentRule]) -> None: + condition_count = self._count_conditions(rules) + if condition_count > settings.SEGMENT_RULES_CONDITIONS_LIMIT: + if self._can_segment_own_more_conditions_than_limit(): + return + raise ValidationError( + { + "segment": [ + f"The segment has {condition_count} conditions, " + f"which exceeds the maximum condition count of " + f"{settings.SEGMENT_RULES_CONDITIONS_LIMIT}." + ] + } + ) + + def _count_conditions(self, rules: list[LegacySegmentRule]) -> int: + return sum( + len(rule.get("conditions", [])) + + sum( + len(nested_rule.get("conditions", [])) + for nested_rule in rule.get("rules", []) + ) + for rule in rules + ) + + def _can_segment_own_more_conditions_than_limit(self) -> bool: + if self.instance is not None and (segment := cast(Segment, self.instance)).id: + return WhitelistedSegment.objects.filter(segment=segment).exists() + return False + + def _set_rules_data(self, validated_data: dict[str, Any]) -> None: + """Set the .rules_data attribute + TODO: Delete this as per https://github.com/Flagsmith/flagsmith/issues/7818 + """ + if "rules" not in validated_data: + return # PATCH support + validated_data["rules_data"] = self._get_clean_rules_and_conditions( + validated_data["rules"] + ) + + def _get_clean_rules_and_conditions( + self, rules: list[LegacySegmentRule] + ) -> list[SegmentRuleType]: + """Remove obsolete items from rules and conditions + + In https://github.com/Flagsmith/flagsmith/issues/7814, we moved from a + SegmentRule and Condition tree to a JSON field. This cleanup exists to + keep the interface compatible.""" + return [ + { + "type": rule["type"], + "conditions": [ + { + "property": condition["property"], + "operator": condition["operator"], + "value": condition.get("value"), + "description": condition.get("description"), + } + for condition in rule.get("conditions", []) + if not condition.get("delete") + ], + # Cleanup type-ignore as per https://github.com/Flagsmith/flagsmith/issues/8280 + "rules": self._get_clean_rules_and_conditions(rule.get("rules", [])), # type: ignore[typeddict-item,arg-type] + } + for rule in rules + if not rule.get("delete") + ] + def _get_rules_and_conditions_without_deleted( self, rules_data: DictList ) -> DictList: @@ -176,8 +288,7 @@ def _get_rules_and_conditions_without_deleted( or conditions including both an `"id"` field and `"delete": true` were later soft-deleted in the database. - TODO: Deprecate this in favor of not sending unwanted rules and - conditions in the input. + TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 """ return [ { @@ -206,25 +317,6 @@ def _validate_project_segment_limit(self, project: Project) -> None: } ) - def _validate_segment_rules_conditions_limit(self, rules_data: DictList) -> None: - if self.instance and getattr(self.instance, "whitelisted_segment", None): - return - - def _count_conditions(rules_data: DictList) -> int: - return sum( - len(rule.get("conditions", [])) - + _count_conditions(rule.get("rules", [])) - for rule in rules_data - ) - - condition_count = _count_conditions(rules_data) - if condition_count > settings.SEGMENT_RULES_CONDITIONS_LIMIT: - raise ValidationError( - { - "segment": f"The segment has {condition_count} conditions, which exceeds the maximum condition count of {settings.SEGMENT_RULES_CONDITIONS_LIMIT}." - } - ) - class SegmentSerializerBasic(serializers.ModelSerializer): # type: ignore[type-arg] class Meta: diff --git a/api/segments/services.py b/api/segments/services.py index e77ddcd3f70a..b4519dfd7786 100644 --- a/api/segments/services.py +++ b/api/segments/services.py @@ -21,7 +21,7 @@ def delete_segment( reducing the number of database queries from O(n) to O(1) where n is the number of rules and conditions. - Note: This is a temporary solution until we redesign the segment data model. + TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 """ from features.models import FeatureSegment from segments.models import Condition, Segment, SegmentRule @@ -88,6 +88,7 @@ def copy_segment_rules_and_conditions( If target has existing rules, they are hard-deleted first. + TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 """ from segments.models import Condition, SegmentRule diff --git a/api/segments/types.py b/api/segments/types.py index ee463461b5ec..3b56d935785e 100644 --- a/api/segments/types.py +++ b/api/segments/types.py @@ -1,5 +1,48 @@ -from typing_extensions import TypedDict +from flag_engine.segments.types import ConditionOperator, RuleType +from typing_extensions import NotRequired, TypedDict class SegmentEngineMetadata(TypedDict): pk: int + + +class SegmentCondition(TypedDict): + property: str | None + operator: ConditionOperator + value: str | None + description: str | None + + +class _BaseSegmentRule(TypedDict): + type: RuleType + conditions: list[SegmentCondition] + + +class _NestedSegmentRule(_BaseSegmentRule): + pass + + +class SegmentRule(_BaseSegmentRule): + rules: list[_NestedSegmentRule] + + +class LegacySegmentCondition(SegmentCondition): + # TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 + id: NotRequired[int] + delete: NotRequired[bool] + + +class _BaseLegacySegmentRule(TypedDict): + # TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 + id: NotRequired[int] + delete: NotRequired[bool] + type: RuleType + conditions: list[LegacySegmentCondition] + + +class _LegacyNestedSegmentRule(_BaseLegacySegmentRule): + pass + + +class LegacySegmentRule(_BaseLegacySegmentRule): + rules: list[_LegacyNestedSegmentRule] diff --git a/api/tests/conftest.py b/api/tests/conftest.py index f03355c7feb9..014fb37306a5 100644 --- a/api/tests/conftest.py +++ b/api/tests/conftest.py @@ -113,6 +113,9 @@ ) from projects.tags.models import Tag from segments.models import Condition, Segment, SegmentRule + +# TODO: Delete alias as per https://github.com/Flagsmith/flagsmith/issues/7818 +from segments.types import SegmentRule as SegmentRuleType from tests.types import ( AdminClientAuthType, EnableFeaturesFixture, @@ -426,8 +429,51 @@ def project_b(organisation: Organisation) -> Project: @pytest.fixture() -def segment(project: Project) -> Segment: - segment: Segment = Segment.objects.create(name="segment", project=project) +def segment_rules() -> list[SegmentRuleType]: + return [ + { + "type": "ALL", + "conditions": [ + { + "property": "pill-taken", + "operator": "EQUAL", + "value": "red", + "description": "Offered by Morpheus.", + } + ], + "rules": [ + { + "type": "ANY", + "conditions": [ + { + "property": "oracle_confidence", + "operator": "GREATER_THAN_INCLUSIVE", + "value": "90", + "description": None, + }, + { + "property": "can_fly", + "operator": "EQUAL", + "value": "True", + "description": "Jumping very high does not count!", + }, + ], + # Cleanup type-ignore as per https://github.com/Flagsmith/flagsmith/issues/8280 + "rules": [], # type: ignore[typeddict-unknown-key] + }, + ], + } + ] + + +@pytest.fixture() +def segment(project: Project, segment_rules: list[SegmentRuleType]) -> Segment: + segment: Segment = Segment.objects.create( + project=project, + name="segment", + description="description", + rules_data=segment_rules, + ) return segment diff --git a/api/tests/types.py b/api/tests/types.py index 6b358db948a3..4547c3fbb146 100644 --- a/api/tests/types.py +++ b/api/tests/types.py @@ -1,11 +1,14 @@ -import typing -from typing import Callable, Literal, Protocol +from typing import Callable, Literal, Optional, Protocol from django_test_migrations.migrator import Migrator from environments.permissions.models import UserEnvironmentPermission from organisations.permissions.models import UserOrganisationPermission from projects.models import UserProjectPermission +from segments.types import SegmentRule + +_SegmentRulesModifier = Callable[[list[SegmentRule]], None] +InvalidSegmentRulesCase = tuple[_SegmentRulesModifier, dict[str, object]] # TODO: these type aliases aren't strictly correct according to mypy # See here for more details: https://github.com/Flagsmith/flagsmith/issues/5140 @@ -39,5 +42,5 @@ class EnableFeaturesFixture(Protocol): def __call__(self, *feature_names: str) -> None: ... -class MigratorFactory(typing.Protocol): - def __call__(self, name: typing.Optional[str] = None) -> Migrator: ... +class MigratorFactory(Protocol): + def __call__(self, name: Optional[str] = None) -> Migrator: ... diff --git a/api/tests/unit/experimentation/test_services.py b/api/tests/unit/experimentation/test_services.py index 19fbb481b840..ba49a8e62e45 100644 --- a/api/tests/unit/experimentation/test_services.py +++ b/api/tests/unit/experimentation/test_services.py @@ -48,7 +48,7 @@ from features.multivariate.models import MultivariateFeatureOption from features.value_types import STRING from features.versioning.dataclasses import MultivariateValueChangeSet -from segments.models import Condition +from segments.models import Condition, Segment, SegmentRule from users.models import FFAdminUser from util.mappers import map_environment_to_environment_document @@ -1509,6 +1509,64 @@ def test_apply_experiment_rollout__no_segment__creates_segment_and_override( ), ) + # Then + experiment.refresh_from_db() + segment = experiment.rollout_segment + assert segment is not None + assert segment.is_system_segment is True + assert segment.rules_data == [ + { + "type": SegmentRule.ALL_RULE, + "conditions": [ + { + "property": "$.identity.key", + "operator": PERCENTAGE_SPLIT, + "value": "42.0", + "description": None, + } + ], + "rules": [], + } + ] + + override = FeatureState.objects.get( + environment=experiment.environment, + feature=experiment.feature, + feature_segment__segment=segment, + ) + assert override.enabled is True + allocations = { + mv.multivariate_feature_option_id: mv.percentage_allocation + for mv in override.multivariate_feature_state_values.all() + } + assert allocations == {option_a.id: 60.0, option_b.id: 40.0} + + +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 +def test_apply_experiment_rollout__no_segment__creates_segment_and_override_x_replaced_above( + experiment: Experiment, + multivariate_options: list[MultivariateFeatureOption], + admin_user: FFAdminUser, +) -> None: + # Given + option_a, option_b, _ = multivariate_options + + # When + services.apply_experiment_rollout( + experiment, + RolloutSpec( + enabled=True, + rollout_percentage=42.0, + feature_state_value="control", + value_type="string", + multivariate_values=[ + MultivariateValueChangeSet(option_a.id, 60.0), + MultivariateValueChangeSet(option_b.id, 40.0), + ], + author=AuthorData(user=admin_user), + ), + ) + # Then experiment.refresh_from_db() segment = experiment.rollout_segment @@ -1700,6 +1758,56 @@ def test_apply_experiment_rollout__existing_segment__updates_percentage_and_enab ), ) + # Then + segment = Segment.objects.get(pk=experiment.rollout_segment_id) + assert segment.rules_data == [ + { + "type": SegmentRule.ALL_RULE, + "conditions": [ + { + "property": "$.identity.key", + "operator": PERCENTAGE_SPLIT, + "value": "80.0", + "description": None, + } + ], + "rules": [], + } + ] + override = FeatureState.objects.get( + environment=experiment.environment, + feature=experiment.feature, + feature_segment__segment=experiment.rollout_segment, + ) + assert override.enabled is False + + +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 +def test_apply_experiment_rollout__existing_segment__updates_percentage_and_enabled_x_replaced_above( + experiment_with_rollout: Experiment, + multivariate_options: list[MultivariateFeatureOption], + admin_user: FFAdminUser, +) -> None: + # Given + experiment = experiment_with_rollout + option_a, option_b, _ = multivariate_options + + # When + services.apply_experiment_rollout( + experiment, + RolloutSpec( + enabled=False, + rollout_percentage=80.0, + feature_state_value="control", + value_type="string", + multivariate_values=[ + MultivariateValueChangeSet(option_a.id, 50.0), + MultivariateValueChangeSet(option_b.id, 50.0), + ], + author=AuthorData(user=admin_user), + ), + ) + # Then condition = Condition.objects.get(rule__segment=experiment.rollout_segment) assert condition.value == "80.0" diff --git a/api/tests/unit/integrations/launch_darkly/snapshots/test_process_import_request__large_segments__correctly_imported__rules_data.json b/api/tests/unit/integrations/launch_darkly/snapshots/test_process_import_request__large_segments__correctly_imported__rules_data.json new file mode 100644 index 000000000000..ba6821f299c4 --- /dev/null +++ b/api/tests/unit/integrations/launch_darkly/snapshots/test_process_import_request__large_segments__correctly_imported__rules_data.json @@ -0,0 +1,454 @@ +{ + "Large Dynamic List (Override for production)": [ + { + "type": "ALL", + "rules": [ + { + "type": "ANY", + "conditions": [ + { + "value": ".*410f8e860cb348ad83218d65834de218\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*f97d9081f7af47c9b21a97b36d3c5fc2\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*7688255f6032482fb3fe4ae9780ca52e\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*fe28a542c57946dbaad085c156e33209\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*895129ac924d4af29817749f6032c8f9\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*a39d13e4cc6c45949bcda57c20b399df\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*d8899d51b30749659c9603e6bc11e9e4\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*732650e37fdd4ff1bfbb2e239fa7dcd6\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*22d230fac4524132b7e050d1dbf05f82\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*46e6d0b3f1474c1a8335ce434a9196aa\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*8a9208af78734d769ed73f0009a28be3\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*cbf2359bb0f0456b83287146a4e22abd\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*c1546a6090364b0ab52cadb6067724da\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*6508fe14f12e40a39b23a4390a60f6e6\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*723973bf3e1f4292bea939e2e7ba021f\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*b3d74f4883814042876421260d662b52\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*8b89fadafbe44e7399b5dea298996017\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*d0ca397bc2a940ba90508fbc7efe6a52\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*a5e55423d2d04700926b73f4460527b4\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*433ee12ed20147e78e0dd7d42dd4b576\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*887e35f48b2344848aeeac7ef712aa15\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*9b878926a653423b9c8749a0440a18f8\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*b772131a81384b3493eaf8cbd7d33bca\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*c4130acfcd2e4688a123614f3002161c\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*87c5eb3b67464ae792252125c351f307\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*a64061b257014657943e5945ac6af7ca\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*eba18f11f40a4b40bbb4fbd52febe4cc\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*1cb30c51d69f4f44873bb38ced4d7952\\.com", + "operator": "REGEX", + "property": "email", + "description": null + } + ] + }, + { + "type": "NONE", + "conditions": [] + } + ], + "conditions": [] + } + ], + "Large Dynamic List (Override for test)": [ + { + "type": "ALL", + "rules": [ + { + "type": "ANY", + "conditions": [ + { + "value": ".*410f8e860cb348ad83218d65834de218\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*f97d9081f7af47c9b21a97b36d3c5fc2\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*7688255f6032482fb3fe4ae9780ca52e\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*fe28a542c57946dbaad085c156e33209\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*895129ac924d4af29817749f6032c8f9\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*a39d13e4cc6c45949bcda57c20b399df\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*d8899d51b30749659c9603e6bc11e9e4\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*732650e37fdd4ff1bfbb2e239fa7dcd6\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*22d230fac4524132b7e050d1dbf05f82\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*46e6d0b3f1474c1a8335ce434a9196aa\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*8a9208af78734d769ed73f0009a28be3\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*cbf2359bb0f0456b83287146a4e22abd\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*c1546a6090364b0ab52cadb6067724da\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*6508fe14f12e40a39b23a4390a60f6e6\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*723973bf3e1f4292bea939e2e7ba021f\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*b3d74f4883814042876421260d662b52\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*8b89fadafbe44e7399b5dea298996017\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*d0ca397bc2a940ba90508fbc7efe6a52\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*a5e55423d2d04700926b73f4460527b4\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*433ee12ed20147e78e0dd7d42dd4b576\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*887e35f48b2344848aeeac7ef712aa15\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*9b878926a653423b9c8749a0440a18f8\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*b772131a81384b3493eaf8cbd7d33bca\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*c4130acfcd2e4688a123614f3002161c\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*87c5eb3b67464ae792252125c351f307\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*a64061b257014657943e5945ac6af7ca\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*eba18f11f40a4b40bbb4fbd52febe4cc\\.com", + "operator": "REGEX", + "property": "email", + "description": null + }, + { + "value": ".*1cb30c51d69f4f44873bb38ced4d7952\\.com", + "operator": "REGEX", + "property": "email", + "description": null + } + ] + }, + { + "type": "NONE", + "conditions": [] + } + ], + "conditions": [] + } + ], + "Large User List (Override for production)": [ + { + "type": "ALL", + "rules": [ + { + "type": "ANY", + "conditions": [ + { + "value": "user-0d693f8d-1faf-4e92-9e11-981daf62fbe2,user-b74e72a1-6172-4cc2-8f57-2bb756525632,user-0957c2a2-b46a-4b10-aee3-6ca44df92dbc,user-5349f0d8-bc9a-42b3-82db-84bc06f26980,user-47ffb2bf-f9b5-47ed-bd23-f56c02bcaf2f,user-e39afdd3-0a1f-4fbf-860b-2165ec5e1b56,user-5e488537-8fca-43e1-b7b8-dbbe83589180,user-e4c8aa60-dcf2-4e42-8799-8cb6e8928296,user-e07af1ff-6604-4f07-a492-0a8193d092c1,user-c228e199-49c6-4e4a-8b46-16dbabe0eecf,user-d4635b5f-39d7-46a5-8539-cbbf13ae17e3,user-73079df8-8507-45c7-8906-add52c729c3d,user-3f4f0ac1-d42a-408d-b647-984d0f969c8c,user-749e75a6-7aa0-49f6-8713-0e4b9a27d797,user-1e2547ac-b064-454c-8622-681ed0c20145,user-ee792cf3-a104-4273-8f7d-11594a3f24fd,user-e04a3dc0-d4f5-4fde-85e2-7fef8da621ef,user-1167a3a0-e865-453f-8a0b-650fbcc60690,user-bbb2f4d2-fe5c-404b-8f57-cbb96dccb409,user-b04ccdf0-2c46-44c3-9aa0-405c09f4a3aa,user-b23a703f-cfc4-4fda-b3bc-9dce64030d78,user-7264f13b-5982-43a7-8f6c-17faf9ecf367,user-7dd09b25-171e-43d7-bb74-6e7864fa5262", + "operator": "IN", + "property": "key", + "description": null + } + ] + }, + { + "type": "ANY", + "conditions": [ + { + "value": "user-5d4ceb7b-d477-4ec0-a42e-a62bcd0498cb,user-0cfa9e2e-1db4-4558-b323-035bc03e472b,user-8888332b-e5b2-4275-996a-6aee0b5058d0,user-07f81cbd-68a4-499b-bdae-8f8831e9238f,user-c1e599ba-ef2e-4073-87c4-96e4d3534510", + "operator": "IN", + "property": "key", + "description": null + } + ] + }, + { + "type": "NONE", + "conditions": [ + { + "value": "user-103", + "operator": "IN", + "property": "key", + "description": null + } + ] + } + ], + "conditions": [] + } + ], + "Large User List (Override for test)": [ + { + "type": "ALL", + "rules": [ + { + "type": "ANY", + "conditions": [ + { + "value": "user-0d693f8d-1faf-4e92-9e11-981daf62fbe2,user-b74e72a1-6172-4cc2-8f57-2bb756525632,user-0957c2a2-b46a-4b10-aee3-6ca44df92dbc,user-5349f0d8-bc9a-42b3-82db-84bc06f26980,user-47ffb2bf-f9b5-47ed-bd23-f56c02bcaf2f,user-e39afdd3-0a1f-4fbf-860b-2165ec5e1b56,user-5e488537-8fca-43e1-b7b8-dbbe83589180,user-e4c8aa60-dcf2-4e42-8799-8cb6e8928296,user-e07af1ff-6604-4f07-a492-0a8193d092c1,user-c228e199-49c6-4e4a-8b46-16dbabe0eecf,user-d4635b5f-39d7-46a5-8539-cbbf13ae17e3,user-73079df8-8507-45c7-8906-add52c729c3d,user-3f4f0ac1-d42a-408d-b647-984d0f969c8c,user-749e75a6-7aa0-49f6-8713-0e4b9a27d797,user-1e2547ac-b064-454c-8622-681ed0c20145,user-ee792cf3-a104-4273-8f7d-11594a3f24fd,user-e04a3dc0-d4f5-4fde-85e2-7fef8da621ef,user-1167a3a0-e865-453f-8a0b-650fbcc60690,user-bbb2f4d2-fe5c-404b-8f57-cbb96dccb409,user-b04ccdf0-2c46-44c3-9aa0-405c09f4a3aa,user-b23a703f-cfc4-4fda-b3bc-9dce64030d78,user-7264f13b-5982-43a7-8f6c-17faf9ecf367,user-7dd09b25-171e-43d7-bb74-6e7864fa5262", + "operator": "IN", + "property": "key", + "description": null + } + ] + }, + { + "type": "ANY", + "conditions": [ + { + "value": "user-5d4ceb7b-d477-4ec0-a42e-a62bcd0498cb,user-0cfa9e2e-1db4-4558-b323-035bc03e472b,user-8888332b-e5b2-4275-996a-6aee0b5058d0,user-07f81cbd-68a4-499b-bdae-8f8831e9238f,user-c1e599ba-ef2e-4073-87c4-96e4d3534510", + "operator": "IN", + "property": "key", + "description": null + } + ] + }, + { + "type": "NONE", + "conditions": [ + { + "value": "user-103", + "operator": "IN", + "property": "key", + "description": null + } + ] + } + ], + "conditions": [] + } + ] +} \ No newline at end of file diff --git a/api/tests/unit/integrations/launch_darkly/test_services.py b/api/tests/unit/integrations/launch_darkly/test_services.py index 251bbc6b6761..fb6e759c534b 100644 --- a/api/tests/unit/integrations/launch_darkly/test_services.py +++ b/api/tests/unit/integrations/launch_darkly/test_services.py @@ -2,6 +2,7 @@ import io import json from operator import attrgetter +from typing import Any from unittest.mock import MagicMock import pytest @@ -26,6 +27,9 @@ from projects.models import Project from projects.tags.models import Tag from segments.models import Condition, Segment, SegmentRule + +# TODO: Delete alias as per https://github.com/Flagsmith/flagsmith/issues/7818 +from segments.types import SegmentRule as SegmentRuleType from users.models import FFAdminUser @@ -103,6 +107,31 @@ def test_process_import_request__api_error__expected_status( assert import_request.status["error_messages"] == [expected_error_message] +@pytest.mark.django_db(transaction=True) +def test_process_import_request__write_error__persists_no_import_data( + mocker: MockerFixture, + project: Project, + import_request: LaunchDarklyImportRequest, +) -> None: + # Given + mocker.patch( + "integrations.launch_darkly.services._create_features_from_ld", + side_effect=RuntimeError(), + ) + + # When + with pytest.raises(RuntimeError): + process_import_request(import_request) + + # Then + import_request.refresh_from_db() + assert import_request.completed_at + assert import_request.status["result"] == "failure" + assert not Environment.objects.filter(project=project).exists() + assert not Feature.objects.filter(project=project).exists() + assert not Segment.objects.filter(project=project).exists() + + @pytest.mark.django_db(transaction=True) def test_process_import_request__success__expected_status( # type: ignore[no-untyped-def] project: Project, @@ -285,7 +314,204 @@ def test_process_import_request__already_completed__does_not_reprocess( @pytest.mark.django_db(transaction=True) -def test_process_import_request__valid_segments__imports_correctly( # type: ignore[no-untyped-def] +def test_process_import_request__valid_segments__creates_segment_per_environment( + project: Project, + import_request: LaunchDarklyImportRequest, +) -> None: + # Given / When + process_import_request(import_request) + + # Then + segments = Segment.objects.filter(project=project, feature_id=None) + + assert set(segments.values_list("name", flat=True)) == { + "User List (Override for test)", + "User List (Override for production)", + "Dynamic List (Override for test)", + "Dynamic List (Override for production)", + "Dynamic List 2 (Override for test)", + "Dynamic List 2 (Override for production)", + } + + +@pytest.mark.parametrize( + "segment_name, expected_rules_data", + [ + pytest.param( + "Dynamic List (Override for test)", + [ + { + "type": SegmentRule.ALL_RULE, + "conditions": [], + "rules": [ + { + "type": SegmentRule.ANY_RULE, + "conditions": [ + { + "property": "email", + "operator": segment_constants.REGEX, + "value": ".*@gmail\\.com", + "description": None, + } + ], + } + ], + } + ], + id="targeting-rules-only", + ), + pytest.param( + "Dynamic List 2 (Override for production)", + [ + { + "type": SegmentRule.ALL_RULE, + "conditions": [], + "rules": [ + { + "type": SegmentRule.ANY_RULE, + "conditions": [ + { + "property": "p1", + "operator": segment_constants.IN, + "value": "1,2", + "description": None, + } + ], + }, + { + "type": SegmentRule.ANY_RULE, + "conditions": [ + { + "property": "p2", + "operator": segment_constants.GREATER_THAN, + "value": "1.0.0:semver", + "description": None, + } + ], + }, + { + "type": SegmentRule.ANY_RULE, + "conditions": [ + { + "property": "p3", + "operator": segment_constants.REGEX, + "value": "foo[0-9]{0,1}", + "description": None, + } + ], + }, + { + "type": SegmentRule.ANY_RULE, # included users + "conditions": [ + { + "property": "key", + "operator": segment_constants.IN, + "value": "foo", + "description": None, + } + ], + }, + { + "type": SegmentRule.NONE_RULE, # excluded users + "conditions": [ + { + "property": "key", + "operator": segment_constants.IN, + "value": "bar", + "description": None, + } + ], + }, + ], + } + ], + id="targeting-rules-and-user-lists", + ), + pytest.param( + "User List (Override for test)", + [ + { + "type": SegmentRule.ALL_RULE, + "conditions": [], + "rules": [ + { + "type": SegmentRule.ANY_RULE, # included users + "conditions": [ + { + "property": "key", + "operator": segment_constants.IN, + "value": "user-102,user-101", + "description": None, + } + ], + }, + { + "type": SegmentRule.NONE_RULE, # excluded users + "conditions": [ + { + "property": "key", + "operator": segment_constants.IN, + "value": "user-103", + "description": None, + } + ], + }, + ], + } + ], + id="user-lists-only", + ), + ], +) +@pytest.mark.django_db(transaction=True) +def test_process_import_request__valid_segments__imports_correctly( + project: Project, + import_request: LaunchDarklyImportRequest, + segment_name: str, + expected_rules_data: list[SegmentRuleType], +) -> None: + # Given / When + process_import_request(import_request) + + # Then + segment = Segment.objects.get(name=segment_name, project=project) + assert segment.rules_data == expected_rules_data + + +@pytest.mark.django_db(transaction=True) +def test_process_import_request__valid_segments__creates_identities_with_key_traits( + project: Project, + import_request: LaunchDarklyImportRequest, +) -> None: + # Given / When + process_import_request(import_request) + + # Then + assert set( + Identity.objects.filter(environment__project=project).values_list( + "identifier", + "identity_traits__trait_key", + "identity_traits__string_value", + ) + ) == { + (identifier, "key", identifier) + for identifier in ( + "bar", + "foo", + "user1", + "user2", + "user-101", + "user-102", + "user-103", + "user-1005", + "user-10006", + ) + } + + +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 +@pytest.mark.django_db(transaction=True) +def test_process_import_request__valid_segments__imports_correctly_x_replaced_above( # type: ignore[no-untyped-def] project: Project, import_request: LaunchDarklyImportRequest, ): @@ -486,7 +712,152 @@ def test_process_import_request__valid_segments__imports_correctly( # type: ign @pytest.mark.django_db(transaction=True) -def test_process_import_request__valid_rules__imports_correctly( # type: ignore[no-untyped-def] +def test_process_import_request__valid_rules__creates_feature_specific_segments( + project: Project, + import_request: LaunchDarklyImportRequest, +) -> None: + # Given / When + process_import_request(import_request) + + # Then + segments = Segment.objects.filter(project=project).exclude(feature_id=None) + + assert set(segments.values_list("name", flat=True)) == { + # Feature Segments + "Regular And", + "Reverted And", + "Just Not", + # Feature Segments without descriptions + "imported-56725db6-3d2a-4ed6-a2a1-60ef94ac62d5", + "imported-a132f4aa-ad51-43c6-8d03-f18d6a5b205d", + "imported-c034ec70-fcb3-4c15-9bea-b9fa0b341b4f", + # Individual targeting rules converted as custom segments + "individual-targeting-variation-0", + "individual-targeting-variation-1", + "individual-targeting-variation-2", + } + + +@pytest.mark.parametrize( + "segment_name, expected_rules_data", + [ + pytest.param( + "Regular And", + [ + { + "type": SegmentRule.ALL_RULE, + "conditions": [], + "rules": [ + { + "type": SegmentRule.ANY_RULE, + "conditions": [ + { + "property": "p1", + "operator": segment_constants.LESS_THAN_INCLUSIVE, + "value": "5", + "description": None, + } + ], + }, + { + "type": SegmentRule.ANY_RULE, + "conditions": [ + { + "property": "p2", + "operator": segment_constants.GREATER_THAN, + "value": "1", + "description": None, + } + ], + }, + ], + } + ], + id="plain-clauses-only", + ), + pytest.param( + "Reverted And", + [ + { + "type": SegmentRule.ALL_RULE, + "conditions": [], + "rules": [ + { + "type": SegmentRule.ANY_RULE, + "conditions": [ + { + "property": "p1", + "operator": segment_constants.REGEX, + "value": ".*bar", + "description": None, + } + ], + }, + { + "type": SegmentRule.NONE_RULE, # negated clauses pool here + "conditions": [ + { + "property": "p2", + "operator": segment_constants.CONTAINS, + "value": "forbidden", + "description": None, + }, + { + "property": "p2", + "operator": segment_constants.CONTAINS, + "value": "words", + "description": None, + }, + ], + }, + ], + } + ], + id="plain-and-negated-clauses", + ), + pytest.param( + "Just Not", + [ + { + "type": SegmentRule.ALL_RULE, + "conditions": [], + "rules": [ + { + "type": SegmentRule.NONE_RULE, + "conditions": [ + { + "property": "p1", + "operator": segment_constants.IN, + "value": "this,that", + "description": None, + } + ], + }, + ], + } + ], + id="negated-clauses-only", + ), + ], +) +@pytest.mark.django_db(transaction=True) +def test_process_import_request__valid_rules__imports_correctly( + project: Project, + import_request: LaunchDarklyImportRequest, + segment_name: str, + expected_rules_data: list[SegmentRuleType], +) -> None: + # Given / When + process_import_request(import_request) + + # Then + segment = Segment.objects.get(name=segment_name, project=project) + assert segment.rules_data == expected_rules_data + + +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 +@pytest.mark.django_db(transaction=True) +def test_process_import_request__valid_rules__imports_correctly_x_replaced_above( # type: ignore[no-untyped-def] project: Project, import_request: LaunchDarklyImportRequest, ): @@ -582,12 +953,156 @@ def test_process_import_request__valid_rules__imports_correctly( # type: ignore } +@pytest.mark.parametrize( + "ld_segment_data, expected_rules_data, expected_error_message", + [ + pytest.param( + { + "rules": [ + { + "clauses": [ + { + "attribute": "p1", + "op": "arcaneOp", + "values": ["x"], + "negate": False, + } + ] + } + ] + }, + [{"type": SegmentRule.ALL_RULE, "conditions": [], "rules": []}], + "Can't map launch darkly operator: arcaneOp" + " skipping for segment: Unsupported (Override for test)", + id="unsupported-operator", + ), + pytest.param( + { + "rules": [ + { + "clauses": [ + { + "attribute": "p1", + "op": "contains", + "values": [ + "x" * (settings.SEGMENT_CONDITION_VALUE_LIMIT + 1) + ], + "negate": False, + } + ] + } + ] + }, + [ + { + "type": SegmentRule.ALL_RULE, + "conditions": [], + "rules": [{"type": SegmentRule.ANY_RULE, "conditions": []}], + } + ], + f"Segment condition value 'xxxxx...xxxxx' for property 'p1' exceeds the" + f" limit of {settings.SEGMENT_CONDITION_VALUE_LIMIT} characters," + f" skipping for segment 'Unsupported (Override for test)'", + id="condition-value-over-limit", + ), + pytest.param( + {"included": ["y" * (settings.SEGMENT_CONDITION_VALUE_LIMIT + 1)]}, + [{"type": SegmentRule.ALL_RULE, "conditions": [], "rules": []}], + f"Targeting key 'yyyyy...yyyyy' exceeds the limit of" + f" {settings.SEGMENT_CONDITION_VALUE_LIMIT} characters, " + f"skipping for segment 'Unsupported (Override for test)'", + id="targeting-key-over-limit", + ), + ], +) +@pytest.mark.django_db(transaction=True) +def test_process_import_request__unsupported_segment_data__skips_and_logs_error( + project: Project, + ld_client_class_mock: MagicMock, + import_request: LaunchDarklyImportRequest, + ld_segment_data: dict[str, Any], + expected_rules_data: list[SegmentRuleType], + expected_error_message: str, +) -> None: + # Given + ld_client_class_mock.return_value.get_segments.return_value = [ + { + "name": "Unsupported", + "key": "unsupported", + "deleted": False, + "included": [], + "excluded": [], + "includedContexts": [], + "excludedContexts": [], + "rules": [], + **ld_segment_data, + } + ] + + # When + process_import_request(import_request) + + # Then + segment = Segment.objects.get( + name="Unsupported (Override for test)", project=project + ) + assert segment.rules_data == expected_rules_data + assert expected_error_message in import_request.status["error_messages"] + + @pytest.mark.django_db(transaction=True) def test_process_import_request__large_segments__correctly_imported( request: pytest.FixtureRequest, ld_client_class_mock: MagicMock, import_request: LaunchDarklyImportRequest, snapshot: SnapshotFixture, +) -> None: + # Given + expected_status_snapshot = snapshot( + "test_process_import_request__large_segments__correctly_imported__import_request_status.json" + ) + expected_rules_data_snapshot = snapshot( + "test_process_import_request__large_segments__correctly_imported__rules_data.json" + ) + expected_segment_names = [ + "Large Dynamic List (Override for test)", + "Large Dynamic List (Override for production)", + "Large User List (Override for test)", + "Large User List (Override for production)", + ] + large_segments_response_path = ( + request.path.parent / "client_responses/get_segments__large_segments.json" + ) + ld_client_class_mock.return_value.get_segments.return_value = json.loads( + large_segments_response_path.read_text() + ) + + # When + process_import_request(import_request) + + # Then + status_json = json.dumps(import_request.status, indent=2, sort_keys=True) + assert status_json == expected_status_snapshot + + segments = sorted( + Segment.objects.filter( + project=import_request.project, name__in=expected_segment_names + ), + key=attrgetter("name"), + ) + rules_data_json = json.dumps( + {segment.name: segment.rules_data for segment in segments}, indent=2 + ) + assert rules_data_json == expected_rules_data_snapshot + + +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 +@pytest.mark.django_db(transaction=True) +def test_process_import_request__large_segments__correctly_imported_x_replaced_above( + request: pytest.FixtureRequest, + ld_client_class_mock: MagicMock, + import_request: LaunchDarklyImportRequest, + snapshot: SnapshotFixture, ) -> None: # Given expected_import_request_status_snapshot, expected_condition_data_snapshot = ( diff --git a/api/tests/unit/segments/conftest.py b/api/tests/unit/segments/conftest.py new file mode 100644 index 000000000000..94cdf8f5f42e --- /dev/null +++ b/api/tests/unit/segments/conftest.py @@ -0,0 +1,79 @@ +from typing import cast + +import pytest +from django.conf import settings +from pytest import FixtureRequest + +from tests.types import InvalidSegmentRulesCase + + +@pytest.fixture( + params=[ + pytest.param( + ( + lambda rules: rules.clear(), + {"rules": {"non_field_errors": ["This list may not be empty."]}}, + ), + id="no-rules-provided", + ), + pytest.param( + ( + lambda rules: rules[0]["conditions"].extend( + {"property": f"prop_{i}", "operator": "EQUAL", "value": "red"} + for i in range(settings.SEGMENT_RULES_CONDITIONS_LIMIT) + ), + { + "segment": [ + f"The segment has {settings.SEGMENT_RULES_CONDITIONS_LIMIT + 3} conditions, " + f"which exceeds the maximum condition count of {settings.SEGMENT_RULES_CONDITIONS_LIMIT}." + ] + }, + ), + id="condition-count-over-limit", + ), + pytest.param( + ( + lambda rules: rules[0]["conditions"][0].update( + value="x" * (settings.SEGMENT_CONDITION_VALUE_LIMIT + 1) + ), + { + "rules": [ + { + "conditions": [ + { + "value": [ + f"Ensure this field has no more than " + f"{settings.SEGMENT_CONDITION_VALUE_LIMIT} characters." + ] + } + ] + } + ] + }, + ), + id="condition-value-over-length-limit", + ), + pytest.param( + ( + lambda rules: rules[0]["rules"][0].update( + rules=[ + { + "type": "ANY", + "conditions": [ + { + "property": "too", + "operator": "EQUAL", + "value": "deep", + }, + ], + }, + ], + ), + {"segment": ["Rules must not be nested more than 2 levels deep."]}, + ), + id="rules-nested-too-deep", + ), + ], +) +def invalid_rules_case(request: FixtureRequest) -> InvalidSegmentRulesCase: + return cast(InvalidSegmentRulesCase, request.param) diff --git a/api/tests/unit/segments/test_unit_segments_migrations.py b/api/tests/unit/segments/test_unit_segments_migrations.py index 6aeb48ed8c30..af1bb8b72822 100644 --- a/api/tests/unit/segments/test_unit_segments_migrations.py +++ b/api/tests/unit/segments/test_unit_segments_migrations.py @@ -1,11 +1,15 @@ import uuid +from importlib import import_module import pytest from django.conf import settings as test_settings +from django.utils import timezone from django_test_migrations.migrator import Migrator from flag_engine.segments import constants from pytest_django.fixtures import SettingsWrapper +migration_0032 = import_module("segments.migrations.0032_add_segment_rules_data") + @pytest.mark.skipif( test_settings.SKIP_MIGRATION_TESTS is True, @@ -243,3 +247,206 @@ def _deep_clone(segment: Segment) -> Segment: # type: ignore[valid-type] new_segment_v3 = NewSegment.objects.get(id=version_3.id) assert new_segment_v3.deleted_at is None + + +@pytest.mark.skipif( + test_settings.SKIP_MIGRATION_TESTS is True, + reason="Skip migration tests to speed up tests where necessary", +) +def test_0032_add_segment_rules_data__forwards__backfill_segment_rules_data( + migrator: Migrator, + monkeypatch: pytest.MonkeyPatch, +) -> None: + # Given + monkeypatch.setattr(migration_0032, "BATCH_SIZE", 2) + state = migrator.apply_initial_migration( + ("segments", "0032_add_segment_rules_data") + ) + + Organisation = state.apps.get_model("organisations", "Organisation") + Project = state.apps.get_model("projects", "Project") + Segment = state.apps.get_model("segments", "Segment") + SegmentRule = state.apps.get_model("segments", "SegmentRule") + Condition = state.apps.get_model("segments", "Condition") + + organisation = Organisation.objects.create(name="Test Org") + project = Project.objects.create(name="Test Project", organisation=organisation) + + segment = Segment.objects.create(name="Current", project=project) + segment.version_of_id = segment.id + segment.save() + top_rule = SegmentRule.objects.create(segment=segment, type="ALL") + nested_rule = SegmentRule.objects.create(rule=top_rule, type="ANY") + deep_rule = SegmentRule.objects.create(rule=nested_rule, type="NONE") + deleted_rule = SegmentRule.objects.create( + rule=top_rule, type="ANY", deleted_at=timezone.now() + ) + orphaned_rule = SegmentRule.objects.create(rule=deleted_rule, type="ANY") + Condition.objects.create( + rule=orphaned_rule, + operator=constants.EQUAL, + property="ghost", + value="true", + ) + Condition.objects.create( + rule=top_rule, + operator=constants.IS_SET, + property="email", + value="", + ) + Condition.objects.create( + rule=nested_rule, + operator=constants.EQUAL, + property="age", + value="21", + description="Adults only", + ) + Condition.objects.create( + rule=nested_rule, + operator=constants.GREATER_THAN, + property="height", + value="210", + deleted_at=timezone.now(), + ) + Condition.objects.create( + rule=deep_rule, + operator=constants.CONTAINS, + property="country", + value="GB", + ) + + deleted_segment = Segment.objects.create( + name="Deleted", project=project, deleted_at=timezone.now() + ) + deleted_segment.version_of_id = deleted_segment.id + deleted_segment.save() + SegmentRule.objects.create(segment=deleted_segment, type="ALL") + + old_version_segment = Segment.objects.create( + name="Old version", project=project, version_of_id=segment.id + ) + SegmentRule.objects.create(segment=old_version_segment, type="ALL") + + empty_segment = Segment.objects.create(name="Empty", project=project) + empty_segment.version_of_id = empty_segment.id + empty_segment.save() + + batched_segments = [] + for i in range(3): # spans several batches + batched_segment = Segment.objects.create(name=f"Batched {i}", project=project) + batched_segment.version_of_id = batched_segment.id + batched_segment.save() + rule = SegmentRule.objects.create(segment=batched_segment, type="ALL") + Condition.objects.create( + rule=rule, + operator=constants.EQUAL, + property="batch", + value=str(i), + ) + batched_segments.append(batched_segment) + + # When + migration_0032.backfill_segment_rules_data(state.apps) + + # Then + segment.refresh_from_db() + deleted_segment.refresh_from_db() + old_version_segment.refresh_from_db() + assert segment.rules_data == [ + { + "type": "ALL", + "conditions": [ + { + "property": "email", + "operator": constants.IS_SET, + "value": "", + "description": None, + } + ], + "rules": [ + { + "type": "ANY", + "conditions": [ + { + "property": "age", + "operator": constants.EQUAL, + "value": "21", + "description": "Adults only", + } + ], + # Our UI never allowed more than two levels, but our API did + "rules": [ + { + "type": "NONE", + "conditions": [ + { + "property": "country", + "operator": constants.CONTAINS, + "value": "GB", + "description": None, + } + ], + } + ], + } + ], + } + ] + assert deleted_segment.rules_data is None + assert old_version_segment.rules_data is None + + empty_segment.refresh_from_db() + assert empty_segment.rules_data == [] + + for i, batched_segment in enumerate(batched_segments): + batched_segment.refresh_from_db() + assert batched_segment.rules_data == [ + { + "type": "ALL", + "conditions": [ + { + "property": "batch", + "operator": constants.EQUAL, + "value": str(i), + "description": None, + } + ], + # NOTE: Empty rules are dropped at any level! + } + ] + + +@pytest.mark.skipif( + test_settings.SKIP_MIGRATION_TESTS is True, + reason="Skip migration tests to speed up tests where necessary", +) +def test_0032_add_segment_rules_data__backwards__nullify_segment_rules_data( + migrator: Migrator, +) -> None: + # Given + state = migrator.apply_initial_migration( + ("segments", "0032_add_segment_rules_data") + ) + + Organisation = state.apps.get_model("organisations", "Organisation") + Project = state.apps.get_model("projects", "Project") + Segment = state.apps.get_model("segments", "Segment") + + organisation = Organisation.objects.create(name="Test Org") + project = Project.objects.create(name="Test Project", organisation=organisation) + + backfilled_segment = Segment.objects.create( + name="Backfilled", + project=project, + rules_data=[{"type": "ALL", "conditions": [], "rules": []}], + ) + blank_segment = Segment.objects.create(name="Blank", project=project) + + # When + migration_0032.nullify_segment_rules_data(state.apps) + + # Then + backfilled_segment.refresh_from_db() + blank_segment.refresh_from_db() + assert backfilled_segment.rules_data is None + assert blank_segment.rules_data is None diff --git a/api/tests/unit/segments/test_unit_segments_models.py b/api/tests/unit/segments/test_unit_segments_models.py index ee63e86e41ae..bef445d7e826 100644 --- a/api/tests/unit/segments/test_unit_segments_models.py +++ b/api/tests/unit/segments/test_unit_segments_models.py @@ -9,6 +9,7 @@ from segments.models import Condition, Segment, SegmentRule +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 def test_Condition_str__valid_condition__returns_readable_representation( segment: Segment, segment_rule: SegmentRule, @@ -28,6 +29,7 @@ def test_Condition_str__valid_condition__returns_readable_representation( assert result == "Condition for ALL rule for Segment - segment: foo EQUAL bar" +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 @pytest.mark.parametrize( "delete", [ @@ -55,6 +57,7 @@ def test_Condition_get_skip_create_audit_log__rule_deleted__returns_true( assert condition.get_skip_create_audit_log() is True +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 @pytest.mark.parametrize( "delete", [ @@ -102,6 +105,7 @@ def test_LiveSegmentManager__cloned_segment_exists__returns_only_highest_version assert queryset4.first() == segment +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 @pytest.mark.parametrize( "get_parents", [ @@ -129,6 +133,7 @@ def test_SegmentRule_clean__invalid_parent_count__raises_validation_error( ) +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 def test_SegmentRule_get_skip_create_audit_log__always__returns_true( segment: Segment, ) -> None: @@ -144,6 +149,7 @@ def test_SegmentRule_get_skip_create_audit_log__always__returns_true( assert result is True +# TODO: Revisit as per https://github.com/Flagsmith/flagsmith/issues/7818 def test_Segment_delete__multiple_rules_conditions__schedules_audit_log_task_once( mocker: MockerFixture, segment: Segment ) -> None: @@ -202,8 +208,42 @@ def test_Segment_clone__empty_segment__returns_new_revision( assert segment.version == original_version + 1 +@pytest.mark.parametrize( + "is_revision, expected_cloned_version, expected_source_version", + [ + pytest.param(True, 5, 6, id="revision"), + pytest.param(False, 1, 5, id="standalone"), + ], +) +def test_Segment_clone__given_is_revision__returns_cloned_segment( + is_revision: bool, + expected_cloned_version: int, + expected_source_version: int, + segment: Segment, +) -> None: + # Given + segment.version = 5 + segment.save() + + # When + cloned_segment = segment.clone(is_revision=is_revision) + + # Then + assert cloned_segment != segment + cloned_segment.refresh_from_db() + assert cloned_segment.uuid != segment.uuid + assert cloned_segment.project == segment.project + assert cloned_segment.name == segment.name + assert cloned_segment.description == segment.description + assert cloned_segment.rules_data == segment.rules_data + assert cloned_segment.version == expected_cloned_version + assert cloned_segment.version_of == (segment if is_revision else cloned_segment) + segment.refresh_from_db() + assert segment.version == expected_source_version + + @pytest.mark.parametrize("is_revision", [True, False]) -def test_Segment_clone__segment_with_rules__returns_new_segment_with_copied_rules_and_conditions( +def test_Segment_clone__segment_with_rules__returns_new_segment_with_copied_rules_and_conditions_x_replaced_above( is_revision: bool, segment: Segment, ) -> None: diff --git a/api/tests/unit/segments/test_unit_segments_services.py b/api/tests/unit/segments/test_unit_segments_services.py index c163b515f18a..c6ff3289e58c 100644 --- a/api/tests/unit/segments/test_unit_segments_services.py +++ b/api/tests/unit/segments/test_unit_segments_services.py @@ -44,6 +44,7 @@ def _create_segment_with_nested_rules( return segment +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 def test_delete_segment__called_with_valid_segment__soft_deletes_segment( project: Project, admin_user: FFAdminUser ) -> None: @@ -59,6 +60,7 @@ def test_delete_segment__called_with_valid_segment__soft_deletes_segment( assert segment.deleted_at is not None +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 def test_delete_segment__segment_with_nested_rules__soft_deletes_all_rules( project: Project, admin_user: FFAdminUser ) -> None: @@ -88,6 +90,7 @@ def test_delete_segment__segment_with_nested_rules__soft_deletes_all_rules( assert rule.deleted_at is not None +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 def test_delete_segment__segment_with_nested_conditions__soft_deletes_all_conditions( project: Project, admin_user: FFAdminUser ) -> None: @@ -109,6 +112,7 @@ def test_delete_segment__segment_with_nested_conditions__soft_deletes_all_condit assert condition.deleted_at is not None +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 def test_delete_segment__called_with_author__creates_audit_log( project: Project, admin_user: FFAdminUser ) -> None: @@ -134,6 +138,7 @@ def test_delete_segment__called_with_author__creates_audit_log( assert audit_log.related_object_uuid == segment_uuid +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 def test_delete_segment__segment_with_revision__deletes_all_versions( project: Project, admin_user: FFAdminUser ) -> None: @@ -152,6 +157,7 @@ def test_delete_segment__segment_with_revision__deletes_all_versions( assert revision.deleted_at is not None +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 def test_delete_segment__varying_segment_sizes__query_count_is_constant( project: Project, admin_user: FFAdminUser ) -> None: @@ -180,6 +186,7 @@ def test_delete_segment__varying_segment_sizes__query_count_is_constant( assert small_query_count == large_query_count == 26 +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 def test_delete_segment__segment_without_rules__soft_deletes_segment( project: Project, admin_user: FFAdminUser ) -> None: @@ -195,6 +202,7 @@ def test_delete_segment__segment_without_rules__soft_deletes_segment( assert segment.deleted_at is not None +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 def test_delete_segment__called_with_master_api_key__records_api_key_in_audit_log( project: Project, organisation: Organisation ) -> None: @@ -221,6 +229,7 @@ def test_delete_segment__called_with_master_api_key__records_api_key_in_audit_lo assert audit_log.author is None +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 def test_delete_segment__segment_with_feature_segment__deletes_feature_segments( project: Project, environment: Environment, @@ -242,6 +251,7 @@ def test_delete_segment__segment_with_feature_segment__deletes_feature_segments( assert not FeatureSegment.objects.filter(id=feature_segment_id).exists() +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 def test_delete_segment__segment_with_feature_state__cascades_to_feature_states( project: Project, environment: Environment, @@ -268,6 +278,7 @@ def test_delete_segment__segment_with_feature_state__cascades_to_feature_states( assert not FeatureState.objects.filter(id=feature_state_id).exists() +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 def test_copy_rules_and_conditions_from__source_with_nested_rules__copies_rules( project: Project, ) -> None: @@ -294,6 +305,7 @@ def test_copy_rules_and_conditions_from__source_with_nested_rules__copies_rules( assert target_condition_count == source_condition_count +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 def test_copy_rules_and_conditions_from__target_has_existing_rules__replaces_existing_rules( project: Project, ) -> None: @@ -324,6 +336,7 @@ def test_copy_rules_and_conditions_from__target_has_existing_rules__replaces_exi assert target_rule_count == source_rule_count +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 def test_copy_rules_and_conditions_from__varying_segment_sizes__query_count_is_constant( project: Project, ) -> None: diff --git a/api/tests/unit/segments/test_unit_segments_views.py b/api/tests/unit/segments/test_unit_segments_views.py index 111e8f2a6d98..85adde8cd9e5 100644 --- a/api/tests/unit/segments/test_unit_segments_views.py +++ b/api/tests/unit/segments/test_unit_segments_views.py @@ -1,6 +1,9 @@ import json import random +from collections.abc import Callable +from copy import deepcopy +import freezegun import pytest from common.projects.permissions import ( MANAGE_SEGMENTS, @@ -40,7 +43,10 @@ SegmentRule, WhitelistedSegment, ) -from tests.types import WithProjectPermissionsCallable + +# TODO: Delete alias as per https://github.com/Flagsmith/flagsmith/issues/7818 +from segments.types import SegmentRule as SegmentRuleType +from tests.types import InvalidSegmentRulesCase, WithProjectPermissionsCallable from util.mappers import map_identity_to_identity_document User = get_user_model() @@ -64,20 +70,85 @@ def test_list_segments__filter_by_identity__returns_only_matching_segments( # t assert res.json().get("count") == 1 -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) -def test_create_segment__no_rules_provided__returns_400(project, client): # type: ignore[no-untyped-def] +def test_create_segment__valid_rules__creates_segment_with_rules( + admin_client: APIClient, + project: Project, + mocker: MockerFixture, + segment_rules: list[SegmentRuleType], +) -> None: # Given - url = reverse("api-v1:projects:project-segments-list", args=[project.id]) - data = {"name": "New segment name", "project": project.id, "rules": []} + timestamp = "2099-01-01T00:00:00Z" # When - res = client.post(url, data=json.dumps(data), content_type="application/json") + with freezegun.freeze_time(timestamp): + response = admin_client.post( + f"/api/v1/projects/{project.id}/segments/", + data={ + "name": "chosen people", + "description": "Can star in Matrix 5", + "rules": segment_rules, + }, + format="json", + ) # Then - assert res.status_code == status.HTTP_400_BAD_REQUEST + assert response.status_code == 201 + created_segment = Segment.objects.get(id=response.json()["id"]) + assert created_segment.project == project + assert created_segment.name == "chosen people" + assert created_segment.description == "Can star in Matrix 5" + assert created_segment.rules_data == segment_rules + assert response.data == { + "id": created_segment.id, + "uuid": str(created_segment.uuid), + "created_at": timestamp, + "updated_at": timestamp, + "name": "chosen people", + "description": "Can star in Matrix 5", + "project": project.id, + "feature": None, + "version_of": created_segment.id, + "metadata": [], + "membership_counts": [], + "managed_by": "", + "rules": [ + { + "id": mocker.ANY, + "type": "ALL", + "conditions": [ + { + "id": mocker.ANY, + "property": "pill-taken", + "operator": "EQUAL", + "value": "red", + "description": "Offered by Morpheus.", + }, + ], + "rules": [ + { + "id": mocker.ANY, + "type": "ANY", + "conditions": [ + { + "id": mocker.ANY, + "property": "oracle_confidence", + "operator": "GREATER_THAN_INCLUSIVE", + "value": "90", + "description": None, + }, + { + "id": mocker.ANY, + "property": "can_fly", + "operator": "EQUAL", + "value": "True", + "description": "Jumping very high does not count!", + }, + ], + }, + ], + }, + ], + } @pytest.mark.parametrize( @@ -170,6 +241,32 @@ def test_create_segment__condition_with_null_value__returns_201(project, client) assert res.status_code == status.HTTP_201_CREATED +def test_create_segment__invalid_rules__returns_400( + admin_client: APIClient, + invalid_rules_case: InvalidSegmentRulesCase, + project: Project, + segment_rules: list[SegmentRuleType], +) -> None: + # Given + rules_breaker, expected_error = invalid_rules_case + rules_breaker(segment_rules) + + # When + response = admin_client.post( + f"/api/v1/projects/{project.id}/segments/", + data={ + "name": "chosen people", + "rules": segment_rules, + }, + format="json", + ) + + # Then + assert response.status_code == 400 + assert response.json() == expected_error + assert not Segment.objects.exists() + + @pytest.mark.parametrize( "client", [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], @@ -323,29 +420,6 @@ def test_update_segment__valid_data__creates_audit_log( ).exists() -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) -def test_patch_segment__valid_data__returns_200(project, segment, client): # type: ignore[no-untyped-def] - # Given - segment = Segment.objects.create(name="Test segment", project=project) - url = reverse( - "api-v1:projects:project-segments-detail", - args=[project.id, segment.id], - ) - data = { - "name": "New segment name", - "rules": [{"type": "ALL", "rules": [], "conditions": []}], - } - - # When - res = client.patch(url, data=json.dumps(data), content_type="application/json") - - # Then - assert res.status_code == status.HTTP_200_OK - - @pytest.mark.parametrize( "client", [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], @@ -721,13 +795,6 @@ def test_list_segments__search_by_name__returns_matching_segment( # type: ignor for segment_name in segment_names: segment = Segment.objects.create(project=project, name=segment_name) - all_rule = SegmentRule.objects.create( - segment=segment, type=SegmentRule.ALL_RULE - ) - any_rule = SegmentRule.objects.create(rule=all_rule, type=SegmentRule.ANY_RULE) - Condition.objects.create( - property="foo", value=str(random.randint(0, 10)), rule=any_rule - ) segments.append(segment) url = "%s?q=%s" % ( @@ -746,85 +813,7 @@ def test_list_segments__search_by_name__returns_matching_segment( # type: ignor assert response_json["results"][0]["name"] == segment_names[0] -@pytest.mark.parametrize( - "client", - [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], -) -def test_create_segment__condition_with_description__returns_description_in_response( # type: ignore[no-untyped-def] - project, client -): - # Given - url = reverse("api-v1:projects:project-segments-list", args=[project.id]) - data = { - "name": "New segment name", - "project": project.id, - "rules": [ - { - "type": "ALL", - "rules": [], - "conditions": [ - { - "operator": EQUAL, - "property": "test-property", - "value": True, - "description": "test-description", - } - ], - } - ], - } - - # When - response = client.post(url, data=json.dumps(data), content_type="application/json") - - # Then - segment_condition_description_value = response.json()["rules"][0]["conditions"][0][ - "description" - ] - assert segment_condition_description_value == "test-description" - - -def test_update_segment__add_new_root_rule__returns_updated_rules( - project: Project, admin_client_new: APIClient, segment: Segment -) -> None: - # Given - url = reverse( - "api-v1:projects:project-segments-detail", args=[project.id, segment.id] - ) - data = { - "name": segment.name, - "project": project.id, - "rules": [ - { - "type": "ANY", - "rules": [ - { - "type": "ALL", - "rules": [], - "conditions": [ - {"property": "foo", "operator": "EQUAL", "value": "bar"} - ], - } - ], - } - ], - } - - # When - response = admin_client_new.put( - url, data=json.dumps(data), content_type="application/json" - ) - # Then - assert response.status_code == status.HTTP_200_OK - assert response.json()["rules"][0]["type"] == "ANY" - assert response.json()["rules"][0]["rules"][0]["type"] == "ALL" - assert response.json()["rules"][0]["rules"][0]["conditions"][0]["property"] == "foo" - assert ( - response.json()["rules"][0]["rules"][0]["conditions"][0]["operator"] == "EQUAL" - ) - assert response.json()["rules"][0]["rules"][0]["conditions"][0]["value"] == "bar" - - +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 def test_update_segment__add_new_nested_rule__creates_new_rule( project: Project, admin_client_new: APIClient, @@ -898,6 +887,7 @@ def test_update_segment__add_new_nested_rule__creates_new_rule( assert segment_rule.rules.count() == 2 +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 def test_update_segment__add_new_condition__creates_new_condition( project: Project, admin_client_new: APIClient, @@ -968,6 +958,7 @@ def test_update_segment__add_new_condition__creates_new_condition( assert expected_new_condition.value == new_condition_value +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 def test_update_segment__delete_and_update_conditions__applies_changes( project: Project, admin_client_new: APIClient, @@ -1066,7 +1057,174 @@ def test_update_segment__system_segment__returns_404( assert response.status_code == status.HTTP_404_NOT_FOUND +@pytest.mark.parametrize("method_name", ["put", "patch"]) +def test_update_segment__valid_rules__updates_segment_with_rules( + admin_client: APIClient, + project: Project, + method_name: str, + mocker: MockerFixture, + segment: Segment, + segment_rules: list[SegmentRuleType], +) -> None: + # Given + timestamp = "2099-01-01T00:00:00Z" + segment_rules[0]["conditions"][0]["value"] = "blue" + segment_rules[0]["rules"] = [] + + # When + with freezegun.freeze_time(timestamp): + method = getattr(admin_client, method_name) + response = method( + f"/api/v1/projects/{project.id}/segments/{segment.id}/", + data={ + "name": "ordinary people", + "description": "What is Matrix", + "rules": segment_rules, + }, + format="json", + ) + + # Then + assert response.status_code == 200 + segment.refresh_from_db() + assert segment.name == "ordinary people" + assert segment.description == "What is Matrix" + assert segment.rules_data == segment_rules + assert response.data == { + "id": segment.id, + "uuid": str(segment.uuid), + "created_at": mocker.ANY, + "updated_at": timestamp, + "name": "ordinary people", + "description": "What is Matrix", + "project": project.id, + "feature": None, + "version_of": segment.id, + "metadata": [], + "membership_counts": [], + "managed_by": "", + "rules": [ + { + "id": mocker.ANY, + "type": "ALL", + "conditions": [ + { + "id": mocker.ANY, + "property": "pill-taken", + "operator": "EQUAL", + "value": "blue", + "description": "Offered by Morpheus.", + }, + ], + "rules": [], + }, + ], + } + + +def test_update_segment__rules_and_conditions_with_ids__ignores_ids( + admin_client: APIClient, + project: Project, + segment: Segment, + segment_rules: list[SegmentRuleType], +) -> None: + # Given + segment_rules[0]["conditions"][0]["value"] = "blue" + expected_rules = deepcopy(segment_rules) + segment_rules[0]["id"] = 42 # type: ignore[typeddict-unknown-key] + segment_rules[0]["conditions"][0]["id"] = 43 # type: ignore[typeddict-unknown-key] + segment_rules[0]["rules"][0]["id"] = 44 # type: ignore[typeddict-unknown-key] + segment_rules[0]["rules"][0]["conditions"][0]["id"] = 45 # type: ignore[typeddict-unknown-key] + + # When + response = admin_client.put( + f"/api/v1/projects/{project.id}/segments/{segment.id}/", + data={ + "name": segment.name, + "rules": segment_rules, + }, + format="json", + ) + + # Then + assert response.status_code == 200 + segment.refresh_from_db() + assert segment.rules_data == expected_rules + + +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 +def test_patch_segment__rules_omitted__preserves_rules( + admin_client: APIClient, + project: Project, + segment_rules: list[SegmentRuleType], +) -> None: + # Given + create_response = admin_client.post( + f"/api/v1/projects/{project.id}/segments/", + data={ + "name": "unpatched people", + "description": "Still in the Matrix", + "rules": segment_rules, + }, + format="json", + ) + segment_id = create_response.json()["id"] + + # When + response = admin_client.patch( + f"/api/v1/projects/{project.id}/segments/{segment_id}/", + data={"name": "patched people"}, + format="json", + ) + + # Then + assert response.status_code == 200 + segment = Segment.objects.get(id=segment_id) + assert segment.name == "patched people" + assert segment.description == "Still in the Matrix" + assert segment.rules_data == segment_rules + assert response.json()["rules"] == create_response.json()["rules"] + + def test_update_segment__versioned_segment__creates_new_version( + admin_client: APIClient, + project: Project, + segment: Segment, + segment_rules: list[SegmentRuleType], +) -> None: + # Given + new_rules = deepcopy(segment_rules) + new_rules[0]["conditions"][0]["value"] = "new value" + + # When + response = admin_client.put( + f"/api/v1/projects/{project.id}/segments/{segment.id}/", + data={ + "name": "new name", + "rules": new_rules, + }, + format="json", + ) + + # Then + assert response.status_code == 200 + versioned_segment = Segment.objects.get(version_of=segment, version=1) + segment.refresh_from_db() + assert versioned_segment.uuid != segment.uuid + assert versioned_segment.project == project + assert versioned_segment.feature is None + assert versioned_segment.name == "segment" + assert versioned_segment.description == "description" + assert versioned_segment.rules_data == segment_rules + assert segment.version == 2 + assert segment.version_of == segment + assert segment.name == "new name" + assert segment.description == "description" + assert segment.rules_data == new_rules + + +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 +def test_update_segment__versioned_segment__creates_new_version_x_replaced_above( project: Project, admin_client_new: APIClient, segment: Segment, @@ -1146,6 +1304,43 @@ def test_update_segment__versioned_segment__creates_new_version( def test_update_segment__exception_during_update__does_not_change_version( + admin_client: APIClient, + mocker: MockerFixture, + project: Project, + segment: Segment, + segment_rules: list[SegmentRuleType], +) -> None: + # Given + new_rules = deepcopy(segment_rules) + new_rules[0]["conditions"][0]["value"] = "new value" + mocker.patch( + "rest_framework.serializers.ModelSerializer.update", + side_effect=Exception("oops"), + ) + + # When + with pytest.raises(Exception): + admin_client.put( + f"/api/v1/projects/{project.id}/segments/{segment.id}/", + data={ + "name": "new name", + "description": "new description", + "rules": new_rules, + }, + format="json", + ) + + # Then + assert Segment.objects.filter(version_of=segment).count() == 1 + segment.refresh_from_db() + assert segment.version == 1 + assert segment.name == "segment" + assert segment.description == "description" + assert segment.rules_data == segment_rules + + +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 +def test_update_segment__exception_during_update__does_not_change_version_x_replaced_above( project: Project, admin_client_new: APIClient, segment: Segment, @@ -1217,11 +1412,47 @@ def test_update_segment__exception_during_update__does_not_change_version( assert segment.version == 1 == Segment.objects.filter(version_of=segment).count() +@pytest.mark.parametrize( + "rules_modifier", + [ + lambda rules: rules[0]["rules"][0]["conditions"][0].update({"delete": True}), + lambda rules: rules[0]["rules"][0]["conditions"].pop(0), + ], +) +def test_update_segment__delete_existing_condition__removes_condition( + admin_client: APIClient, + project: Project, + rules_modifier: Callable[[list[SegmentRuleType]], None], + segment: Segment, + segment_rules: list[SegmentRuleType], +) -> None: + # Given + expected = deepcopy(segment_rules) + del expected[0]["rules"][0]["conditions"][0] + rules_modifier(segment_rules) + + # When + response = admin_client.put( + f"/api/v1/projects/{project.id}/segments/{segment.id}/", + data={ + "name": segment.name, + "rules": segment_rules, + }, + format="json", + ) + + # Then + assert response.status_code == 200 + segment.refresh_from_db() + assert segment.rules_data == expected + + +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 @pytest.mark.parametrize( "client", [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) -def test_update_segment__delete_existing_condition__removes_condition( # type: ignore[no-untyped-def] +def test_update_segment__delete_existing_condition__removes_condition_x_replaced_above( # type: ignore[no-untyped-def] project, client, segment, segment_rule ): # Given @@ -1271,11 +1502,47 @@ def test_update_segment__delete_existing_condition__removes_condition( # type: assert nested_rule.conditions.count() == 0 +@pytest.mark.parametrize( + "rules_modifier", + [ + lambda rules: rules[0]["rules"][0].update({"delete": True}), + lambda rules: rules[0]["rules"].pop(0), + ], +) +def test_update_segment__delete_existing_rule__removes_rule( + admin_client: APIClient, + project: Project, + rules_modifier: Callable[[list[SegmentRuleType]], None], + segment: Segment, + segment_rules: list[SegmentRuleType], +) -> None: + # Given + expected = deepcopy(segment_rules) + del expected[0]["rules"][0] + rules_modifier(segment_rules) + + # When + response = admin_client.put( + f"/api/v1/projects/{project.id}/segments/{segment.id}/", + data={ + "name": segment.name, + "rules": segment_rules, + }, + format="json", + ) + + # Then + assert response.status_code == 200 + segment.refresh_from_db() + assert segment.rules_data == expected + + +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 @pytest.mark.parametrize( "client", [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) -def test_update_segment__delete_existing_rule__removes_rule( # type: ignore[no-untyped-def] +def test_update_segment__delete_existing_rule__removes_rule_x_replaced_above( # type: ignore[no-untyped-def] project, client, segment, segment_rule ): # Given @@ -1480,7 +1747,41 @@ def test_create_segment__missing_required_metadata__returns_400( assert response.status_code == status.HTTP_400_BAD_REQUEST -def test_update_segment__exceeds_max_conditions__returns_400( +@pytest.mark.parametrize("method_name", ["put", "patch"]) +def test_update_segment__invalid_rules__returns_400( + admin_client: APIClient, + project: Project, + method_name: str, + invalid_rules_case: InvalidSegmentRulesCase, + segment: Segment, + segment_rules: list[SegmentRuleType], +) -> None: + # Given + rules_breaker, expected_error = invalid_rules_case + expected_rules = deepcopy(segment_rules) + rules_breaker(segment_rules) + + # When + method = getattr(admin_client, method_name) + response = method( + f"/api/v1/projects/{project.id}/segments/{segment.id}/", + data={ + "name": segment.name, + "rules": segment_rules, + }, + format="json", + ) + + # Then + assert response.status_code == 400 + assert response.json() == expected_error + segment.refresh_from_db() + assert segment.rules_data == expected_rules + assert Segment.objects.filter(version_of=segment).count() == 1 + + +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 +def test_update_segment__exceeds_max_conditions__returns_400_x_replaced_above( project: Project, admin_client: APIClient, segment: Segment, @@ -1692,6 +1993,69 @@ def test_create_segment__duplicate_metadata_id_from_other_segment__keeps_metadat def test_update_segment__whitelisted_segment_exceeds_max_conditions__returns_200( + admin_client: APIClient, + mocker: MockerFixture, + project: Project, + segment: Segment, +) -> None: + # Given + WhitelistedSegment.objects.create(segment=segment) + timestamp = "2099-01-01T00:00:00Z" + over_limit_rule: SegmentRuleType = { + "type": "ALL", + "conditions": [ + { + "property": f"prop_{i}", + "operator": "EQUAL", + "value": "red", + "description": None, + } + for i in range(settings.SEGMENT_RULES_CONDITIONS_LIMIT + 1) + ], + "rules": [], + } + + # When + with freezegun.freeze_time(timestamp): + response = admin_client.put( + f"/api/v1/projects/{project.id}/segments/{segment.id}/", + data={"name": segment.name, "rules": [over_limit_rule]}, + format="json", + ) + + # Then + assert response.status_code == 200 + assert response.data == { + "id": segment.id, + "uuid": str(segment.uuid), + "created_at": mocker.ANY, + "updated_at": timestamp, + "name": segment.name, + "description": segment.description, + "project": project.id, + "feature": None, + "version_of": segment.id, + "metadata": [], + "membership_counts": [], + "managed_by": "", + "rules": [ + { + "id": mocker.ANY, + "type": "ALL", + "conditions": [ + {"id": mocker.ANY, **condition} + for condition in over_limit_rule["conditions"] + ], + "rules": [], + }, + ], + } + segment.refresh_from_db() + assert segment.rules_data == [over_limit_rule] + + +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 +def test_update_segment__whitelisted_segment_exceeds_max_conditions__returns_200_x_replaced_above( project: Project, admin_client: APIClient, segment: Segment, @@ -1765,63 +2129,6 @@ def test_update_segment__whitelisted_segment_exceeds_max_conditions__returns_200 assert nested_rule.conditions.count() == 11 -def test_create_segment__exceeds_max_conditions__returns_400( - project: Project, - admin_client: APIClient, - settings: SettingsWrapper, -) -> None: - # Given - url = reverse("api-v1:projects:project-segments-list", args=[project.id]) - - # Reduce value for test debugging. - settings.SEGMENT_RULES_CONDITIONS_LIMIT = 10 - new_condition_property = "prop_" - new_condition_value = "red" - new_conditions = [] - for i in range(settings.SEGMENT_RULES_CONDITIONS_LIMIT + 1): - new_conditions.append( - { - "property": f"{new_condition_property}{i}", - "operator": EQUAL, - "value": new_condition_value, - } - ) - - data = { - "name": "segment_name", - "project": project.id, - "rules": [ - { - "conditions": [], - "type": "ALL", - "rules": [ - { - "type": "ANY", - "rules": [], - "conditions": [ - *new_conditions, - ], - } - ], - } - ], - } - - # When - response = admin_client.post( - url, data=json.dumps(data), content_type="application/json" - ) - - # Then - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert response.json() == { - "segment": [ - "The segment has 11 conditions, which exceeds the maximum condition count of 10." - ] - } - assert Segment.objects.count() == 0 - - def test_list_segments__include_feature_specific_true__returns_all_segments( staff_client: APIClient, with_project_permissions: WithProjectPermissionsCallable, @@ -1890,9 +2197,34 @@ def test_clone_segment__valid_name__returns_cloned_segment( assert response.status_code == status.HTTP_201_CREATED response_data = response.json() - assert response_data["name"] == new_segment_name - assert response_data["project"] == project.id - assert response_data["id"] != segment.id + cloned_segment = Segment.objects.get(id=response_data["id"]) + assert cloned_segment != segment + assert cloned_segment.uuid != segment.uuid + assert cloned_segment.name == new_segment_name + assert cloned_segment.description == segment.description + assert cloned_segment.project == project + assert cloned_segment.feature is None + assert cloned_segment.version == 1 + assert cloned_segment.version_of == cloned_segment + assert cloned_segment.rules_data == segment.rules_data + assert ( + response_data + == { + "id": cloned_segment.id, + "uuid": str(cloned_segment.uuid), + "created_at": mocker.ANY, + "updated_at": mocker.ANY, + "name": new_segment_name, + "description": segment.description, + "project": project.id, + "feature": None, + "version_of": cloned_segment.id, + "metadata": [], + "membership_counts": [], + "managed_by": "", + "rules": [], # TODO: Should contain rules as per https://github.com/Flagsmith/flagsmith/issues/7818 + } + ) def test_clone_segment__no_name_provided__returns_400( diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index 8bafb8eaf0c9..cd0c5571b89a 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -607,7 +607,7 @@ Attributes: ### `segments.serializers.segment_revision_created` Logged at `info` from: - - `api/segments/serializers.py:159` + - `api/segments/serializers.py:185` Attributes: - `revision_id` @@ -690,7 +690,7 @@ Attributes: ### `warehouse.connection.connected` Logged at `info` from: - - `api/experimentation/services.py:1118` + - `api/experimentation/services.py:1147` Attributes: - `environment.id` @@ -699,8 +699,8 @@ Attributes: ### `warehouse.connection.event_names_failed` Logged at `warning` from: - - `api/experimentation/services.py:223` - - `api/experimentation/services.py:1218` + - `api/experimentation/services.py:226` + - `api/experimentation/services.py:1247` Attributes: - `environment.id` @@ -710,7 +710,7 @@ Attributes: ### `warehouse.connection.event_stats_failed` Logged at `warning` from: - - `api/experimentation/services.py:1181` + - `api/experimentation/services.py:1210` Attributes: - `environment.id` @@ -719,7 +719,7 @@ Attributes: ### `warehouse.connection.test_event_sent` Logged at `info` from: - - `api/experimentation/services.py:892` + - `api/experimentation/services.py:921` Attributes: - `environment.id` @@ -728,7 +728,7 @@ Attributes: ### `warehouse.connection.verification_failed` Logged at `warning` from: - - `api/experimentation/services.py:1093` + - `api/experimentation/services.py:1122` Attributes: - `environment.id` @@ -738,7 +738,7 @@ Attributes: ### `warehouse.connection.verification_succeeded` Logged at `info` from: - - `api/experimentation/services.py:1103` + - `api/experimentation/services.py:1132` Attributes: - `environment.id` @@ -747,7 +747,7 @@ Attributes: ### `warehouse.delivery.all_objects_rejected` Logged at `error` from: - - `api/experimentation/services.py:1048` + - `api/experimentation/services.py:1077` Attributes: - `connection.id` @@ -758,7 +758,7 @@ Attributes: ### `warehouse.delivery.budget_exhausted` Logged at `info` from: - - `api/experimentation/services.py:937` + - `api/experimentation/services.py:966` Attributes: - `connection.id` @@ -769,7 +769,7 @@ Attributes: ### `warehouse.delivery.completed` Logged at `info` from: - - `api/experimentation/services.py:1058` + - `api/experimentation/services.py:1087` Attributes: - `connection.id` @@ -782,7 +782,7 @@ Attributes: ### `warehouse.delivery.failed` Logged at `error` from: - - `api/experimentation/services.py:1031` + - `api/experimentation/services.py:1060` Attributes: - `connection.id` @@ -793,7 +793,7 @@ Attributes: ### `warehouse.delivery.object_rejected` Logged at `error` from: - - `api/experimentation/services.py:966` + - `api/experimentation/services.py:995` Attributes: - `connection.id` @@ -805,7 +805,7 @@ Attributes: ### `warehouse.srm.overallocated` Logged at `error` from: - - `api/experimentation/services.py:514` + - `api/experimentation/services.py:517` Attributes: - `environment.id` @@ -815,7 +815,7 @@ Attributes: ### `warehouse.srm.unkeyed_variant` Logged at `error` from: - - `api/experimentation/services.py:500` + - `api/experimentation/services.py:503` Attributes: - `environment.id` diff --git a/mcp/src/flagsmith_mcp/openapi.json b/mcp/src/flagsmith_mcp/openapi.json index 785a8d1fd5c1..cdb4319246ba 100644 --- a/mcp/src/flagsmith_mcp/openapi.json +++ b/mcp/src/flagsmith_mcp/openapi.json @@ -6881,7 +6881,8 @@ ] }, "project": { - "type": "integer" + "type": "integer", + "readOnly": true }, "feature": { "type": [ @@ -6928,7 +6929,6 @@ }, "required": [ "name", - "project", "rules" ] }, diff --git a/openapi.yaml b/openapi.yaml index f63fa4b79d6d..e84747945314 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -18649,6 +18649,7 @@ components: - 'null' project: type: integer + readOnly: true feature: type: - integer @@ -18681,7 +18682,6 @@ components: - 'null' required: - name - - project - rules ChangeRequestUpdate: description: Adds nested create feature @@ -24755,6 +24755,7 @@ components: - 'null' project: type: integer + readOnly: true feature: type: - integer @@ -26435,6 +26436,7 @@ components: - 'null' project: type: integer + readOnly: true feature: type: - integer @@ -26463,7 +26465,6 @@ components: readOnly: true required: - name - - project - rules SegmentAssociatedFeatureState: type: object