diff --git a/api/api/urls/future.py b/api/api/urls/future.py index 7fd42673cfe0..fab2b5db62b3 100644 --- a/api/api/urls/future.py +++ b/api/api/urls/future.py @@ -7,7 +7,7 @@ from django.urls import path -from features.future.views import FlagAPIView +from features.future.views import FlagAPIView, SegmentOverrideAPIView app_name = "future" @@ -17,4 +17,10 @@ FlagAPIView.as_view(), name="flag", ), + path( + "environments//features/" + "/segment-overrides//", + SegmentOverrideAPIView.as_view(), + name="segment-override", + ), ] diff --git a/api/features/future/exceptions.py b/api/features/future/exceptions.py index 43717e2fcf53..fe1699272780 100644 --- a/api/features/future/exceptions.py +++ b/api/features/future/exceptions.py @@ -1,7 +1,7 @@ """https://docs.flagsmith.com/managing-flags/updating-flags""" from rest_framework import status -from rest_framework.exceptions import APIException +from rest_framework.exceptions import APIException, NotFound class ChangeRequestsEnabledError(APIException): @@ -23,3 +23,9 @@ class DuplicatePriorityError(APIException): status_code = status.HTTP_400_BAD_REQUEST default_detail = "Segment overrides must not share a priority." + + +class SegmentOverrideNotFoundError(NotFound): + """Raised where a flag serves a segment nothing of its own.""" + + default_detail = "Segment override not found." diff --git a/api/features/future/permissions.py b/api/features/future/permissions.py index 6a9d84dc03fd..2ea1511b29d6 100644 --- a/api/features/future/permissions.py +++ b/api/features/future/permissions.py @@ -48,3 +48,10 @@ def check_update_permissions( raise NotFound() if any(property_name in properties for property_name in denied): raise PermissionDenied() + + +def check_segment_overrides_permissions( + user: UserABC, environment: Environment +) -> None: + """Authorise a caller to write a flag's segment overrides.""" + check_update_permissions(user, environment, {"segment_overrides": None}) diff --git a/api/features/future/services.py b/api/features/future/services.py index 30bb62eaa6c2..d2fe64aed62c 100644 --- a/api/features/future/services.py +++ b/api/features/future/services.py @@ -9,7 +9,10 @@ from api_keys.user import APIKeyUser from environments.models import Environment -from features.future.exceptions import DuplicatePriorityError +from features.future.exceptions import ( + DuplicatePriorityError, + SegmentOverrideNotFoundError, +) from features.future.mappers import ( map_environment_default, map_segment_override, @@ -70,6 +73,27 @@ def _get_feature_states_to_write( ) +def _publish_version( + version: EnvironmentFeatureVersion, author: FFAdminUser | APIKeyUser +) -> None: + # `UserABC.__subclasshook__` matches any user against `APIKeyUser` + published_by = author if isinstance(author, FFAdminUser) else None + version.publish( + published_by=published_by, + published_by_api_key=None if published_by else author.key, + ) + + +def _get_overrides_by_segment_id( + feature_states: Sequence[FeatureState], +) -> dict[int, FeatureState]: + return { + feature_segment.segment_id: feature_state + for feature_state in feature_states + if (feature_segment := feature_state.feature_segment) is not None + } + + def _write_variants(feature_state: FeatureState, variants: Sequence[Variant]) -> None: weighted = { multivariate_value.multivariate_feature_option_id: multivariate_value @@ -299,22 +323,13 @@ def update_flag( feature=feature, version=version, environment_default=environment_default, - overrides={ - feature_segment.segment_id: feature_state - for feature_state in feature_states - if (feature_segment := feature_state.feature_segment) is not None - }, + overrides=_get_overrides_by_segment_id(feature_states), changes=override_changes, replace=replace, ) if version is not None: - # `UserABC.__subclasshook__` matches any user against `APIKeyUser` - published_by = author if isinstance(author, FFAdminUser) else None - version.publish( - published_by=published_by, - published_by_api_key=None if published_by else author.key, - ) + _publish_version(version, author) logger.info( "flag.updated", @@ -330,6 +345,45 @@ def update_flag( return get_flag(environment=environment, feature=feature) +def delete_segment_override( + *, + environment: Environment, + feature: Feature, + segment_id: int, + author: FFAdminUser | APIKeyUser, +) -> UpdateFlagResponse: + """Remove a flag's override for one segment, leaving the rest of the flag alone.""" + with transaction.atomic(): + version = _create_draft_version(environment, feature) + feature_states = _get_feature_states_to_write(environment, feature, version) + + if segment_id not in _get_overrides_by_segment_id(feature_states): + raise SegmentOverrideNotFoundError() + + _delete_segment_overrides( + environment=environment, + feature=feature, + version=version, + segment_ids=[segment_id], + ) + + if version is not None: + _publish_version(version, author) + + 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=[], + segment_overrides__updated__segment__ids=[], + segment_overrides__deleted__segment__ids=[segment_id], + ) + + return get_flag(environment=environment, feature=feature) + + def get_flag(*, environment: Environment, feature: Feature) -> UpdateFlagResponse: """Read what a flag serves in an environment.""" feature_states = _get_feature_states(environment, feature) diff --git a/api/features/future/views.py b/api/features/future/views.py index 569a05b4cf16..613946f543e4 100644 --- a/api/features/future/views.py +++ b/api/features/future/views.py @@ -15,10 +15,11 @@ from features.future.exceptions import ChangeRequestsEnabledError from features.future.permissions import ( check_read_permissions, + check_segment_overrides_permissions, check_update_permissions, ) from features.future.serializers import UpdateFlagSerializer -from features.future.services import get_flag, update_flag +from features.future.services import delete_segment_override, get_flag, update_flag from features.future.types import UpdateFlagRequest, UpdateFlagResponse from features.models import Feature @@ -41,6 +42,22 @@ def _get_feature(environment: Environment, feature_id: int) -> Feature: raise NotFound() from None +def _check_change_requests_disabled(environment: Environment, feature: Feature) -> None: + """Refuse to write a flag that can only be changed by a change request.""" + if not environment.is_workflow_enabled: + return + api_error = ChangeRequestsEnabledError() + logger.warning( + "flag.update_rejected", + organisation__id=environment.project.organisation_id, + project__id=environment.project_id, + environment__id=environment.id, + feature__id=feature.id, + reason=api_error.default_code, + ) + raise api_error + + class FlagAPIView(APIView): """Read or update what a flag serves in an environment.""" @@ -96,18 +113,7 @@ def _update_flag( environment = _get_environment(environment_key) check_update_permissions(request.user, environment, request.data) feature = _get_feature(environment, feature_id) - - if environment.is_workflow_enabled: - api_error = ChangeRequestsEnabledError() - logger.warning( - "flag.update_rejected", - organisation__id=environment.project.organisation_id, - project__id=environment.project_id, - environment__id=environment.id, - feature__id=feature.id, - reason=api_error.default_code, - ) - raise api_error + _check_change_requests_disabled(environment, feature) serializer = UpdateFlagSerializer( data=request.data, @@ -124,3 +130,37 @@ def _update_flag( author=request.user, ) ) + + +class SegmentOverrideAPIView(APIView): + """Remove what a flag serves to a segment in an environment.""" + + permission_classes = [IsAuthenticated] + + @extend_schema( + # Responds with the flag, like the other methods, rather than no content. + responses={200: UpdateFlagResponse}, + tags=["experimental"], + description=( + "Remove the flag's override for the segment, " + "leaving the rest of the flag as it is." + ), + ) + def delete( + self, request: Request, environment_key: str, feature_id: int, segment_id: int + ) -> Response: + assert not isinstance(request.user, AnonymousUser) + + environment = _get_environment(environment_key) + check_segment_overrides_permissions(request.user, environment) + feature = _get_feature(environment, feature_id) + _check_change_requests_disabled(environment, feature) + + return Response( + delete_segment_override( + environment=environment, + feature=feature, + segment_id=segment_id, + author=request.user, + ) + ) diff --git a/api/tests/integration/features/future/test_flag_endpoint.py b/api/tests/integration/features/future/test_flag_endpoint.py index bb4cfee59408..2ffbc35a92da 100644 --- a/api/tests/integration/features/future/test_flag_endpoint.py +++ b/api/tests/integration/features/future/test_flag_endpoint.py @@ -90,6 +90,42 @@ def mv_feature_variants( ] +@pytest.fixture() +def two_segment_overrides( + admin_client_new: APIClient, + environment_api_key: str, + feature: int, + log: StructuredLogCapture, + segment: int, + segment_2: int, + versioned_environment: Environment, +) -> None: + response = admin_client_new.patch( + f"/api/__future__/environments/{environment_api_key}/features/{feature}/", + UpdateFlagRequest( + { + "segment_overrides": [ + { + "segment": {"id": segment}, + "enabled": True, + "priority": 0, + "value": {"type": "string", "value": "enterprise"}, + }, + { + "segment": {"id": segment_2}, + "enabled": True, + "priority": 1, + "value": {"type": "string", "value": "startup"}, + }, + ], + } + ), + format="json", + ) + assert response.status_code == 200 + log.events.clear() + + @pytest.mark.parametrize( "changes", [ @@ -2060,3 +2096,277 @@ def test_update_flag__variants_on_standard_feature__responds_400( .multivariate_feature_state_values.exists() ) assert log.events == [] + + +def test_delete_segment_override__existing_override__removes_override( + admin_client_new: APIClient, + default_feature_value: str, + environment_api_key: str, + feature: int, + log: StructuredLogCapture, + segment: int, + segment_2: int, + two_segment_overrides: None, + versioned_environment: Environment, +) -> None: + # Given / When + response = admin_client_new.delete( + f"/api/__future__/environments/{environment_api_key}/features/{feature}" + f"/segment-overrides/{segment}/", + ) + + # Then + assert response.status_code == 200 + assert response.json() == { + "environment_default": { + "enabled": False, + "value": {"type": "string", "value": default_feature_value}, + "variants": [], + }, + "segment_overrides": [ + { + "segment": {"id": segment_2}, + "priority": 1, + "enabled": True, + "value": {"type": "string", "value": "startup"}, + "variants": [], + }, + ], + } + live_feature_states = FeatureState.objects.get_live_feature_states( + environment=versioned_environment, + feature_id=feature, + ) + assert dict( + live_feature_states.exclude(feature_segment=None).values_list( + "feature_segment__segment_id", "feature_segment__priority" + ) + ) == {segment_2: 1} + assert live_feature_states.get(feature_segment=None).enabled is False + assert log.events == [ + { + "level": "info", + "event": "flag.updated", + "organisation__id": versioned_environment.project.organisation_id, + "project__id": versioned_environment.project_id, + "environment__id": versioned_environment.id, + "feature__id": feature, + "segment_overrides__created__segment__ids": [], + "segment_overrides__updated__segment__ids": [], + "segment_overrides__deleted__segment__ids": [segment], + }, + ] + + +def test_delete_segment_override__environment_versioned__publishes_new_version( + admin_client_new: APIClient, + environment_api_key: str, + feature: int, + segment: int, + two_segment_overrides: None, + versioned_environment: Environment, +) -> None: + # Given + versions = EnvironmentFeatureVersion.objects.filter( + environment=versioned_environment, + feature_id=feature, + published_at__isnull=False, + ) + version_count = versions.count() + + # When + response = admin_client_new.delete( + f"/api/__future__/environments/{environment_api_key}/features/{feature}" + f"/segment-overrides/{segment}/", + ) + + # Then + assert response.status_code == 200 + assert versions.count() == version_count + ( + 1 if versioned_environment.use_v2_feature_versioning else 0 + ) + + +def test_delete_segment_override__no_override__responds_404( + admin_client_new: APIClient, + environment_api_key: str, + feature: int, + log: StructuredLogCapture, + segment: int, + versioned_environment: Environment, +) -> None: + # Given + versions = EnvironmentFeatureVersion.objects.filter( + environment=versioned_environment, feature_id=feature + ) + version_count = versions.count() + + # When + response = admin_client_new.delete( + f"/api/__future__/environments/{environment_api_key}/features/{feature}" + f"/segment-overrides/{segment}/", + ) + + # Then + assert response.status_code == 404 + assert response.json() == {"detail": "Segment override not found."} + assert versions.count() == version_count + assert log.events == [] + + +def test_delete_segment_override__unknown_feature__responds_404( + admin_client_new: APIClient, + environment_api_key: str, + feature: int, + log: StructuredLogCapture, + segment: int, + versioned_environment: Environment, +) -> None: + # Given + unknown_feature = feature + 1 + + # When + response = admin_client_new.delete( + f"/api/__future__/environments/{environment_api_key}" + f"/features/{unknown_feature}/segment-overrides/{segment}/", + ) + + # Then + assert response.status_code == 404 + assert response.json() == {"detail": "Not found."} + assert log.events == [] + + +def test_delete_segment_override__unknown_environment__responds_404( + admin_client_new: APIClient, + feature: int, + log: StructuredLogCapture, + segment: int, + versioned_environment: Environment, +) -> None: + # Given / When + response = admin_client_new.delete( + f"/api/__future__/environments/unknown-api-key/features/{feature}" + f"/segment-overrides/{segment}/", + ) + + # Then + assert response.status_code == 404 + assert response.json() == {"detail": "Not found."} + assert log.events == [] + + +def test_delete_segment_override__change_requests_enabled__responds_409( + admin_client_new: APIClient, + environment_api_key: str, + feature: int, + log: StructuredLogCapture, + segment: int, + two_segment_overrides: None, + versioned_environment: Environment, +) -> None: + # Given + versioned_environment.minimum_change_request_approvals = 2 + versioned_environment.save() + + # When + response = admin_client_new.delete( + f"/api/__future__/environments/{environment_api_key}/features/{feature}" + f"/segment-overrides/{segment}/", + ) + + # Then + assert response.status_code == 409 + assert response.json() == { + "detail": "Cannot update flags in an environment with change requests enabled.", + "code": "change_requests_enabled", + } + assert ( + FeatureState.objects.get_live_feature_states( + environment=versioned_environment, + feature_id=feature, + ) + .filter(feature_segment__segment_id=segment) + .exists() + ) + assert log.events == [ + { + "level": "warning", + "event": "flag.update_rejected", + "organisation__id": versioned_environment.project.organisation_id, + "project__id": versioned_environment.project_id, + "environment__id": versioned_environment.id, + "feature__id": feature, + "reason": "change_requests_enabled", + }, + ] + + +def test_delete_segment_override__user_without_environment_permissions__responds_404( + environment_api_key: str, + feature: int, + log: StructuredLogCapture, + non_admin_client: APIClient, + segment: int, + two_segment_overrides: None, + versioned_environment: Environment, +) -> None: + # Given / When + response = non_admin_client.delete( + f"/api/__future__/environments/{environment_api_key}/features/{feature}" + f"/segment-overrides/{segment}/", + ) + + # Then + assert response.status_code == 404 + assert response.json() == {"detail": "Not found."} + assert ( + FeatureState.objects.get_live_feature_states( + environment=versioned_environment, + feature_id=feature, + ) + .filter(feature_segment__segment_id=segment) + .exists() + ) + assert log.events == [] + + +@pytest.mark.parametrize( + "permission, expected_status_code", + [ + pytest.param(MANAGE_SEGMENT_OVERRIDES, 200, id="manage_segment_overrides"), + pytest.param(UPDATE_FEATURE_STATE, 403, id="update_feature_state"), + ], +) +def test_delete_segment_override__environment_permission__gates_override( + environment: int, + environment_api_key: str, + expected_status_code: int, + feature: int, + organisation: int, + permission: str, + segment: int, + staff_client: APIClient, + staff_user: FFAdminUser, + two_segment_overrides: None, + versioned_environment: Environment, + with_environment_permissions: WithEnvironmentPermissionsCallable, +) -> None: + # Given + staff_user.add_organisation(Organisation.objects.get(id=organisation)) + with_environment_permissions([permission], environment, False) + + # When + response = staff_client.delete( + f"/api/__future__/environments/{environment_api_key}/features/{feature}" + f"/segment-overrides/{segment}/", + ) + + # Then + assert response.status_code == expected_status_code + assert FeatureState.objects.get_live_feature_states( + environment=versioned_environment, + feature_id=feature, + ).filter(feature_segment__segment_id=segment).exists() is ( + expected_status_code != 200 + ) diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index 89df6e7eb72f..380b42abe603 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -267,7 +267,7 @@ Attributes: ### `features.flag.update_rejected` Logged at `warning` from: - - `api/features/future/views.py:102` + - `api/features/future/views.py:50` Attributes: - `environment.id` @@ -279,7 +279,8 @@ Attributes: ### `features.flag.updated` Logged at `info` from: - - `api/features/future/services.py:319` + - `api/features/future/services.py:334` + - `api/features/future/services.py:373` Attributes: - `environment.id` diff --git a/docs/docs/managing-flags/updating-flags.md b/docs/docs/managing-flags/updating-flags.md index c131f45626ca..27900b6c5eaa 100644 --- a/docs/docs/managing-flags/updating-flags.md +++ b/docs/docs/managing-flags/updating-flags.md @@ -208,7 +208,38 @@ give it its own. `PUT` restores that for whatever it omits. ### Remove a segment override -To remove a segment override, `PUT` the full list of overrides without it. `PUT` replaces the whole set, deleting any +To remove a single segment override, `DELETE` it by segment: + +```bash +curl -X DELETE 'https://api.flagsmith.com/api/__future__/environments/{environment_key}/features/{feature_id}/segment-overrides/{segment_id}/' \ + -H 'Authorization: Api-Key {api_key}' +``` + +Like the update methods, this responds with the flag's complete state in the environment: + +```http +HTTP/1.1 200 OK +Content-Type: application/json + +{ + "environment_default": {"enabled": false, "value": {"type": "string", "value": "standard"}, "variants": []}, + "segment_overrides": [ + { + "segment": {"id": 101}, + "priority": 10, + "enabled": true, + "value": {"type": "string", "value": "enterprise"}, + "variants": [] + } + ] +} +``` + +The remaining overrides keep their priorities. + +A flag that has no override for the segment will respond `404`. + +Alternatively, `PUT` the full list of overrides without the one to remove. `PUT` replaces the whole set, deleting any override not listed: ```bash diff --git a/openapi.yaml b/openapi.yaml index b65e065590eb..5eaa7d92ec94 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -113,6 +113,38 @@ paths: - Master API Key: [] tags: - experimental + '/api/__future__/environments/{environment_key}/features/{feature_id}/segment-overrides/{segment_id}/': + delete: + operationId: api___future___environments_features_segment_overrides_destroy + description: 'Remove the flag''s override for the segment, leaving the rest of the flag as it is.' + parameters: + - name: environment_key + in: path + required: true + schema: + type: string + - name: feature_id + in: path + required: true + schema: + type: integer + - name: segment_id + in: path + required: true + schema: + type: integer + responses: + '200': + description: '' + content: + application/json: + schema: + $ref: '#/components/schemas/UpdateFlagResponse' + security: + - tokenAuth: [] + - Master API Key: [] + tags: + - experimental '/api/experiments/environments/{environment_key}/delete-segment-override/': post: operationId: api_experiments_environments_delete_segment_override_create