diff --git a/api/app/settings/common.py b/api/app/settings/common.py index cd1d21a4165e..c306b49386da 100644 --- a/api/app/settings/common.py +++ b/api/app/settings/common.py @@ -115,6 +115,7 @@ "environments.identities", "environments.identities.traits", "features", + "features.dependencies", "features.feature_external_resources", "features.feature_health", "features.import_export", diff --git a/api/features/dependencies/__init__.py b/api/features/dependencies/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/api/features/dependencies/apps.py b/api/features/dependencies/apps.py new file mode 100644 index 000000000000..6f0e0ffb3bbe --- /dev/null +++ b/api/features/dependencies/apps.py @@ -0,0 +1,7 @@ +from core.apps import BaseAppConfig + + +class FeatureDependenciesConfig(BaseAppConfig): + name = "features.dependencies" + label = "feature_dependencies" + default = True diff --git a/api/features/dependencies/exceptions.py b/api/features/dependencies/exceptions.py new file mode 100644 index 000000000000..f83ac2749fc9 --- /dev/null +++ b/api/features/dependencies/exceptions.py @@ -0,0 +1,48 @@ +from typing import TypedDict + +from rest_framework import status +from rest_framework.exceptions import APIException + +from features.dependencies.types import DependencyPath, ReferencingEnvironment + + +class _CircularDependencyDetail(TypedDict): + """The body served where a feature is refused for depending on itself.""" + + code: str + environment: ReferencingEnvironment + path: DependencyPath + + +class CircularDependencyError(APIException): + """Raised where a feature would end up depending on itself.""" + + status_code = status.HTTP_400_BAD_REQUEST + default_code = "circular_dependency" + + def __init__( + self, environment: ReferencingEnvironment, path: DependencyPath + ) -> None: + super().__init__() + detail: _CircularDependencyDetail = { + "code": self.default_code, + "environment": environment, + "path": path, + } + self.detail = detail # type: ignore[assignment] + + +class PrerequisiteFeatureNotFoundError(APIException): + """Raised where a segment condition names a feature that does not exist.""" + + status_code = status.HTTP_400_BAD_REQUEST + default_code = "prerequisite_feature_not_found" + + def __init__(self, prerequisite_feature: str, condition_json_path: str) -> None: + super().__init__( + { + "code": self.default_code, + "prerequisite_feature": prerequisite_feature, + "condition_json_path": condition_json_path, + } + ) diff --git a/api/features/dependencies/mappers.py b/api/features/dependencies/mappers.py new file mode 100644 index 000000000000..c965301c291f --- /dev/null +++ b/api/features/dependencies/mappers.py @@ -0,0 +1,57 @@ +import jsonpath_rfc9535 +from jsonpath_rfc9535.exceptions import JSONPathError +from jsonpath_rfc9535.segments import JSONPathChildSegment, JSONPathSegment +from jsonpath_rfc9535.selectors import NameSelector + +from features.dependencies.types import FeatureName, JSONPathStr +from segments.types import SegmentRule + + +def map_rules_to_prerequisite_feature_names( + rules: list[SegmentRule], +) -> dict[JSONPathStr, FeatureName]: + """Returns the feature names keyed by the condition $.flags JSONPath""" + return { + f"{rule_json_path}.conditions[{condition_index}]": feature_name + for rule_json_path, rule in _get_rules_by_json_path(rules).items() + for condition_index, condition in enumerate(rule["conditions"]) + if (condition_property := condition["property"]) + and (feature_name := _get_prerequisite_feature_name(condition_property)) + is not None + } + + +def _get_rules_by_json_path( + rules: list[SegmentRule], +) -> dict[JSONPathStr, SegmentRule]: + rules_by_json_path: dict[JSONPathStr, SegmentRule] = {} + for rule_index, rule in enumerate(rules): + rule_json_path = f"$[{rule_index}]" + rules_by_json_path[rule_json_path] = rule + for nested_index, nested_rule in enumerate(rule.get("rules", [])): + rules_by_json_path[f"{rule_json_path}.rules[{nested_index}]"] = nested_rule + return rules_by_json_path + + +def _get_prerequisite_feature_name(condition_property: str) -> FeatureName | None: + """Return the feature name a `$.flags.` condition points at, if it does.""" + # Because of historical decisions, `$['flags']` can be a trait. + if not condition_property.startswith("$.flags"): + return None + try: + query_segments = jsonpath_rfc9535.compile(condition_property).segments + except JSONPathError: + return None + if len(query_segments) < 2 or _get_selected_name(query_segments[0]) != "flags": + return None + return _get_selected_name(query_segments[1]) + + +def _get_selected_name(query_segment: JSONPathSegment) -> str | None: + if ( + not isinstance(query_segment, JSONPathChildSegment) + or len(query_segment.selectors) != 1 + ): + return None + selector = query_segment.selectors[0] + return selector.name if isinstance(selector, NameSelector) else None diff --git a/api/features/dependencies/migrations/0001_initial.py b/api/features/dependencies/migrations/0001_initial.py new file mode 100644 index 000000000000..f6df7c453c95 --- /dev/null +++ b/api/features/dependencies/migrations/0001_initial.py @@ -0,0 +1,48 @@ +# Generated by Django 5.2.17 on 2026-09-14 18:40 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + + initial = True + + dependencies = [ + ("features", "0067_add_feature_state_mv_hashing_salt"), + ("segments", "0032_add_segment_rules_data"), + ] + + operations = [ + migrations.CreateModel( + name="SegmentFlagReference", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("condition_json_path", models.TextField()), + ( + "prerequisite_feature", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="segment_references", + to="features.feature", + ), + ), + ( + "segment", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="flag_references", + to="segments.segment", + ), + ), + ], + ), + ] diff --git a/api/features/dependencies/migrations/__init__.py b/api/features/dependencies/migrations/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/api/features/dependencies/models.py b/api/features/dependencies/models.py new file mode 100644 index 000000000000..cec48e38b925 --- /dev/null +++ b/api/features/dependencies/models.py @@ -0,0 +1,26 @@ +from django.db import models + + +class SegmentFlagReference(models.Model): + """A reference to a pre-requisite flag from the segment + + When a segment rule points to a feature as a pre-requisite, an object of + this type must exist to materialise the relationship and enable easier + backreferencing, e.g. for validating circular dependencies, and querying + without inferring from JSON rules. + """ + + segment = models.ForeignKey( + "segments.Segment", + on_delete=models.CASCADE, + related_name="flag_references", + ) + + prerequisite_feature = models.ForeignKey( + "features.Feature", + on_delete=models.CASCADE, + related_name="segment_references", + ) + + # JSONPath (RFC 9535) locating the rule condition + condition_json_path = models.TextField() diff --git a/api/features/dependencies/services.py b/api/features/dependencies/services.py new file mode 100644 index 000000000000..c9a0dfddf4a3 --- /dev/null +++ b/api/features/dependencies/services.py @@ -0,0 +1,185 @@ +import typing +from collections import defaultdict +from collections.abc import Collection + +import structlog + +from environments.models import Environment +from features.dependencies.exceptions import ( + CircularDependencyError, + PrerequisiteFeatureNotFoundError, +) +from features.dependencies.mappers import map_rules_to_prerequisite_feature_names +from features.dependencies.models import SegmentFlagReference +from features.dependencies.types import DependencyEdge, DependencyPath, FeatureName +from features.models import Feature, FeatureSegment +from segments.services import get_all_live_or_scheduled_overrides + +if typing.TYPE_CHECKING: + from segments.models import Segment + +logger = structlog.get_logger("features") + + +def index_segment_flag_references(segment: "Segment") -> None: + """Materialise the segment's `$.flags` conditions as SegmentFlagReference rows.""" + feature_names_by_json_path = map_rules_to_prerequisite_feature_names( + segment.rules_data or [] + ) + feature_ids_by_name = dict( + Feature.objects.filter( + project_id=segment.project_id, + name__in=set(feature_names_by_json_path.values()), + ).values_list("name", "id") + ) + for condition_json_path, feature_name in feature_names_by_json_path.items(): + if feature_name not in feature_ids_by_name: + raise PrerequisiteFeatureNotFoundError( + prerequisite_feature=feature_name, + condition_json_path=condition_json_path, + ) + references = SegmentFlagReference.objects.filter(segment=segment) + previous_feature_names = set( + references.values_list("prerequisite_feature__name", flat=True) + ) + references.delete() + SegmentFlagReference.objects.bulk_create( + SegmentFlagReference( + segment=segment, + prerequisite_feature_id=feature_ids_by_name[feature_name], + condition_json_path=condition_json_path, + ) + for condition_json_path, feature_name in feature_names_by_json_path.items() + ) + for override in FeatureSegment.objects.filter(segment=segment).select_related( + "environment__project", "feature" + ): + report_flag_dependencies( + environment=override.environment, + feature=override.feature, + created=feature_ids_by_name.keys() - previous_feature_names, + deleted=previous_feature_names - feature_ids_by_name.keys(), + ) + + +def delete_segment_flag_references(segment: "Segment") -> None: + """Drop the segment's index rows, reporting every dependency lost.""" + references = SegmentFlagReference.objects.filter(segment=segment).select_related( + "prerequisite_feature" + ) + overrides = FeatureSegment.objects.filter(segment=segment).select_related( + "environment", "feature" + ) + for override in overrides: + for reference in references: + logger.info( + "dependencies.deleted", + organisation__id=segment.project.organisation_id, + project__id=segment.project_id, + environment__key=override.environment.api_key, + feature__name=override.feature.name, + prerequisite_feature__name=reference.prerequisite_feature.name, + ) + references.delete() + + +def report_flag_dependencies( + *, + environment: Environment, + feature: Feature, + created: Collection[FeatureName], + deleted: Collection[FeatureName], +) -> None: + """Report the prerequisites a feature gains and loses in an environment.""" + log = logger.bind( + organisation__id=environment.project.organisation_id, + project__id=environment.project_id, + environment__key=environment.api_key, + feature__name=feature.name, + ) + for feature_name in deleted: + log.info("dependencies.deleted", prerequisite_feature__name=feature_name) + for feature_name in created: + log.info("dependencies.created", prerequisite_feature__name=feature_name) + + +def validate_segment_flag_dependencies(segment: "Segment") -> None: + """Raise if any feature the segment overrides ends up depending on itself.""" + existing_references = SegmentFlagReference.objects.filter(segment=segment) + if not existing_references.exists(): + return + edges_by_environment_id: dict[int, dict[FeatureName, list[DependencyEdge]]] = {} + for override in ( + get_all_live_or_scheduled_overrides() + .filter(segment=segment) + .select_related("environment", "feature") + ): + if override.environment_id not in edges_by_environment_id: + edges_by_environment_id[override.environment_id] = _get_dependency_edges( + override.environment + ) + edges = edges_by_environment_id[override.environment_id] + pending: list[DependencyPath] = [ + [edge] for edge in edges[override.feature.name] + ] + visited: set[str] = set() + while pending: + path = pending.pop() + if (prerequisite_feature_name := path[-1]["needs"]) in visited: + continue + if prerequisite_feature_name == override.feature.name: + logger.info( + "dependencies.create_failed", + organisation__id=segment.project.organisation_id, + project__id=segment.project_id, + environment__key=override.environment.api_key, + feature__name=override.feature.name, + prerequisite_feature__name=path[0]["needs"], + ) + raise CircularDependencyError( + environment={ + "key": override.environment.api_key, + "name": override.environment.name, + }, + path=path, + ) + visited.add(prerequisite_feature_name) + pending += [[*path, edge] for edge in edges[prerequisite_feature_name]] + + +def _get_dependency_edges( + environment: Environment, +) -> dict[FeatureName, list[DependencyEdge]]: + edges: dict[FeatureName, list[DependencyEdge]] = defaultdict(list) + for ( + feature_name, + prerequisite_feature_name, + segment_id, + segment_name, + condition_json_path, + ) in ( + get_all_live_or_scheduled_overrides() + .filter( + environment=environment, + segment__flag_references__isnull=False, + ) + .values_list( + "feature__name", + "segment__flag_references__prerequisite_feature__name", + "segment_id", + "segment__name", + "segment__flag_references__condition_json_path", + ) + ): + edges[feature_name].append( + { + "feature": feature_name, + "needs": prerequisite_feature_name, + "segment": { + "id": segment_id, + "name": segment_name, + "condition_json_path": condition_json_path, + }, + } + ) + return edges diff --git a/api/features/dependencies/types.py b/api/features/dependencies/types.py new file mode 100644 index 000000000000..05e2083573d2 --- /dev/null +++ b/api/features/dependencies/types.py @@ -0,0 +1,30 @@ +import typing + +FeatureName = str +JSONPathStr = str + + +class ReferencingSegment(typing.TypedDict): + """The segment whose rules hold the `$.flags` condition making up a dependency.""" + + id: int + name: str + condition_json_path: str + + +class ReferencingEnvironment(typing.TypedDict): + """The environment whose live overrides make up a dependency graph.""" + + key: str + name: str + + +class DependencyEdge(typing.TypedDict): + """One feature's dependency on another.""" + + feature: str + needs: str + segment: ReferencingSegment + + +DependencyPath = list[DependencyEdge] diff --git a/api/features/feature_segments/views.py b/api/features/feature_segments/views.py index a44d24a0f587..9dabc59b2abb 100644 --- a/api/features/feature_segments/views.py +++ b/api/features/feature_segments/views.py @@ -1,14 +1,17 @@ import logging from common.projects.permissions import VIEW_PROJECT +from django.db import transaction from django.utils.decorators import method_decorator from drf_spectacular.utils import extend_schema from rest_framework import viewsets from rest_framework.decorators import action from rest_framework.generics import get_object_or_404 from rest_framework.response import Response +from rest_framework.serializers import BaseSerializer from environments.models import Environment +from features.dependencies.services import validate_segment_flag_dependencies from features.feature_segments.serializers import ( FeatureSegmentChangePrioritiesSerializer, FeatureSegmentCreateSerializer, @@ -79,6 +82,11 @@ def get_queryset(self): # type: ignore[no-untyped-def] return queryset + @transaction.atomic + def perform_create(self, serializer: BaseSerializer[FeatureSegment]) -> None: + feature_segment = serializer.save() + validate_segment_flag_dependencies(feature_segment.segment) + def get_serializer_class(self): # type: ignore[no-untyped-def] if self.action in ["create", "update", "partial_update"]: return FeatureSegmentCreateSerializer diff --git a/api/features/future/services.py b/api/features/future/services.py index d2fe64aed62c..3ab180a6f3a7 100644 --- a/api/features/future/services.py +++ b/api/features/future/services.py @@ -9,6 +9,12 @@ from api_keys.user import APIKeyUser from environments.models import Environment +from features.dependencies.models import SegmentFlagReference +from features.dependencies.services import ( + report_flag_dependencies, + validate_segment_flag_dependencies, +) +from features.dependencies.types import FeatureName from features.future.exceptions import ( DuplicatePriorityError, SegmentOverrideNotFoundError, @@ -29,11 +35,18 @@ from features.multivariate.models import MultivariateFeatureStateValue from features.versioning.models import EnvironmentFeatureVersion from features.versioning.versioning_service import get_environment_flags_list +from segments.models import Segment from users.models import FFAdminUser logger = structlog.get_logger("features") +class _OverriddenSegments(NamedTuple): + created: list[int] + updated: list[int] + deleted: list[int] + + def _get_feature_states( environment: Environment, feature: Feature ) -> list[FeatureState]: @@ -234,12 +247,6 @@ def _check_priorities( raise DuplicatePriorityError(f"Duplicate priority: {duplicate}.") -class WrittenSegmentOverrides(NamedTuple): - created: list[int] - updated: list[int] - deleted: list[int] - - def _write_segment_overrides( *, environment: Environment, @@ -249,24 +256,24 @@ def _write_segment_overrides( overrides: dict[int, FeatureState], changes: Sequence[SegmentOverrideRequest], replace: bool, -) -> WrittenSegmentOverrides: - written = WrittenSegmentOverrides([], [], []) +) -> _OverriddenSegments: + segments = _OverriddenSegments([], [], []) if replace: - written.deleted.extend( + segments.deleted.extend( sorted(overrides.keys() - {change["segment"]["id"] for change in changes}) ) _delete_segment_overrides( environment=environment, feature=feature, version=version, - segment_ids=written.deleted, + segment_ids=segments.deleted, ) for position, change in enumerate(changes): segment_id = change["segment"]["id"] if feature_state := overrides.get(segment_id): - written.updated.append(segment_id) + segments.updated.append(segment_id) else: feature_state = _create_segment_override( environment=environment, @@ -275,17 +282,31 @@ def _write_segment_overrides( segment_id=segment_id, priority=change.get("priority", position), ) - written.created.append(segment_id) + segments.created.append(segment_id) _write_segment_override( feature_state, change, - replace=replace or segment_id in written.created, + replace=replace or segment_id in segments.created, environment_default=environment_default, ) _check_priorities(environment, feature, version) - return written + return segments + + +def _get_prerequisite_feature_names( + segments: _OverriddenSegments, +) -> tuple[set[FeatureName], set[FeatureName]]: + """Return the prerequisite feature names the flag gains and loses.""" + created: set[FeatureName] = set() + deleted: set[FeatureName] = set() + for segment_id, feature_name in SegmentFlagReference.objects.filter( + segment_id__in=[*segments.created, *segments.deleted] + ).values_list("segment_id", "prerequisite_feature__name"): + names = created if segment_id in segments.created else deleted + names.add(feature_name) + return created, deleted def update_flag( @@ -301,7 +322,7 @@ def update_flag( if writes_nothing: return get_flag(environment=environment, feature=feature) - written = WrittenSegmentOverrides([], [], []) + segments = _OverriddenSegments([], [], []) with transaction.atomic(): version = _create_draft_version(environment, feature) @@ -318,7 +339,7 @@ def update_flag( ) if (override_changes := changes.get("segment_overrides")) is not None: - written = _write_segment_overrides( + segments = _write_segment_overrides( environment=environment, feature=feature, version=version, @@ -331,15 +352,25 @@ def update_flag( if version is not None: _publish_version(version, author) + for segment in Segment.objects.filter(id__in=segments.created): + validate_segment_flag_dependencies(segment) + created, deleted = _get_prerequisite_feature_names(segments) + report_flag_dependencies( + environment=environment, + feature=feature, + created=created, + deleted=deleted, + ) + logger.info( "flag.updated", organisation__id=environment.project.organisation_id, project__id=environment.project_id, environment__id=environment.id, feature__id=feature.id, - segment_overrides__created__segment__ids=written.created, - segment_overrides__updated__segment__ids=written.updated, - segment_overrides__deleted__segment__ids=written.deleted, + segment_overrides__created__segment__ids=segments.created, + segment_overrides__updated__segment__ids=segments.updated, + segment_overrides__deleted__segment__ids=segments.deleted, ) return get_flag(environment=environment, feature=feature) @@ -370,6 +401,16 @@ def delete_segment_override( if version is not None: _publish_version(version, author) + _, deleted = _get_prerequisite_feature_names( + _OverriddenSegments([], [], [segment_id]) + ) + report_flag_dependencies( + environment=environment, + feature=feature, + created=[], + deleted=deleted, + ) + logger.info( "flag.updated", organisation__id=environment.project.organisation_id, diff --git a/api/features/views.py b/api/features/views.py index 9ddf9f6b590e..db62e6f445f1 100644 --- a/api/features/views.py +++ b/api/features/views.py @@ -7,6 +7,7 @@ from common.projects.permissions import VIEW_PROJECT from django.conf import settings from django.core.cache import caches +from django.db import transaction from django.db.models import ( BooleanField, Case, @@ -62,6 +63,7 @@ EnvironmentKeyPermissions, NestedEnvironmentPermissions, ) +from features.dependencies.services import validate_segment_flag_dependencies from features.feature_lifecycle.services import ( annotate_feature_queryset_with_lifecycle_stage, is_feature_lifecycle_enabled, @@ -1221,5 +1223,8 @@ def create_segment_override( # type: ignore[no-untyped-def] data=request.data, context={"environment": environment, "feature": feature} ) serializer.is_valid(raise_exception=True) - serializer.save(environment=environment, feature=feature) # type: ignore[no-untyped-call] + with transaction.atomic(): + feature_state = serializer.save(environment=environment, feature=feature) # type: ignore[no-untyped-call] + if feature_segment := feature_state.feature_segment: + validate_segment_flag_dependencies(feature_segment.segment) return Response(serializer.data, status=201) diff --git a/api/segments/serializers.py b/api/segments/serializers.py index dee190c21376..45221c128347 100644 --- a/api/segments/serializers.py +++ b/api/segments/serializers.py @@ -10,6 +10,10 @@ from cohorts.models import Cohort from edge_api.utils import is_edge_enabled +from features.dependencies.services import ( + index_segment_flag_references, + validate_segment_flag_dependencies, +) from metadata.serializers import MetadataSerializer, MetadataSerializerMixin from projects.models import Project from segment_membership.constants import MAX_SEGMENT_MEMBERS_PAGE_SIZE @@ -217,7 +221,9 @@ def validate(self, attrs: dict[str, Any]) -> dict[str, Any]: 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] + with transaction.atomic(): + segment = super().create(validated_data) # type: ignore[no-untyped-call] + index_segment_flag_references(segment) self._update_metadata(segment, metadata_data) enqueue_membership_refresh(segment.project) return segment @@ -234,6 +240,8 @@ def update(self, segment: Segment, validated_data: dict[str, Any]): # type: ign revision_id=segment_revision.id, ) segment = super().update(segment, validated_data) # type: ignore[no-untyped-call] + index_segment_flag_references(segment) + validate_segment_flag_dependencies(segment) self._update_metadata(segment, metadata) enqueue_membership_refresh(segment.project) return segment diff --git a/api/segments/services.py b/api/segments/services.py index 18615e652839..198ccf214da6 100644 --- a/api/segments/services.py +++ b/api/segments/services.py @@ -1,5 +1,6 @@ import typing import uuid +from datetime import datetime from django.db import models, transaction from django.db.models import QuerySet @@ -41,17 +42,10 @@ def delete_segment( segment: "Segment", author: AuthorData, ) -> None: - """ - Delete a segment using optimized bulk operations. - - Uses bulk UPDATE/DELETE operations instead of individual soft-deletes, - reducing the number of database queries from O(n) to O(1) where n is - the number of rules and conditions. - - TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 - """ + """Delete a segment and all of its components""" + from features.dependencies.services import delete_segment_flag_references from features.models import FeatureSegment - from segments.models import Condition, Segment, SegmentRule + from segments.models import Segment from segments.tasks import create_segment_deleted_audit_log now = timezone.now() @@ -67,6 +61,36 @@ def delete_segment( ).values_list("id", flat=True) ) + with transaction.atomic(): + delete_segment_flag_references(segment) + FeatureSegment.objects.filter(segment_id__in=segment_ids).delete() + _delete_legacy_rules_and_conditions(segment_ids, now) + Segment.objects.filter(id__in=segment_ids).update(deleted_at=now) + + create_segment_deleted_audit_log.delay( + args=( + project_id, + segment_name, + segment_id, + segment_uuid, + author.user.id if author.user else None, + author.api_key.id if author.api_key else None, + now.isoformat(), + ) + ) + + +def _delete_legacy_rules_and_conditions( + segment_ids: list[int], + now: datetime, +) -> None: + """Delete a segment's rules and conditions (rows) using bulk operations + + This is only needed until `Segment.rules_data` becomes the source of truth. + TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 + """ + from segments.models import Condition, SegmentRule + top_level_rule_ids = list( SegmentRule.objects.filter(segment_id__in=segment_ids).values_list( "id", flat=True @@ -87,23 +111,8 @@ def delete_segment( all_rule_ids_list = list(all_rule_ids) - with transaction.atomic(): - FeatureSegment.objects.filter(segment_id__in=segment_ids).delete() - Condition.objects.filter(rule_id__in=all_rule_ids_list).update(deleted_at=now) - SegmentRule.objects.filter(id__in=all_rule_ids_list).update(deleted_at=now) - Segment.objects.filter(id__in=segment_ids).update(deleted_at=now) - - create_segment_deleted_audit_log.delay( - args=( - project_id, - segment_name, - segment_id, - segment_uuid, - author.user.id if author.user else None, - author.api_key.id if author.api_key else None, - now.isoformat(), - ) - ) + Condition.objects.filter(rule_id__in=all_rule_ids_list).update(deleted_at=now) + SegmentRule.objects.filter(id__in=all_rule_ids_list).update(deleted_at=now) def copy_segment_rules_and_conditions( diff --git a/api/tests/integration/conftest.py b/api/tests/integration/conftest.py index ac6174e4d7e6..76c7f47ed18d 100644 --- a/api/tests/integration/conftest.py +++ b/api/tests/integration/conftest.py @@ -15,8 +15,10 @@ from app.utils import create_hash from app_analytics.influxdb_wrapper import InfluxDBWrapper from environments.enums import EnvironmentDocumentCacheMode +from features.future.types import SegmentOverrideRequest, UpdateFlagRequest from organisations.models import Organisation from tests.integration.helpers import create_mv_option_with_api +from tests.types import CreateSegmentOverrideFixture from users.models import FFAdminUser @@ -385,6 +387,32 @@ def feature_segment(admin_client, segment, feature, environment): # type: ignor return response.json()["id"] +@pytest.fixture() +def create_segment_override(admin_client: APIClient) -> CreateSegmentOverrideFixture: + """Return a callable putting a segment override live, whichever versioning is in use.""" + + def _create_segment_override( + environment_api_key: str, + feature_id: int, + segment_id: int, + enabled: bool = True, + priority: int | None = None, + ) -> None: + segment_override = SegmentOverrideRequest( + {"segment": {"id": segment_id}, "enabled": enabled} + ) + if priority is not None: + segment_override["priority"] = priority + response = admin_client.patch( + f"/api/__future__/environments/{environment_api_key}/features/{feature_id}/", + UpdateFlagRequest({"segment_overrides": [segment_override]}), + format="json", + ) + assert response.status_code == status.HTTP_200_OK + + return _create_segment_override + + @pytest.fixture() def segment_featurestate( admin_client: APIClient, diff --git a/api/tests/integration/features/conftest.py b/api/tests/integration/features/conftest.py new file mode 100644 index 000000000000..89e995c231fd --- /dev/null +++ b/api/tests/integration/features/conftest.py @@ -0,0 +1,36 @@ +import pytest +from rest_framework.test import APIClient + +from environments.models import Environment +from features.versioning.tasks import enable_v2_versioning + + +@pytest.fixture() +def other_environment( + admin_client: APIClient, + project: int, + versioned_environment: Environment, +) -> Environment: + """A second environment in the project, versioned like the first one.""" + response = admin_client.post( + "/api/v1/environments/", + data={"name": "Other Environment", "project": project}, + format="json", + ) + assert response.status_code == 201 + environment: Environment = Environment.objects.get( + api_key=response.json()["api_key"] + ) + if versioned_environment.use_v2_feature_versioning: + enable_v2_versioning(environment_id=environment.id) + return environment + + +@pytest.fixture(params=["feature_versioning_v1", "feature_versioning_v2"]) +def versioned_environment( + request: pytest.FixtureRequest, + environment: int, +) -> Environment: + if request.param == "feature_versioning_v2": + enable_v2_versioning(environment_id=environment) + return Environment.objects.get(id=environment) # type: ignore[no-any-return] diff --git a/api/tests/integration/features/dependencies/test_segment_management.py b/api/tests/integration/features/dependencies/test_segment_management.py new file mode 100644 index 000000000000..f6d939968460 --- /dev/null +++ b/api/tests/integration/features/dependencies/test_segment_management.py @@ -0,0 +1,932 @@ +import pytest +from pytest_structlog import StructuredLogCapture +from rest_framework.test import APIClient + +from environments.models import Environment +from features.dependencies.models import SegmentFlagReference +from features.future.types import UpdateFlagRequest +from features.models import Feature, FeatureSegment +from segments.models import Segment +from tests.types import CreateSegmentOverrideFixture + + +def test_create_segment__valid_flag_dependency__indexes_created( + admin_client: APIClient, + project: int, +) -> None: + # Given + flag_a = Feature.objects.create(name="flag_a", project_id=project) + flag_b = Feature.objects.create(name="flag_b", project_id=project) + flag_c = Feature.objects.create(name="flag_c", project_id=project) + + # When + response = admin_client.post( + f"/api/v1/projects/{project}/segments/", + data={ + "name": "power_users", + "rules": [ + { + "type": "ALL", + "conditions": [ + { + "property": "$.flags.flag_a.enabled", + "operator": "EQUAL", + "value": True, + }, + ], + "rules": [ + { + "type": "ANY", + "conditions": [ + { + "property": "$.flags.flag_b.enabled", + "operator": "EQUAL", + "value": True, + }, + { + "property": "$.flags['flag_c'].enabled", + "operator": "EQUAL", + "value": True, + }, + ], + }, + ], + } + ], + }, + format="json", + ) + + # Then + assert response.status_code == 201 + segment_id = response.json()["id"] + assert list( + SegmentFlagReference.objects.order_by("condition_json_path").values( + "segment", + "prerequisite_feature", + "condition_json_path", + ) + ) == [ + { + "segment": segment_id, + "prerequisite_feature": flag_a.id, + "condition_json_path": "$[0].conditions[0]", + }, + { + "segment": segment_id, + "prerequisite_feature": flag_b.id, + "condition_json_path": "$[0].rules[0].conditions[0]", + }, + { + "segment": segment_id, + "prerequisite_feature": flag_c.id, + "condition_json_path": "$[0].rules[0].conditions[1]", + }, + ] + + +@pytest.mark.parametrize( + ("property", "prerequisite_feature"), + [ + ("$.flags.unicorn.enabled", "unicorn"), + ("$.flags[''].enabled", ""), + ], +) +def test_create_segment__nonexistent_prerequisite__responds_400( + admin_client: APIClient, + project: int, + property: str, + prerequisite_feature: str, +) -> None: + # Given / When + response = admin_client.post( + f"/api/v1/projects/{project}/segments/", + data={ + "name": "segment", + "rules": [ + { + "type": "ALL", + "conditions": [ + { + "property": property, + "operator": "EQUAL", + "value": True, + }, + ], + } + ], + }, + format="json", + ) + + # Then + assert response.status_code == 400 + assert response.json() == { + "code": "prerequisite_feature_not_found", + "prerequisite_feature": prerequisite_feature, + "condition_json_path": "$[0].conditions[0]", + } + assert not Segment.objects.exists() + assert not SegmentFlagReference.objects.exists() + + +def test_update_segment_update_rules__nonexistent_prerequisite__responds_400( + admin_client: APIClient, + project: int, +) -> None: + # Given + segment = Segment.objects.create(name="segment", project_id=project) + + # When + response = admin_client.put( + f"/api/v1/projects/{project}/segments/{segment.id}/", + data={ + "name": "segment", + "project": project, + "rules": [ + { + "type": "ALL", + "conditions": [ + { + "property": "$.flags.unicorn.enabled", + "operator": "EQUAL", + "value": True, + }, + ], + } + ], + }, + format="json", + ) + + # Then + assert response.status_code == 400 + assert response.json() == { + "code": "prerequisite_feature_not_found", + "prerequisite_feature": "unicorn", + "condition_json_path": "$[0].conditions[0]", + } + assert not SegmentFlagReference.objects.exists() + + +@pytest.mark.usefixtures("versioned_environment") +def test_update_segment_update_rules__valid_flag_dependency__indexes_updated( + admin_client: APIClient, + create_segment_override: CreateSegmentOverrideFixture, + environment_api_key: str, + log: StructuredLogCapture, + organisation: int, + project: int, +) -> None: + # Given + chicken = Feature.objects.create(name="chicken", project_id=project) + egg = Feature.objects.create(name="egg", project_id=project) + rooster = Feature.objects.create(name="rooster", project_id=project) + hen = Feature.objects.create(name="hen", project_id=project) + segment = Segment.objects.create(name="segment", project_id=project) + create_segment_override( + environment_api_key=environment_api_key, + feature_id=chicken.id, + segment_id=segment.id, + ) + SegmentFlagReference.objects.create( + segment=segment, + prerequisite_feature=egg, + condition_json_path="$[0].conditions[0]", + ) + SegmentFlagReference.objects.create( + segment=segment, + prerequisite_feature=hen, + condition_json_path="$[0].conditions[1]", + ) + + # When + response = admin_client.put( + f"/api/v1/projects/{project}/segments/{segment.id}/", + data={ + "name": "segment", + "project": project, + "rules": [ + { + "type": "ALL", + "conditions": [ + { + "property": "$.flags.rooster.enabled", + "operator": "EQUAL", + "value": True, + }, + { + "property": "$.flags.hen.enabled", + "operator": "EQUAL", + "value": True, + }, + ], + } + ], + }, + format="json", + ) + + # Then + assert response.status_code == 200 + assert list( + SegmentFlagReference.objects.order_by("condition_json_path").values( + "segment", + "prerequisite_feature", + "condition_json_path", + ) + ) == [ + { + "segment": segment.id, + "prerequisite_feature": rooster.id, + "condition_json_path": "$[0].conditions[0]", + }, + { + "segment": segment.id, + "prerequisite_feature": hen.id, + "condition_json_path": "$[0].conditions[1]", + }, + ] + assert not log.has("dependencies.deleted", prerequisite_feature__name="hen") + assert not log.has("dependencies.created", prerequisite_feature__name="hen") + assert log.has( + "dependencies.deleted", + level="info", + organisation__id=organisation, + project__id=project, + environment__key=environment_api_key, + feature__name="chicken", + prerequisite_feature__name="egg", + ) + assert log.has( + "dependencies.created", + level="info", + organisation__id=organisation, + project__id=project, + environment__key=environment_api_key, + feature__name="chicken", + prerequisite_feature__name="rooster", + ) + + +@pytest.mark.usefixtures("versioned_environment") +def test_delete_segment__flag_dependency__indexes_removed( + admin_client: APIClient, + create_segment_override: CreateSegmentOverrideFixture, + environment_api_key: str, + log: StructuredLogCapture, + organisation: int, + project: int, +) -> None: + # Given + chicken = Feature.objects.create(name="chicken", project_id=project) + egg = Feature.objects.create(name="egg", project_id=project) + segment = Segment.objects.create(name="segment", project_id=project) + create_segment_override( + environment_api_key=environment_api_key, + feature_id=chicken.id, + segment_id=segment.id, + ) + SegmentFlagReference.objects.create( + segment=segment, + prerequisite_feature=egg, + condition_json_path="$[0].conditions[0]", + ) + + # When + response = admin_client.delete( + f"/api/v1/projects/{project}/segments/{segment.id}/", + ) + + # Then + assert response.status_code == 204 + assert not SegmentFlagReference.objects.exists() + assert log.has( + "dependencies.deleted", + level="info", + organisation__id=organisation, + project__id=project, + environment__key=environment_api_key, + feature__name="chicken", + prerequisite_feature__name="egg", + ) + + +def test_update_segment_add_override__circular_flag_dependency__responds_400( + admin_client: APIClient, + create_segment_override: CreateSegmentOverrideFixture, + environment: int, + environment_api_key: str, + environment_name: str, + log: StructuredLogCapture, + organisation: int, + project: int, +) -> None: + # Given + chicken = Feature.objects.create(name="chicken", project_id=project) + egg = Feature.objects.create(name="egg", project_id=project) + segment1 = Segment.objects.create(name="segment1", project_id=project) + segment2 = Segment.objects.create(name="segment2", project_id=project) + SegmentFlagReference.objects.create( + segment=segment1, + prerequisite_feature=egg, + condition_json_path="$[0].conditions[1]", + ) + create_segment_override( + environment_api_key=environment_api_key, + feature_id=chicken.id, + segment_id=segment1.id, + ) + SegmentFlagReference.objects.create( + segment=segment2, + prerequisite_feature=chicken, + condition_json_path="$[1].conditions[2]", + ) + + # When + response = admin_client.post( + f"/api/v1/environments/{environment_api_key}/features/{egg.id}/create-segment-override/", + data={ + "feature_state_value": {}, + "feature_segment": {"segment": segment2.id}, + "enabled": True, + }, + format="json", + ) + + # Then + assert response.status_code == 400 + assert response.json() == { + "code": "circular_dependency", + "environment": {"key": environment_api_key, "name": environment_name}, + "path": [ + { + "feature": "egg", + "needs": "chicken", + "segment": { + "id": segment2.id, + "name": segment2.name, + "condition_json_path": "$[1].conditions[2]", + }, + }, + { + "feature": "chicken", + "needs": "egg", + "segment": { + "id": segment1.id, + "name": segment1.name, + "condition_json_path": "$[0].conditions[1]", + }, + }, + ], + } + assert not FeatureSegment.objects.filter(segment=segment2, feature=egg).exists() + assert log.has( + "dependencies.create_failed", + level="info", + organisation__id=organisation, + project__id=project, + environment__key=environment_api_key, + feature__name="egg", + prerequisite_feature__name="chicken", + ) + + +@pytest.mark.usefixtures("versioned_environment") +def test_update_flag_add_override__flag_dependency__reports_dependency_created( + admin_client: APIClient, + environment_api_key: str, + log: StructuredLogCapture, + organisation: int, + project: int, +) -> None: + # Given + chicken = Feature.objects.create(name="chicken", project_id=project) + egg = Feature.objects.create(name="egg", project_id=project) + segment = Segment.objects.create(name="segment", project_id=project) + SegmentFlagReference.objects.create( + segment=segment, + prerequisite_feature=egg, + condition_json_path="$[0].conditions[0]", + ) + + # When + response = admin_client.patch( + f"/api/__future__/environments/{environment_api_key}/features/{chicken.id}/", + UpdateFlagRequest( + {"segment_overrides": [{"segment": {"id": segment.id}, "enabled": True}]} + ), + format="json", + ) + + # Then + assert response.status_code == 200 + assert log.has( + "dependencies.created", + level="info", + organisation__id=organisation, + project__id=project, + environment__key=environment_api_key, + feature__name="chicken", + prerequisite_feature__name="egg", + ) + + +@pytest.mark.usefixtures("versioned_environment") +def test_delete_override__flag_dependency__reports_dependency_deleted( + admin_client: APIClient, + create_segment_override: CreateSegmentOverrideFixture, + environment_api_key: str, + log: StructuredLogCapture, + organisation: int, + project: int, +) -> None: + # Given + chicken = Feature.objects.create(name="chicken", project_id=project) + egg = Feature.objects.create(name="egg", project_id=project) + segment = Segment.objects.create(name="segment", project_id=project) + SegmentFlagReference.objects.create( + segment=segment, + prerequisite_feature=egg, + condition_json_path="$[0].conditions[0]", + ) + create_segment_override( + environment_api_key=environment_api_key, + feature_id=chicken.id, + segment_id=segment.id, + ) + + # When + response = admin_client.delete( + f"/api/__future__/environments/{environment_api_key}" + f"/features/{chicken.id}/segment-overrides/{segment.id}/", + ) + + # Then + assert response.status_code == 200 + assert log.has( + "dependencies.deleted", + level="info", + organisation__id=organisation, + project__id=project, + environment__key=environment_api_key, + feature__name="chicken", + prerequisite_feature__name="egg", + ) + + +@pytest.mark.usefixtures("versioned_environment") +def test_update_flag_add_override__circular_flag_dependency__responds_400( + admin_client: APIClient, + create_segment_override: CreateSegmentOverrideFixture, + environment_api_key: str, + environment_name: str, + log: StructuredLogCapture, + organisation: int, + project: int, +) -> None: + # Given + chicken = Feature.objects.create(name="chicken", project_id=project) + egg = Feature.objects.create(name="egg", project_id=project) + segment1 = Segment.objects.create(name="segment1", project_id=project) + segment2 = Segment.objects.create(name="segment2", project_id=project) + SegmentFlagReference.objects.create( + segment=segment1, + prerequisite_feature=egg, + condition_json_path="$[0].conditions[1]", + ) + create_segment_override( + environment_api_key=environment_api_key, + feature_id=chicken.id, + segment_id=segment1.id, + ) + SegmentFlagReference.objects.create( + segment=segment2, + prerequisite_feature=chicken, + condition_json_path="$[1].conditions[2]", + ) + + # When + response = admin_client.patch( + f"/api/__future__/environments/{environment_api_key}/features/{egg.id}/", + UpdateFlagRequest( + {"segment_overrides": [{"segment": {"id": segment2.id}, "enabled": True}]} + ), + format="json", + ) + + # Then + assert response.status_code == 400 + assert response.json() == { + "code": "circular_dependency", + "environment": {"key": environment_api_key, "name": environment_name}, + "path": [ + { + "feature": "egg", + "needs": "chicken", + "segment": { + "id": segment2.id, + "name": segment2.name, + "condition_json_path": "$[1].conditions[2]", + }, + }, + { + "feature": "chicken", + "needs": "egg", + "segment": { + "id": segment1.id, + "name": segment1.name, + "condition_json_path": "$[0].conditions[1]", + }, + }, + ], + } + assert not FeatureSegment.objects.filter(segment=segment2, feature=egg).exists() + assert log.has( + "dependencies.create_failed", + level="info", + organisation__id=organisation, + project__id=project, + environment__key=environment_api_key, + feature__name="egg", + prerequisite_feature__name="chicken", + ) + + +@pytest.mark.usefixtures("versioned_environment") +def test_update_segment_update_rules__circular_flag_dependency__responds_400( + admin_client: APIClient, + create_segment_override: CreateSegmentOverrideFixture, + environment_api_key: str, + environment_name: str, + log: StructuredLogCapture, + organisation: int, + project: int, +) -> None: + # Given + chicken = Feature.objects.create(name="chicken", project_id=project) + egg = Feature.objects.create(name="egg", project_id=project) + segment1 = Segment.objects.create(name="segment1", project_id=project) + segment2 = Segment.objects.create(name="segment2", project_id=project) + create_segment_override( + environment_api_key=environment_api_key, + feature_id=chicken.id, + segment_id=segment1.id, + ) + create_segment_override( + environment_api_key=environment_api_key, + feature_id=egg.id, + segment_id=segment2.id, + ) + SegmentFlagReference.objects.create( + segment=segment1, + prerequisite_feature=egg, + condition_json_path="$[0].conditions[1]", + ) + + # When + response = admin_client.put( + f"/api/v1/projects/{project}/segments/{segment2.id}/", + data={ + "name": "segment2", + "project": project, + "rules": [ + { + "type": "ALL", + "conditions": [ + { + "property": "$.flags.chicken.enabled", + "operator": "EQUAL", + "value": True, + }, + ], + } + ], + }, + format="json", + ) + + # Then + assert response.status_code == 400 + assert response.json() == { + "code": "circular_dependency", + "environment": {"key": environment_api_key, "name": environment_name}, + "path": [ + { + "feature": "egg", + "needs": "chicken", + "segment": { + "id": segment2.id, + "name": segment2.name, + "condition_json_path": "$[0].conditions[0]", + }, + }, + { + "feature": "chicken", + "needs": "egg", + "segment": { + "id": segment1.id, + "name": segment1.name, + "condition_json_path": "$[0].conditions[1]", + }, + }, + ], + } + assert not SegmentFlagReference.objects.filter(segment=segment2).exists() + assert log.has( + "dependencies.create_failed", + level="info", + organisation__id=organisation, + project__id=project, + environment__key=environment_api_key, + feature__name="egg", + prerequisite_feature__name="chicken", + ) + + +@pytest.mark.usefixtures("versioned_environment") +def test_update_segment_update_rules__cycle_in_another_environment__responds_400( + admin_client: APIClient, + create_segment_override: CreateSegmentOverrideFixture, + environment_api_key: str, + log: StructuredLogCapture, + organisation: int, + other_environment: Environment, + project: int, +) -> None: + # Given + chicken = Feature.objects.create(name="chicken", project_id=project) + egg = Feature.objects.create(name="egg", project_id=project) + segment1 = Segment.objects.create(name="segment1", project_id=project) + segment2 = Segment.objects.create(name="segment2", project_id=project) + SegmentFlagReference.objects.create( + segment=segment1, + prerequisite_feature=egg, + condition_json_path="$[0].conditions[0]", + ) + create_segment_override( + environment_api_key=environment_api_key, + feature_id=egg.id, + segment_id=segment2.id, + ) + create_segment_override( + environment_api_key=other_environment.api_key, + feature_id=egg.id, + segment_id=segment2.id, + ) + create_segment_override( + environment_api_key=other_environment.api_key, + feature_id=chicken.id, + segment_id=segment1.id, + ) + + # When + response = admin_client.put( + f"/api/v1/projects/{project}/segments/{segment2.id}/", + data={ + "name": "segment2", + "project": project, + "rules": [ + { + "type": "ALL", + "conditions": [ + { + "property": "$.flags.chicken.enabled", + "operator": "EQUAL", + "value": True, + }, + ], + } + ], + }, + format="json", + ) + + # Then + assert response.status_code == 400 + assert response.json() == { + "code": "circular_dependency", + "environment": { + "key": other_environment.api_key, + "name": other_environment.name, + }, + "path": [ + { + "feature": "egg", + "needs": "chicken", + "segment": { + "id": segment2.id, + "name": segment2.name, + "condition_json_path": "$[0].conditions[0]", + }, + }, + { + "feature": "chicken", + "needs": "egg", + "segment": { + "id": segment1.id, + "name": segment1.name, + "condition_json_path": "$[0].conditions[0]", + }, + }, + ], + } + assert not SegmentFlagReference.objects.filter(segment=segment2).exists() + assert log.has( + "dependencies.create_failed", + level="info", + organisation__id=organisation, + project__id=project, + environment__key=other_environment.api_key, + feature__name="egg", + prerequisite_feature__name="chicken", + ) + + +@pytest.mark.usefixtures("versioned_environment") +def test_update_segment_update_rules__longer_dependency_cycle_path__responds_400( + admin_client: APIClient, + create_segment_override: CreateSegmentOverrideFixture, + environment_api_key: str, + environment_name: str, + log: StructuredLogCapture, + organisation: int, + project: int, +) -> None: + # Given + chicken = Feature.objects.create(name="chicken", project_id=project) + egg = Feature.objects.create(name="egg", project_id=project) + rooster = Feature.objects.create(name="rooster", project_id=project) + segment1 = Segment.objects.create(name="segment1", project_id=project) + segment2 = Segment.objects.create(name="segment2", project_id=project) + segment3 = Segment.objects.create(name="segment3", project_id=project) + SegmentFlagReference.objects.create( + segment=segment1, + prerequisite_feature=egg, + condition_json_path="$[0].conditions[0]", + ) + create_segment_override( + environment_api_key=environment_api_key, + feature_id=chicken.id, + segment_id=segment1.id, + ) + SegmentFlagReference.objects.create( + segment=segment2, + prerequisite_feature=rooster, + condition_json_path="$[0].conditions[0]", + ) + create_segment_override( + environment_api_key=environment_api_key, + feature_id=egg.id, + segment_id=segment2.id, + ) + create_segment_override( + environment_api_key=environment_api_key, + feature_id=rooster.id, + segment_id=segment3.id, + ) + + # When + response = admin_client.put( + f"/api/v1/projects/{project}/segments/{segment3.id}/", + data={ + "name": "segment3", + "project": project, + "rules": [ + { + "type": "ALL", + "conditions": [ + { + "property": "$.flags.chicken.enabled", + "operator": "EQUAL", + "value": True, + }, + ], + } + ], + }, + format="json", + ) + + # Then + assert response.status_code == 400 + assert response.json() == { + "code": "circular_dependency", + "environment": {"key": environment_api_key, "name": environment_name}, + "path": [ + { + "feature": "rooster", + "needs": "chicken", + "segment": { + "id": segment3.id, + "name": segment3.name, + "condition_json_path": "$[0].conditions[0]", + }, + }, + { + "feature": "chicken", + "needs": "egg", + "segment": { + "id": segment1.id, + "name": segment1.name, + "condition_json_path": "$[0].conditions[0]", + }, + }, + { + "feature": "egg", + "needs": "rooster", + "segment": { + "id": segment2.id, + "name": segment2.name, + "condition_json_path": "$[0].conditions[0]", + }, + }, + ], + } + assert not SegmentFlagReference.objects.filter(segment=segment3).exists() + assert log.has( + "dependencies.create_failed", + level="info", + organisation__id=organisation, + project__id=project, + environment__key=environment_api_key, + feature__name="rooster", + prerequisite_feature__name="chicken", + ) + + +@pytest.mark.usefixtures("versioned_environment") +def test_update_segment_update_rules__shared_prerequisite__responds_200( + admin_client: APIClient, + create_segment_override: CreateSegmentOverrideFixture, + environment_api_key: str, + log: StructuredLogCapture, + organisation: int, + project: int, +) -> None: + # Given + chicken = Feature.objects.create(name="chicken", project_id=project) + egg = Feature.objects.create(name="egg", project_id=project) + other_segment = Segment.objects.create(name="other_segment", project_id=project) + segment = Segment.objects.create(name="segment", project_id=project) + SegmentFlagReference.objects.create( + segment=other_segment, + prerequisite_feature=egg, + condition_json_path="$[0].conditions[0]", + ) + create_segment_override( + environment_api_key=environment_api_key, + feature_id=chicken.id, + segment_id=other_segment.id, + priority=0, + ) + create_segment_override( + environment_api_key=environment_api_key, + feature_id=chicken.id, + segment_id=segment.id, + priority=1, + ) + + # When + response = admin_client.put( + f"/api/v1/projects/{project}/segments/{segment.id}/", + data={ + "name": "segment", + "project": project, + "rules": [ + { + "type": "ALL", + "conditions": [ + { + "property": "$.flags.egg.enabled", + "operator": "EQUAL", + "value": True, + }, + ], + } + ], + }, + format="json", + ) + + # Then + assert response.status_code == 200 + assert list( + SegmentFlagReference.objects.filter(segment=segment).values( + "prerequisite_feature", "condition_json_path" + ) + ) == [ + { + "prerequisite_feature": egg.id, + "condition_json_path": "$[0].conditions[0]", + }, + ] + assert log.has( + "dependencies.created", + level="info", + organisation__id=organisation, + project__id=project, + environment__key=environment_api_key, + feature__name="chicken", + prerequisite_feature__name="egg", + ) diff --git a/api/tests/integration/features/future/test_flag_endpoint.py b/api/tests/integration/features/future/test_flag_endpoint.py index 39af42a6b4cd..d1aa6e3a0bd6 100644 --- a/api/tests/integration/features/future/test_flag_endpoint.py +++ b/api/tests/integration/features/future/test_flag_endpoint.py @@ -12,21 +12,15 @@ from features.future.types import UpdateFlagRequest from features.models import FeatureState from features.versioning.models import EnvironmentFeatureVersion -from features.versioning.tasks import enable_v2_versioning from organisations.models import Organisation from tests.integration.helpers import create_mv_option_with_api from tests.types import WithEnvironmentPermissionsCallable from users.models import FFAdminUser -@pytest.fixture(params=["feature_versioning_v1", "feature_versioning_v2"], autouse=True) -def versioned_environment( - request: pytest.FixtureRequest, - environment: int, -) -> Environment: - if request.param == "feature_versioning_v2": - enable_v2_versioning(environment_id=environment) - return Environment.objects.get(id=environment) # type: ignore[no-any-return] +@pytest.fixture(autouse=True) +def versioned_environment(versioned_environment: Environment) -> Environment: + return versioned_environment @pytest.fixture() diff --git a/api/tests/types.py b/api/tests/types.py index 4547c3fbb146..ffbfd955bda5 100644 --- a/api/tests/types.py +++ b/api/tests/types.py @@ -44,3 +44,14 @@ def __call__(self, *feature_names: str) -> None: ... class MigratorFactory(Protocol): def __call__(self, name: Optional[str] = None) -> Migrator: ... + + +class CreateSegmentOverrideFixture(Protocol): + def __call__( + self, + environment_api_key: str, + feature_id: int, + segment_id: int, + enabled: bool = True, + priority: int | None = None, + ) -> None: ... diff --git a/api/tests/unit/features/dependencies/__init__.py b/api/tests/unit/features/dependencies/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/api/tests/unit/features/dependencies/test_mappers.py b/api/tests/unit/features/dependencies/test_mappers.py new file mode 100644 index 000000000000..1ff222affbfc --- /dev/null +++ b/api/tests/unit/features/dependencies/test_mappers.py @@ -0,0 +1,40 @@ +import pytest + +from features.dependencies.mappers import map_rules_to_prerequisite_feature_names +from segments.types import SegmentRule + + +@pytest.mark.parametrize( + "condition_property", + [ + "$.flags['unterminated", + "$.flags['flag_a','flag_b'].enabled", + "$.flags..enabled", + "$.traits.flags.enabled", + "$.flagsmith.enabled", + "$.flags", + ], +) +def test_map_rules_to_prerequisite_feature_names__invalid_prerequisite_jsonpath__ignores_property( + condition_property: str, +) -> None: + # Given + rules: list[SegmentRule] = [ + { + "type": "ALL", + "conditions": [ + { + "property": condition_property, + "operator": "EQUAL", + "value": "true", + "description": None, + } + ], + } + ] + + # When + feature_names_by_json_path = map_rules_to_prerequisite_feature_names(rules) + + # Then + assert feature_names_by_json_path == {} diff --git a/api/tests/unit/features/dependencies/test_services.py b/api/tests/unit/features/dependencies/test_services.py new file mode 100644 index 000000000000..6ccdc613bbb8 --- /dev/null +++ b/api/tests/unit/features/dependencies/test_services.py @@ -0,0 +1,63 @@ +import pytest +from pytest_django import DjangoAssertNumQueries + +from environments.models import Environment +from features.dependencies.models import SegmentFlagReference +from features.dependencies.services import validate_segment_flag_dependencies +from features.models import Feature, FeatureSegment, FeatureState +from projects.models import Project +from segments.models import Segment + + +@pytest.mark.parametrize("environment_count", [1, 2, 3]) +def test_validate_segment_flag_dependencies__overrides_across_environments__queries_once_per_environment( + django_assert_num_queries: DjangoAssertNumQueries, + environment_count: int, + project: Project, + segment: Segment, +) -> None: + # Given + for environment_index in range(environment_count): + environment = Environment.objects.create( + name=f"environment{environment_index}", project=project + ) + for feature_name in ["chicken", "egg", "hen"]: + feature, _ = Feature.objects.get_or_create( + name=feature_name, project=project + ) + feature_segment = FeatureSegment.objects.create( + feature=feature, segment=segment, environment=environment + ) + FeatureState.objects.create( + feature=feature, + environment=environment, + feature_segment=feature_segment, + ) + SegmentFlagReference.objects.create( + segment=segment, + prerequisite_feature=Feature.objects.create(name="corn", project=project), + condition_json_path="$[0].conditions[0]", + ) + + # When / Then + with django_assert_num_queries(2 + environment_count): + validate_segment_flag_dependencies(segment) + + +def test_validate_segment_flag_dependencies__no_references__skips( + django_assert_num_queries: DjangoAssertNumQueries, + environment: Environment, + feature: Feature, + segment: Segment, +) -> None: + # Given + feature_segment = FeatureSegment.objects.create( + feature=feature, segment=segment, environment=environment + ) + FeatureState.objects.create( + feature=feature, environment=environment, feature_segment=feature_segment + ) + + # When / Then + with django_assert_num_queries(1): + validate_segment_flag_dependencies(segment) diff --git a/api/tests/unit/segments/test_unit_segments_services.py b/api/tests/unit/segments/test_unit_segments_services.py index fdde18c7f2e9..93c0be9e3389 100644 --- a/api/tests/unit/segments/test_unit_segments_services.py +++ b/api/tests/unit/segments/test_unit_segments_services.py @@ -187,8 +187,8 @@ def test_delete_segment__varying_segment_sizes__query_count_is_constant( large_query_count = len(ctx_large.captured_queries) # Then - # 11 for the delete, 15 for the audit log task (runs synchronously in tests) - assert small_query_count == large_query_count == 26 + # 13 for the delete, 15 for the audit log task (runs synchronously in tests) + assert small_query_count == large_query_count == 28 # TODO: Delete as per https://github.com/Flagsmith/flagsmith/issues/7818 diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index 6185878c46b4..79394941051d 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -269,6 +269,43 @@ Attributes: - `environment.id` - `organisation.id` +### `features.dependencies.create_failed` + +Logged at `info` from: + - `api/features/dependencies/services.py:131` + +Attributes: + - `environment.key` + - `feature.name` + - `organisation.id` + - `prerequisite_feature.name` + - `project.id` + +### `features.dependencies.created` + +Logged at `info` from: + - `api/features/dependencies/services.py:103` + +Attributes: + - `environment.key` + - `feature.name` + - `organisation.id` + - `prerequisite_feature.name` + - `project.id` + +### `features.dependencies.deleted` + +Logged at `info` from: + - `api/features/dependencies/services.py:75` + - `api/features/dependencies/services.py:101` + +Attributes: + - `environment.key` + - `feature.name` + - `organisation.id` + - `prerequisite_feature.name` + - `project.id` + ### `features.flag.update_rejected` Logged at `warning` from: @@ -284,8 +321,8 @@ Attributes: ### `features.flag.updated` Logged at `info` from: - - `api/features/future/services.py:334` - - `api/features/future/services.py:373` + - `api/features/future/services.py:365` + - `api/features/future/services.py:414` Attributes: - `environment.id` @@ -650,7 +687,7 @@ Attributes: ### `segments.serializers.segment_revision_created` Logged at `info` from: - - `api/segments/serializers.py:231` + - `api/segments/serializers.py:237` Attributes: - `revision_id`