diff --git a/api/core/workflows_services.py b/api/core/workflows_services.py index 4481e08ed2ef..41ec50f3cdd3 100644 --- a/api/core/workflows_services.py +++ b/api/core/workflows_services.py @@ -11,6 +11,7 @@ from features.workflows.core.exceptions import ( CannotModifyManagedSegmentError, ChangeRequestNotApprovedError, + ChangeRequestStaleError, ) if TYPE_CHECKING: @@ -33,6 +34,8 @@ def commit(self, committed_by: "FFAdminUser") -> None: # raising any later would leave the change request half-applied. self._validate_segments_are_not_cohort_managed() + self._raise_if_stale() + self._publish_feature_states() self._publish_environment_feature_versions(committed_by) self._publish_change_sets(committed_by) @@ -51,6 +54,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 32a87acc3def..36a154cfd3c7 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 9f3036f37bed..147e514266c2 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 @@ -24,6 +24,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, @@ -35,6 +36,7 @@ CannotModifyManagedSegmentError, ChangeRequestDeletionError, ChangeRequestNotApprovedError, + ChangeRequestStaleError, ) from features.workflows.core.models import ( ChangeRequest, @@ -233,6 +235,142 @@ def test_change_request_commit__valid_request__emits_structlog_event( } in log.events +def _create_conflicting_change_requests( + environment: Environment, + feature: Feature, + segment: Segment, + user: FFAdminUser, + ignore_conflicts: bool = False, +) -> ChangeRequest: + """ + Set up an existing, published segment override on `feature`, plus two + change requests that both target it. + + CR A captures the full state of that override (e.g., as part of + reordering overrides on the feature) when it is created. CR B changes + the value of the same override, and is committed here, leaving CR A + stale. CR A is returned, uncommitted. + """ + current_version = EnvironmentFeatureVersion.objects.get_latest_versions_as_queryset( + environment.id + ).get(feature=feature) + feature_segment = FeatureSegment.objects.create( + segment=segment, + feature=feature, + environment=environment, + environment_feature_version=current_version, + ) + FeatureState.objects.create( + environment=environment, + feature=feature, + feature_segment=feature_segment, + environment_feature_version=current_version, + enabled=False, + ) + + change_request_a: ChangeRequest = ChangeRequest.objects.create( + environment=environment, + title="CR A", + user=user, + ignore_conflicts=ignore_conflicts, + ) + 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, title="CR B", user=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(user) + + return change_request_a + + +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 + change_request_a = _create_conflicting_change_requests( + environment_v2_versioning, feature, segment, 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. + change_request_a = _create_conflicting_change_requests( + environment_v2_versioning, feature, segment, admin_user, ignore_conflicts=True + ) + + # 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 + + 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 False + assert override.get_feature_state_value() == "original value" + + 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 8bafb8eaf0c9..9a9ff5f34157 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -825,17 +825,27 @@ Attributes: ### `workflows.change_request.committed` Logged at `info` from: - - `api/core/workflows_services.py:45` + - `api/core/workflows_services.py:48` Attributes: - `environment.id` - `feature_states.count` - `organisation.id` +### `workflows.change_request.stale` + +Logged at `warning` from: + - `api/core/workflows_services.py:67` + +Attributes: + - `change_request.id` + - `environment.id` + - `organisation.id` + ### `workflows.missing_live_segment` Logged at `warning` from: - - `api/core/workflows_services.py:130` + - `api/core/workflows_services.py:151` Attributes: - `draft_segment` @@ -843,7 +853,7 @@ Attributes: ### `workflows.segment_revision_created` Logged at `info` from: - - `api/core/workflows_services.py:135` + - `api/core/workflows_services.py:156` Attributes: - `revision_id`