From ef75472c3b50d1df2b08e9fbf9f0d71962da049b Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Fri, 7 Aug 2026 20:50:50 -0300 Subject: [PATCH 01/23] 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 02/23] 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 03/23] 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 04/23] 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 9b5e9e73b0f45e4a7c166bb0c0e27f99812523c2 Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Tue, 11 Aug 2026 11:37:47 -0300 Subject: [PATCH 05/23] stick with the rules! --- api/segments/serializers.py | 2 ++ .../unit/segments/test_unit_segments_views.py | 34 +++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/api/segments/serializers.py b/api/segments/serializers.py index 49eed14bc357..a702c255f86a 100644 --- a/api/segments/serializers.py +++ b/api/segments/serializers.py @@ -181,6 +181,8 @@ def _set_rules_data(self, validated_data: dict[str, Any]) -> None: """Set the .rules_data attribute TODO: Delete this as per https://github.com/Flagsmith/flagsmith/issues/7818 """ + if "rules" not in validated_data: + return # PATCH support validated_data["rules_data"] = self._cleanup_rules_and_conditions( validated_data["rules"] ) diff --git a/api/tests/unit/segments/test_unit_segments_views.py b/api/tests/unit/segments/test_unit_segments_views.py index bc8234aba451..b662e862602f 100644 --- a/api/tests/unit/segments/test_unit_segments_views.py +++ b/api/tests/unit/segments/test_unit_segments_views.py @@ -1113,6 +1113,40 @@ def test_update_segment__valid_rules__updates_segment_with_rules( } +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 +def test_patch_segment__rules_omitted__preserves_rules( + admin_client: APIClient, + project: Project, + segment_rules: list[SegmentRuleType], +) -> None: + # Given + create_response = admin_client.post( + f"/api/v1/projects/{project.id}/segments/", + data={ + "name": "unpatched people", + "description": "Still in the Matrix", + "rules": segment_rules, + }, + format="json", + ) + segment_id = create_response.json()["id"] + + # When + response = admin_client.patch( + f"/api/v1/projects/{project.id}/segments/{segment_id}/", + data={"name": "patched people"}, + format="json", + ) + + # Then + assert response.status_code == 200 + segment = Segment.objects.get(id=segment_id) + assert segment.name == "patched people" + assert segment.description == "Still in the Matrix" + assert segment.rules_data == segment_rules + assert response.json()["rules"] == create_response.json()["rules"] + + def test_update_segment__versioned_segment__creates_new_version( admin_client: APIClient, project: Project, From c32aea733a2d4280c5603bc2a3ca59516d3d3bc7 Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Tue, 11 Aug 2026 14:30:46 -0300 Subject: [PATCH 06/23] DRF is like a maze --- api/segments/serializers.py | 63 ++++++++++++++-- api/segments/validators.py | 71 ------------------- .../observability/_events-catalogue.md | 2 +- 3 files changed, 60 insertions(+), 76 deletions(-) delete mode 100644 api/segments/validators.py diff --git a/api/segments/serializers.py b/api/segments/serializers.py index a702c255f86a..ed6a2caa7953 100644 --- a/api/segments/serializers.py +++ b/api/segments/serializers.py @@ -1,6 +1,7 @@ -from typing import Any +from typing import Any, cast import structlog +from django.conf import settings from django.db import transaction from drf_writable_nested.serializers import WritableNestedModelSerializer from rest_framework import serializers @@ -12,17 +13,18 @@ from segment_membership.constants import MAX_SEGMENT_MEMBERS_PAGE_SIZE from segment_membership.models import SegmentMembershipCount from segment_membership.services import enqueue_membership_refresh -from segments.models import Condition, Segment, SegmentRule +from segments.models import Condition, Segment, SegmentRule, WhitelistedSegment from segments.types import LegacySegmentRule # TODO: Delete alias as per https://github.com/Flagsmith/flagsmith/issues/7818 from segments.types import SegmentRule as SegmentRuleType -from segments.validators import SegmentRulesValidator logger = structlog.get_logger(__name__) DictList = list[dict[str, Any]] +SEGMENT_RULES_MAX_DEPTH = 2 + class SegmentMembershipCountSerializer( serializers.ModelSerializer[SegmentMembershipCount] @@ -138,7 +140,10 @@ class Meta: "project", "version_of", ] - validators = [SegmentRulesValidator()] + + def to_internal_value(self, data: dict[str, Any]) -> Any: + self._validate_rules_depth(data.get("rules", [])) + return super().to_internal_value(data) def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: attrs = super().validate(attrs) @@ -151,6 +156,10 @@ def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: self._validate_required_metadata(organisation, metadata, project) self._validate_project_segment_limit(project) + + if "rules" in attrs: + self._validate_rules_condition_count(attrs["rules"]) + return attrs def create(self, validated_data: dict[str, Any]): # type: ignore[no-untyped-def] @@ -177,6 +186,52 @@ def update(self, segment: Segment, validated_data: dict[str, Any]): # type: ign enqueue_membership_refresh(segment.project) return segment + def _validate_rules_depth( + self, rules_data: list[LegacySegmentRule], _depth: int = 1 + ) -> None: + # Raise loudly because the interface ignores rules nested too deep + 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_rules_depth(rule_data.get("rules", []), _depth + 1) + + def _validate_rules_condition_count( + self, rules_data: list[LegacySegmentRule] + ) -> None: + if self._can_segment_own_more_conditions_than_limit(): + return + + 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 + ) + + def _can_segment_own_more_conditions_than_limit(self) -> bool: + if self.instance is not None and (segment := cast(Segment, self.instance)).id: + return WhitelistedSegment.objects.filter(segment=segment).exists() + return False + def _set_rules_data(self, validated_data: dict[str, Any]) -> None: """Set the .rules_data attribute TODO: Delete this as per https://github.com/Flagsmith/flagsmith/issues/7818 diff --git a/api/segments/validators.py b/api/segments/validators.py deleted file mode 100644 index 85e7a7685840..000000000000 --- a/api/segments/validators.py +++ /dev/null @@ -1,71 +0,0 @@ -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 f13eed8d5fd8..fa9328d8a886 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:170` + - `api/segments/serializers.py:179` Attributes: - `revision_id` From 2d70da3fb0f99f47851cbf631f46c87b71af26d1 Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Tue, 11 Aug 2026 14:40:10 -0300 Subject: [PATCH 07/23] they're sneaky! --- api/tests/unit/segments/conftest.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/api/tests/unit/segments/conftest.py b/api/tests/unit/segments/conftest.py index 3a6e0ee989c0..94cdf8f5f42e 100644 --- a/api/tests/unit/segments/conftest.py +++ b/api/tests/unit/segments/conftest.py @@ -1,3 +1,5 @@ +from typing import cast + import pytest from django.conf import settings from pytest import FixtureRequest @@ -74,4 +76,4 @@ ], ) def invalid_rules_case(request: FixtureRequest) -> InvalidSegmentRulesCase: - return request.param # type: ignore[no-any-return] + return cast(InvalidSegmentRulesCase, request.param) From 09ab3d3db30d48d78dfd95cd546d9f25b29e9b98 Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Tue, 11 Aug 2026 15:51:33 -0300 Subject: [PATCH 08/23] improve types --- api/integrations/launch_darkly/services.py | 6 +- .../migrations/0031_add_segment_rules_data.py | 4 +- api/segments/serializers.py | 89 +++++++++++-------- api/segments/types.py | 22 ++++- api/tests/conftest.py | 1 - .../observability/_events-catalogue.md | 2 +- 6 files changed, 77 insertions(+), 47 deletions(-) diff --git a/api/integrations/launch_darkly/services.py b/api/integrations/launch_darkly/services.py index 81c93c27765a..f0696ddbef6b 100644 --- a/api/integrations/launch_darkly/services.py +++ b/api/integrations/launch_darkly/services.py @@ -508,11 +508,11 @@ def _create_feature_segment_from_clauses( segment_name=segment.name, clauses=clauses, ) - rules_data = segment.rules_data or [ # LaunchDarkly environments share the segment + rules = 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 + rules[0]["rules"] += subrules + segment.rules_data = rules segment.save(update_fields=["rules_data"]) # Create a targeting rule for the new feature-specific segment. diff --git a/api/segments/migrations/0031_add_segment_rules_data.py b/api/segments/migrations/0031_add_segment_rules_data.py index 5c7098d9bf5c..c122e89ae88d 100644 --- a/api/segments/migrations/0031_add_segment_rules_data.py +++ b/api/segments/migrations/0031_add_segment_rules_data.py @@ -5,7 +5,7 @@ from django.db import migrations, models from django.db.backends.base.schema import BaseDatabaseSchemaEditor -RuleType = dict[str, typing.Any] +SegmentRule = dict[str, typing.Any] BATCH_SIZE = 500 @@ -50,7 +50,7 @@ def nullify_segment_rules_data( Segment.objects.filter(rules_data__isnull=False).update(rules_data=None) -def _rasterise_segment_rules(obj: typing.Any) -> list[RuleType]: +def _rasterise_segment_rules(obj: typing.Any) -> list[SegmentRule]: return [ { "type": rule.type, diff --git a/api/segments/serializers.py b/api/segments/serializers.py index ed6a2caa7953..f644281eaf51 100644 --- a/api/segments/serializers.py +++ b/api/segments/serializers.py @@ -14,7 +14,11 @@ from segment_membership.models import SegmentMembershipCount from segment_membership.services import enqueue_membership_refresh from segments.models import Condition, Segment, SegmentRule, WhitelistedSegment -from segments.types import LegacySegmentRule +from segments.types import ( + LegacySegmentCondition, + LegacySegmentRule, + SegmentCondition, +) # TODO: Delete alias as per https://github.com/Flagsmith/flagsmith/issues/7818 from segments.types import SegmentRule as SegmentRuleType @@ -186,29 +190,25 @@ def update(self, segment: Segment, validated_data: dict[str, Any]): # type: ign enqueue_membership_refresh(segment.project) return segment - def _validate_rules_depth( - self, rules_data: list[LegacySegmentRule], _depth: int = 1 - ) -> None: + def _validate_rules_depth(self, rules: list[LegacySegmentRule]) -> None: # Raise loudly because the interface ignores rules nested too deep - 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_rules_depth(rule_data.get("rules", []), _depth + 1) - - def _validate_rules_condition_count( - self, rules_data: list[LegacySegmentRule] - ) -> None: + for rule in rules: + for nested_rule in rule.get("rules", []): + if nested_rule.get("rules"): + raise ValidationError( + { + "segment": [ + f"Rules must not be nested more than " + f"{SEGMENT_RULES_MAX_DEPTH} levels deep." + ] + } + ) + + def _validate_rules_condition_count(self, rules: list[LegacySegmentRule]) -> None: if self._can_segment_own_more_conditions_than_limit(): return - condition_count = self._count_conditions(rules_data) + condition_count = self._count_conditions(rules) if condition_count > settings.SEGMENT_RULES_CONDITIONS_LIMIT: raise ValidationError( { @@ -220,11 +220,14 @@ def _validate_rules_condition_count( } ) - def _count_conditions(self, rules_data: list[LegacySegmentRule]) -> int: + def _count_conditions(self, rules: list[LegacySegmentRule]) -> int: return sum( - len(rule_data.get("conditions", [])) - + self._count_conditions(rule_data.get("rules", [])) - for rule_data in rules_data + len(rule.get("conditions", [])) + + sum( + len(nested_rule.get("conditions", [])) + for nested_rule in rule.get("rules", []) + ) + for rule in rules ) def _can_segment_own_more_conditions_than_limit(self) -> bool: @@ -243,7 +246,7 @@ def _set_rules_data(self, validated_data: dict[str, Any]) -> None: ) def _cleanup_rules_and_conditions( - self, rules_data: list[LegacySegmentRule] + self, rules: list[LegacySegmentRule] ) -> list[SegmentRuleType]: """Remove any `id` fields and `delete: true` items from rules and conditions @@ -252,21 +255,35 @@ def _cleanup_rules_and_conditions( keep the interface compatible.""" return [ { - "type": rule_data["type"], - "conditions": [ + "type": rule["type"], + "conditions": self._cleanup_conditions(rule.get("conditions", [])), + "rules": [ { - "property": condition_data["property"], - "operator": condition_data["operator"], - "value": condition_data.get("value"), - "description": condition_data.get("description"), + "type": nested_rule["type"], + "conditions": self._cleanup_conditions( + nested_rule.get("conditions", []) + ), } - for condition_data in rule_data.get("conditions", []) - if not condition_data.get("delete") + for nested_rule in rule.get("rules", []) + if not nested_rule.get("delete") ], - "rules": self._cleanup_rules_and_conditions(rule_data.get("rules", [])), } - for rule_data in rules_data - if not rule_data.get("delete") + for rule in rules + if not rule.get("delete") + ] + + def _cleanup_conditions( + self, conditions: list[LegacySegmentCondition] + ) -> list[SegmentCondition]: + return [ + { + "property": condition["property"], + "operator": condition["operator"], + "value": condition.get("value"), + "description": condition.get("description"), + } + for condition in conditions + if not condition.get("delete") ] def _get_rules_and_conditions_without_deleted( diff --git a/api/segments/types.py b/api/segments/types.py index e06a9bbee36b..f2ccec663312 100644 --- a/api/segments/types.py +++ b/api/segments/types.py @@ -13,10 +13,17 @@ class SegmentCondition(TypedDict): description: str | None -class SegmentRule(TypedDict): +class _BaseSegmentRule(TypedDict): type: RuleType conditions: list[SegmentCondition] - rules: list["SegmentRule"] + + +class NestedSegmentRule(_BaseSegmentRule): + pass + + +class SegmentRule(_BaseSegmentRule): + rules: list[NestedSegmentRule] class LegacySegmentCondition(SegmentCondition): @@ -24,9 +31,16 @@ class LegacySegmentCondition(SegmentCondition): delete: NotRequired[bool] -class LegacySegmentRule(TypedDict): +class _BaseLegacySegmentRule(TypedDict): id: NotRequired[int] delete: NotRequired[bool] type: RuleType conditions: list[LegacySegmentCondition] - rules: list["LegacySegmentRule"] + + +class LegacyNestedSegmentRule(_BaseLegacySegmentRule): + pass + + +class LegacySegmentRule(_BaseLegacySegmentRule): + rules: list[LegacyNestedSegmentRule] diff --git a/api/tests/conftest.py b/api/tests/conftest.py index 8050a4924d08..67305fe7cfdc 100644 --- a/api/tests/conftest.py +++ b/api/tests/conftest.py @@ -458,7 +458,6 @@ def segment_rules() -> list[SegmentRuleType]: "description": "Jumping very high does not count!", }, ], - "rules": [], }, ], } diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index fa9328d8a886..b43fdaf31b20 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:179` + - `api/segments/serializers.py:183` Attributes: - `revision_id` From 04b05a6aaf4d592a8eea5783b737b5c20601f30e Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Tue, 11 Aug 2026 17:30:12 -0300 Subject: [PATCH 09/23] it gets darker the deeper you go --- .../migrations/0031_add_segment_rules_data.py | 14 ++- .../segments/test_unit_segments_migrations.py | 88 ++++++++++++++++++- 2 files changed, 96 insertions(+), 6 deletions(-) diff --git a/api/segments/migrations/0031_add_segment_rules_data.py b/api/segments/migrations/0031_add_segment_rules_data.py index c122e89ae88d..21422683aa5a 100644 --- a/api/segments/migrations/0031_add_segment_rules_data.py +++ b/api/segments/migrations/0031_add_segment_rules_data.py @@ -32,7 +32,7 @@ def backfill_segment_rules_data( 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 + models.Prefetch("live_rules__live_rules__rules", rules, to_attr="live_rules"), # Empty almost all cases ).order_by("id") last_id = 0 # don't leroy jenkins local memory @@ -61,11 +61,19 @@ def _rasterise_segment_rules(obj: typing.Any) -> list[SegmentRule]: "value": condition.value, "description": condition.description, } - for condition in rule.live_conditions + for condition in ( + rule.live_conditions + if hasattr(rule, "live_conditions") # We only prefetch two levels deep + else rule.conditions.filter(deleted_at__isnull=True) + ) ], "rules": _rasterise_segment_rules(rule), } - for rule in obj.live_rules + for rule in ( + obj.live_rules + if hasattr(obj, "live_rules") # We only prefetch two levels deep + else obj.rules.filter(deleted_at__isnull=True) + ) ] diff --git a/api/tests/unit/segments/test_unit_segments_migrations.py b/api/tests/unit/segments/test_unit_segments_migrations.py index 5a2749436afd..f748b6d1cc44 100644 --- a/api/tests/unit/segments/test_unit_segments_migrations.py +++ b/api/tests/unit/segments/test_unit_segments_migrations.py @@ -255,8 +255,10 @@ def _deep_clone(segment: Segment) -> Segment: # type: ignore[valid-type] ) def test_0031_add_segment_rules_data__forwards__backfill_segment_rules_data( migrator: Migrator, + monkeypatch: pytest.MonkeyPatch, ) -> None: # Given + monkeypatch.setattr(migration_0031, "BATCH_SIZE", 2) state = migrator.apply_initial_migration( ("segments", "0031_add_segment_rules_data") ) @@ -275,7 +277,23 @@ def test_0031_add_segment_rules_data__forwards__backfill_segment_rules_data( 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()) + deep_rule = SegmentRule.objects.create(rule=nested_rule, type="NONE") + deleted_rule = SegmentRule.objects.create( + rule=top_rule, type="ANY", deleted_at=timezone.now() + ) + orphaned_rule = SegmentRule.objects.create(rule=deleted_rule, type="ANY") + Condition.objects.create( + rule=orphaned_rule, + operator=constants.EQUAL, + property="ghost", + value="true", + ) + Condition.objects.create( + rule=top_rule, + operator=constants.IS_SET, + property="email", + value="", + ) Condition.objects.create( rule=nested_rule, operator=constants.EQUAL, @@ -290,6 +308,12 @@ def test_0031_add_segment_rules_data__forwards__backfill_segment_rules_data( value="210", deleted_at=timezone.now(), ) + Condition.objects.create( + rule=deep_rule, + operator=constants.CONTAINS, + property="country", + value="GB", + ) deleted_segment = Segment.objects.create( name="Deleted", project=project, deleted_at=timezone.now() @@ -303,6 +327,24 @@ def test_0031_add_segment_rules_data__forwards__backfill_segment_rules_data( ) SegmentRule.objects.create(segment=old_version_segment, type="ALL") + empty_segment = Segment.objects.create(name="Empty", project=project) + empty_segment.version_of_id = empty_segment.id + empty_segment.save() + + batched_segments = [] + for i in range(3): # spans several batches + batched_segment = Segment.objects.create(name=f"Batched {i}", project=project) + batched_segment.version_of_id = batched_segment.id + batched_segment.save() + rule = SegmentRule.objects.create(segment=batched_segment, type="ALL") + Condition.objects.create( + rule=rule, + operator=constants.EQUAL, + property="batch", + value=str(i), + ) + batched_segments.append(batched_segment) + # When migration_0031.backfill_segment_rules_data(state.apps) @@ -313,7 +355,14 @@ def test_0031_add_segment_rules_data__forwards__backfill_segment_rules_data( assert segment.rules_data == [ { "type": "ALL", - "conditions": [], + "conditions": [ + { + "property": "email", + "operator": constants.IS_SET, + "value": "", + "description": None, + } + ], "rules": [ { "type": "ANY", @@ -325,7 +374,20 @@ def test_0031_add_segment_rules_data__forwards__backfill_segment_rules_data( "description": "Adults only", } ], - "rules": [], + "rules": [ # We mistakenly supported deeper rules in the past + { + "type": "NONE", + "conditions": [ + { + "property": "country", + "operator": constants.CONTAINS, + "value": "GB", + "description": None, + } + ], + "rules": [], + } + ], } ], } @@ -333,6 +395,26 @@ def test_0031_add_segment_rules_data__forwards__backfill_segment_rules_data( assert deleted_segment.rules_data is None assert old_version_segment.rules_data is None + empty_segment.refresh_from_db() + assert empty_segment.rules_data == [] + + for i, batched_segment in enumerate(batched_segments): + batched_segment.refresh_from_db() + assert batched_segment.rules_data == [ + { + "type": "ALL", + "conditions": [ + { + "property": "batch", + "operator": constants.EQUAL, + "value": str(i), + "description": None, + } + ], + "rules": [], + } + ] + @pytest.mark.skipif( test_settings.SKIP_MIGRATION_TESTS is True, From 8d91dede6380672f84e8e673d29a1c59d13127cf Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Tue, 11 Aug 2026 18:25:55 -0300 Subject: [PATCH 10/23] experiments too! --- api/experimentation/services.py | 31 ++++- .../unit/experimentation/test_services.py | 110 +++++++++++++++++- .../observability/_events-catalogue.md | 28 ++--- 3 files changed, 153 insertions(+), 16 deletions(-) diff --git a/api/experimentation/services.py b/api/experimentation/services.py index ee8c1c2bff75..ed064b4556f2 100644 --- a/api/experimentation/services.py +++ b/api/experimentation/services.py @@ -16,7 +16,7 @@ from django.db import transaction from django.db.models import Q from django.utils import timezone -from flag_engine.segments.constants import PERCENTAGE_SPLIT +from flag_engine.segments.constants import ALL_RULE, PERCENTAGE_SPLIT from rest_framework.exceptions import ValidationError from audit.models import AuditLog @@ -81,6 +81,9 @@ from integrations.flagsmith.client import get_openfeature_client from segments.models import Condition, Segment, SegmentRule +# TODO: Delete alias as per https://github.com/Flagsmith/flagsmith/issues/7818 +from segments.types import SegmentRule as SegmentRuleType + _ROLLOUT_VALUE_TYPE: dict[str, "FeatureValueType"] = { INTEGER: "integer", STRING: "string", @@ -645,6 +648,23 @@ def transition_experiment_status( return experiment +def _rollout_segment_rules(rollout_percentage: float) -> list[SegmentRuleType]: + return [ + { + "type": ALL_RULE, + "conditions": [ + { + "property": "$.identity.key", + "operator": PERCENTAGE_SPLIT, + "value": str(rollout_percentage), + "description": None, + } + ], + "rules": [], + } + ] + + def _create_rollout_segment( experiment: Experiment, rollout_percentage: float ) -> Segment: @@ -652,7 +672,10 @@ def _create_rollout_segment( name=f"experiment-{experiment.id}-rollout", project=experiment.feature.project, is_system_segment=True, + rules_data=_rollout_segment_rules(rollout_percentage), ) + + # TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 rule = SegmentRule.objects.create(segment=segment, type=SegmentRule.ALL_RULE) Condition.objects.create( rule=rule, @@ -660,6 +683,7 @@ def _create_rollout_segment( property="$.identity.key", value=str(rollout_percentage), ) + return segment @@ -684,11 +708,16 @@ def validate_rollout_spec(experiment: Experiment, spec: RolloutSpec) -> None: def _sync_rollout_segment(experiment: Experiment, rollout_percentage: float) -> Segment: segment = experiment.rollout_segment if segment is not None: + segment.rules_data = _rollout_segment_rules(rollout_percentage) + segment.save(update_fields=["rules_data"]) + + # TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 condition = Condition.objects.get( rule__segment=segment, operator=PERCENTAGE_SPLIT ) condition.value = str(rollout_percentage) condition.save() + return segment segment = _create_rollout_segment(experiment, rollout_percentage) experiment.rollout_segment = segment diff --git a/api/tests/unit/experimentation/test_services.py b/api/tests/unit/experimentation/test_services.py index 19fbb481b840..ba49a8e62e45 100644 --- a/api/tests/unit/experimentation/test_services.py +++ b/api/tests/unit/experimentation/test_services.py @@ -48,7 +48,7 @@ from features.multivariate.models import MultivariateFeatureOption from features.value_types import STRING from features.versioning.dataclasses import MultivariateValueChangeSet -from segments.models import Condition +from segments.models import Condition, Segment, SegmentRule from users.models import FFAdminUser from util.mappers import map_environment_to_environment_document @@ -1509,6 +1509,64 @@ def test_apply_experiment_rollout__no_segment__creates_segment_and_override( ), ) + # Then + experiment.refresh_from_db() + segment = experiment.rollout_segment + assert segment is not None + assert segment.is_system_segment is True + assert segment.rules_data == [ + { + "type": SegmentRule.ALL_RULE, + "conditions": [ + { + "property": "$.identity.key", + "operator": PERCENTAGE_SPLIT, + "value": "42.0", + "description": None, + } + ], + "rules": [], + } + ] + + override = FeatureState.objects.get( + environment=experiment.environment, + feature=experiment.feature, + feature_segment__segment=segment, + ) + assert override.enabled is True + allocations = { + mv.multivariate_feature_option_id: mv.percentage_allocation + for mv in override.multivariate_feature_state_values.all() + } + assert allocations == {option_a.id: 60.0, option_b.id: 40.0} + + +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 +def test_apply_experiment_rollout__no_segment__creates_segment_and_override_x_replaced_above( + experiment: Experiment, + multivariate_options: list[MultivariateFeatureOption], + admin_user: FFAdminUser, +) -> None: + # Given + option_a, option_b, _ = multivariate_options + + # When + services.apply_experiment_rollout( + experiment, + RolloutSpec( + enabled=True, + rollout_percentage=42.0, + feature_state_value="control", + value_type="string", + multivariate_values=[ + MultivariateValueChangeSet(option_a.id, 60.0), + MultivariateValueChangeSet(option_b.id, 40.0), + ], + author=AuthorData(user=admin_user), + ), + ) + # Then experiment.refresh_from_db() segment = experiment.rollout_segment @@ -1700,6 +1758,56 @@ def test_apply_experiment_rollout__existing_segment__updates_percentage_and_enab ), ) + # Then + segment = Segment.objects.get(pk=experiment.rollout_segment_id) + assert segment.rules_data == [ + { + "type": SegmentRule.ALL_RULE, + "conditions": [ + { + "property": "$.identity.key", + "operator": PERCENTAGE_SPLIT, + "value": "80.0", + "description": None, + } + ], + "rules": [], + } + ] + override = FeatureState.objects.get( + environment=experiment.environment, + feature=experiment.feature, + feature_segment__segment=experiment.rollout_segment, + ) + assert override.enabled is False + + +# TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 +def test_apply_experiment_rollout__existing_segment__updates_percentage_and_enabled_x_replaced_above( + experiment_with_rollout: Experiment, + multivariate_options: list[MultivariateFeatureOption], + admin_user: FFAdminUser, +) -> None: + # Given + experiment = experiment_with_rollout + option_a, option_b, _ = multivariate_options + + # When + services.apply_experiment_rollout( + experiment, + RolloutSpec( + enabled=False, + rollout_percentage=80.0, + feature_state_value="control", + value_type="string", + multivariate_values=[ + MultivariateValueChangeSet(option_a.id, 50.0), + MultivariateValueChangeSet(option_b.id, 50.0), + ], + author=AuthorData(user=admin_user), + ), + ) + # Then condition = Condition.objects.get(rule__segment=experiment.rollout_segment) assert condition.value == "80.0" diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index b43fdaf31b20..c292c70c1f4c 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -660,7 +660,7 @@ Attributes: ### `warehouse.connection.connected` Logged at `info` from: - - `api/experimentation/services.py:1118` + - `api/experimentation/services.py:1147` Attributes: - `environment.id` @@ -669,8 +669,8 @@ Attributes: ### `warehouse.connection.event_names_failed` Logged at `warning` from: - - `api/experimentation/services.py:223` - - `api/experimentation/services.py:1218` + - `api/experimentation/services.py:226` + - `api/experimentation/services.py:1247` Attributes: - `environment.id` @@ -680,7 +680,7 @@ Attributes: ### `warehouse.connection.event_stats_failed` Logged at `warning` from: - - `api/experimentation/services.py:1181` + - `api/experimentation/services.py:1210` Attributes: - `environment.id` @@ -689,7 +689,7 @@ Attributes: ### `warehouse.connection.test_event_sent` Logged at `info` from: - - `api/experimentation/services.py:892` + - `api/experimentation/services.py:921` Attributes: - `environment.id` @@ -698,7 +698,7 @@ Attributes: ### `warehouse.connection.verification_failed` Logged at `warning` from: - - `api/experimentation/services.py:1093` + - `api/experimentation/services.py:1122` Attributes: - `environment.id` @@ -708,7 +708,7 @@ Attributes: ### `warehouse.connection.verification_succeeded` Logged at `info` from: - - `api/experimentation/services.py:1103` + - `api/experimentation/services.py:1132` Attributes: - `environment.id` @@ -717,7 +717,7 @@ Attributes: ### `warehouse.delivery.all_objects_rejected` Logged at `error` from: - - `api/experimentation/services.py:1048` + - `api/experimentation/services.py:1077` Attributes: - `connection.id` @@ -728,7 +728,7 @@ Attributes: ### `warehouse.delivery.budget_exhausted` Logged at `info` from: - - `api/experimentation/services.py:937` + - `api/experimentation/services.py:966` Attributes: - `connection.id` @@ -739,7 +739,7 @@ Attributes: ### `warehouse.delivery.completed` Logged at `info` from: - - `api/experimentation/services.py:1058` + - `api/experimentation/services.py:1087` Attributes: - `connection.id` @@ -752,7 +752,7 @@ Attributes: ### `warehouse.delivery.failed` Logged at `error` from: - - `api/experimentation/services.py:1031` + - `api/experimentation/services.py:1060` Attributes: - `connection.id` @@ -763,7 +763,7 @@ Attributes: ### `warehouse.delivery.object_rejected` Logged at `error` from: - - `api/experimentation/services.py:966` + - `api/experimentation/services.py:995` Attributes: - `connection.id` @@ -775,7 +775,7 @@ Attributes: ### `warehouse.srm.overallocated` Logged at `error` from: - - `api/experimentation/services.py:514` + - `api/experimentation/services.py:517` Attributes: - `environment.id` @@ -785,7 +785,7 @@ Attributes: ### `warehouse.srm.unkeyed_variant` Logged at `error` from: - - `api/experimentation/services.py:500` + - `api/experimentation/services.py:503` Attributes: - `environment.id` From 1a4e699eb1d7cc7c68c5e7d3fbd6e7a08d53dd52 Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Tue, 11 Aug 2026 18:49:45 -0300 Subject: [PATCH 11/23] no dangling third floor this ain't Backdoors --- api/integrations/launch_darkly/services.py | 74 +++++++++---------- .../migrations/0031_add_segment_rules_data.py | 41 +++++----- api/segments/types.py | 4 +- ...ments__correctly_imported__rules_data.json | 10 --- .../launch_darkly/test_services.py | 17 +---- .../segments/test_unit_segments_migrations.py | 1 - 6 files changed, 61 insertions(+), 86 deletions(-) diff --git a/api/integrations/launch_darkly/services.py b/api/integrations/launch_darkly/services.py index f0696ddbef6b..dce97b722ede 100644 --- a/api/integrations/launch_darkly/services.py +++ b/api/integrations/launch_darkly/services.py @@ -295,14 +295,15 @@ def _create_feature_segments_for_segment_match_clauses( return feature_states -def _clauses_to_segment_subrules( +def _add_clauses_to_segment_rule( 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 + rule: SegmentRuleType, +) -> None: + """Add Launch Darkly clauses to a segment's "ALL" root rule as subrules.""" + subrules = rule["rules"] + negated_subrule_index: Optional[int] = None for clause in clauses: _property = clause["attribute"] @@ -339,33 +340,22 @@ def _clauses_to_segment_subrules( ) 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 + if negated_subrule_index is None: + subrules.append({"type": constants.NONE_RULE, "conditions": []}) + negated_subrule_index = len(subrules) - 1 + subrules[negated_subrule_index]["conditions"] += conditions else: - subrules.append( - {"type": constants.ANY_RULE, "conditions": conditions, "rules": []} - ) + subrules.append({"type": constants.ANY_RULE, "conditions": conditions}) - return subrules - -def _users_to_segment_subrules( +def _add_users_to_segment_rule( 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] = [] + rule: SegmentRuleType, +) -> None: + """Add Launch Darkly's targeted user lists to a segment's "ALL" root rule as subrules.""" for identities_string in iter_chunked_concat( values=users, delimiter=",", @@ -381,7 +371,7 @@ def _users_to_segment_subrules( ), ) continue - subrules.append( + rule["rules"].append( { "type": constants.NONE_RULE if negate else constants.ANY_RULE, "conditions": [ @@ -392,10 +382,8 @@ def _users_to_segment_subrules( "description": None, } ], - "rules": [], } ) - return subrules def _create_segment_rule_for_segment( @@ -503,15 +491,16 @@ def _create_feature_segment_from_clauses( name=rule_name, project=project, feature=feature ) - subrules = _clauses_to_segment_subrules( + rules: list[SegmentRuleType] = ( + segment.rules_data # LaunchDarkly environments share the segment + or [{"type": constants.ALL_RULE, "conditions": [], "rules": []}] + ) + _add_clauses_to_segment_rule( import_request=import_request, segment_name=segment.name, clauses=clauses, + rule=rules[0], ) - rules = segment.rules_data or [ # LaunchDarkly environments share the segment - {"type": constants.ALL_RULE, "conditions": [], "rules": []} - ] - rules[0]["rules"] += subrules segment.rules_data = rules segment.save(update_fields=["rules_data"]) @@ -1123,15 +1112,20 @@ def _create_segments_from_ld( # TODO: Tagging segments is not supported yet. https://github.com/Flagsmith/flagsmith/issues/3241 - subrules: list[SegmentRuleType] = [] + root_rule: SegmentRuleType = { + "type": constants.ALL_RULE, + "conditions": [], + "rules": [], + } # Create the segment rule for the segment. rules = ld_segment["rules"] for rule in rules: - subrules += _clauses_to_segment_subrules( + _add_clauses_to_segment_rule( import_request=import_request, segment_name=segment.name, clauses=rule["clauses"], + rule=root_rule, ) # TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 _create_segment_rule_for_segment( @@ -1155,17 +1149,19 @@ def _create_segments_from_ld( ] ) - subrules += _users_to_segment_subrules( + _add_users_to_segment_rule( import_request=import_request, segment_name=segment.name, users=ld_segment["included"], negate=False, + rule=root_rule, ) - subrules += _users_to_segment_subrules( + _add_users_to_segment_rule( import_request=import_request, segment_name=segment.name, users=ld_segment["excluded"], negate=True, + rule=root_rule, ) # TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 @@ -1196,9 +1192,7 @@ def _create_segments_from_ld( # 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.rules_data = [root_rule] segment.save(update_fields=["rules_data"]) return segments_by_ld_key diff --git a/api/segments/migrations/0031_add_segment_rules_data.py b/api/segments/migrations/0031_add_segment_rules_data.py index 21422683aa5a..54b07aa602a6 100644 --- a/api/segments/migrations/0031_add_segment_rules_data.py +++ b/api/segments/migrations/0031_add_segment_rules_data.py @@ -52,23 +52,7 @@ def nullify_segment_rules_data( def _rasterise_segment_rules(obj: typing.Any) -> list[SegmentRule]: return [ - { - "type": rule.type, - "conditions": [ - { - "property": condition.property, - "operator": condition.operator, - "value": condition.value, - "description": condition.description, - } - for condition in ( - rule.live_conditions - if hasattr(rule, "live_conditions") # We only prefetch two levels deep - else rule.conditions.filter(deleted_at__isnull=True) - ) - ], - "rules": _rasterise_segment_rules(rule), - } + _rasterise_segment_rule(rule) for rule in ( obj.live_rules if hasattr(obj, "live_rules") # We only prefetch two levels deep @@ -77,6 +61,29 @@ def _rasterise_segment_rules(obj: typing.Any) -> list[SegmentRule]: ] +def _rasterise_segment_rule(rule: typing.Any) -> SegmentRule: + rule_data: SegmentRule = { + "type": rule.type, + "conditions": [ + { + "property": condition.property, + "operator": condition.operator, + "value": condition.value, + "description": condition.description, + } + for condition in ( + rule.live_conditions + if hasattr(rule, "live_conditions") # We only prefetch two levels deep + else rule.conditions.filter(deleted_at__isnull=True) + ) + ], + } + # Top-level rules always carry "rules"; nested ones only for legacy deeper nesting. + if (subrules := _rasterise_segment_rules(rule)) or rule.segment_id: + rule_data["rules"] = subrules + return rule_data + + class Migration(migrations.Migration): dependencies = [ diff --git a/api/segments/types.py b/api/segments/types.py index f2ccec663312..0d7f280cc412 100644 --- a/api/segments/types.py +++ b/api/segments/types.py @@ -18,12 +18,12 @@ class _BaseSegmentRule(TypedDict): conditions: list[SegmentCondition] -class NestedSegmentRule(_BaseSegmentRule): +class _NestedSegmentRule(_BaseSegmentRule): pass class SegmentRule(_BaseSegmentRule): - rules: list[NestedSegmentRule] + rules: list[_NestedSegmentRule] class LegacySegmentCondition(SegmentCondition): 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 index c316bb7a42a8..ba6821f299c4 100644 --- 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 @@ -5,7 +5,6 @@ "rules": [ { "type": "ANY", - "rules": [], "conditions": [ { "value": ".*410f8e860cb348ad83218d65834de218\\.com", @@ -179,7 +178,6 @@ }, { "type": "NONE", - "rules": [], "conditions": [] } ], @@ -192,7 +190,6 @@ "rules": [ { "type": "ANY", - "rules": [], "conditions": [ { "value": ".*410f8e860cb348ad83218d65834de218\\.com", @@ -366,7 +363,6 @@ }, { "type": "NONE", - "rules": [], "conditions": [] } ], @@ -379,7 +375,6 @@ "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", @@ -391,7 +386,6 @@ }, { "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", @@ -403,7 +397,6 @@ }, { "type": "NONE", - "rules": [], "conditions": [ { "value": "user-103", @@ -423,7 +416,6 @@ "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", @@ -435,7 +427,6 @@ }, { "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", @@ -447,7 +438,6 @@ }, { "type": "NONE", - "rules": [], "conditions": [ { "value": "user-103", diff --git a/api/tests/unit/integrations/launch_darkly/test_services.py b/api/tests/unit/integrations/launch_darkly/test_services.py index 3cf6c714fc3b..d13c7c2f8385 100644 --- a/api/tests/unit/integrations/launch_darkly/test_services.py +++ b/api/tests/unit/integrations/launch_darkly/test_services.py @@ -329,7 +329,6 @@ def test_process_import_request__valid_segments__creates_segment_per_environment "description": None, } ], - "rules": [], } ], } @@ -353,7 +352,6 @@ def test_process_import_request__valid_segments__creates_segment_per_environment "description": None, } ], - "rules": [], }, { "type": SegmentRule.ANY_RULE, @@ -365,7 +363,6 @@ def test_process_import_request__valid_segments__creates_segment_per_environment "description": None, } ], - "rules": [], }, { "type": SegmentRule.ANY_RULE, @@ -377,7 +374,6 @@ def test_process_import_request__valid_segments__creates_segment_per_environment "description": None, } ], - "rules": [], }, { "type": SegmentRule.ANY_RULE, # included users @@ -389,7 +385,6 @@ def test_process_import_request__valid_segments__creates_segment_per_environment "description": None, } ], - "rules": [], }, { "type": SegmentRule.NONE_RULE, # excluded users @@ -401,7 +396,6 @@ def test_process_import_request__valid_segments__creates_segment_per_environment "description": None, } ], - "rules": [], }, ], } @@ -425,7 +419,6 @@ def test_process_import_request__valid_segments__creates_segment_per_environment "description": None, } ], - "rules": [], }, { "type": SegmentRule.NONE_RULE, # excluded users @@ -437,7 +430,6 @@ def test_process_import_request__valid_segments__creates_segment_per_environment "description": None, } ], - "rules": [], }, ], } @@ -741,7 +733,6 @@ def test_process_import_request__valid_rules__creates_feature_specific_segments( "description": None, } ], - "rules": [], }, { "type": SegmentRule.ANY_RULE, @@ -753,7 +744,6 @@ def test_process_import_request__valid_rules__creates_feature_specific_segments( "description": None, } ], - "rules": [], }, ], } @@ -777,7 +767,6 @@ def test_process_import_request__valid_rules__creates_feature_specific_segments( "description": None, } ], - "rules": [], }, { "type": SegmentRule.NONE_RULE, # negated clauses pool here @@ -795,7 +784,6 @@ def test_process_import_request__valid_rules__creates_feature_specific_segments( "description": None, }, ], - "rules": [], }, ], } @@ -819,7 +807,6 @@ def test_process_import_request__valid_rules__creates_feature_specific_segments( "description": None, } ], - "rules": [], }, ], } @@ -985,9 +972,7 @@ def test_process_import_request__valid_rules__imports_correctly_x_replaced_above { "type": SegmentRule.ALL_RULE, "conditions": [], - "rules": [ - {"type": SegmentRule.ANY_RULE, "conditions": [], "rules": []} - ], + "rules": [{"type": SegmentRule.ANY_RULE, "conditions": []}], } ], f"Segment condition value 'xxxxx...xxxxx' for property 'p1' exceeds the" diff --git a/api/tests/unit/segments/test_unit_segments_migrations.py b/api/tests/unit/segments/test_unit_segments_migrations.py index f748b6d1cc44..cccbe77137c2 100644 --- a/api/tests/unit/segments/test_unit_segments_migrations.py +++ b/api/tests/unit/segments/test_unit_segments_migrations.py @@ -385,7 +385,6 @@ def test_0031_add_segment_rules_data__forwards__backfill_segment_rules_data( "description": None, } ], - "rules": [], } ], } From 218c14b615016d105316a0a9a2e27d400f6b8373 Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Tue, 11 Aug 2026 19:39:29 -0300 Subject: [PATCH 12/23] lost not forgotten --- .../unit/segments/test_unit_segments_views.py | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/api/tests/unit/segments/test_unit_segments_views.py b/api/tests/unit/segments/test_unit_segments_views.py index b662e862602f..7ea1c2117fbc 100644 --- a/api/tests/unit/segments/test_unit_segments_views.py +++ b/api/tests/unit/segments/test_unit_segments_views.py @@ -1113,6 +1113,36 @@ def test_update_segment__valid_rules__updates_segment_with_rules( } +def test_update_segment__rules_and_conditions_with_ids__ignores_ids( + admin_client: APIClient, + project: Project, + segment: Segment, + segment_rules: list[SegmentRuleType], +) -> None: + # Given + segment_rules[0]["conditions"][0]["value"] = "blue" + expected_rules = deepcopy(segment_rules) + segment_rules[0]["id"] = 42 # type: ignore[typeddict-unknown-key] + segment_rules[0]["conditions"][0]["id"] = 43 # type: ignore[typeddict-unknown-key] + segment_rules[0]["rules"][0]["id"] = 44 # type: ignore[typeddict-unknown-key] + segment_rules[0]["rules"][0]["conditions"][0]["id"] = 45 # type: ignore[typeddict-unknown-key] + + # When + response = admin_client.put( + f"/api/v1/projects/{project.id}/segments/{segment.id}/", + data={ + "name": segment.name, + "rules": segment_rules, + }, + format="json", + ) + + # Then + assert response.status_code == 200 + segment.refresh_from_db() + assert segment.rules_data == expected_rules + + # TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 def test_patch_segment__rules_omitted__preserves_rules( admin_client: APIClient, From 6f0394ba3976f1feeb55f268973f206f606f21c2 Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Tue, 11 Aug 2026 19:39:59 -0300 Subject: [PATCH 13/23] =?UTF-8?q?=E2=9A=9B=EF=B8=8F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- api/integrations/launch_darkly/services.py | 70 ++++++++++--------- .../launch_darkly/test_services.py | 24 +++++++ 2 files changed, 60 insertions(+), 34 deletions(-) diff --git a/api/integrations/launch_darkly/services.py b/api/integrations/launch_darkly/services.py index dce97b722ede..ec8c8ebd6a3e 100644 --- a/api/integrations/launch_darkly/services.py +++ b/api/integrations/launch_darkly/services.py @@ -6,6 +6,7 @@ from django.conf import settings from django.core import signing +from django.db import transaction from django.utils import timezone from flag_engine.segments import constants from flag_engine.segments.types import ConditionOperator @@ -1273,41 +1274,42 @@ def process_import_request( ) raise - # Create environments - environments_by_ld_environment_key = _create_environments_from_ld( - ld_environments=ld_environments, - project_id=import_request.project_id, - ) + with transaction.atomic(): + # Create environments + environments_by_ld_environment_key = _create_environments_from_ld( + ld_environments=ld_environments, + project_id=import_request.project_id, + ) - # Create segments using `ld_segment_tags` - # TODO populate with LD tags when https://github.com/Flagsmith/flagsmith/issues/3241 is done - segment_tags_by_ld_tag: dict[str, Tag] = {} - segments_by_ld_key = _create_segments_from_ld( - import_request=import_request, - ld_segments=ld_segments, - environments_by_ld_environment_key=environments_by_ld_environment_key, - tags_by_ld_tag=segment_tags_by_ld_tag, - project_id=import_request.project_id, - ) + # Create segments using `ld_segment_tags` + # TODO populate with LD tags when https://github.com/Flagsmith/flagsmith/issues/3241 is done + segment_tags_by_ld_tag: dict[str, Tag] = {} + segments_by_ld_key = _create_segments_from_ld( + import_request=import_request, + ld_segments=ld_segments, + environments_by_ld_environment_key=environments_by_ld_environment_key, + tags_by_ld_tag=segment_tags_by_ld_tag, + project_id=import_request.project_id, + ) - # Create flags - flag_tags_by_ld_tag = _create_tags_from_ld( - ld_tags=ld_flag_tags, - project_id=import_request.project_id, - ) - _create_features_from_ld( - import_request=import_request, - ld_flags=ld_flags, - environments_by_ld_environment_key=environments_by_ld_environment_key, - tags_by_ld_tag=flag_tags_by_ld_tag, - segments_by_ld_key=segments_by_ld_key, - project_id=import_request.project_id, - ) + # Create flags + flag_tags_by_ld_tag = _create_tags_from_ld( + ld_tags=ld_flag_tags, + project_id=import_request.project_id, + ) + _create_features_from_ld( + import_request=import_request, + ld_flags=ld_flags, + environments_by_ld_environment_key=environments_by_ld_environment_key, + tags_by_ld_tag=flag_tags_by_ld_tag, + segments_by_ld_key=segments_by_ld_key, + project_id=import_request.project_id, + ) - # Count deprecated flags for reporting - import_request.status["deprecated_flag_count"] = sum( - 1 for ld_flag in ld_flags if ld_flag["deprecated"] - ) + # Count deprecated flags for reporting + import_request.status["deprecated_flag_count"] = sum( + 1 for ld_flag in ld_flags if ld_flag["deprecated"] + ) - # Refresh membership counts for the segments the import just created. - enqueue_membership_refresh(import_request.project) + # Refresh membership counts for the segments the import just created. + enqueue_membership_refresh(import_request.project) diff --git a/api/tests/unit/integrations/launch_darkly/test_services.py b/api/tests/unit/integrations/launch_darkly/test_services.py index d13c7c2f8385..516a9da3191c 100644 --- a/api/tests/unit/integrations/launch_darkly/test_services.py +++ b/api/tests/unit/integrations/launch_darkly/test_services.py @@ -107,6 +107,30 @@ def test_process_import_request__api_error__expected_status( assert import_request.status["error_messages"] == [expected_error_message] +@pytest.mark.django_db(transaction=True) +def test_process_import_request__write_error__persists_no_import_data( + mocker: MockerFixture, + project: Project, + import_request: LaunchDarklyImportRequest, +) -> None: + # Given + mocker.patch( + "integrations.launch_darkly.services.enqueue_membership_refresh", + side_effect=RuntimeError(), + ) + + # When + with pytest.raises(RuntimeError): + process_import_request(import_request) + + # Then + assert import_request.completed_at + assert import_request.status["result"] == "failure" + assert not Environment.objects.filter(project=project).exists() + assert not Feature.objects.filter(project=project).exists() + assert not Segment.objects.filter(project=project).exists() + + @pytest.mark.django_db(transaction=True) def test_process_import_request__success__expected_status( # type: ignore[no-untyped-def] project: Project, From 7711f78b87ef8e9082704719d0817544919c0632 Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Tue, 11 Aug 2026 20:03:26 -0300 Subject: [PATCH 14/23] change requests --- api/segments/serializers.py | 1 - 1 file changed, 1 deletion(-) diff --git a/api/segments/serializers.py b/api/segments/serializers.py index f644281eaf51..07a148e0ef8e 100644 --- a/api/segments/serializers.py +++ b/api/segments/serializers.py @@ -142,7 +142,6 @@ class Meta: read_only_fields = [ "membership_counts", "project", - "version_of", ] def to_internal_value(self, data: dict[str, Any]) -> Any: From 080cde018cb659dffdf7e81f47c3287d6db6a13c Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Tue, 11 Aug 2026 20:07:55 -0300 Subject: [PATCH 15/23] =?UTF-8?q?post-=E2=9A=9B=EF=B8=8F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- api/integrations/launch_darkly/services.py | 4 +++- api/tests/unit/integrations/launch_darkly/test_services.py | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/api/integrations/launch_darkly/services.py b/api/integrations/launch_darkly/services.py index ec8c8ebd6a3e..800f1e8c9d4e 100644 --- a/api/integrations/launch_darkly/services.py +++ b/api/integrations/launch_darkly/services.py @@ -1312,4 +1312,6 @@ def process_import_request( ) # Refresh membership counts for the segments the import just created. - enqueue_membership_refresh(import_request.project) + transaction.on_commit( + lambda: enqueue_membership_refresh(import_request.project) + ) diff --git a/api/tests/unit/integrations/launch_darkly/test_services.py b/api/tests/unit/integrations/launch_darkly/test_services.py index 516a9da3191c..dd254812283c 100644 --- a/api/tests/unit/integrations/launch_darkly/test_services.py +++ b/api/tests/unit/integrations/launch_darkly/test_services.py @@ -115,7 +115,7 @@ def test_process_import_request__write_error__persists_no_import_data( ) -> None: # Given mocker.patch( - "integrations.launch_darkly.services.enqueue_membership_refresh", + "integrations.launch_darkly.services._create_features_from_ld", side_effect=RuntimeError(), ) From beacb94debd9831e837bdf626f2e0efcf75239ca Mon Sep 17 00:00:00 2001 From: "flagsmith-engineering[bot]" Date: Tue, 11 Aug 2026 23:09:53 +0000 Subject: [PATCH 16/23] chore: Update documentation artefacts --- .../observability/_events-catalogue.md | 2 +- mcp/src/flagsmith_mcp/openapi.json | 4 ++-- openapi.yaml | 11 +++++++++-- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index c292c70c1f4c..3b09180050d6 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:183` + - `api/segments/serializers.py:182` Attributes: - `revision_id` diff --git a/mcp/src/flagsmith_mcp/openapi.json b/mcp/src/flagsmith_mcp/openapi.json index 330b2f25c097..7c21086f6a0c 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": [ @@ -6901,7 +6902,6 @@ }, "required": [ "name", - "project", "rules" ] }, diff --git a/openapi.yaml b/openapi.yaml index ccfdf0f08579..4e6c077e71ae 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -18537,6 +18537,7 @@ components: - 'null' project: type: integer + readOnly: true feature: type: - integer @@ -18564,7 +18565,6 @@ components: - 'null' required: - name - - project - rules ChangeRequestUpdate: description: Adds nested create feature @@ -22243,6 +22243,7 @@ components: readOnly: true url: type: string + format: uri maxLength: 200 enabled: type: boolean @@ -24193,6 +24194,7 @@ components: readOnly: true url: type: string + format: uri maxLength: 200 enabled: type: boolean @@ -24582,6 +24584,7 @@ components: - 'null' project: type: integer + readOnly: true feature: type: - integer @@ -25045,6 +25048,7 @@ components: url: type: string format: uri + maxLength: 200 enabled: type: boolean created_at: @@ -25066,6 +25070,7 @@ components: readOnly: true url: type: string + format: uri maxLength: 200 secret: type: string @@ -26255,6 +26260,7 @@ components: - 'null' project: type: integer + readOnly: true feature: type: - integer @@ -26278,7 +26284,6 @@ components: readOnly: true required: - name - - project - rules SegmentAssociatedFeatureState: type: object @@ -28035,6 +28040,7 @@ components: url: type: string format: uri + maxLength: 200 enabled: type: boolean created_at: @@ -28058,6 +28064,7 @@ components: readOnly: true url: type: string + format: uri maxLength: 200 secret: type: string From 06f13bb900ec1a7420d60ae2a6b69ebe99e8e4f0 Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Tue, 11 Aug 2026 20:49:46 -0300 Subject: [PATCH 17/23] =?UTF-8?q?=F0=9F=A5=95?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- api/tests/unit/integrations/launch_darkly/test_services.py | 1 + 1 file changed, 1 insertion(+) diff --git a/api/tests/unit/integrations/launch_darkly/test_services.py b/api/tests/unit/integrations/launch_darkly/test_services.py index dd254812283c..fb6e759c534b 100644 --- a/api/tests/unit/integrations/launch_darkly/test_services.py +++ b/api/tests/unit/integrations/launch_darkly/test_services.py @@ -124,6 +124,7 @@ def test_process_import_request__write_error__persists_no_import_data( process_import_request(import_request) # Then + import_request.refresh_from_db() assert import_request.completed_at assert import_request.status["result"] == "failure" assert not Environment.objects.filter(project=project).exists() From 4ab75688a204fde885f69d323cd662ada0e72efa Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Wed, 12 Aug 2026 11:10:47 -0300 Subject: [PATCH 18/23] update tests --- .../segments/test_unit_segments_migrations.py | 16 ++++++++-------- .../unit/segments/test_unit_segments_views.py | 4 ++++ 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/api/tests/unit/segments/test_unit_segments_migrations.py b/api/tests/unit/segments/test_unit_segments_migrations.py index cccbe77137c2..332539c39f74 100644 --- a/api/tests/unit/segments/test_unit_segments_migrations.py +++ b/api/tests/unit/segments/test_unit_segments_migrations.py @@ -8,7 +8,7 @@ from flag_engine.segments import constants from pytest_django.fixtures import SettingsWrapper -migration_0031 = import_module("segments.migrations.0031_add_segment_rules_data") +migration_0032 = import_module("segments.migrations.0032_add_segment_rules_data") @pytest.mark.skipif( @@ -253,14 +253,14 @@ def _deep_clone(segment: Segment) -> Segment: # type: ignore[valid-type] 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( +def test_0032_add_segment_rules_data__forwards__backfill_segment_rules_data( migrator: Migrator, monkeypatch: pytest.MonkeyPatch, ) -> None: # Given - monkeypatch.setattr(migration_0031, "BATCH_SIZE", 2) + monkeypatch.setattr(migration_0032, "BATCH_SIZE", 2) state = migrator.apply_initial_migration( - ("segments", "0031_add_segment_rules_data") + ("segments", "0032_add_segment_rules_data") ) Organisation = state.apps.get_model("organisations", "Organisation") @@ -346,7 +346,7 @@ def test_0031_add_segment_rules_data__forwards__backfill_segment_rules_data( batched_segments.append(batched_segment) # When - migration_0031.backfill_segment_rules_data(state.apps) + migration_0032.backfill_segment_rules_data(state.apps) # Then segment.refresh_from_db() @@ -419,12 +419,12 @@ def test_0031_add_segment_rules_data__forwards__backfill_segment_rules_data( 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( +def test_0032_add_segment_rules_data__backwards__nullify_segment_rules_data( migrator: Migrator, ) -> None: # Given state = migrator.apply_initial_migration( - ("segments", "0031_add_segment_rules_data") + ("segments", "0032_add_segment_rules_data") ) Organisation = state.apps.get_model("organisations", "Organisation") @@ -442,7 +442,7 @@ def test_0031_add_segment_rules_data__backwards__nullify_segment_rules_data( blank_segment = Segment.objects.create(name="Blank", project=project) # When - migration_0031.nullify_segment_rules_data(state.apps) + migration_0032.nullify_segment_rules_data(state.apps) # Then backfilled_segment.refresh_from_db() diff --git a/api/tests/unit/segments/test_unit_segments_views.py b/api/tests/unit/segments/test_unit_segments_views.py index 1a5dc729bfa5..85adde8cd9e5 100644 --- a/api/tests/unit/segments/test_unit_segments_views.py +++ b/api/tests/unit/segments/test_unit_segments_views.py @@ -110,6 +110,7 @@ def test_create_segment__valid_rules__creates_segment_with_rules( "version_of": created_segment.id, "metadata": [], "membership_counts": [], + "managed_by": "", "rules": [ { "id": mocker.ANY, @@ -1101,6 +1102,7 @@ def test_update_segment__valid_rules__updates_segment_with_rules( "version_of": segment.id, "metadata": [], "membership_counts": [], + "managed_by": "", "rules": [ { "id": mocker.ANY, @@ -2035,6 +2037,7 @@ def test_update_segment__whitelisted_segment_exceeds_max_conditions__returns_200 "version_of": segment.id, "metadata": [], "membership_counts": [], + "managed_by": "", "rules": [ { "id": mocker.ANY, @@ -2218,6 +2221,7 @@ def test_clone_segment__valid_name__returns_cloned_segment( "version_of": cloned_segment.id, "metadata": [], "membership_counts": [], + "managed_by": "", "rules": [], # TODO: Should contain rules as per https://github.com/Flagsmith/flagsmith/issues/7818 } ) From 96daa3390472fa05894dd45b594b5187ee0683e8 Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Wed, 12 Aug 2026 13:14:14 -0300 Subject: [PATCH 19/23] =?UTF-8?q?classy=20=F0=9F=8D=B7?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- api/segments/serializers.py | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/api/segments/serializers.py b/api/segments/serializers.py index c907f3fdd109..e8ade3572557 100644 --- a/api/segments/serializers.py +++ b/api/segments/serializers.py @@ -191,19 +191,23 @@ def update(self, segment: Segment, validated_data: dict[str, Any]): # type: ign enqueue_membership_refresh(segment.project) return segment - def _validate_rules_depth(self, rules: list[LegacySegmentRule]) -> None: - # Raise loudly because the interface ignores rules nested too deep + def _validate_rules_depth( + self, rules: list[LegacySegmentRule], _depth: int = 1 + ) -> None: + # The serializer just ignores rules nested too deep, so we raise for clarity. + if rules and _depth > SEGMENT_RULES_MAX_DEPTH: + raise ValidationError( + { + "segment": [ + f"Rules must not be nested more than " + f"{SEGMENT_RULES_MAX_DEPTH} levels deep." + ] + } + ) for rule in rules: - for nested_rule in rule.get("rules", []): - if nested_rule.get("rules"): - raise ValidationError( - { - "segment": [ - f"Rules must not be nested more than " - f"{SEGMENT_RULES_MAX_DEPTH} levels deep." - ] - } - ) + self._validate_rules_depth( + cast(list[LegacySegmentRule], rule.get("rules", [])), _depth + 1 + ) def _validate_rules_condition_count(self, rules: list[LegacySegmentRule]) -> None: if self._can_segment_own_more_conditions_than_limit(): From c80e05fcee3afb884a5391d7666837b4bdd434d0 Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Wed, 12 Aug 2026 15:15:03 -0300 Subject: [PATCH 20/23] lax the schema --- api/segments/migrations/0032_add_segment_rules_data.py | 3 +-- api/tests/unit/segments/test_unit_segments_migrations.py | 5 +++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/api/segments/migrations/0032_add_segment_rules_data.py b/api/segments/migrations/0032_add_segment_rules_data.py index 5ce6728b02b7..77f78a82a9b6 100644 --- a/api/segments/migrations/0032_add_segment_rules_data.py +++ b/api/segments/migrations/0032_add_segment_rules_data.py @@ -78,8 +78,7 @@ def _rasterise_segment_rule(rule: typing.Any) -> SegmentRule: ) ], } - # Top-level rules always carry "rules"; nested ones only for legacy deeper nesting. - if (subrules := _rasterise_segment_rules(rule)) or rule.segment_id: + if (subrules := _rasterise_segment_rules(rule)): rule_data["rules"] = subrules return rule_data diff --git a/api/tests/unit/segments/test_unit_segments_migrations.py b/api/tests/unit/segments/test_unit_segments_migrations.py index 332539c39f74..af1bb8b72822 100644 --- a/api/tests/unit/segments/test_unit_segments_migrations.py +++ b/api/tests/unit/segments/test_unit_segments_migrations.py @@ -374,7 +374,8 @@ def test_0032_add_segment_rules_data__forwards__backfill_segment_rules_data( "description": "Adults only", } ], - "rules": [ # We mistakenly supported deeper rules in the past + # Our UI never allowed more than two levels, but our API did + "rules": [ { "type": "NONE", "conditions": [ @@ -410,7 +411,7 @@ def test_0032_add_segment_rules_data__forwards__backfill_segment_rules_data( "description": None, } ], - "rules": [], + # NOTE: Empty rules are dropped at any level! } ] From 424e836454d5fd908829a6ca8b38d2069a69c8d9 Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Wed, 12 Aug 2026 18:02:53 -0300 Subject: [PATCH 21/23] strenghten types --- api/segments/models.py | 7 ++++++- api/segments/serializers.py | 3 +++ api/segments/types.py | 6 ++++-- .../observability/_events-catalogue.md | 2 +- 4 files changed, 14 insertions(+), 4 deletions(-) diff --git a/api/segments/models.py b/api/segments/models.py index 300d4de46020..2fd49c38716d 100644 --- a/api/segments/models.py +++ b/api/segments/models.py @@ -30,6 +30,9 @@ from projects.models import Project from segments.services import copy_segment_rules_and_conditions +# TODO: Delete alias as per https://github.com/Flagsmith/flagsmith/issues/7818 +from segments.types import SegmentRule as SegmentRuleType + ModelT = typing.TypeVar("ModelT", bound=models.Model) logger = logging.getLogger(__name__) @@ -105,7 +108,9 @@ class Segment( Feature, on_delete=models.CASCADE, related_name="segments", null=True ) - rules_data = models.JSONField(null=True) + rules_data: models.JSONField[ + list[SegmentRuleType], list[SegmentRuleType] | None + ] = models.JSONField(null=True) version = models.IntegerField(default=1, null=True) diff --git a/api/segments/serializers.py b/api/segments/serializers.py index e8ade3572557..0ac57238fb78 100644 --- a/api/segments/serializers.py +++ b/api/segments/serializers.py @@ -3,6 +3,7 @@ import structlog from django.conf import settings from django.db import transaction +from drf_spectacular.utils import extend_schema_field from drf_writable_nested.serializers import WritableNestedModelSerializer from rest_framework import serializers from rest_framework.exceptions import ValidationError @@ -84,6 +85,8 @@ class Meta: ] +# TODO: Replace with list[types.SegmentRule] as per https://github.com/Flagsmith/flagsmith/issues/7818 +@extend_schema_field(list[LegacySegmentRule]) # type: ignore[arg-type] class SegmentRuleSerializer(_BaseSegmentRuleSerializer): rules = _NestedSegmentRuleSerializer( many=True, diff --git a/api/segments/types.py b/api/segments/types.py index 0d7f280cc412..3b56d935785e 100644 --- a/api/segments/types.py +++ b/api/segments/types.py @@ -27,20 +27,22 @@ class SegmentRule(_BaseSegmentRule): class LegacySegmentCondition(SegmentCondition): + # TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 id: NotRequired[int] delete: NotRequired[bool] class _BaseLegacySegmentRule(TypedDict): + # TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 id: NotRequired[int] delete: NotRequired[bool] type: RuleType conditions: list[LegacySegmentCondition] -class LegacyNestedSegmentRule(_BaseLegacySegmentRule): +class _LegacyNestedSegmentRule(_BaseLegacySegmentRule): pass class LegacySegmentRule(_BaseLegacySegmentRule): - rules: list[LegacyNestedSegmentRule] + rules: list[_LegacyNestedSegmentRule] diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index 0d92374e2f2a..39267949c122 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -607,7 +607,7 @@ Attributes: ### `segments.serializers.segment_revision_created` Logged at `info` from: - - `api/segments/serializers.py:184` + - `api/segments/serializers.py:188` Attributes: - `revision_id` From c6e9507c6d57015b173782b22455b94f3bf585d6 Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Wed, 12 Aug 2026 18:23:40 -0300 Subject: [PATCH 22/23] one less db call for most --- api/segments/serializers.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/api/segments/serializers.py b/api/segments/serializers.py index 0ac57238fb78..4d362355dd5f 100644 --- a/api/segments/serializers.py +++ b/api/segments/serializers.py @@ -213,11 +213,10 @@ def _validate_rules_depth( ) def _validate_rules_condition_count(self, rules: list[LegacySegmentRule]) -> None: - if self._can_segment_own_more_conditions_than_limit(): - return - condition_count = self._count_conditions(rules) if condition_count > settings.SEGMENT_RULES_CONDITIONS_LIMIT: + if self._can_segment_own_more_conditions_than_limit(): + return raise ValidationError( { "segment": [ From 30ee857bd6ebde25df73145c7de8a935b4569dc7 Mon Sep 17 00:00:00 2001 From: Evandro Myller Date: Wed, 12 Aug 2026 18:49:43 -0300 Subject: [PATCH 23/23] naming is hard, now docstrings... --- api/segments/serializers.py | 39 ++++++------------- api/tests/conftest.py | 2 + .../observability/_events-catalogue.md | 2 +- 3 files changed, 15 insertions(+), 28 deletions(-) diff --git a/api/segments/serializers.py b/api/segments/serializers.py index 4d362355dd5f..9e9c36fd4bb6 100644 --- a/api/segments/serializers.py +++ b/api/segments/serializers.py @@ -16,9 +16,7 @@ from segment_membership.services import enqueue_membership_refresh from segments.models import Condition, Segment, SegmentRule, WhitelistedSegment from segments.types import ( - LegacySegmentCondition, LegacySegmentRule, - SegmentCondition, ) # TODO: Delete alias as per https://github.com/Flagsmith/flagsmith/issues/7818 @@ -248,14 +246,14 @@ def _set_rules_data(self, validated_data: dict[str, Any]) -> None: """ if "rules" not in validated_data: return # PATCH support - validated_data["rules_data"] = self._cleanup_rules_and_conditions( + validated_data["rules_data"] = self._get_clean_rules_and_conditions( validated_data["rules"] ) - def _cleanup_rules_and_conditions( + def _get_clean_rules_and_conditions( self, rules: list[LegacySegmentRule] ) -> list[SegmentRuleType]: - """Remove any `id` fields and `delete: true` items from rules and conditions + """Remove obsolete items from rules and conditions In https://github.com/Flagsmith/flagsmith/issues/7814, we moved from a SegmentRule and Condition tree to a JSON field. This cleanup exists to @@ -263,36 +261,23 @@ def _cleanup_rules_and_conditions( return [ { "type": rule["type"], - "conditions": self._cleanup_conditions(rule.get("conditions", [])), - "rules": [ + "conditions": [ { - "type": nested_rule["type"], - "conditions": self._cleanup_conditions( - nested_rule.get("conditions", []) - ), + "property": condition["property"], + "operator": condition["operator"], + "value": condition.get("value"), + "description": condition.get("description"), } - for nested_rule in rule.get("rules", []) - if not nested_rule.get("delete") + for condition in rule.get("conditions", []) + if not condition.get("delete") ], + # Cleanup type-ignore as per https://github.com/Flagsmith/flagsmith/issues/8280 + "rules": self._get_clean_rules_and_conditions(rule.get("rules", [])), # type: ignore[typeddict-item,arg-type] } for rule in rules if not rule.get("delete") ] - def _cleanup_conditions( - self, conditions: list[LegacySegmentCondition] - ) -> list[SegmentCondition]: - return [ - { - "property": condition["property"], - "operator": condition["operator"], - "value": condition.get("value"), - "description": condition.get("description"), - } - for condition in conditions - if not condition.get("delete") - ] - def _get_rules_and_conditions_without_deleted( self, rules_data: DictList ) -> DictList: diff --git a/api/tests/conftest.py b/api/tests/conftest.py index 67305fe7cfdc..014fb37306a5 100644 --- a/api/tests/conftest.py +++ b/api/tests/conftest.py @@ -458,6 +458,8 @@ def segment_rules() -> list[SegmentRuleType]: "description": "Jumping very high does not count!", }, ], + # Cleanup type-ignore as per https://github.com/Flagsmith/flagsmith/issues/8280 + "rules": [], # type: ignore[typeddict-unknown-key] }, ], } diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index 39267949c122..cd0c5571b89a 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -607,7 +607,7 @@ Attributes: ### `segments.serializers.segment_revision_created` Logged at `info` from: - - `api/segments/serializers.py:188` + - `api/segments/serializers.py:185` Attributes: - `revision_id`