From 302c848ab399406029319682712844c35eab0f16 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 8 May 2024 12:03:17 +0100 Subject: [PATCH 1/7] Add endpoint to revert v2 versioning --- api/environments/models.py | 6 --- api/environments/views.py | 17 +++++++- api/features/versioning/tasks.py | 21 +++++++++- .../test_integration_v2_versioning.py | 41 ++++++++++++++++++- 4 files changed, 76 insertions(+), 9 deletions(-) diff --git a/api/environments/models.py b/api/environments/models.py index d4f9982408e4..2cec24c57594 100644 --- a/api/environments/models.py +++ b/api/environments/models.py @@ -19,7 +19,6 @@ AFTER_DELETE, AFTER_SAVE, AFTER_UPDATE, - BEFORE_UPDATE, LifecycleModel, hook, ) @@ -44,7 +43,6 @@ from environments.exceptions import EnvironmentHeaderNotPresentError from environments.managers import EnvironmentManager from features.models import Feature, FeatureSegment, FeatureState -from features.versioning.exceptions import FeatureVersioningError from metadata.models import Metadata from projects.models import IdentityOverridesV2MigrationStatus, Project from segments.models import Segment @@ -150,10 +148,6 @@ def clear_environment_cache(self): # TODO: this could rebuild the cache itself (using an async task) environment_cache.delete(self.initial_value("api_key")) - @hook(BEFORE_UPDATE, when="use_v2_feature_versioning", was=True, is_now=False) - def validate_use_v2_feature_versioning(self): - raise FeatureVersioningError("Cannot revert from v2 feature versioning.") - @hook(AFTER_DELETE) def delete_from_dynamo(self): if self.project.enable_dynamo_db and environment_wrapper.is_enabled: diff --git a/api/environments/views.py b/api/environments/views.py index 14815b13fdc5..8dcfd71b7e6e 100644 --- a/api/environments/views.py +++ b/api/environments/views.py @@ -17,7 +17,10 @@ NestedEnvironmentPermissions, ) from environments.sdk.schemas import SDKEnvironmentDocumentModel -from features.versioning.tasks import enable_v2_versioning +from features.versioning.tasks import ( + disable_v2_versioning, + enable_v2_versioning, +) from permissions.permissions_calculator import get_environment_permission_data from permissions.serializers import ( PermissionModelSerializer, @@ -216,6 +219,18 @@ def enable_v2_versioning(self, request: Request, api_key: str) -> Response: enable_v2_versioning.delay(kwargs={"environment_id": environment.id}) return Response(status=status.HTTP_202_ACCEPTED) + @swagger_auto_schema(request_body=no_body, responses={202: ""}) + @action(detail=True, methods=["POST"], url_path="disable-v2-versioning") + def disable_v2_versioning(self, request: Request, api_key: str) -> Response: + environment = self.get_object() + if environment.use_v2_feature_versioning is False: + return Response( + status=status.HTTP_400_BAD_REQUEST, + data={"detail": "Environment is not using v2 versioning."}, + ) + disable_v2_versioning.delay(kwargs={"environment_id": environment.id}) + return Response(status=status.HTTP_202_ACCEPTED) + class NestedEnvironmentViewSet(viewsets.GenericViewSet): model_class = None diff --git a/api/features/versioning/tasks.py b/api/features/versioning/tasks.py index 57c8f2c3b932..204eb0928dbb 100644 --- a/api/features/versioning/tasks.py +++ b/api/features/versioning/tasks.py @@ -24,7 +24,7 @@ @register_task_handler() -def enable_v2_versioning(environment_id: int): +def enable_v2_versioning(environment_id: int) -> None: from environments.models import Environment environment = Environment.objects.get(id=environment_id) @@ -35,6 +35,25 @@ def enable_v2_versioning(environment_id: int): environment.save() +@register_task_handler() +def disable_v2_versioning(environment_id: int) -> None: + from environments.models import Environment + from features.models import FeatureState + + environment = Environment.objects.get(id=environment_id) + + queryset = get_environment_flags_queryset(environment) + + FeatureState.objects.filter(identity_id__isnull=True).exclude( + id__in=[fs.id for fs in queryset] + ).delete() + + queryset.update(version=1, live_from=timezone.now()) + + environment.use_v2_feature_versioning = False + environment.save() + + def _create_initial_feature_versions(environment: "Environment"): from features.models import Feature, FeatureSegment diff --git a/api/tests/integration/features/versioning/test_integration_v2_versioning.py b/api/tests/integration/features/versioning/test_integration_v2_versioning.py index c1f512f7e956..6fec8a5c0018 100644 --- a/api/tests/integration/features/versioning/test_integration_v2_versioning.py +++ b/api/tests/integration/features/versioning/test_integration_v2_versioning.py @@ -196,7 +196,7 @@ def verify_consistent_responses(num_expected_flags: int) -> None: # still behave the same verify_consistent_responses(num_expected_flags=2) - # finally, let's publish the new version and verify that we get a different response + # now, let's publish the new version and verify that we get a different response publish_ef_version_url = reverse( "api-v1:versioning:environment-feature-versions-publish", args=[environment, feature, ef_version_uuid], @@ -237,6 +237,45 @@ def verify_consistent_responses(num_expected_flags: int) -> None: == "v2-segment-override-value" ) + # finally, let's test that we can revert the v2 versioning, and we still get the + # same response + disable_versioning_url = reverse( + "api-v1:environments:environment-disable-v2-versioning", + args=[environment_api_key], + ) + environment_update_response = admin_client.post(disable_versioning_url) + assert environment_update_response.status_code == status.HTTP_202_ACCEPTED + + time.sleep(0.5) + + environment_flags_response_after_revert = get_environment_flags_response_json( + num_expected_flags=2 + ) + identity_flags_response_after_revert = get_identity_flags_response_json( + num_expected_flags=2 + ) + + # Verify that the environment flags have the same state / value + environment_flag_tuples_pre_revert = { + (f["enabled"], f["feature_state_value"], f["feature"]["id"]) + for f in environment_flags_response_json_after_publish + } + environment_flag_tuples_post_revert = { + (f["enabled"], f["feature_state_value"], f["feature"]["id"]) + for f in environment_flags_response_after_revert + } + assert environment_flag_tuples_pre_revert == environment_flag_tuples_post_revert + + identity_flag_tuples_pre_revert = { + (f["enabled"], f["feature_state_value"], f["feature"]["id"]) + for f in identity_flags_response_json_after_publish["flags"] + } + identity_flag_tuples_post_revert = { + (f["enabled"], f["feature_state_value"], f["feature"]["id"]) + for f in identity_flags_response_after_revert["flags"] + } + assert identity_flag_tuples_pre_revert == identity_flag_tuples_post_revert + def test_v2_versioning_mv_feature( admin_client: "APIClient", From 53d35d05c3b7451898915c0c73f4350f053b9613 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 8 May 2024 15:25:05 +0100 Subject: [PATCH 2/7] Clear environment feature versions --- api/features/versioning/tasks.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/api/features/versioning/tasks.py b/api/features/versioning/tasks.py index 204eb0928dbb..43e0a8b35d49 100644 --- a/api/features/versioning/tasks.py +++ b/api/features/versioning/tasks.py @@ -39,16 +39,24 @@ def enable_v2_versioning(environment_id: int) -> None: def disable_v2_versioning(environment_id: int) -> None: from environments.models import Environment from features.models import FeatureState + from features.versioning.models import EnvironmentFeatureVersion environment = Environment.objects.get(id=environment_id) - queryset = get_environment_flags_queryset(environment) + latest_feature_states = get_environment_flags_queryset(environment) + # delete any feature states associated with older versions FeatureState.objects.filter(identity_id__isnull=True).exclude( - id__in=[fs.id for fs in queryset] + id__in=[fs.id for fs in latest_feature_states] ).delete() - queryset.update(version=1, live_from=timezone.now()) + # update the latest feature states to be the latest version according + # to the old versioning system + latest_feature_states.update( + version=1, live_from=timezone.now(), environment_feature_version=None + ) + + EnvironmentFeatureVersion.objects.filter(environment=environment).delete() environment.use_v2_feature_versioning = False environment.save() From 5b830e838105d63fb60e4e62365527e5d75fd23c Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Thu, 9 May 2024 11:02:57 +0100 Subject: [PATCH 3/7] Fix test --- api/features/versioning/tasks.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/api/features/versioning/tasks.py b/api/features/versioning/tasks.py index 43e0a8b35d49..c3b7c1d581da 100644 --- a/api/features/versioning/tasks.py +++ b/api/features/versioning/tasks.py @@ -38,7 +38,7 @@ def enable_v2_versioning(environment_id: int) -> None: @register_task_handler() def disable_v2_versioning(environment_id: int) -> None: from environments.models import Environment - from features.models import FeatureState + from features.models import FeatureSegment, FeatureState from features.versioning.models import EnvironmentFeatureVersion environment = Environment.objects.get(id=environment_id) @@ -50,11 +50,14 @@ def disable_v2_versioning(environment_id: int) -> None: id__in=[fs.id for fs in latest_feature_states] ).delete() - # update the latest feature states to be the latest version according - # to the old versioning system + # update the latest feature states (and respective feature segments) to be the + # latest version according to the old versioning system latest_feature_states.update( version=1, live_from=timezone.now(), environment_feature_version=None ) + FeatureSegment.objects.filter(environment=environment).update( + environment_feature_version=None + ) EnvironmentFeatureVersion.objects.filter(environment=environment).delete() From 3ce0c255571684655a8b291f3564cd3928d306f8 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Thu, 9 May 2024 11:40:20 +0100 Subject: [PATCH 4/7] Fix issue not returning identity overrides --- api/features/managers.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/api/features/managers.py b/api/features/managers.py index a285e89c2a61..9e8f0336fb00 100644 --- a/api/features/managers.py +++ b/api/features/managers.py @@ -37,7 +37,14 @@ def get_live_feature_states( environment ) latest_version_uuids = [efv.uuid for efv in latest_versions] - qs_filter &= Q(environment_feature_version__uuid__in=latest_version_uuids) + + # Note that since identity overrides aren't part of the versioning system, + # we need to make sure we also return them here. We can still then subsequently + # filter them out with the `additional_filters` if needed. + qs_filter &= Q( + Q(environment_feature_version__uuid__in=latest_version_uuids) + | Q(identity__isnull=False) + ) else: qs_filter &= Q( live_from__isnull=False, From 7d81faef490d581d230d3f792282690b7ee68abc Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Thu, 9 May 2024 11:40:33 +0100 Subject: [PATCH 5/7] Add unit test for disable_v2_versioning task --- .../versioning/test_unit_versioning_tasks.py | 89 ++++++++++++++++++- 1 file changed, 88 insertions(+), 1 deletion(-) diff --git a/api/tests/unit/features/versioning/test_unit_versioning_tasks.py b/api/tests/unit/features/versioning/test_unit_versioning_tasks.py index c91b6c8136d8..f5569b31cfa6 100644 --- a/api/tests/unit/features/versioning/test_unit_versioning_tasks.py +++ b/api/tests/unit/features/versioning/test_unit_versioning_tasks.py @@ -1,12 +1,19 @@ from pytest_mock import MockerFixture +from environments.identities.models import Identity from environments.models import Environment -from features.models import Feature +from features.models import Feature, FeatureSegment, FeatureState from features.versioning.models import EnvironmentFeatureVersion from features.versioning.tasks import ( + disable_v2_versioning, enable_v2_versioning, trigger_update_version_webhooks, ) +from features.versioning.versioning_service import ( + get_environment_flags_queryset, +) +from segments.models import Segment +from users.models import FFAdminUser from webhooks.webhooks import WebhookEventType @@ -28,6 +35,86 @@ def test_enable_v2_versioning( assert environment.use_v2_feature_versioning is True +def test_disable_v2_versioning( + environment_v2_versioning: Environment, + feature: Feature, + segment: Segment, + staff_user: FFAdminUser, + identity: Identity, +) -> None: + # Given + # First, let's create a new version for the given feature which we'll also add a segment override to + v2 = EnvironmentFeatureVersion.objects.create( + environment=environment_v2_versioning, feature=feature + ) + + v2_environment_flag = v2.feature_states.filter(feature=feature).first() + v2_environment_flag.enabled = True + v2_environment_flag.save() + + FeatureState.objects.create( + feature_segment=FeatureSegment.objects.create( + environment=environment_v2_versioning, + feature=feature, + segment=segment, + environment_feature_version=v2, + ), + feature=feature, + environment=environment_v2_versioning, + enabled=True, + environment_feature_version=v2, + ) + + v2.publish(staff_user) + + # Now, let's create a new version which we won't publish (and hence should be ignored after we disabled + # v2 versioning) + v3 = EnvironmentFeatureVersion.objects.create( + environment=environment_v2_versioning, feature=feature + ) + + v3_environment_flag = v3.feature_states.filter(feature=feature).first() + v3_environment_flag.enabled = False + v3_environment_flag.save() + + # Let's also create an identity override to confirm it is not affected. + FeatureState.objects.create( + identity=identity, + feature=feature, + enabled=True, + environment=environment_v2_versioning, + ) + + # When + disable_v2_versioning(environment_v2_versioning.id) + environment_v2_versioning.refresh_from_db() + + # Then + latest_feature_states = get_environment_flags_queryset( + environment=environment_v2_versioning + ) + + assert latest_feature_states.count() == 3 + assert ( + latest_feature_states.filter( + feature=feature, feature_segment__isnull=True, identity__isnull=True + ) + .first() + .enabled + is True + ) + assert ( + latest_feature_states.filter(feature=feature, feature_segment__segment=segment) + .first() + .enabled + is True + ) + assert ( + latest_feature_states.filter(feature=feature, identity=identity).first().enabled + is True + ) + + def test_trigger_update_version_webhooks( environment_v2_versioning: Environment, feature: Feature, mocker: MockerFixture ) -> None: From ccdec08918d710ead85be80717b8afc0909d5971 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Thu, 9 May 2024 11:57:04 +0100 Subject: [PATCH 6/7] Add test to meet coverage restrictions --- api/conftest.py | 6 +++-- api/tests/types.py | 2 +- .../versioning/test_unit_versioning_views.py | 22 +++++++++++++++++++ 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/api/conftest.py b/api/conftest.py index a84d4666238e..cfa1c70e124b 100644 --- a/api/conftest.py +++ b/api/conftest.py @@ -336,11 +336,13 @@ def with_environment_permissions( """ def _with_environment_permissions( - permission_keys: list[str], environment_id: int | None = None + permission_keys: list[str], + environment_id: int | None = None, + admin: bool = False, ) -> UserEnvironmentPermission: environment_id = environment_id or environment.id uep, __ = UserEnvironmentPermission.objects.get_or_create( - environment_id=environment_id, user=staff_user + environment_id=environment_id, user=staff_user, defaults={"admin": admin} ) uep.permissions.add(*permission_keys) diff --git a/api/tests/types.py b/api/tests/types.py index f0bce7b808b3..b87b5e3924a1 100644 --- a/api/tests/types.py +++ b/api/tests/types.py @@ -11,5 +11,5 @@ [list[str], int | None], UserOrganisationPermission ] WithEnvironmentPermissionsCallable = Callable[ - [list[str], int | None], UserEnvironmentPermission + [list[str], int | None, bool], UserEnvironmentPermission ] diff --git a/api/tests/unit/features/versioning/test_unit_versioning_views.py b/api/tests/unit/features/versioning/test_unit_versioning_views.py index 4f0b2267f180..0608a1508056 100644 --- a/api/tests/unit/features/versioning/test_unit_versioning_views.py +++ b/api/tests/unit/features/versioning/test_unit_versioning_views.py @@ -582,3 +582,25 @@ def test_filter_versions_by_is_live( assert not_live_versions_response_json["results"][0]["uuid"] == str( unpublished_environment_feature_version.uuid ) + + +def test_disable_v2_versioning_returns_bad_request_if_not_using_v2_versioning( + environment: Environment, + staff_client: "APIClient", + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + url = reverse( + "api-v1:environments:environment-disable-v2-versioning", + args=[environment.api_key], + ) + + with_environment_permissions( + permission_keys=[], environment_id=environment.id, admin=True + ) + + # When + response = staff_client.post(url) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST From eae43e8767cc168360e9aa09280378374fc450ee Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Thu, 9 May 2024 14:41:50 +0100 Subject: [PATCH 7/7] Improve logic for updating feature segments --- api/features/versioning/tasks.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/api/features/versioning/tasks.py b/api/features/versioning/tasks.py index c3b7c1d581da..b706aa2d5674 100644 --- a/api/features/versioning/tasks.py +++ b/api/features/versioning/tasks.py @@ -44,10 +44,14 @@ def disable_v2_versioning(environment_id: int) -> None: environment = Environment.objects.get(id=environment_id) latest_feature_states = get_environment_flags_queryset(environment) + latest_feature_state_ids = [fs.id for fs in latest_feature_states] - # delete any feature states associated with older versions + # delete any feature states and feature segments associated with older versions FeatureState.objects.filter(identity_id__isnull=True).exclude( - id__in=[fs.id for fs in latest_feature_states] + id__in=latest_feature_state_ids + ).delete() + FeatureSegment.objects.exclude( + feature_states__id__in=latest_feature_state_ids ).delete() # update the latest feature states (and respective feature segments) to be the @@ -55,9 +59,9 @@ def disable_v2_versioning(environment_id: int) -> None: latest_feature_states.update( version=1, live_from=timezone.now(), environment_feature_version=None ) - FeatureSegment.objects.filter(environment=environment).update( - environment_feature_version=None - ) + FeatureSegment.objects.filter( + feature_states__id__in=latest_feature_state_ids + ).update(environment_feature_version=None) EnvironmentFeatureVersion.objects.filter(environment=environment).delete()