From ef75472c3b50d1df2b08e9fbf9f0d71962da049b Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Fri, 7 Aug 2026 20:50:50 -0300 Subject: [PATCH 1/5] tdd for rules_data --- .../migrations/0031_add_segment_rules_data.py | 23 + api/segments/models.py | 4 + api/segments/serializers.py | 51 +- api/segments/services.py | 3 +- api/segments/types.py | 29 +- api/tests/conftest.py | 49 +- api/tests/types.py | 11 +- api/tests/unit/segments/conftest.py | 77 +++ .../segments/test_unit_segments_models.py | 42 +- .../segments/test_unit_segments_services.py | 13 + .../unit/segments/test_unit_segments_views.py | 628 +++++++++++++----- .../observability/_events-catalogue.md | 4 +- 12 files changed, 738 insertions(+), 196 deletions(-) create mode 100644 api/segments/migrations/0031_add_segment_rules_data.py create mode 100644 api/tests/unit/segments/conftest.py diff --git a/api/segments/migrations/0031_add_segment_rules_data.py b/api/segments/migrations/0031_add_segment_rules_data.py new file mode 100644 index 000000000000..8d49b6cc6df6 --- /dev/null +++ b/api/segments/migrations/0031_add_segment_rules_data.py @@ -0,0 +1,23 @@ +# Generated by Django 5.2.16 on 2026-08-07 15:09 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("segments", "0030_add_default_to_segment_version"), + ] + + 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), + ), + ] diff --git a/api/segments/models.py b/api/segments/models.py index 5d0ddeb0636f..eb07ade282d3 100644 --- a/api/segments/models.py +++ b/api/segments/models.py @@ -98,6 +98,10 @@ class Segment( Feature, on_delete=models.CASCADE, related_name="segments", null=True ) + rules_data = 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 7bb06eaa0f85..9a005451cc93 100644 --- a/api/segments/serializers.py +++ b/api/segments/serializers.py @@ -14,6 +14,10 @@ from segment_membership.models import SegmentMembershipCount from segment_membership.services import enqueue_membership_refresh from segments.models import Condition, Segment, SegmentRule +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__) @@ -101,6 +105,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: @@ -127,7 +133,11 @@ class Meta: "metadata", "membership_counts", ] - read_only_fields = ["membership_counts"] + read_only_fields = [ + "membership_counts", + "project", + "version_of", + ] def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: attrs = super().validate(attrs) @@ -147,6 +157,7 @@ def create(self, validated_data: dict[str, Any]): # type: ignore[no-untyped-def metadata_data = validated_data.pop("metadata", []) segment = super().create(validated_data) # type: ignore[no-untyped-call] self._update_metadata(segment, metadata_data) + self._set_rules_data(segment, validated_data["rules"]) enqueue_membership_refresh(segment.project) return segment @@ -162,9 +173,44 @@ def update(self, segment: Segment, validated_data: dict[str, Any]): # type: ign ) segment = super().update(segment, validated_data) # type: ignore[no-untyped-call] self._update_metadata(segment, metadata) + self._set_rules_data(segment, validated_data["rules"]) enqueue_membership_refresh(segment.project) return segment + def _set_rules_data(self, segment: Segment, rules: list[LegacySegmentRule]) -> None: + """Set the .rules_data attribute + TODO: Delete this as per https://github.com/Flagsmith/flagsmith/issues/7818 + """ + segment.rules_data = self._cleanup_rules_and_conditions(rules) + segment.save(update_fields=["rules_data"]) + + def _cleanup_rules_and_conditions( + self, rules_data: list[LegacySegmentRule] + ) -> list[SegmentRuleType]: + """Remove any `id` fields and `delete: true` 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_data["type"], + "conditions": [ + { + "property": condition_data["property"], + "operator": condition_data["operator"], + "value": condition_data.get("value"), + "description": condition_data.get("description"), + } + for condition_data in rule_data.get("conditions", []) + if not condition_data.get("delete") + ], + "rules": self._cleanup_rules_and_conditions(rule_data.get("rules", [])), + } + for rule_data in rules_data + if not rule_data.get("delete") + ] + def _get_rules_and_conditions_without_deleted( self, rules_data: DictList ) -> DictList: @@ -175,8 +221,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 [ { 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..e06a9bbee36b 100644 --- a/api/segments/types.py +++ b/api/segments/types.py @@ -1,5 +1,32 @@ -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 SegmentRule(TypedDict): + type: RuleType + conditions: list[SegmentCondition] + rules: list["SegmentRule"] + + +class LegacySegmentCondition(SegmentCondition): + id: NotRequired[int] + delete: NotRequired[bool] + + +class LegacySegmentRule(TypedDict): + id: NotRequired[int] + delete: NotRequired[bool] + type: RuleType + conditions: list[LegacySegmentCondition] + rules: list["LegacySegmentRule"] diff --git a/api/tests/conftest.py b/api/tests/conftest.py index f03355c7feb9..8050a4924d08 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,50 @@ 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!", + }, + ], + "rules": [], + }, + ], + } + ] + + +@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/segments/conftest.py b/api/tests/unit/segments/conftest.py new file mode 100644 index 000000000000..3a6e0ee989c0 --- /dev/null +++ b/api/tests/unit/segments/conftest.py @@ -0,0 +1,77 @@ +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 request.param # type: ignore[no-any-return] 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 c015d2e348f1..bc8234aba451 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, @@ -33,7 +36,10 @@ from organisations.models import Organisation from projects.models import Project from segments.models import Condition, Segment, 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() @@ -57,20 +63,84 @@ 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": [], + "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( @@ -163,6 +233,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")], @@ -316,29 +412,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")], @@ -714,13 +787,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" % ( @@ -739,85 +805,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, @@ -891,6 +879,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, @@ -961,6 +950,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, @@ -1059,7 +1049,109 @@ 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": [], + "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__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, @@ -1139,6 +1231,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, @@ -1210,11 +1339,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 @@ -1264,11 +1429,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 @@ -1473,7 +1674,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, @@ -1685,6 +1920,68 @@ 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": [], + "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, @@ -1758,63 +2055,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, @@ -1883,9 +2123,33 @@ 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": [], + "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 591712fced24..687f5eced8aa 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -103,7 +103,7 @@ Attributes: ### `core.encrypted_field.decrypt_failed` Logged at `warning` from: - - `api/core/fields.py:37` + - `api/core/fields.py:62` Attributes: - `exc_info` @@ -577,7 +577,7 @@ Attributes: ### `segments.serializers.segment_revision_created` Logged at `info` from: - - `api/segments/serializers.py:158` + - `api/segments/serializers.py:169` Attributes: - `revision_id` From 35a8bf86320847abbb47a37dec2c47743d05571b Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Fri, 7 Aug 2026 22:01:39 -0300 Subject: [PATCH 2/5] fulfill rules_data --- api/segments/serializers.py | 34 +++------ api/segments/validators.py | 71 +++++++++++++++++++ .../observability/_events-catalogue.md | 2 +- 3 files changed, 80 insertions(+), 27 deletions(-) create mode 100644 api/segments/validators.py diff --git a/api/segments/serializers.py b/api/segments/serializers.py index 9a005451cc93..49eed14bc357 100644 --- a/api/segments/serializers.py +++ b/api/segments/serializers.py @@ -1,7 +1,6 @@ from typing import Any import structlog -from django.conf import settings from django.db import transaction from drf_writable_nested.serializers import WritableNestedModelSerializer from rest_framework import serializers @@ -18,6 +17,7 @@ # TODO: Delete alias as per https://github.com/Flagsmith/flagsmith/issues/7818 from segments.types import SegmentRule as SegmentRuleType +from segments.validators import SegmentRulesValidator logger = structlog.get_logger(__name__) @@ -138,6 +138,7 @@ class Meta: "project", "version_of", ] + validators = [SegmentRulesValidator()] def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: attrs = super().validate(attrs) @@ -149,20 +150,20 @@ 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) 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) - self._set_rules_data(segment, validated_data["rules"]) enqueue_membership_refresh(segment.project) return segment 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) @@ -173,16 +174,16 @@ def update(self, segment: Segment, validated_data: dict[str, Any]): # type: ign ) segment = super().update(segment, validated_data) # type: ignore[no-untyped-call] self._update_metadata(segment, metadata) - self._set_rules_data(segment, validated_data["rules"]) enqueue_membership_refresh(segment.project) return segment - def _set_rules_data(self, segment: Segment, rules: list[LegacySegmentRule]) -> None: + 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 """ - segment.rules_data = self._cleanup_rules_and_conditions(rules) - segment.save(update_fields=["rules_data"]) + validated_data["rules_data"] = self._cleanup_rules_and_conditions( + validated_data["rules"] + ) def _cleanup_rules_and_conditions( self, rules_data: list[LegacySegmentRule] @@ -250,25 +251,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/validators.py b/api/segments/validators.py new file mode 100644 index 000000000000..85e7a7685840 --- /dev/null +++ b/api/segments/validators.py @@ -0,0 +1,71 @@ +from typing import Any + +from django.conf import settings +from rest_framework import serializers +from rest_framework.exceptions import ValidationError + +from segments.models import WhitelistedSegment +from segments.types import LegacySegmentRule + +SEGMENT_RULES_MAX_DEPTH = 2 + + +class SegmentRulesValidator: + """ + Validate segment rules against platform limits: nesting depth and, + unless the segment is whitelisted, total condition count. + """ + + requires_context = True + + def __call__( + self, + attrs: dict[str, Any], + serializer: serializers.BaseSerializer[Any], + ) -> None: + rules_data: list[LegacySegmentRule] = serializer.initial_data.get("rules", []) + self._validate_depth(rules_data) + if not self._is_whitelisted(serializer): + self._validate_condition_count(rules_data) + + def _validate_depth( + self, rules_data: list[LegacySegmentRule], _depth: int = 1 + ) -> None: + for rule_data in rules_data: + if _depth >= SEGMENT_RULES_MAX_DEPTH and rule_data.get("rules"): + raise ValidationError( + { + "segment": ( + f"Rules must not be nested more than " + f"{SEGMENT_RULES_MAX_DEPTH} levels deep." + ) + } + ) + self._validate_depth(rule_data.get("rules", []), _depth + 1) + + def _validate_condition_count(self, rules_data: list[LegacySegmentRule]) -> None: + condition_count = self._count_conditions(rules_data) + if condition_count > settings.SEGMENT_RULES_CONDITIONS_LIMIT: + 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_data: list[LegacySegmentRule]) -> int: + return sum( + len(rule_data.get("conditions", [])) + + self._count_conditions(rule_data.get("rules", [])) + for rule_data in rules_data + ) + + @staticmethod + def _is_whitelisted(serializer: serializers.BaseSerializer[Any]) -> bool: + return bool( + (segment := serializer.instance) + and WhitelistedSegment.objects.filter(segment=segment).exists() + ) diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index 687f5eced8aa..f13eed8d5fd8 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -577,7 +577,7 @@ Attributes: ### `segments.serializers.segment_revision_created` Logged at `info` from: - - `api/segments/serializers.py:169` + - `api/segments/serializers.py:170` Attributes: - `revision_id` From 854f0971fe127dca04a4e57fa0f49a25ceb6c098 Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Mon, 10 Aug 2026 20:36:10 -0300 Subject: [PATCH 3/5] json, json everywhere --- .../migrations/0031_add_segment_rules_data.py | 70 ++++++++++ api/segments/models.py | 4 +- .../segments/test_unit_segments_migrations.py | 125 ++++++++++++++++++ 3 files changed, 196 insertions(+), 3 deletions(-) diff --git a/api/segments/migrations/0031_add_segment_rules_data.py b/api/segments/migrations/0031_add_segment_rules_data.py index 8d49b6cc6df6..5c7098d9bf5c 100644 --- a/api/segments/migrations/0031_add_segment_rules_data.py +++ b/api/segments/migrations/0031_add_segment_rules_data.py @@ -1,6 +1,72 @@ # 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 + +RuleType = 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"), # rasterise recurses one level deeper + ).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[RuleType]: + return [ + { + "type": rule.type, + "conditions": [ + { + "property": condition.property, + "operator": condition.operator, + "value": condition.value, + "description": condition.description, + } + for condition in rule.live_conditions + ], + "rules": _rasterise_segment_rules(rule), + } + for rule in obj.live_rules + ] class Migration(migrations.Migration): @@ -20,4 +86,8 @@ class Migration(migrations.Migration): 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 eb07ade282d3..5ab906b11f3f 100644 --- a/api/segments/models.py +++ b/api/segments/models.py @@ -98,9 +98,7 @@ class Segment( Feature, on_delete=models.CASCADE, related_name="segments", null=True ) - rules_data = models.JSONField( - null=True, - ) + rules_data = models.JSONField(null=True) version = models.IntegerField(default=1, null=True) diff --git a/api/tests/unit/segments/test_unit_segments_migrations.py b/api/tests/unit/segments/test_unit_segments_migrations.py index 6aeb48ed8c30..5a2749436afd 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_0031 = import_module("segments.migrations.0031_add_segment_rules_data") + @pytest.mark.skipif( test_settings.SKIP_MIGRATION_TESTS is True, @@ -243,3 +247,124 @@ 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_0031_add_segment_rules_data__forwards__backfill_segment_rules_data( + migrator: Migrator, +) -> None: + # Given + state = migrator.apply_initial_migration( + ("segments", "0031_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") + SegmentRule.objects.create(rule=top_rule, type="ANY", deleted_at=timezone.now()) + 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(), + ) + + 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") + + # When + migration_0031.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": [], + "rules": [ + { + "type": "ANY", + "conditions": [ + { + "property": "age", + "operator": constants.EQUAL, + "value": "21", + "description": "Adults only", + } + ], + "rules": [], + } + ], + } + ] + assert deleted_segment.rules_data is None + assert old_version_segment.rules_data is None + + +@pytest.mark.skipif( + test_settings.SKIP_MIGRATION_TESTS is True, + reason="Skip migration tests to speed up tests where necessary", +) +def test_0031_add_segment_rules_data__backwards__nullify_segment_rules_data( + migrator: Migrator, +) -> None: + # Given + state = migrator.apply_initial_migration( + ("segments", "0031_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_0031.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 From 857dfa69b65950c613d2aa4c23a1720d73150263 Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Mon, 10 Aug 2026 23:58:20 -0300 Subject: [PATCH 4/5] KeepMessly --- api/integrations/launch_darkly/services.py | 173 +++++- ...ments__correctly_imported__rules_data.json | 464 ++++++++++++++++ .../launch_darkly/test_services.py | 509 +++++++++++++++++- 3 files changed, 1121 insertions(+), 25 deletions(-) create mode 100644 api/tests/unit/integrations/launch_darkly/snapshots/test_process_import_request__large_segments__correctly_imported__rules_data.json diff --git a/api/integrations/launch_darkly/services.py b/api/integrations/launch_darkly/services.py index 9c33997bb588..81c93c27765a 100644 --- a/api/integrations/launch_darkly/services.py +++ b/api/integrations/launch_darkly/services.py @@ -8,6 +8,7 @@ from django.core import signing 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 +42,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 +146,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 +295,109 @@ def _create_feature_segments_for_segment_match_clauses( return feature_states +def _clauses_to_segment_subrules( + import_request: LaunchDarklyImportRequest, + segment_name: str, + clauses: list[Clause], +) -> list[SegmentRuleType]: + """Convert Launch Darkly clauses into subrules for a segment's "ALL" root rule.""" + subrules: list[SegmentRuleType] = [] + negated_subrule: Optional[SegmentRuleType] = 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 is None: + negated_subrule = { + "type": constants.NONE_RULE, + "conditions": [], + "rules": [], + } + subrules.append(negated_subrule) + negated_subrule["conditions"] += conditions + else: + subrules.append( + {"type": constants.ANY_RULE, "conditions": conditions, "rules": []} + ) + + return subrules + + +def _users_to_segment_subrules( + import_request: LaunchDarklyImportRequest, + segment_name: str, + users: list[str], + negate: bool, +) -> list[SegmentRuleType]: + """Convert Launch Darkly's targeted user lists into subrules for a segment's "ALL" root rule.""" + if len(users) == 0: + return [] + + subrules: list[SegmentRuleType] = [] + 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 + subrules.append( + { + "type": constants.NONE_RULE if negate else constants.ANY_RULE, + "conditions": [ + { + "property": "key", + "operator": constants.IN, + "value": identities_string, + "description": None, + } + ], + "rules": [], + } + ) + return subrules + + def _create_segment_rule_for_segment( import_request: LaunchDarklyImportRequest, segment: Segment, @@ -341,14 +449,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 +457,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 +503,20 @@ def _create_feature_segment_from_clauses( name=rule_name, project=project, feature=feature ) + subrules = _clauses_to_segment_subrules( + import_request=import_request, + segment_name=segment.name, + clauses=clauses, + ) + rules_data = segment.rules_data or [ # LaunchDarkly environments share the segment + {"type": constants.ALL_RULE, "conditions": [], "rules": []} + ] + rules_data[0]["rules"] += subrules + segment.rules_data = rules_data + 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 +1081,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 +1123,17 @@ def _create_segments_from_ld( # TODO: Tagging segments is not supported yet. https://github.com/Flagsmith/flagsmith/issues/3241 + subrules: list[SegmentRuleType] = [] + # Create the segment rule for the segment. rules = ld_segment["rules"] for rule in rules: + subrules += _clauses_to_segment_subrules( + import_request=import_request, + segment_name=segment.name, + clauses=rule["clauses"], + ) + # TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 _create_segment_rule_for_segment( import_request=import_request, segment=segment, @@ -1048,6 +1155,20 @@ def _create_segments_from_ld( ] ) + subrules += _users_to_segment_subrules( + import_request=import_request, + segment_name=segment.name, + users=ld_segment["included"], + negate=False, + ) + subrules += _users_to_segment_subrules( + import_request=import_request, + segment_name=segment.name, + users=ld_segment["excluded"], + negate=True, + ) + + # TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 _include_users_to_segment( import_request=import_request, segment=segment, @@ -1072,8 +1193,14 @@ 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 = [ + {"type": constants.ALL_RULE, "conditions": [], "rules": subrules} + ] + segment.save(update_fields=["rules_data"]) + return segments_by_ld_key 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..c316bb7a42a8 --- /dev/null +++ b/api/tests/unit/integrations/launch_darkly/snapshots/test_process_import_request__large_segments__correctly_imported__rules_data.json @@ -0,0 +1,464 @@ +{ + "Large Dynamic List (Override for production)": [ + { + "type": "ALL", + "rules": [ + { + "type": "ANY", + "rules": [], + "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", + "rules": [], + "conditions": [] + } + ], + "conditions": [] + } + ], + "Large Dynamic List (Override for test)": [ + { + "type": "ALL", + "rules": [ + { + "type": "ANY", + "rules": [], + "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", + "rules": [], + "conditions": [] + } + ], + "conditions": [] + } + ], + "Large User List (Override for production)": [ + { + "type": "ALL", + "rules": [ + { + "type": "ANY", + "rules": [], + "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", + "rules": [], + "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", + "rules": [], + "conditions": [ + { + "value": "user-103", + "operator": "IN", + "property": "key", + "description": null + } + ] + } + ], + "conditions": [] + } + ], + "Large User List (Override for test)": [ + { + "type": "ALL", + "rules": [ + { + "type": "ANY", + "rules": [], + "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", + "rules": [], + "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", + "rules": [], + "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..3cf6c714fc3b 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 @@ -285,7 +289,212 @@ 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, + } + ], + "rules": [], + } + ], + } + ], + 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, + } + ], + "rules": [], + }, + { + "type": SegmentRule.ANY_RULE, + "conditions": [ + { + "property": "p2", + "operator": segment_constants.GREATER_THAN, + "value": "1.0.0:semver", + "description": None, + } + ], + "rules": [], + }, + { + "type": SegmentRule.ANY_RULE, + "conditions": [ + { + "property": "p3", + "operator": segment_constants.REGEX, + "value": "foo[0-9]{0,1}", + "description": None, + } + ], + "rules": [], + }, + { + "type": SegmentRule.ANY_RULE, # included users + "conditions": [ + { + "property": "key", + "operator": segment_constants.IN, + "value": "foo", + "description": None, + } + ], + "rules": [], + }, + { + "type": SegmentRule.NONE_RULE, # excluded users + "conditions": [ + { + "property": "key", + "operator": segment_constants.IN, + "value": "bar", + "description": None, + } + ], + "rules": [], + }, + ], + } + ], + 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, + } + ], + "rules": [], + }, + { + "type": SegmentRule.NONE_RULE, # excluded users + "conditions": [ + { + "property": "key", + "operator": segment_constants.IN, + "value": "user-103", + "description": None, + } + ], + "rules": [], + }, + ], + } + ], + 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 +695,157 @@ 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, + } + ], + "rules": [], + }, + { + "type": SegmentRule.ANY_RULE, + "conditions": [ + { + "property": "p2", + "operator": segment_constants.GREATER_THAN, + "value": "1", + "description": None, + } + ], + "rules": [], + }, + ], + } + ], + 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, + } + ], + "rules": [], + }, + { + "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, + }, + ], + "rules": [], + }, + ], + } + ], + 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, + } + ], + "rules": [], + }, + ], + } + ], + 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 +941,158 @@ 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": [], "rules": []} + ], + } + ], + 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 = ( From f09bd194c85563e28b8a9d37b138fed0ca77dfa8 Mon Sep 17 00:00:00 2001 From: "flagsmith-engineering[bot]" Date: Tue, 11 Aug 2026 03:11:08 +0000 Subject: [PATCH 5/5] chore: Update documentation artefacts --- mcp/src/flagsmith_mcp/openapi.json | 7 ++++--- openapi.yaml | 14 ++++++++++++-- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/mcp/src/flagsmith_mcp/openapi.json b/mcp/src/flagsmith_mcp/openapi.json index 330b2f25c097..cf000c9cbee5 100644 --- a/mcp/src/flagsmith_mcp/openapi.json +++ b/mcp/src/flagsmith_mcp/openapi.json @@ -6865,7 +6865,8 @@ ] }, "project": { - "type": "integer" + "type": "integer", + "readOnly": true }, "feature": { "type": [ @@ -6877,7 +6878,8 @@ "type": [ "integer", "null" - ] + ], + "readOnly": true }, "rules": { "type": "array", @@ -6901,7 +6903,6 @@ }, "required": [ "name", - "project", "rules" ] }, diff --git a/openapi.yaml b/openapi.yaml index ccfdf0f08579..713bbf265848 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -18537,6 +18537,7 @@ components: - 'null' project: type: integer + readOnly: true feature: type: - integer @@ -18545,6 +18546,7 @@ components: type: - integer - 'null' + readOnly: true rules: type: array items: @@ -18564,7 +18566,6 @@ components: - 'null' required: - name - - project - rules ChangeRequestUpdate: description: Adds nested create feature @@ -22243,6 +22244,7 @@ components: readOnly: true url: type: string + format: uri maxLength: 200 enabled: type: boolean @@ -24193,6 +24195,7 @@ components: readOnly: true url: type: string + format: uri maxLength: 200 enabled: type: boolean @@ -24582,6 +24585,7 @@ components: - 'null' project: type: integer + readOnly: true feature: type: - integer @@ -24590,6 +24594,7 @@ components: type: - integer - 'null' + readOnly: true rules: type: array items: @@ -25045,6 +25050,7 @@ components: url: type: string format: uri + maxLength: 200 enabled: type: boolean created_at: @@ -25066,6 +25072,7 @@ components: readOnly: true url: type: string + format: uri maxLength: 200 secret: type: string @@ -26255,6 +26262,7 @@ components: - 'null' project: type: integer + readOnly: true feature: type: - integer @@ -26263,6 +26271,7 @@ components: type: - integer - 'null' + readOnly: true rules: type: array items: @@ -26278,7 +26287,6 @@ components: readOnly: true required: - name - - project - rules SegmentAssociatedFeatureState: type: object @@ -28035,6 +28043,7 @@ components: url: type: string format: uri + maxLength: 200 enabled: type: boolean created_at: @@ -28058,6 +28067,7 @@ components: readOnly: true url: type: string + format: uri maxLength: 200 secret: type: string