From 6b53f4008c5cae817b156c8549f800eb29f16eab Mon Sep 17 00:00:00 2001 From: Deep Santoshwar Date: Wed, 5 Aug 2026 17:04:04 +0530 Subject: [PATCH] fix(versioning): block manual commit of a stale change request A Change Request commit skipped the conflict check that scheduled publishes already run, so committing a CR whose captured overrides had since been changed by another published CR would silently overwrite that newer change. Run the same VersionChangeSet conflict check on manual commits and reject them with ChangeRequestStaleError unless ignore_conflicts is set, consistent with scheduled publishes. --- api/core/workflows_services.py | 25 ++- api/features/workflows/core/exceptions.py | 8 + .../core/test_unit_workflows_models.py | 167 ++++++++++++++++++ .../observability/_events-catalogue.md | 16 +- 4 files changed, 212 insertions(+), 4 deletions(-) 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`