diff --git a/api/core/workflows_services.py b/api/core/workflows_services.py index 2547681e56a2..afa285d4d671 100644 --- a/api/core/workflows_services.py +++ b/api/core/workflows_services.py @@ -8,7 +8,10 @@ from features.versioning.models import EnvironmentFeatureVersion from features.versioning.signals import environment_feature_version_published from features.versioning.tasks import trigger_update_version_webhooks -from features.workflows.core.exceptions import ChangeRequestNotApprovedError +from features.workflows.core.exceptions import ( + ChangeRequestNotApprovedError, + ChangeRequestStaleError, +) if TYPE_CHECKING: from features.workflows.core.models import ChangeRequest @@ -27,6 +30,8 @@ def commit(self, committed_by: "FFAdminUser") -> None: "Change request has not been approved by all required approvers." ) + self._raise_if_stale() + self._publish_feature_states() self._publish_environment_feature_versions(committed_by) self._publish_change_sets(committed_by) @@ -45,6 +50,24 @@ def commit(self, committed_by: "FFAdminUser") -> None: self.change_request.save() + def _raise_if_stale(self) -> None: + # Mirror the conflict check already performed for scheduled change + # sets (see `publish_version_change_set`) so that a manual commit + # can't silently overwrite overrides published by another change + # request since this one was created. + if self.change_request.ignore_conflicts: + return + + for change_set in self.change_request.change_sets.all(): + if change_set.get_conflicts(): + logger.warning( + "change_request.stale", + organisation__id=self.change_request.project.organisation_id, + environment__id=self.change_request.environment_id, + change_request__id=self.change_request.id, + ) + raise ChangeRequestStaleError() + def _publish_feature_states(self) -> None: now = timezone.now() diff --git a/api/features/workflows/core/exceptions.py b/api/features/workflows/core/exceptions.py index 54f150e8b21b..67a5988ea753 100644 --- a/api/features/workflows/core/exceptions.py +++ b/api/features/workflows/core/exceptions.py @@ -10,6 +10,14 @@ class ChangeRequestNotApprovedError(FeatureWorkflowError): status_code = status.HTTP_400_BAD_REQUEST # type: ignore[assignment] +class ChangeRequestStaleError(FeatureWorkflowError): + status_code = status.HTTP_400_BAD_REQUEST # type: ignore[assignment] + default_detail = ( + "This change request is out of date with changes published since it " + "was created. Please refresh and reapply your changes." + ) + + class CannotApproveOwnChangeRequest(FeatureWorkflowError): status_code = status.HTTP_400_BAD_REQUEST # type: ignore[assignment] diff --git a/api/tests/unit/features/workflows/core/test_unit_workflows_models.py b/api/tests/unit/features/workflows/core/test_unit_workflows_models.py index dce1054a8169..71b8f47dc51b 100644 --- a/api/tests/unit/features/workflows/core/test_unit_workflows_models.py +++ b/api/tests/unit/features/workflows/core/test_unit_workflows_models.py @@ -23,6 +23,7 @@ from core.helpers import get_current_site_url from environments.models import Environment from features.models import Feature, FeatureSegment, FeatureState +from features.value_types import STRING from features.versioning.models import ( EnvironmentFeatureVersion, VersionChangeSet, @@ -33,6 +34,7 @@ CannotApproveOwnChangeRequest, ChangeRequestDeletionError, ChangeRequestNotApprovedError, + ChangeRequestStaleError, ) from features.workflows.core.models import ( ChangeRequest, @@ -231,6 +233,171 @@ def test_change_request_commit__valid_request__emits_structlog_event( } in log.events +def test_change_request_commit__stale_change_set__raises_exception_and_does_not_revert_conflicting_change( + environment_v2_versioning: Environment, + feature: Feature, + segment: Segment, + admin_user: FFAdminUser, +) -> None: + # Given + # An existing, published segment override on the feature. + current_version = EnvironmentFeatureVersion.objects.get_latest_versions_as_queryset( + environment_v2_versioning.id + ).get(feature=feature) + feature_segment = FeatureSegment.objects.create( + segment=segment, + feature=feature, + environment=environment_v2_versioning, + environment_feature_version=current_version, + ) + FeatureState.objects.create( + environment=environment_v2_versioning, + feature=feature, + feature_segment=feature_segment, + environment_feature_version=current_version, + enabled=False, + ) + + # CR A captures the full state of that override (e.g., as part of + # reordering overrides on the feature) when it is created. + change_request_a = ChangeRequest.objects.create( + environment=environment_v2_versioning, title="CR A", user=admin_user + ) + VersionChangeSet.objects.create( + change_request=change_request_a, + feature=feature, + feature_states_to_update=json.dumps( + [ + { + "feature_segment": {"segment": segment.id}, + "enabled": False, + "feature_state_value": { + "type": STRING, + "string_value": "original value", + }, + } + ] + ), + ) + + # And CR B changes the value of that same override, and is published + # first. + change_request_b = ChangeRequest.objects.create( + environment=environment_v2_versioning, title="CR B", user=admin_user + ) + VersionChangeSet.objects.create( + change_request=change_request_b, + feature=feature, + feature_states_to_update=json.dumps( + [ + { + "feature_segment": {"segment": segment.id}, + "enabled": True, + "feature_state_value": { + "type": STRING, + "string_value": "concurrent value", + }, + } + ] + ), + ) + change_request_b.commit(admin_user) + + # When / Then + # Committing CR A should now be blocked, since it is stale: its + # captured override state conflicts with CR B's published change. + with pytest.raises(ChangeRequestStaleError): + change_request_a.commit(admin_user) + + # and CR B's change has not been silently reverted. + latest_flags = get_environment_flags_list( + environment=environment_v2_versioning, feature_name=feature.name + ) + override = next(fs for fs in latest_flags if fs.feature_segment_id is not None) + assert override.enabled is True + assert override.get_feature_state_value() == "concurrent value" + assert change_request_a.committed_at is None + + +def test_change_request_commit__stale_change_set_but_ignore_conflicts__commits_and_reverts_change( + environment_v2_versioning: Environment, + feature: Feature, + segment: Segment, + admin_user: FFAdminUser, +) -> None: + # Given + # Same setup as above, but CR A has `ignore_conflicts` set, which is + # the existing opt-out already respected by scheduled publishes. + current_version = EnvironmentFeatureVersion.objects.get_latest_versions_as_queryset( + environment_v2_versioning.id + ).get(feature=feature) + feature_segment = FeatureSegment.objects.create( + segment=segment, + feature=feature, + environment=environment_v2_versioning, + environment_feature_version=current_version, + ) + FeatureState.objects.create( + environment=environment_v2_versioning, + feature=feature, + feature_segment=feature_segment, + environment_feature_version=current_version, + enabled=False, + ) + + change_request_a = ChangeRequest.objects.create( + environment=environment_v2_versioning, + title="CR A", + user=admin_user, + ignore_conflicts=True, + ) + VersionChangeSet.objects.create( + change_request=change_request_a, + feature=feature, + feature_states_to_update=json.dumps( + [ + { + "feature_segment": {"segment": segment.id}, + "enabled": False, + "feature_state_value": { + "type": STRING, + "string_value": "original value", + }, + } + ] + ), + ) + + change_request_b = ChangeRequest.objects.create( + environment=environment_v2_versioning, title="CR B", user=admin_user + ) + VersionChangeSet.objects.create( + change_request=change_request_b, + feature=feature, + feature_states_to_update=json.dumps( + [ + { + "feature_segment": {"segment": segment.id}, + "enabled": True, + "feature_state_value": { + "type": STRING, + "string_value": "concurrent value", + }, + } + ] + ), + ) + change_request_b.commit(admin_user) + + # When + change_request_a.commit(admin_user) + + # Then + # commit succeeds, and (as documented by `ignore_conflicts`) CR A's + # captured state overwrites CR B's published change. + assert change_request_a.committed_at is not None + + def test_change_request_create__valid_environment__creates_audit_log( # type: ignore[no-untyped-def] environment, admin_user ): diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index c012f1efc5e7..d0d460951409 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -755,17 +755,27 @@ Attributes: ### `workflows.change_request.committed` Logged at `info` from: - - `api/core/workflows_services.py:39` + - `api/core/workflows_services.py:44` Attributes: - `environment.id` - `feature_states.count` - `organisation.id` +### `workflows.change_request.stale` + +Logged at `warning` from: + - `api/core/workflows_services.py:63` + +Attributes: + - `change_request.id` + - `environment.id` + - `organisation.id` + ### `workflows.missing_live_segment` Logged at `warning` from: - - `api/core/workflows_services.py:114` + - `api/core/workflows_services.py:137` Attributes: - `draft_segment` @@ -773,7 +783,7 @@ Attributes: ### `workflows.segment_revision_created` Logged at `info` from: - - `api/core/workflows_services.py:119` + - `api/core/workflows_services.py:142` Attributes: - `revision_id`