fix(Segments): Improve and consolidate service for fetching overrides

api/features/versioning/managers.py

@@ -1,8 +1,7 @@
 import typing
-from datetime import datetime
 from pathlib import Path
 
-from django.db.models import OuterRef
+from django.db.models import Exists, OuterRef
 from django.db.models.query import QuerySet, RawQuerySet
 from django.utils import timezone
 from softdelete.models import SoftDeleteManager  # type: ignore[import-untyped]
@@ -16,21 +15,17 @@
 
 
 class EnvironmentFeatureVersionManager(SoftDeleteManager):  # type: ignore[misc]
-    def get_versions_live_since(
-        self,
-        feature_id: int | OuterRef,
-        environment_id: int | OuterRef,
-        live_from: datetime | OuterRef,
-    ) -> QuerySet["EnvironmentFeatureVersion"]:
-        """
-        Get the published versions of a flag that went live between the provided `live_from` and now.
-        """
-        return self.filter(  # type: ignore[no-any-return]
-            feature_id=feature_id,
-            environment_id=environment_id,
+    def get_live_or_scheduled(self) -> QuerySet["EnvironmentFeatureVersion"]:
+        """Get the published versions that are live now or scheduled to go live."""
+        superseding_versions = self.filter(
+            feature_id=OuterRef("feature_id"),
+            environment_id=OuterRef("environment_id"),
             published_at__isnull=False,
+            live_from__gt=OuterRef("live_from"),
             live_from__lte=timezone.now(),
-            live_from__gt=live_from,
+        )
+        return self.filter(published_at__isnull=False).exclude(  # type: ignore[no-any-return]
+            Exists(superseding_versions)
         )
 
     def get_latest_versions_by_environment_id(self, environment_id: int) -> RawQuerySet:  # type: ignore[type-arg]

api/segments/serializers.py

@@ -16,7 +16,7 @@
 from segment_membership.models import SegmentMembershipCount
 from segment_membership.services import enqueue_membership_refresh
 from segments.models import Condition, Segment, SegmentRule, WhitelistedSegment
-from segments.services import get_overrides_in_effect
+from segments.services import get_all_live_or_scheduled_overrides
 from segments.types import (
     LegacySegmentRule,
 )
@@ -191,7 +191,7 @@ def get_has_overrides(self, segment: Segment) -> bool:
         # is serialized outside that queryset.
         if (has_overrides := getattr(segment, "has_overrides", None)) is not None:
             return bool(has_overrides)
-        return get_overrides_in_effect().filter(segment=segment).exists()
+        return get_all_live_or_scheduled_overrides().filter(segment=segment).exists()
 
     def to_internal_value(self, data: dict[str, Any]) -> Any:
         self._validate_rules_depth(data.get("rules", []))

api/segments/services.py

@@ -6,43 +6,34 @@
 from django.utils import timezone
 
 from core.dataclasses import AuthorData
-from features.models import FeatureSegment
+from features.models import FeatureSegment, FeatureState
 from features.versioning.models import EnvironmentFeatureVersion
 
 if typing.TYPE_CHECKING:
     from segments.models import Segment
 
 
-def get_overrides_in_effect() -> "QuerySet[FeatureSegment]":
-    """
-    Get the feature overrides that are live now or scheduled to go live.
-
-    Returns a global queryset; narrow the result down with additional filters.
-    """
-    # Without v2 versioning, a feature segment is the current state, so it
-    # counts unless every feature state on it is held by an open change request.
-    without_v2_versioning = models.Q(
-        models.Q(environment__use_v2_feature_versioning=False),
-        models.Q(feature_states__change_request__isnull=True)
-        | models.Q(feature_states__change_request__committed_at__isnull=False),
-    )
+def get_all_live_or_scheduled_overrides() -> "QuerySet[FeatureSegment]":
+    """Get the feature overrides that are live now or scheduled to go live."""
+    no_change_request = models.Q(change_request__isnull=True)
+    committed_change_request = models.Q(change_request__committed_at__isnull=False)
+    with_feature_versioning_v1 = models.Q(
+        environment__use_v2_feature_versioning=False,
+    ) & (no_change_request | committed_change_request)
 
-    # With v2 versioning, a published version counts unless another published
-    # version has gone live since, which covers both the version that is live
-    # now and any version scheduled to go live later.
-    with_v2_versioning = models.Q(
+    with_feature_versioning_v2 = models.Q(
         environment__use_v2_feature_versioning=True,
-        environment_feature_version__published_at__isnull=False,
-    ) & ~models.Exists(
-        EnvironmentFeatureVersion.objects.get_versions_live_since(
-            feature_id=models.OuterRef("feature_id"),
-            environment_id=models.OuterRef("environment_id"),
-            live_from=models.OuterRef("environment_feature_version__live_from"),
-        )
+        environment_feature_version__in=(
+            EnvironmentFeatureVersion.objects.get_live_or_scheduled()
+        ),
     )
 
+    live_or_scheduled_feature_states = FeatureState.objects.filter(
+        with_feature_versioning_v1 | with_feature_versioning_v2,
+        feature_segment_id=models.OuterRef("pk"),
+    )
     return FeatureSegment.objects.filter(  # type: ignore[no-any-return]
-        without_v2_versioning | with_v2_versioning
+        models.Exists(live_or_scheduled_feature_states)
     )
 
 

api/segments/views.py

@@ -46,7 +46,7 @@
     SegmentMembersResponseSerializer,
     SegmentSerializer,
 )
-from .services import delete_segment, get_overrides_in_effect
+from .services import delete_segment, get_all_live_or_scheduled_overrides
 
 if TYPE_CHECKING:
     from users.models import FFAdminUser
@@ -106,7 +106,9 @@ def get_queryset(self):  # type: ignore[no-untyped-def]
             project=project, is_system_segment=False
         ).annotate(
             has_overrides=models.Exists(
-                get_overrides_in_effect().filter(segment_id=models.OuterRef("pk"))
+                get_all_live_or_scheduled_overrides().filter(
+                    segment_id=models.OuterRef("pk")
+                )
             )
         )
 
@@ -268,7 +270,7 @@ def _check_segment_is_deletable(self, segment: Segment) -> None:
         """
         if not segment.project.is_workflow_enabled:
             return
-        if not get_overrides_in_effect().filter(segment=segment).exists():
+        if not get_all_live_or_scheduled_overrides().filter(segment=segment).exists():
             return
         api_error = ChangeRequestsEnabledError(
             "Cannot delete a segment with feature overrides in a project with "

api/tests/unit/segments/test_unit_segments_services.py

@@ -1,7 +1,10 @@
+from datetime import timedelta
 from typing import cast
 
+import pytest
 from django.db import connection, reset_queries
 from django.test.utils import CaptureQueriesContext
+from django.utils import timezone
 from flag_engine.segments.constants import EQUAL
 
 from api_keys.models import MasterAPIKey
@@ -11,10 +14,12 @@
 from core.dataclasses import AuthorData
 from environments.models import Environment
 from features.models import Feature, FeatureSegment, FeatureState
+from features.versioning.models import EnvironmentFeatureVersion
+from features.workflows.core.models import ChangeRequest
 from organisations.models import Organisation
 from projects.models import Project
 from segments.models import Condition, Segment, SegmentRule
-from segments.services import delete_segment
+from segments.services import delete_segment, get_all_live_or_scheduled_overrides
 from users.models import FFAdminUser
 
 
@@ -363,3 +368,187 @@ def test_copy_rules_and_conditions_from__varying_segment_sizes__query_count_is_c
 
     # Then - query count should be the same (O(depth) not O(n))
     assert small_query_count == large_query_count == 10
+
+
+def test_get_all_live_or_scheduled_overrides__feature_versioning_v1_uncommitted_change_request__returns_no_overrides(
+    feature_segment: FeatureSegment,
+    feature: Feature,
+    environment: Environment,
+    change_request: ChangeRequest,
+) -> None:
+    # Given
+    FeatureState.objects.create(
+        feature_segment=feature_segment,
+        feature=feature,
+        environment=environment,
+        change_request=change_request,
+        version=None,
+    )
+
+    # When
+    overrides = list(get_all_live_or_scheduled_overrides())
+
+    # Then
+    assert overrides == []
+
+
+@pytest.mark.usefixtures("segment_featurestate")
+def test_get_all_live_or_scheduled_overrides__feature_versioning_v1_committed_change_request__returns_distinct_overrides(
+    feature_segment: FeatureSegment,
+    feature: Feature,
+    environment: Environment,
+    change_request: ChangeRequest,
+    admin_user: FFAdminUser,
+) -> None:
+    # Given
+    FeatureState.objects.create(
+        feature_segment=feature_segment,
+        feature=feature,
+        environment=environment,
+        change_request=change_request,
+        version=None,
+    )
+    change_request.commit(admin_user)
+
+    # When
+    overrides = list(get_all_live_or_scheduled_overrides())
+
+    # Then
+    assert overrides == [feature_segment]
+
+
+@pytest.mark.usefixtures("segment_featurestate")
+def test_get_all_live_or_scheduled_overrides__feature_versioning_v1_scheduled_feature_change__returns_distinct_overrides(
+    feature_segment: FeatureSegment,
+    feature: Feature,
+    environment: Environment,
+    change_request: ChangeRequest,
+    admin_user: FFAdminUser,
+) -> None:
+    # Given
+    FeatureState.objects.create(
+        feature_segment=feature_segment,
+        feature=feature,
+        environment=environment,
+        change_request=change_request,
+        live_from=timezone.now() + timedelta(days=1),
+        version=None,
+    )
+    change_request.commit(admin_user)
+
+    # When
+    overrides = list(get_all_live_or_scheduled_overrides())
+
+    # Then
+    assert overrides == [feature_segment]
+
+
+def test_get_all_live_or_scheduled_overrides__feature_versioning_v2_uncommitted_change_request__returns_no_overrides(
+    environment_v2_versioning: Environment,
+    feature: Feature,
+    segment: Segment,
+    change_request: ChangeRequest,
+) -> None:
+    # Given
+    version = EnvironmentFeatureVersion.objects.create(
+        environment=environment_v2_versioning,
+        feature=feature,
+        change_request=change_request,
+    )
+    FeatureState.objects.create(
+        feature_segment=FeatureSegment.objects.create(
+            feature=feature,
+            segment=segment,
+            environment=environment_v2_versioning,
+            environment_feature_version=version,
+        ),
+        feature=feature,
+        environment=environment_v2_versioning,
+        environment_feature_version=version,
+    )
+
+    # When
+    overrides = list(get_all_live_or_scheduled_overrides())
+
+    # Then
+    assert overrides == []
+
+
+def test_get_all_live_or_scheduled_overrides__feature_versioning_v2_committed_change_request__returns_distinct_overrides(
+    environment_v2_versioning: Environment,
+    feature: Feature,
+    segment: Segment,
+    change_request: ChangeRequest,
+    admin_user: FFAdminUser,
+) -> None:
+    # Given
+    live_version = EnvironmentFeatureVersion.objects.get(
+        environment=environment_v2_versioning, feature=feature
+    )
+    live_version.live_from = timezone.now() - timedelta(days=1)
+    live_version.save()
+    FeatureState.objects.create(
+        feature_segment=FeatureSegment.objects.create(
+            feature=feature,
+            segment=segment,
+            environment=environment_v2_versioning,
+            environment_feature_version=live_version,
+        ),
+        feature=feature,
+        environment=environment_v2_versioning,
+        environment_feature_version=live_version,
+    )
+    version = EnvironmentFeatureVersion.objects.create(
+        environment=environment_v2_versioning,
+        feature=feature,
+        change_request=change_request,
+    )
+    committed_override = FeatureSegment.objects.get(environment_feature_version=version)
+    change_request.commit(admin_user)
+
+    # When
+    overrides = list(get_all_live_or_scheduled_overrides())
+
+    # Then
+    assert overrides == [committed_override]
+
+
+def test_get_all_live_or_scheduled_overrides__feature_versioning_v2_scheduled_feature_change__returns_distinct_overrides(
+    environment_v2_versioning: Environment,
+    feature: Feature,
+    segment: Segment,
+    change_request: ChangeRequest,
+    admin_user: FFAdminUser,
+) -> None:
+    # Given
+    live_version = EnvironmentFeatureVersion.objects.get(
+        environment=environment_v2_versioning, feature=feature
+    )
+    live_override = FeatureSegment.objects.create(
+        feature=feature,
+        segment=segment,
+        environment=environment_v2_versioning,
+        environment_feature_version=live_version,
+    )
+    FeatureState.objects.create(
+        feature_segment=live_override,
+        feature=feature,
+        environment=environment_v2_versioning,
+        environment_feature_version=live_version,
+    )
+    scheduled_version = EnvironmentFeatureVersion.objects.create(
+        environment=environment_v2_versioning,
+        feature=feature,
+        change_request=change_request,
+        live_from=timezone.now() + timedelta(days=1),
+    )
+    scheduled_override = FeatureSegment.objects.get(
+        environment_feature_version=scheduled_version
+    )
+    change_request.commit(admin_user)
+
+    # When
+    overrides = list(get_all_live_or_scheduled_overrides().order_by("id"))
+
+    # Then
+    assert overrides == [live_override, scheduled_override]

docs/docs/deployment-self-hosting/observability/_events-catalogue.md

@@ -639,7 +639,7 @@ Attributes:
 ### `segments.delete_rejected`
 
 Logged at `warning` from:
- - `api/segments/views.py:277`
+ - `api/segments/views.py:279`
 
 Attributes:
  - `organisation.id`
@@ -659,7 +659,7 @@ Attributes:
 ### `segments.update_rejected`
 
 Logged at `warning` from:
- - `api/segments/views.py:253`
+ - `api/segments/views.py:255`
 
 Attributes:
  - `organisation.id`