fix(versioning): prevent deleted segment overrides returning (#3850)

api/conftest.py

@@ -364,6 +364,7 @@ def _with_project_permissions(
 @pytest.fixture()
 def environment_v2_versioning(environment):
     enable_v2_versioning(environment.id)
+    environment.refresh_from_db()
     return environment
 
 

api/features/managers.py

@@ -8,6 +8,8 @@
 from ordered_model.models import OrderedModelManager
 from softdelete.models import SoftDeleteManager
 
+from features.versioning.models import EnvironmentFeatureVersion
+
 if typing.TYPE_CHECKING:
     from environments.models import Environment
     from features.models import FeatureState
@@ -31,11 +33,11 @@ def get_live_feature_states(
 
         qs_filter = Q(environment=environment, deleted_at__isnull=True)
         if environment.use_v2_feature_versioning:
-            qs_filter &= Q(
-                environment_feature_version__isnull=False,
-                environment_feature_version__published_at__isnull=False,
-                environment_feature_version__live_from__lte=now,
+            latest_versions = EnvironmentFeatureVersion.objects.get_latest_versions(
+                environment
             )
+            latest_version_uuids = [efv.uuid for efv in latest_versions]
+            qs_filter &= Q(environment_feature_version__uuid__in=latest_version_uuids)
         else:
             qs_filter &= Q(
                 live_from__isnull=False,

api/features/versioning/managers.py

@@ -0,0 +1,23 @@
+import typing
+from pathlib import Path
+
+from django.db.models.query import RawQuerySet
+from softdelete.models import SoftDeleteManager
+
+if typing.TYPE_CHECKING:
+    from environments.models import Environment
+
+
+with open(Path(__file__).parent.resolve() / "sql/get_latest_versions.sql") as f:
+    get_latest_versions_sql = f.read()
+
+
+class EnvironmentFeatureVersionManager(SoftDeleteManager):
+    def get_latest_versions(self, environment: "Environment") -> RawQuerySet:
+        """
+        Get the latest EnvironmentFeatureVersion objects
+        for a given environment.
+        """
+        return self.raw(
+            get_latest_versions_sql, params={"environment_id": environment.id}
+        )

api/features/versioning/models.py

@@ -12,6 +12,7 @@
 from django.utils import timezone
 
 from features.versioning.exceptions import FeatureVersioningError
+from features.versioning.managers import EnvironmentFeatureVersionManager
 from features.versioning.signals import environment_feature_version_published
 
 if typing.TYPE_CHECKING:
@@ -61,6 +62,8 @@ class EnvironmentFeatureVersion(
         blank=True,
     )
 
+    objects = EnvironmentFeatureVersionManager()
+
     class Meta:
         indexes = [Index(fields=("environment", "feature"))]
         ordering = ("-live_from",)

api/features/versioning/sql/get_latest_versions.sql

@@ -0,0 +1,25 @@
+select
+	efv1."uuid",
+	efv1."published_at",
+	efv1."live_from"
+from
+	feature_versioning_environmentfeatureversion efv1
+join (
+	select
+		efv2."feature_id",
+		efv2."environment_id",
+		MAX(efv2."live_from") as "latest_release"
+	from
+		feature_versioning_environmentfeatureversion efv2
+	where
+		efv2."deleted_at" is null
+		and efv2."published_at" is not null
+	group by
+		efv2."feature_id",
+		efv2."environment_id"
+) latest_release_dates on
+	efv1."feature_id" = latest_release_dates."feature_id"
+	and efv1."environment_id" = latest_release_dates."environment_id"
+	and efv1."live_from" = latest_release_dates."latest_release"
+where
+	efv1.environment_id = %(environment_id)s;

api/features/versioning/versioning_service.py

@@ -14,6 +14,9 @@ def get_environment_flags_queryset(
     """
     Get a queryset of the latest live versions of an environments' feature states
     """
+    if environment.use_v2_feature_versioning:
+        return _get_feature_states_queryset(environment, feature_name)
+
     feature_states_list = get_environment_flags_list(environment, feature_name)
     return FeatureState.objects.filter(id__in=[fs.id for fs in feature_states_list])
 
@@ -36,26 +39,16 @@ def get_environment_flags_list(
     feature states. The logic to grab the latest version is then handled in python
     by building a dictionary. Returns a list of FeatureState objects.
     """
-    additional_select_related_args = additional_select_related_args or tuple()
-    additional_prefetch_related_args = additional_prefetch_related_args or tuple()
-
-    feature_states = (
-        FeatureState.objects.get_live_feature_states(
-            environment=environment, additional_filters=additional_filters
-        )
-        .select_related(
-            "environment",
-            "feature",
-            "feature_state_value",
-            "environment_feature_version",
-            "feature_segment",
-            *additional_select_related_args,
-        )
-        .prefetch_related(*additional_prefetch_related_args)
+    feature_states = _get_feature_states_queryset(
+        environment,
+        feature_name,
+        additional_filters,
+        additional_select_related_args,
+        additional_prefetch_related_args,
     )
 
-    if feature_name:
-        feature_states = feature_states.filter(feature__name__iexact=feature_name)
+    if environment.use_v2_feature_versioning:
+        return list(feature_states)
 
     # Build up a dictionary in the form
     # {(feature_id, feature_segment_id, identity_id): feature_state}
@@ -87,3 +80,36 @@ def get_current_live_environment_feature_version(
         .order_by("-live_from")
         .first()
     )
+
+
+def _get_feature_states_queryset(
+    environment: "Environment",
+    feature_name: str = None,
+    additional_filters: Q = None,
+    additional_select_related_args: typing.Iterable[str] = None,
+    additional_prefetch_related_args: typing.Iterable[
+        typing.Union[str, Prefetch]
+    ] = None,
+) -> QuerySet[FeatureState]:
+    additional_select_related_args = additional_select_related_args or tuple()
+    additional_prefetch_related_args = additional_prefetch_related_args or tuple()
+
+    queryset = (
+        FeatureState.objects.get_live_feature_states(
+            environment=environment, additional_filters=additional_filters
+        )
+        .select_related(
+            "environment",
+            "feature",
+            "feature_state_value",
+            "environment_feature_version",
+            "feature_segment",
+            *additional_select_related_args,
+        )
+        .prefetch_related(*additional_prefetch_related_args)
+    )
+
+    if feature_name:
+        queryset = queryset.filter(feature__name__iexact=feature_name)
+
+    return queryset

api/tests/unit/features/conftest.py

@@ -1,13 +1,7 @@
-import typing
-
 import pytest
 
-from environments.models import Environment
 from features.models import FeatureState
 
-if typing.TYPE_CHECKING:
-    from projects.models import Project
-
 
 @pytest.fixture()
 def feature_state_version_generator(environment, feature, request):
@@ -32,10 +26,3 @@ def feature_state_version_generator(environment, feature, request):
         ),
         expected_result,
     )
-
-
-@pytest.fixture()
-def environment_v2_versioning(project: "Project") -> Environment:
-    return Environment.objects.create(
-        name="v2_versioning", project=project, use_v2_feature_versioning=True
-    )

api/tests/unit/features/test_unit_features_views.py

@@ -1901,6 +1901,59 @@ def test_list_features_provides_information_on_number_of_overrides(
     assert response_json["results"][0]["num_identity_overrides"] == 1
 
 
+def test_list_features_provides_correct_information_on_number_of_overrides_based_on_version(
+    feature: Feature,
+    segment: Segment,
+    project: Project,
+    environment_v2_versioning: Environment,
+    admin_client_new: APIClient,
+    admin_user: FFAdminUser,
+):
+    # Given
+    url = "%s?environment=%d" % (
+        reverse("api-v1:projects:project-features-list", args=[project.id]),
+        environment_v2_versioning.id,
+    )
+
+    # let's create a new version with a segment override
+    version_2 = EnvironmentFeatureVersion.objects.create(
+        environment=environment_v2_versioning, feature=feature
+    )
+    FeatureState.objects.create(
+        feature=feature,
+        environment=environment_v2_versioning,
+        feature_segment=FeatureSegment.objects.create(
+            feature=feature,
+            environment=environment_v2_versioning,
+            segment=segment,
+        ),
+        environment_feature_version=version_2,
+    )
+    version_2.publish(admin_user)
+
+    # and now let's create a new version which removes the segment override
+    version_3 = EnvironmentFeatureVersion.objects.create(
+        environment=environment_v2_versioning, feature=feature
+    )
+    FeatureState.objects.filter(
+        environment_feature_version=version_3,
+        feature_segment__segment=segment,
+    ).delete()
+    version_3.publish(admin_user)
+
+    # When
+    response = admin_client_new.get(url)
+
+    # Then
+    assert response.status_code == status.HTTP_200_OK
+
+    response_json = response.json()
+    assert response_json["count"] == 1
+
+    # The number of segment overrides in the latest version should be 0
+    assert response_json["results"][0]["num_segment_overrides"] == 0
+
+
 def test_list_features_provides_segment_overrides_for_dynamo_enabled_project(
     dynamo_enabled_project: Project,
     dynamo_enabled_project_environment_one: Environment,
@@ -2915,7 +2968,7 @@ def test_feature_list_last_modified_values(
         Feature.objects.create(name=f"feature_{i}", project=project)
 
     # When
-    with django_assert_num_queries(16):  # TODO: reduce this number of queries!
+    with django_assert_num_queries(18):  # TODO: reduce this number of queries!
         response = staff_client.get(url)
 
     # Then

api/tests/unit/features/versioning/test_unit_versioning_versioning_service.py

@@ -13,6 +13,7 @@
     get_environment_flags_queryset,
 )
 from projects.models import Project
+from segments.models import Segment
 from users.models import FFAdminUser
 
 
@@ -114,7 +115,7 @@ def test_get_environment_flags_v2_versioning_returns_latest_live_versions_of_fea
     environment_v2_versioning: Environment,
     feature: Feature,
     admin_user: FFAdminUser,
-):
+) -> None:
     # Given
     # a second feature with its corresponding environment feature version
     feature_2 = Feature.objects.create(name="feature_2", project=project)
@@ -150,6 +151,47 @@ def test_get_environment_flags_v2_versioning_returns_latest_live_versions_of_fea
     }
 
 
+def test_get_environment_flags_v2_versioning_does_not_return_removed_segment_override(
+    project: Project,
+    feature: Feature,
+    admin_user: FFAdminUser,
+    segment: Segment,
+    segment_featurestate: FeatureState,
+    environment_v2_versioning: Environment,
+) -> None:
+    # Given
+    # The initial version has a segment override
+    initial_version = EnvironmentFeatureVersion.objects.get(
+        environment=environment_v2_versioning, feature=feature
+    )
+    assert FeatureState.objects.filter(
+        feature=feature,
+        environment=environment_v2_versioning,
+        feature_segment__segment=segment,
+        environment_feature_version=initial_version,
+    ).exists()
+
+    # Now let's create a new version, remove the segment override and publish the version
+    new_version = EnvironmentFeatureVersion.objects.create(
+        environment=environment_v2_versioning, feature=feature
+    )
+    FeatureState.objects.filter(
+        feature=feature,
+        environment=environment_v2_versioning,
+        feature_segment__segment=segment,
+        environment_feature_version=new_version,
+    ).delete()
+    new_version.publish(published_by=admin_user)
+
+    # When
+    environment_feature_states = get_environment_flags_list(
+        environment=environment_v2_versioning,
+    )
+
+    # Then
+    assert len(environment_feature_states) == 1
+
+
 def test_get_current_live_environment_feature_version(
     environment_v2_versioning: Environment, staff_user: FFAdminUser, feature: Feature
 ) -> None: