diff --git a/api/core/workflows_services.py b/api/core/workflows_services.py index 2547681e56a2..3bd088e98811 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 ( + ChangeRequestConflictError, + ChangeRequestNotApprovedError, +) if TYPE_CHECKING: from features.workflows.core.models import ChangeRequest @@ -27,6 +30,7 @@ def commit(self, committed_by: "FFAdminUser") -> None: "Change request has not been approved by all required approvers." ) + self._check_for_conflicts() self._publish_feature_states() self._publish_environment_feature_versions(committed_by) self._publish_change_sets(committed_by) @@ -45,6 +49,27 @@ def commit(self, committed_by: "FFAdminUser") -> None: self.change_request.save() + def _check_for_conflicts(self) -> None: + # Stale detection already runs when a scheduled change set is published + # by the task processor (see ``publish_version_change_set``). Manual + # commits publish their change sets immediately and previously skipped + # this check, silently reverting any changes that were published after + # the change request was created. Run the same conflict check here so + # that an out-of-date commit is blocked rather than overwriting newer + # changes. + if self.change_request.ignore_conflicts: + return + + now = timezone.now() + for change_set in self.change_request.change_sets.all(): + # Scheduled change sets are conflict-checked when their task fires, + # since further conflicts can appear before they go live. + is_scheduled = change_set.live_from and change_set.live_from >= now + if is_scheduled: + continue + if change_set.get_conflicts(): + raise ChangeRequestConflictError() + 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..a248f4ca0d7c 100644 --- a/api/features/workflows/core/exceptions.py +++ b/api/features/workflows/core/exceptions.py @@ -16,3 +16,13 @@ class CannotApproveOwnChangeRequest(FeatureWorkflowError): class ChangeRequestDeletionError(FeatureWorkflowError): status_code = status.HTTP_400_BAD_REQUEST # type: ignore[assignment] + + +class ChangeRequestConflictError(FeatureWorkflowError): + status_code = status.HTTP_409_CONFLICT # type: ignore[assignment] + default_code = "change_request_conflict" + default_detail = ( + "This change request conflicts with changes that were published since " + "it was created. Refresh the change request, or set ignore_conflicts to " + "commit it anyway." + ) 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 d34656841eae..f84379a4cfe3 100644 --- a/api/tests/unit/features/versioning/test_unit_versioning_tasks.py +++ b/api/tests/unit/features/versioning/test_unit_versioning_tasks.py @@ -31,6 +31,7 @@ get_environment_flags_dict, get_environment_flags_queryset, ) +from features.workflows.core.exceptions import ChangeRequestConflictError from features.workflows.core.models import ChangeRequest from organisations.models import Organisation from projects.models import Project @@ -681,6 +682,158 @@ def test_publish_version_change_set__conflict_with_scheduled_change__sends_confl ) +def test_manual_commit__conflict_with_published_change__blocks_commit( + feature: Feature, + environment_v2_versioning: Environment, + staff_user: FFAdminUser, +) -> None: + # Given + # A change request whose change set updates the environment default. It is + # created first, so it captures the current state of the flag. + change_request_1 = ChangeRequest.objects.create( + title="CR 1", + environment=environment_v2_versioning, + user=staff_user, + ) + change_set_1 = VersionChangeSet.objects.create( + feature=feature, + change_request=change_request_1, + feature_states_to_update=json.dumps( + [ + { + "feature": feature.id, + "enabled": True, + "feature_segment": None, + "feature_state_value": {"type": "unicode", "string_value": "foo"}, + } + ] + ), + ) + + # Another change request changes the same environment default and is + # committed immediately, making the first change request out of date. + conflict_feature_value = "bar" + conflict_feature_enabled = False + conflict_change_request = ChangeRequest.objects.create( + title="Conflict CR", + environment=environment_v2_versioning, + user=staff_user, + ) + VersionChangeSet.objects.create( + feature=feature, + change_request=conflict_change_request, + feature_states_to_update=json.dumps( + [ + { + "feature": feature.id, + "enabled": conflict_feature_enabled, + "feature_segment": None, + "feature_state_value": { + "type": "unicode", + "string_value": conflict_feature_value, + }, + } + ] + ), + ) + conflict_change_request.commit(staff_user) + + # Sanity check: the first change request is now considered in conflict. + assert change_set_1.get_conflicts() + + # When + # We commit the (now stale) first change request immediately. + with pytest.raises(ChangeRequestConflictError): + change_request_1.commit(staff_user) + + # Then + # The stale commit is blocked... + change_request_1.refresh_from_db() + assert change_request_1.committed_at is None + + # ...and the newer change is still reflected in the flags. + latest_flags = get_environment_flags_dict(environment=environment_v2_versioning) + assert latest_flags[(feature.id, None, None)].enabled is conflict_feature_enabled + assert ( + latest_flags[(feature.id, None, None)].get_feature_state_value() + == conflict_feature_value + ) + + +def test_manual_commit__conflict_with_ignore_conflicts__commits( + feature: Feature, + environment_v2_versioning: Environment, + staff_user: FFAdminUser, +) -> None: + # Given + # A stale change request that has ignore_conflicts set explicitly. + change_request_1 = ChangeRequest.objects.create( + title="CR 1", + environment=environment_v2_versioning, + user=staff_user, + ignore_conflicts=True, + ) + stale_feature_value = "foo" + stale_feature_enabled = True + VersionChangeSet.objects.create( + feature=feature, + change_request=change_request_1, + feature_states_to_update=json.dumps( + [ + { + "feature": feature.id, + "enabled": stale_feature_enabled, + "feature_segment": None, + "feature_state_value": { + "type": "unicode", + "string_value": stale_feature_value, + }, + } + ] + ), + ) + + conflict_change_request = ChangeRequest.objects.create( + title="Conflict CR", + environment=environment_v2_versioning, + user=staff_user, + ) + VersionChangeSet.objects.create( + feature=feature, + change_request=conflict_change_request, + feature_states_to_update=json.dumps( + [ + { + "feature": feature.id, + "enabled": False, + "feature_segment": None, + "feature_state_value": { + "type": "unicode", + "string_value": "bar", + }, + } + ] + ), + ) + conflict_change_request.commit(staff_user) + + # When + # The change request is committed despite the conflict. + change_request_1.commit(staff_user) + + # Then + # The commit succeeds and the change request's values win. + change_request_1.refresh_from_db() + assert change_request_1.committed_at is not None + + latest_flags = get_environment_flags_dict(environment=environment_v2_versioning) + assert latest_flags[(feature.id, None, None)].enabled is stale_feature_enabled + assert ( + latest_flags[(feature.id, None, None)].get_feature_state_value() + == stale_feature_value + ) + + def test_publish_version_change_set__segment_override_does_not_exist__raises_validation_error( change_request: ChangeRequest, environment_v2_versioning: Environment,